Repository navigation
[ CDM-243 ] [ CDM-245 ] Orcid Provider MFA Support & Token response mfa key
#471
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
Changes from all commits
ee7a514
5991d79
8197a61
92bcfa3
7c6a378
a4c2a7b
2540fd3
264be6c
f0b1612
4588bcf
2c1e074
234e80f
4d277a4
ac4a7fe
219d328
f84cb2b
00d3ca1
9a8415a
f5ca03f
4b67b4e
d45605e
7702956
fd763ef
4ce5166
53b2d65
bddba2c
7074ff6
a9091bc
c514203
540ff32
0b2cdc1
c919468
5043d4b
fc9d0ee
d266fac
3857ac6
c8ff9d9
c8b9689
df6b15e
c316f15
facd37c
747c4a4
565a520
21ad651
2d45b4f
a57a3ba
caf6894
8a8d346
451e9f6
9a6d66b
9001081
e4bbbd6
d219699
6495675
9d4c137
010b964
b55cc36
cc77ba4
980961b
a210941
0e99fd7
b27150d
d921886
0c94f7d
37f1807
48c61e5
6814a61
2291fcc
8c59a94
6398a65
959b647
1117f60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
MrCreosote marked this conversation as resolved.
Resolved
Member
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. There don't appear to be unit tests for any of the changes in this file |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -77,6 +77,7 @@ | |||||||||
| import us.kbase.auth2.lib.exceptions.UserExistsException; | ||||||||||
| import us.kbase.auth2.lib.identity.IdentityProvider; | ||||||||||
| import us.kbase.auth2.lib.identity.RemoteIdentity; | ||||||||||
| import us.kbase.auth2.lib.identity.MfaStatus; | ||||||||||
| import us.kbase.auth2.lib.identity.RemoteIdentityID; | ||||||||||
| import us.kbase.auth2.lib.storage.AuthStorage; | ||||||||||
| import us.kbase.auth2.lib.storage.exceptions.AuthStorageException; | ||||||||||
|
|
@@ -745,13 +746,22 @@ public void forceResetAllPasswords(final IncomingToken token) | |||||||||
|
|
||||||||||
| private NewToken login(final UserName userName, final TokenCreationContext tokenCtx) | ||||||||||
| throws AuthStorageException { | ||||||||||
| return login(userName, tokenCtx, MfaStatus.UNKNOWN); | ||||||||||
| } | ||||||||||
|
Comment on lines
747
to
+750
Member
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.
Suggested change
Let's delete this method and make it explicit in the calling methods. Looking at the code, I think the createUser method needs to be updated to pass mfa status as well |
||||||||||
|
|
||||||||||
| private NewToken login( | ||||||||||
| final UserName userName, | ||||||||||
| final TokenCreationContext tokenCtx, | ||||||||||
| final MfaStatus mfa | ||||||||||
| ) throws AuthStorageException { | ||||||||||
| final NewToken nt = new NewToken(StoredToken.getBuilder( | ||||||||||
| TokenType.LOGIN, randGen.randomUUID(), userName) | ||||||||||
| .withLifeTime(clock.instant(), | ||||||||||
| cfg.getAppConfig().getTokenLifetimeMS(TokenLifetimeType.LOGIN)) | ||||||||||
| .withContext(tokenCtx) | ||||||||||
| .build(), | ||||||||||
| randGen.getToken()); | ||||||||||
| TokenType.LOGIN, randGen.randomUUID(), userName) | ||||||||||
| .withLifeTime(clock.instant(), | ||||||||||
| cfg.getAppConfig().getTokenLifetimeMS(TokenLifetimeType.LOGIN)) | ||||||||||
| .withContext(tokenCtx) | ||||||||||
| .withMfa(mfa) | ||||||||||
| .build(), | ||||||||||
| randGen.getToken()); | ||||||||||
| storage.storeToken(nt.getStoredToken(), nt.getTokenHash()); | ||||||||||
| setLastLogin(userName); | ||||||||||
| logInfo("Logged in user {} with token {}", | ||||||||||
|
|
@@ -905,7 +915,8 @@ public NewToken createToken( | |||||||||
| final NewToken nt = new NewToken(StoredToken.getBuilder(tokenType, id, au.getUserName()) | ||||||||||
| .withLifeTime(clock.instant(), life) | ||||||||||
| .withContext(tokenCtx) | ||||||||||
| .withTokenName(tokenName).build(), | ||||||||||
| .withTokenName(tokenName) | ||||||||||
| .build(), | ||||||||||
| randGen.getToken()); | ||||||||||
| storage.storeToken(nt.getStoredToken(), nt.getTokenHash()); | ||||||||||
| logInfo("User {} created {} token {}", au.getUserName().getName(), tokenType, id); | ||||||||||
|
|
@@ -1789,6 +1800,7 @@ public LoginToken login( // enough args here to start considering a builder | |||||||||
| storage.deleteTemporarySessionData(token.getHashedToken()); | ||||||||||
| final Set<RemoteIdentity> ris = idp.getIdentities( | ||||||||||
| authcode, tids.getPKCECodeVerifier().get(), false, environment); | ||||||||||
| final MfaStatus mfa = ris.iterator().next().getDetails().getMfa(); | ||||||||||
| final LoginState lstate = getLoginState(ris, Instant.MIN); | ||||||||||
| final ProviderConfig pc = cfg.getAppConfig().getProviderConfig(idp.getProviderName()); | ||||||||||
| final LoginToken loginToken; | ||||||||||
|
|
@@ -1801,23 +1813,23 @@ public LoginToken login( // enough args here to start considering a builder | |||||||||
| * this call may be the result of a redirect from a 3rd party | ||||||||||
| * provider. Any controllable error should be thrown when the process flow is back | ||||||||||
| * under the control of the primary auth UI. | ||||||||||
| * | ||||||||||
| * | ||||||||||
| * There's a tiny chance here that the user could be made an admin, be enabled, or | ||||||||||
| * non-admin login be enabled between this step and the getLoginState() step. The | ||||||||||
| * consequence is that they'll have to choose from one account in the next step, | ||||||||||
| * so who cares. | ||||||||||
| */ | ||||||||||
| if (!cfg.getAppConfig().isLoginAllowed() && !Role.isAdmin(user.getRoles())) { | ||||||||||
| loginToken = storeIdentitiesTemporarily(lstate); | ||||||||||
| loginToken = storeIdentitiesTemporarily(lstate, mfa); | ||||||||||
| } else if (user.isDisabled()) { | ||||||||||
| loginToken = storeIdentitiesTemporarily(lstate); | ||||||||||
| loginToken = storeIdentitiesTemporarily(lstate, mfa); | ||||||||||
| } else { | ||||||||||
| loginToken = new LoginToken(login(user.getUserName(), tokenCtx)); | ||||||||||
| loginToken = new LoginToken(login(user.getUserName(), tokenCtx, mfa)); | ||||||||||
| } | ||||||||||
| } else { | ||||||||||
| // store the identities so the user can create an account or choose from more than one | ||||||||||
| // account | ||||||||||
| loginToken = storeIdentitiesTemporarily(lstate); | ||||||||||
| loginToken = storeIdentitiesTemporarily(lstate, mfa); | ||||||||||
| } | ||||||||||
| return loginToken; | ||||||||||
| } | ||||||||||
|
|
@@ -1831,13 +1843,13 @@ private void checkState(final TemporarySessionData tids, final String state) | |||||||||
| } | ||||||||||
|
|
||||||||||
| // ignores expiration date of login state | ||||||||||
| private LoginToken storeIdentitiesTemporarily(final LoginState ls) | ||||||||||
| private LoginToken storeIdentitiesTemporarily(final LoginState ls, final MfaStatus mfa) | ||||||||||
| throws AuthStorageException { | ||||||||||
| final Set<RemoteIdentity> store = new HashSet<>(ls.getIdentities()); | ||||||||||
| ls.getUsers().stream().forEach(u -> store.addAll(ls.getIdentities(u))); | ||||||||||
| final TemporarySessionData data = TemporarySessionData.create( | ||||||||||
| randGen.randomUUID(), clock.instant(), LOGIN_TOKEN_LIFETIME_MS) | ||||||||||
| .login(store); | ||||||||||
| .login(store, mfa); | ||||||||||
| final TemporaryToken tt = storeTemporarySessionData(data); | ||||||||||
| logInfo("Stored temporary token {} with {} login identities", tt.getId(), store.size()); | ||||||||||
| return new LoginToken(tt); | ||||||||||
|
|
@@ -2033,16 +2045,19 @@ public NewToken createUser( | |||||||||
| public NewToken testModeCreateToken( | ||||||||||
| final UserName userName, | ||||||||||
| final TokenName tokenName, | ||||||||||
| final TokenType tokenType) | ||||||||||
| final TokenType tokenType, | ||||||||||
| final MfaStatus mfa) | ||||||||||
| throws TestModeException, AuthStorageException, NoSuchUserException { | ||||||||||
| ensureTestMode(); | ||||||||||
| requireNonNull(userName, "userName"); | ||||||||||
| requireNonNull(tokenType, "tokenType"); | ||||||||||
| requireNonNull(mfa, "mfa"); | ||||||||||
| storage.testModeGetUser(userName); // ensure user exists | ||||||||||
| final UUID id = randGen.randomUUID(); | ||||||||||
| final NewToken nt = new NewToken(StoredToken.getBuilder(tokenType, id, userName) | ||||||||||
| .withLifeTime(clock.instant(), TEST_MODE_DATA_LIFETIME_MS) | ||||||||||
| .withNullableTokenName(tokenName) | ||||||||||
| .withMfa(mfa) | ||||||||||
| .build(), | ||||||||||
| randGen.getToken()); | ||||||||||
| storage.testModeStoreToken(nt.getStoredToken(), nt.getTokenHash()); | ||||||||||
|
|
@@ -2339,7 +2354,8 @@ public NewToken login( | |||||||||
| linked, u.get().getUserName().getName()); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| return login(u.get().getUserName(), tokenCtx); | ||||||||||
| final MfaStatus mfaStatus = ri.get().getDetails().getMfa(); | ||||||||||
| return login(u.get().getUserName(), tokenCtx, mfaStatus); | ||||||||||
|
dauglyon marked this conversation as resolved.
Resolved
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| private Optional<RemoteIdentity> getIdentity( | ||||||||||
|
|
@@ -3136,7 +3152,7 @@ public <T extends ExternalConfig> AuthConfigSetWithUpdateTime<T> getConfig( | |||||||||
|
|
||||||||||
| /** Returns the suggested cache time for tokens in milliseconds. | ||||||||||
| * @return the suggested cache time. | ||||||||||
| * @throws AuthStorageException if an error occurred accessing the storage system. | ||||||||||
| * @throws AuthStorageException if an error occurred accessing the storage system. | ||||||||||
| */ | ||||||||||
| public long getSuggestedTokenCacheTime() throws AuthStorageException { | ||||||||||
| return cfg.getAppConfig().getTokenLifetimeMS(TokenLifetimeType.EXT_CACHE); | ||||||||||
|
|
||||||||||
|
MrCreosote marked this conversation as resolved.
Resolved
Member
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. The admintoken, adminusertokens, tokencreate, and tokens mustache templates need to be updated to show MFA status, which probably means that their underlying APIs need to be updated as well I was pulled off auth before I could add tests for the admin endpoints / templates so they can be tested manually |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| package us.kbase.auth2.lib.identity; | ||
| // TODO CODE move to lib/token | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonValue; | ||
|
|
||
| /** An enumeration representing the multi-factor authentication status of a user's login. | ||
| * @author dlyon@lbl.gov | ||
| * | ||
| */ | ||
| public enum MfaStatus { | ||
|
|
||
| /* first arg is ID, second arg is description. ID CANNOT change | ||
| * since that field is stored in the DB. | ||
| */ | ||
| /** User authenticated with MFA during token creation. */ | ||
| USED ("Used", "MFA used"), | ||
| /** User explicitly chose not to use MFA when available. */ | ||
| NOT_USED ("NotUsed", "MFA not used"), | ||
| /** MFA status unknown or not applicable to authentication method. */ | ||
| UNKNOWN ("Unknown", "MFA status unknown"); | ||
|
|
||
| private static final Map<String, MfaStatus> STATUS_MAP = new HashMap<>(); | ||
| static { | ||
| for (final MfaStatus status: MfaStatus.values()) { | ||
| STATUS_MAP.put(status.getID(), status); | ||
| } | ||
| } | ||
|
|
||
| private final String id; | ||
| private final String description; | ||
|
|
||
| private MfaStatus(final String id, final String description) { | ||
| this.id = id; | ||
| this.description = description; | ||
| } | ||
|
|
||
| /** Get the ID of this MFA status. | ||
| * @return the ID. | ||
| */ | ||
| @JsonValue | ||
| public String getID() { | ||
| return id; | ||
| } | ||
|
|
||
| /** Get the description of this MFA status. | ||
| * @return the description. | ||
| */ | ||
| public String getDescription() { | ||
| return description; | ||
| } | ||
|
|
||
| /** Get an MFA status from its ID. | ||
| * @param id the ID of the MFA status. | ||
| * @return the MFA status. | ||
| * @throws IllegalArgumentException if there is no MFA status matching the ID. | ||
| */ | ||
| public static MfaStatus fromID(final String id) { | ||
| if (!STATUS_MAP.containsKey(id)) { | ||
| throw new IllegalArgumentException("Invalid MFA status: " + id); | ||
| } | ||
| return STATUS_MAP.get(id); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.