Automated copy feature/OIDC oauth groups rev2 #1213
Conversation
remove trailing space Co-authored-by: Alexander VanTol <Avantol13@users.noreply.github.com>
...to make user creation more uniform, reflecting what is also done elsewhere in fence/sync/sync_users.py _upsert_userinfo() for example
* fix use real bucket name * fix * update comment * use regex * update version
Update documentation link in setup.md
…ustralianBioCommons/fence into automatedCopy-feature/oidc-oauth-groups-rev2
Pull Request Test Coverage Report for Build 12750495859Details
💛 - Coveralls |
|
Please find the ci env pod logs here |
| ) | ||
|
|
||
| self.post_login( | ||
| user=flask.g.user, |
There was a problem hiding this comment.
I think this will error if flask.g.user doesn't exist (like you're checking for in getattr(flask.g, "user", None). Either you need to raise an exception afterthe logger.error, or you need to make sure this code block doesn't get hit when .user doesn't exist
| # If all methods fail, return None | ||
| return None | ||
|
|
||
| def introspect_token(self, token): |
There was a problem hiding this comment.
We don't support token introspection elsewhere already and I'd rather not add this in, since it will likely require a security review (since this is trusting another systems endpoint to return information to us outside of the already approved content of the token itself). Unless you absolutely need this method, I recommend removing and relying on the exp in the token content itself
| @@ -142,6 +314,13 @@ def post_login(self, user=None, token_result=None, **kwargs): | |||
| client_id=flask.session.get("client_id"), | |||
| ) | |||
|
|
|||
| # this attribute is only applicable to some OAuth clients | |||
| # (e.g., not all clients need is_read_authz_groups_from_tokens_enabled) | |||
There was a problem hiding this comment.
I'm not sure I understand this clarifying comment. Isn't read_authz_groups_from_tokens just a boolean on every client?
| # to do this, only include error messages for known http status codes | ||
| # that are less that 500 | ||
| # Decide whether to re-raise errors or handle gracefully based on the debug flag | ||
| debug_mode = config.get("DEBUG", False) |
There was a problem hiding this comment.
You don't need to safely check for top level config. It is guaranteed to be there and be the default value if it wasn't overwritten. So you can just do: if config["DEBUG"]:
| # Decide whether to re-raise errors or handle gracefully based on the debug flag | ||
| debug_mode = config.get("DEBUG", False) | ||
|
|
||
| if debug_mode: |
There was a problem hiding this comment.
I don't think we want this, b/c in a deployed runtime setting where we are temporarily turning this configuration on to get more detailed logs into our logging system, this is altering the actual error handling behavior so we wouldn't see the same flow. I propose removing this and leaving it the way it was before so that regardless of the DEBUG flag, the flow of the exceptions is the same
| @@ -72,18 +96,69 @@ def get_jwt_keys(self, jwks_uri): | |||
| return None | |||
| return resp.json()["keys"] | |||
|
|
|||
| def decode_and_validate_token(self, token_id, keys, audience, verify_aud=True): | |||
There was a problem hiding this comment.
I feel the name token_id is misleading since it's the entire token. I would suggest simply token
There was a problem hiding this comment.
Also, we already have a validation function that takes into account denylisted tokens. Was there some issue with using the existing code? fence.jwt.validate.validate_jwt
| ) | ||
| refresh_token = token_response["refresh_token"] | ||
| # Fetching the expires at from token_response. | ||
| # Defaulting to 1 hour if not available. |
There was a problem hiding this comment.
This should part of the configuration, not hard-coded here
There was a problem hiding this comment.
We also need an info log for audit trail, that a token was persisted for x user for y amount of time
There was a problem hiding this comment.
unless the store_refresh_token function already handles that
There was a problem hiding this comment.
handled at store_refresh_token functio
| a. Retrieves the list of groups from Arborist. | ||
| b. Retrieves the user's groups from the IdP. | ||
| c. Adds the user to groups in Arborist that match the groups from the IdP. | ||
| d. Removes the user from groups in Arborist that they are no longer part of in the IdP. |
There was a problem hiding this comment.
step d. is the problematic one. If we simply rely on Arborist to expire policies, then we could avoid the comparison here (and the issue where there are multiple sources of truth for group membership)
There was a problem hiding this comment.
It looks like the actual logic for this got removed 👍 , so I think we just need to update this comment
|
|
||
| idp_group_names = set(authz_groups_from_idp) | ||
|
|
||
| # Expiration for group membership. Default 7 days |
There was a problem hiding this comment.
we must make sure that this duration does not exceed the refresh token expiration. B/c if it does, we're breaking policy and extending beyond the source of truth's maximum allowed validity of the information and approval of the user for us to act on their behalf
Link to JIRA ticket if there is one:
New Features
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes