PXP-10113 Client credentials grant #1033
Conversation
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
Pull Request Test Coverage Report for Build 12674
💛 - Coveralls |
PlanXCyborg
added
test-google-googleServiceAccountRemovalTest
test-google-googleServiceAccountTest
test-pidgin-getCoremetadataTest
test-portal-coremetadatapageTest
test-portal-dataUploadTest
test-portal-indexingPageTest
test-sheepdogAndPeregrine-submitAndQueryNodesTest
test-sheepdogAndPeregrine-submitFileTest
labels
Avantol13
removed
test-google-googleServiceAccountTest
test-portal-indexingPageTest
test-portal-dataUploadTest
test-google-googleServiceAccountRemovalTest
test-pidgin-getCoremetadataTest
test-portal-coremetadatapageTest
test-sheepdogAndPeregrine-submitAndQueryNodesTest
test-sheepdogAndPeregrine-submitFileTest
labels
README.md
Outdated
| @@ -515,6 +515,14 @@ To specify allowed scopes, use the `allowed-scopes` argument: | |||
| fence-create client-create ... --allowed-scopes openid user data | |||
| ``` | |||
|
|
|||
| #### Register an Oauth Client for a Client Credentials flow | |||
|
|
|||
| As a Gen3 commons administrator, if you want to create an oauth client for a client credentials flow: | |||
There was a problem hiding this comment.
we may want more docs than this, or an explanation of when you might want this (and how authorization is applied the same as users and this only works against certain endpoints)
| client_id = "" | ||
| try: | ||
| if not token: | ||
| token = validate_request(scope=[], audience=config.get("BASE_URL")) |
There was a problem hiding this comment.
we may want to consider a scope for client-level information similar to user, maybe client. I'm not sure when we wouldn't provide this though.. and if the spec doesn't dictate then maybe it's not necessary
There was a problem hiding this comment.
good point - since we're not allowing client tokens for these endpoints anyway, i'm leaving this as-is so we don't complicate things. let me know if you think we should add the scope.
| @@ -643,7 +643,9 @@ def check_legacy_authorization(self, action): | |||
| ) | |||
| return self.index_document.get("uploader") == username | |||
|
|
|||
| given_acls = set(filter_auth_ids(action, flask.g.user.project_access)) | |||
| given_acls = set() | |||
There was a problem hiding this comment.
Okay so my read of this is that we're not actually allowing client credentials tokens to generate signed URLs, right? All these changes are just to ensure that a sane 401 is returned (instead of 500-ing off of all the user specific access stuff) and pave the road for potentially adding that support in the future?
There was a problem hiding this comment.
it does work (s3 only) if the client has read-storage access to the resource. we can block it if you would rather we do that though
There was a problem hiding this comment.
ehhh, I feel like we want to restrict client direct access to data until we absolutely need to support it. Least privilege and all that.
| @@ -260,6 +279,7 @@ def check_client_secret(self, client_secret): | |||
|
|
|||
| def check_requested_scopes(self, scopes): | |||
| if "openid" not in scopes: | |||
| logger.error(f"Invalid scopes: 'openid' not in requested scopes ({scopes})") | |||
There was a problem hiding this comment.
This is one of those error lines that I imagine you added after hours of frustration trying to figure out why something wasn't working, lol. Or at least that's what it'd be for me
Jira Ticket: PXP-10113
New Features
Improvements
fence-create client-createnow outputs the whole stacktrace in case of errorDeployment changes
Client.redirect_uriis now optional)