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

Allow authentication via URI query parameter #642

Conversation

SaberStrat
Copy link

Introduce new annotation for resources that makes the authentication filter look at the URI query for a parameter containing the API key instead of a header.
This enables clients that cannot supply headers to use authentication- locked resources.

Addressed Issue

#641

Introduce new annotation for resources that makes the authentication
filter look at the URI query for a parameter containing the API key
instead of a header.
This enables clients that cannot supply headers to use authentication-
locked resources.

Signed-off-by: Kirill.Sybin <[email protected]>
@SaberStrat
Copy link
Author

SaberStrat commented Aug 26, 2024

I asked in the issue whether to implement this as a generally available alternative to passing the API key via header without having to explicitly enable it, or as behavior that has to be activates, but got no reply. So I did it the latter way, that changes nothing about existing behavior until someone activates it for them.

Let me know if this implementation is okay, then I'll add tests for it. If not, well I still look forward to the feedback :)

Kirill.Sybin added 2 commits August 31, 2024 16:57
Renaming as per reviewer suggestion to better reflect its purpose:
stevespringett#642 (comment)

Signed-off-by: Kirill.Sybin <[email protected]>
Implement reviewer suggestion to replace either-or logic of looking at
the URI query parameter instead of the header into a hierarchy, which
keeps the header as the preferred location for the API key in every case
and makes the query parameter the opt-in alternative that's enabled by
the annotation.
Review:
stevespringett#642 (comment)

Signed-off-by: Kirill.Sybin <[email protected]>
@SaberStrat
Copy link
Author

SaberStrat commented Sep 1, 2024

Actually, I couldn't think of tests that are neither trivial nor not more involved than what I will be testing with tests of the downstream project using this PR's feature (i.e. DependencyTrack/dependency-track#4059). Test coverage here isn't that huge, so there wasn't much for me to go off of or take inspiration from either :/.

If you see tests as necessary, I'll accept ideas pointing me in a concrete direction what to test :).

Copy link
Collaborator

@nscuro nscuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SaberStrat!

@nscuro nscuro merged commit f9f6cf1 into stevespringett:master Sep 6, 2024
2 checks passed
@SaberStrat SaberStrat deleted the feature/641-allow-auth-via-uri-query-param branch September 6, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants