-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Support for OIDC API Auth and OIDC integration tests #16977
Changes from 25 commits
6182222
d1aa6d1
9d3d45a
32dc586
9367a69
257ebbc
e4fd79b
a7a0492
54d0bd2
9992c38
41e70f2
4e2f56a
c78788e
9cc1a6a
cc2d8c1
92851c0
7f05d3f
e1323b7
65ee1e6
3d1244d
517f0e7
ca8cb47
5d316fb
56edd29
fed04ed
a21c9b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -294,6 +294,13 @@ def by_api_key(self, api_key: str, sa_session=None): | |
raise exceptions.AuthenticationFailed("Provided API key has expired.") | ||
return provided_key.user | ||
|
||
def by_oidc_access_token(self, access_token: str): | ||
if hasattr(self.app, "authnz_manager") and self.app.authnz_manager: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be more explicit to find which type of app we need (is it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's not great, I've simply followed the pattern used elsewhere. This is because authnz_manager is assigned conditionally even in the UniverseApplication constructor and is undefined by default. This is something that should probably be handled during the proposed refactoring run. |
||
user = self.app.authnz_manager.match_access_token_to_user(self.app.model.session, access_token) # type: ignore[attr-defined] | ||
return user | ||
else: | ||
return None | ||
|
||
def check_bootstrap_admin_api_key(self, api_key): | ||
bootstrap_admin_api_key = getattr(self.app.config, "bootstrap_admin_api_key", None) | ||
if not bootstrap_admin_api_key: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the :* missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config setting should specify only the prefix. The
*
is the exact scope that's requested, meaning all permissions. That leaves space for future expansion, like perhaps<prefix>:datasets
to request access to datasets etc. For now, we only support<prefix>:*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should then
e.g "https://galaxyproject.org/api" be changed
to be clear?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've expanded the description a bit. Let me know whether this looks better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks a lot!