Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding authorisation options for API access #1042

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bpiraeus
Copy link

Fixes #

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

Description of the Change

Checklist

  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional)
  • man page updated (if applicable)
  • bash completion updated (if applicable)
  • documentation updated
  • author name in AUTHORS

@bpiraeus
Copy link
Author

I went through the failing tests, and unless I'm missing something, those are not from my changes, not sure what I can do about that short of refactoring someone else's code. I'm happy to muck around if needed, but you'd likely prefer someone with a lot more chops than myself to mitigate those issues.

@randombenj
Copy link
Contributor

@bpiraeus It looks like mostly deprecation errors

If you want to, feel free to investigate if there are some simple fixes for the deprecated packages (x/term) seems to be quite an easy one, not sure about the crypto one ...

@bpiraeus
Copy link
Author

Cleaned up a few oddballs in my code that gosimple flagged, also moved the terminal bits to the appropriate package, the pgp ones appear as if they're likely going to want 3rd party library.

@neolynx neolynx self-assigned this Feb 6, 2024
@aptly-dev aptly-dev deleted a comment from randombenj Aug 13, 2024
@neolynx
Copy link
Member

neolynx commented Aug 13, 2024

introducing authorization per repo is a nice idea.

however, authentication without SSL is not that nice, so probably we should implement this as well with this change.

since different authentication types are supported, the variable LdapGroup should probably AuthorizedGroup

@neolynx neolynx force-pushed the ldap_auth_integration branch from 7291b04 to 888025c Compare August 13, 2024 18:24
@neolynx
Copy link
Member

neolynx commented Aug 13, 2024

rebased on master

@neolynx neolynx added the needs review Ready for review & merge label Aug 13, 2024
@neolynx neolynx added needs tests and removed needs review Ready for review & merge labels Oct 8, 2024
  - ldap currently the only supported method
adding authorisation options for local repositories
  - ldap groups per repo
@neolynx neolynx force-pushed the ldap_auth_integration branch from 888025c to ef42975 Compare October 9, 2024 19:32
@neolynx neolynx added needs rebase The PR needs to be rebased on master help wanted ! Please help getting this PR merged :-) labels Nov 1, 2024
@neolynx
Copy link
Member

neolynx commented Nov 1, 2024

@bpiraeus are you still interested in continuing this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted ! Please help getting this PR merged :-) needs rebase The PR needs to be rebased on master needs tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants