feat(DCF-1446): Assign MFA policy on login #1106
Conversation
| @@ -114,4 +114,4 @@ def post_login(self, user=None, token_result=None, id_from_idp=None): | |||
| user=user, refresh_token=refresh_token, expires=expires + issued_time | |||
| ) | |||
|
|
|||
| super(RASCallback, self).post_login(id_from_idp=id_from_idp) | |||
| super(RASCallback, self).post_login(token_result=token_result) | |||
There was a problem hiding this comment.
This says token_result but the return value from get_user_id isn't the id_token, its just a dict containing the username or email or both depending on the IdP used. I tagged the mfa property into that dict.
| @@ -63,6 +66,7 @@ def __init__( | |||
| username_field="email", | |||
| email_field="email", | |||
| id_from_idp_field="sub", | |||
| app=flask.current_app, | |||
There was a problem hiding this comment.
is there a reason for this vs from flask import current_app at the import level of the module?
There was a problem hiding this comment.
I threw this on here because I found trying to emulate
fence/tests/data/test_indexed_file.py
Line 34 in 291f3e5
k-burt-uch
force-pushed
the
feat/DCF-1446-mfa-arborist-policy-assignment
branch
from
22e9421 to
3cd9608
Compare
fence/blueprints/login/base.py
Outdated
| policy_id="mfa_policy", | ||
| ) | ||
| else: | ||
| logger.info(f"Revoking mfa_policy for {username}") |
There was a problem hiding this comment.
Open to your opinion but should we remove mfa_policy regardless of whether or not is_mfa_enabled is true. Im just thinking about a case where it was turned on and then turned off and the user still has the mfa policy
There was a problem hiding this comment.
Great point - done.
There was a problem hiding this comment.
Revisiting this: I reverted my changes here. I think its better if usersync cleans up the mfa_policy if mfa is turned off rather than during login.
| @@ -42,7 +42,7 @@ def get_user_id(self, code): | |||
| claims = self.get_jwt_claims_identity(token_endpoint, jwks_endpoint, code) | |||
|
|
|||
| if claims.get("sub"): | |||
| return {"sub": claims["sub"]} | |||
| return {"sub": claims["sub"], "mfa": self.has_mfa_claim(claims)} | |||
There was a problem hiding this comment.
Also it took very little time to add this to each identity provider, however given the scope of work on DCF is limited to just RAS, I'm fine with reverting changes for the other idps. They're easy enough to insert as other IdPs are needed.
There was a problem hiding this comment.
I say leave it, might avoid some confusion in the future
fence/blueprints/login/base.py
Outdated
| self.app.arborist.revoke_user_policy( | ||
| username=username, | ||
| policy_id="mfa_policy", | ||
| ) |
There was a problem hiding this comment.
What should we do if these arborist requests fail?
| @@ -63,6 +66,7 @@ def __init__( | |||
| username_field="email", | |||
| email_field="email", | |||
| id_from_idp_field="sub", | |||
| app=flask.current_app, | |||
There was a problem hiding this comment.
I threw this on here because I found trying to emulate
fence/tests/data/test_indexed_file.py
Line 34 in 291f3e5
fence/blueprints/login/base.py
Outdated
| policy_id="mfa_policy", | ||
| ) | ||
| else: | ||
| logger.info(f"Revoking mfa_policy for {username}") |
There was a problem hiding this comment.
Great point - done.
k-burt-uch
force-pushed
the
feat/DCF-1446-mfa-arborist-policy-assignment
branch
4 times, most recently
from
66d2f95 to
da8ba54
Compare
Adds the mfa_policy on login when the configured claim and value are present in the user's id_token.
New Features
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes