-
Notifications
You must be signed in to change notification settings - Fork 9
[ 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
base: develop
Are you sure you want to change the base?
Changes from all commits
e78616e
984fdb2
e23c5dd
5255abf
99f0573
748c8a2
589d52a
099ca8d
4cbf2ec
ef094fd
ac606a5
9dfc0a7
53ed176
ae74a21
935f193
7bbf759
3d9e93e
8d85ad9
2db7814
328704c
90ce1af
58a2aec
c766704
37d2ef6
62d6066
f45fb6e
bd080e6
ec619c6
d43d01e
6768eaf
bba441b
c46b2da
697b8c9
164e3cb
a4a0262
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 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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,19 @@ public void forceResetAllPasswords(final IncomingToken token) | |||||||||||||||
|
||||||||||||||||
private NewToken login(final UserName userName, final TokenCreationContext tokenCtx) | ||||||||||||||||
throws AuthStorageException { | ||||||||||||||||
return login(userName, tokenCtx, MfaStatus.UNKNOWN); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
private NewToken login(final UserName userName, final TokenCreationContext tokenCtx, | ||||||||||||||||
final MfaStatus mfa) throws AuthStorageException { | ||||||||||||||||
Comment on lines
+752
to
+753
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
My rule is either all args on 1 line or each arg on its own line. More readable |
||||||||||||||||
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 +912,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); | ||||||||||||||||
|
@@ -2339,7 +2347,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); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
private Optional<RemoteIdentity> getIdentity( | ||||||||||||||||
|
@@ -3142,6 +3151,7 @@ public long getSuggestedTokenCacheTime() throws AuthStorageException { | |||||||||||||||
return cfg.getAppConfig().getTokenLifetimeMS(TokenLifetimeType.EXT_CACHE); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
/** Get the external configuration without providing any credentials. | ||||||||||||||||
Comment on lines
3152
to
3155
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
|
||||||||||||||||
* | ||||||||||||||||
* This method should not be exposed in a public API. | ||||||||||||||||
|
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. You might want to rework this in a similar way to TokenType so that we can change the enum name without breaking things later: |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
package us.kbase.auth2.lib.identity; | ||
|
||
/** An enumeration representing the multi-factor authentication status of a user's login. */ | ||
public enum MfaStatus { | ||
/** User authenticated with MFA during token creation. */ | ||
USED, | ||
/** User explicitly chose not to use MFA when available. */ | ||
NOT_USED, | ||
/** MFA status unknown or not applicable to authentication method. */ | ||
UNKNOWN; | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,6 +10,7 @@ public class RemoteIdentityDetails { | |||||
private final String username; | ||||||
private final String fullname; | ||||||
private final String email; | ||||||
private final MfaStatus mfa; | ||||||
|
||||||
/** Create a new set of details. | ||||||
* @param username the user name of the identity. | ||||||
|
@@ -20,6 +21,20 @@ public RemoteIdentityDetails( | |||||
final String username, | ||||||
final String fullname, | ||||||
final String email) { | ||||||
this(username, fullname, email, MfaStatus.UNKNOWN); | ||||||
} | ||||||
|
||||||
/** Create a new set of details. | ||||||
* @param username the user name of the identity. | ||||||
* @param fullname the full name of the identity. Null is acceptable. | ||||||
* @param email the email address of the identity. Null is acceptable. | ||||||
* @param mfa the multi-factor authentication status. | ||||||
*/ | ||||||
public RemoteIdentityDetails( | ||||||
final String username, | ||||||
final String fullname, | ||||||
final String email, | ||||||
final MfaStatus mfa) { | ||||||
super(); | ||||||
if (username == null || username.trim().isEmpty()) { | ||||||
throw new IllegalArgumentException( | ||||||
|
@@ -36,6 +51,7 @@ public RemoteIdentityDetails( | |||||
} else { | ||||||
this.email = email.trim(); | ||||||
} | ||||||
this.mfa = mfa; | ||||||
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
|
||||||
} | ||||||
|
||||||
/** Get the user name for the identity. | ||||||
|
@@ -57,13 +73,21 @@ public String getFullname() { | |||||
public String getEmail() { | ||||||
return email; | ||||||
} | ||||||
|
||||||
/** Get the multi-factor authentication status. | ||||||
* @return the MFA status. | ||||||
*/ | ||||||
public MfaStatus getMfa() { | ||||||
return mfa; | ||||||
} | ||||||
|
||||||
@Override | ||||||
public int hashCode() { | ||||||
final int prime = 31; | ||||||
int result = 1; | ||||||
result = prime * result + ((email == null) ? 0 : email.hashCode()); | ||||||
result = prime * result + ((fullname == null) ? 0 : fullname.hashCode()); | ||||||
result = prime * result + ((mfa == null) ? 0 : mfa.name().hashCode()); | ||||||
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
Getting the hash directly from the enum class is fine, it's done this way in other places in the codebase. There's a couple other places in the PR where this should be fixed |
||||||
result = prime * result + ((username == null) ? 0 : username.hashCode()); | ||||||
return result; | ||||||
} | ||||||
|
@@ -94,6 +118,13 @@ public boolean equals(Object obj) { | |||||
} else if (!fullname.equals(other.fullname)) { | ||||||
return false; | ||||||
} | ||||||
if (mfa == null) { | ||||||
if (other.mfa != null) { | ||||||
return false; | ||||||
} | ||||||
} else if (!mfa.equals(other.mfa)) { | ||||||
return false; | ||||||
} | ||||||
if (username == null) { | ||||||
if (other.username != null) { | ||||||
return false; | ||||||
|
@@ -113,6 +144,8 @@ public String toString() { | |||||
builder.append(fullname); | ||||||
builder.append(", email="); | ||||||
builder.append(email); | ||||||
builder.append(", mfa="); | ||||||
builder.append(mfa); | ||||||
builder.append("]"); | ||||||
return builder.toString(); | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -90,6 +90,7 @@ | |||||||||||
import us.kbase.auth2.lib.exceptions.UserExistsException; | ||||||||||||
import us.kbase.auth2.lib.identity.RemoteIdentity; | ||||||||||||
import us.kbase.auth2.lib.identity.RemoteIdentityDetails; | ||||||||||||
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; | ||||||||||||
|
@@ -568,6 +569,10 @@ private DisplayName getDisplayName(final String displayName) throws AuthStorageE | |||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
private MfaStatus getMfaStatus(final String mfaString) { | ||||||||||||
return mfaString != null ? MfaStatus.valueOf(mfaString) : MfaStatus.UNKNOWN; | ||||||||||||
Comment on lines
+572
to
+573
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
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. If you haven't already, can you add tests for the case where there are remoteidentity and token documents without mfa fields? Should just be unit tests on this class |
||||||||||||
} | ||||||||||||
|
||||||||||||
private EmailAddress getEmail(final String email) throws AuthStorageException { | ||||||||||||
if (email == null) { | ||||||||||||
return EmailAddress.UNKNOWN; | ||||||||||||
|
@@ -871,7 +876,8 @@ private void storeToken(final String collection, final StoredToken token, final | |||||||||||
.append(Fields.TOKEN_DEVICE, ctx.getDevice().orElse(null)) | ||||||||||||
.append(Fields.TOKEN_IP, ctx.getIpAddress().isPresent() ? | ||||||||||||
ctx.getIpAddress().get().getHostAddress() : null) | ||||||||||||
.append(Fields.TOKEN_CUSTOM_CONTEXT, toCustomContextList(ctx.getCustomContext())); | ||||||||||||
.append(Fields.TOKEN_CUSTOM_CONTEXT, toCustomContextList(ctx.getCustomContext())) | ||||||||||||
.append(Fields.TOKEN_MFA, token.getMfa().name()); | ||||||||||||
try { | ||||||||||||
db.getCollection(collection).insertOne(td); | ||||||||||||
} catch (MongoWriteException mwe) { | ||||||||||||
|
@@ -969,6 +975,7 @@ private StoredToken getToken(final Document t) throws AuthStorageException { | |||||||||||
t.getDate(Fields.TOKEN_EXPIRY).toInstant()) | ||||||||||||
.withNullableTokenName(getTokenName(t.getString(Fields.TOKEN_NAME))) | ||||||||||||
.withContext(toTokenCreationContext(t)) | ||||||||||||
.withMfa(getMfaStatus(t.getString(Fields.TOKEN_MFA))) | ||||||||||||
.build(); | ||||||||||||
} | ||||||||||||
|
||||||||||||
|
@@ -1596,7 +1603,8 @@ private void updateIdentity(final RemoteIdentity remoteID) | |||||||||||
final Document update = new Document("$set", | ||||||||||||
new Document(pre + Fields.IDENTITIES_USER, rid.getUsername()) | ||||||||||||
.append(pre + Fields.IDENTITIES_EMAIL, rid.getEmail()) | ||||||||||||
.append(pre + Fields.IDENTITIES_NAME, rid.getFullname())); | ||||||||||||
.append(pre + Fields.IDENTITIES_NAME, rid.getFullname()) | ||||||||||||
.append(pre + Fields.IDENTITIES_MFA, rid.getMfa().name())); | ||||||||||||
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. This is updating the user document, which I don't think you want to do |
||||||||||||
try { | ||||||||||||
// id might have been unlinked, so we just assume | ||||||||||||
// the update worked. If it was just unlinked we don't care. | ||||||||||||
|
@@ -1729,7 +1737,8 @@ private Document toDocument(final RemoteIdentity id) { | |||||||||||
.append(Fields.IDENTITIES_PROV_ID, id.getRemoteID().getProviderIdentityId()) | ||||||||||||
.append(Fields.IDENTITIES_USER, rid.getUsername()) | ||||||||||||
.append(Fields.IDENTITIES_NAME, rid.getFullname()) | ||||||||||||
.append(Fields.IDENTITIES_EMAIL, rid.getEmail()); | ||||||||||||
.append(Fields.IDENTITIES_EMAIL, rid.getEmail()) | ||||||||||||
.append(Fields.IDENTITIES_MFA, rid.getMfa().name()); | ||||||||||||
} | ||||||||||||
|
||||||||||||
@Override | ||||||||||||
|
@@ -1789,7 +1798,8 @@ private Set<RemoteIdentity> toIdentities(final List<Document> ids) { | |||||||||||
final RemoteIdentityDetails det = new RemoteIdentityDetails( | ||||||||||||
i.getString(Fields.IDENTITIES_USER), | ||||||||||||
i.getString(Fields.IDENTITIES_NAME), | ||||||||||||
i.getString(Fields.IDENTITIES_EMAIL)); | ||||||||||||
i.getString(Fields.IDENTITIES_EMAIL), | ||||||||||||
getMfaStatus(i.getString(Fields.IDENTITIES_MFA))); | ||||||||||||
ret.add(new RemoteIdentity(rid, det)); | ||||||||||||
} | ||||||||||||
return ret; | ||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,6 +8,7 @@ | |||||
|
||||||
import us.kbase.auth2.lib.TokenCreationContext; | ||||||
import us.kbase.auth2.lib.UserName; | ||||||
import us.kbase.auth2.lib.identity.MfaStatus; | ||||||
|
||||||
/** A token associated with a user stored in the authentication storage system. | ||||||
* | ||||||
|
@@ -23,6 +24,7 @@ public class StoredToken { | |||||
private final UserName userName; | ||||||
private final Instant creationDate; | ||||||
private final Instant expirationDate; | ||||||
private final MfaStatus mfa; | ||||||
|
||||||
private StoredToken( | ||||||
final UUID id, | ||||||
|
@@ -31,7 +33,8 @@ private StoredToken( | |||||
final UserName userName, | ||||||
final TokenCreationContext context, | ||||||
final Instant creationDate, | ||||||
final Instant expirationDate) { | ||||||
final Instant expirationDate, | ||||||
final MfaStatus mfa) { | ||||||
// this stuff is here just in case naughty users use casting to skip a builder step | ||||||
requireNonNull(creationDate, "created"); | ||||||
// no way to test this one | ||||||
|
@@ -43,6 +46,7 @@ private StoredToken( | |||||
this.expirationDate = expirationDate; | ||||||
this.creationDate = creationDate; | ||||||
this.id = id; | ||||||
this.mfa = mfa; | ||||||
} | ||||||
|
||||||
/** Get the type of the token. | ||||||
|
@@ -93,6 +97,13 @@ public Instant getCreationDate() { | |||||
public Instant getExpirationDate() { | ||||||
return expirationDate; | ||||||
} | ||||||
|
||||||
/** Get the multi-factor authentication status for this token. | ||||||
* @return the MFA status. | ||||||
*/ | ||||||
public MfaStatus getMfa() { | ||||||
return mfa; | ||||||
} | ||||||
|
||||||
@Override | ||||||
public int hashCode() { | ||||||
|
@@ -105,6 +116,7 @@ public int hashCode() { | |||||
result = prime * result + ((tokenName == null) ? 0 : tokenName.hashCode()); | ||||||
result = prime * result + ((type == null) ? 0 : type.hashCode()); | ||||||
result = prime * result + ((userName == null) ? 0 : userName.hashCode()); | ||||||
result = prime * result + ((mfa == null) ? 0 : mfa.name().hashCode()); | ||||||
return result; | ||||||
} | ||||||
|
||||||
|
@@ -165,6 +177,13 @@ public boolean equals(Object obj) { | |||||
} else if (!userName.equals(other.userName)) { | ||||||
return false; | ||||||
} | ||||||
if (mfa == null) { | ||||||
if (other.mfa != null) { | ||||||
return false; | ||||||
} | ||||||
} else if (!mfa.equals(other.mfa)) { | ||||||
return false; | ||||||
} | ||||||
return true; | ||||||
} | ||||||
|
||||||
|
@@ -224,6 +243,12 @@ public interface OptionalsStep { | |||||
*/ | ||||||
OptionalsStep withContext(TokenCreationContext context); | ||||||
|
||||||
/** Specify whether the token was created with multi-factor authentication. | ||||||
* @param mfa the MFA status. | ||||||
* @return this builder. | ||||||
*/ | ||||||
OptionalsStep withMfa(MfaStatus mfa); | ||||||
|
||||||
/** Build the token. | ||||||
* @return a new StoredToken. | ||||||
*/ | ||||||
|
@@ -239,6 +264,7 @@ private static class Builder implements LifeStep, OptionalsStep { | |||||
private final UserName userName; | ||||||
private Instant creationDate; | ||||||
private Instant expirationDate; | ||||||
private MfaStatus mfa = MfaStatus.UNKNOWN; | ||||||
|
||||||
private Builder(final TokenType type, final UUID id, final UserName userName) { | ||||||
requireNonNull(type, "type"); | ||||||
|
@@ -269,10 +295,16 @@ public OptionalsStep withContext(final TokenCreationContext context) { | |||||
return this; | ||||||
} | ||||||
|
||||||
@Override | ||||||
public OptionalsStep withMfa(final MfaStatus mfa) { | ||||||
this.mfa = mfa; | ||||||
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
|
||||||
return this; | ||||||
} | ||||||
|
||||||
@Override | ||||||
public StoredToken build() { | ||||||
return new StoredToken(id, type, tokenName, userName, context, | ||||||
creationDate, expirationDate); | ||||||
creationDate, expirationDate, mfa); | ||||||
} | ||||||
|
||||||
@Override | ||||||
|
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.
According to codecov there's still some uncovered code, which should be coverable since the orcid interactions are mocked and everything else can be unit tested