Skip to content

Commit

Permalink
Persist immutable id (a number) rather than mutable one IQSS#3338
Browse files Browse the repository at this point in the history
The persistentuserid field in authenticateduserlookup now contains an
immutable value (i.e. "1938468") rather than a mutable one such as a
username (i.e. "jane_doe"). This is much preferred because most systems
allow you to change usernames (from "jane_doe" to "jane_smith", for
example, sometimes with side effects) in the case of divorce, etc.

Until IQSS#1445 is fixed, we'll persist the mutable value ("jane_doe") in
the useridentifier field in the authenticateduser column because it's
what is (unfortunately) displayed in the UI. This value is also used for
the assigneeidentifier field in the roleassignment table (see IQSS#1151).
  • Loading branch information
pdurbin committed Oct 5, 2016
1 parent f020323 commit e1b9dcf
Show file tree
Hide file tree
Showing 7 changed files with 26 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ public abstract class AbstractOAuth2Idp {
protected static class ParsedUserResponse {
public final AuthenticatedUserDisplayInfo displayInfo;
public final String userIdInProvider;
public final String username;

public ParsedUserResponse(AuthenticatedUserDisplayInfo displayInfo, String userIdInProvider) {
public ParsedUserResponse(AuthenticatedUserDisplayInfo displayInfo, String userIdInProvider, String username) {
this.displayInfo = displayInfo;
this.userIdInProvider = userIdInProvider;
this.username = username;
}

}
Expand Down Expand Up @@ -69,7 +71,7 @@ public OAuth2UserRecord getUserRecord(String code, String state) throws IOExcept
final String body = response.getBody();
if ( responseCode == 200 ) {
ParsedUserResponse parsed = parseUserResponse(body);
return new OAuth2UserRecord(getId(), parsed.userIdInProvider, accessToken.getAccessToken(), parsed.displayInfo);
return new OAuth2UserRecord(getId(), parsed.userIdInProvider, parsed.username, accessToken.getAccessToken(), parsed.displayInfo);
} else {
throw new OAuth2Exception(responseCode, body, "Error getting the user info record.");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ public void exchangeCodeForToken() throws IOException {

if ( dvUser == null ) {
// need to create the user
dvUser = authenticationSvc.createAuthenticatedUser(idtf, oauthUser.getIdInService(), oauthUser.getDisplayInfo(), true);
String proposedAuthenticatedUserIdentifier = oauthUser.getUsername();
dvUser = authenticationSvc.createAuthenticatedUser(idtf, proposedAuthenticatedUserIdentifier, oauthUser.getDisplayInfo(), true);

} else {
// update profile
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,20 @@ public class OAuth2UserRecord {

private final String serviceId;

/** Probably the username. */
/** An immutable value, probably a number. Not a username that may change. */
private final String idInService;

/** A potentially mutable String that is easier on the eye than a number. */
private final String username;

private final String accessToken;

private final AuthenticatedUserDisplayInfo displayInfo;

public OAuth2UserRecord(String aServiceId, String anIdInService, String anAccessToken, AuthenticatedUserDisplayInfo aDisplayInfo) {
public OAuth2UserRecord(String aServiceId, String anIdInService, String aUsername, String anAccessToken, AuthenticatedUserDisplayInfo aDisplayInfo) {
serviceId = aServiceId;
idInService = anIdInService;
username = aUsername;
accessToken = anAccessToken;
displayInfo = aDisplayInfo;
}
Expand All @@ -35,6 +39,10 @@ public String getIdInService() {
return idInService;
}

public String getUsername() {
return username;
}

public String getAccessToken() {
return accessToken;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ protected ParsedUserResponse parseUserResponse( String responseBody ) {
response.getString("company",""),
""
);
return new ParsedUserResponse(displayInfo, response.getString("login"));
Integer immutableUserId = response.getInt("id");
String username = response.getString("login");
return new ParsedUserResponse(displayInfo, immutableUserId.toString(), username);
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ protected ParsedUserResponse parseUserResponse(String responseBody) {
"",
""
);
return new ParsedUserResponse(displayInfo, response.getString("id"));
String immutableUserId = response.getString("id");
String username = null;
return new ParsedUserResponse(displayInfo, immutableUserId, username);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ public BaseApi getApiInstance() {
protected ParsedUserResponse parseUserResponse(String responseBody) {
Logger.getAnonymousLogger().info("ORCiD Response:");
Logger.getAnonymousLogger().info(responseBody);
return new ParsedUserResponse(new AuthenticatedUserDisplayInfo("fn", "ln", "email", "aff", "pos"), "id in ORCiD");
String username = null;
return new ParsedUserResponse(new AuthenticatedUserDisplayInfo("fn", "ln", "email", "aff", "pos"), "id in ORCiD", username);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,13 @@ public class GitHubOAuth2IdpTest extends GitHubOAuth2Idp {
public void testParseUserResponse() {
AbstractOAuth2Idp.ParsedUserResponse expResult = new AbstractOAuth2Idp.ParsedUserResponse(
new AuthenticatedUserDisplayInfo("Jane", "", "jane@janedoe.com", "ACME Sprokets, Inc.", ""),
"1938468",
"jane_doe"
);
AbstractOAuth2Idp.ParsedUserResponse result = parseUserResponse(GITHUB_RESPONSE);

assertEquals(expResult.displayInfo, result.displayInfo);
assertEquals(expResult.userIdInProvider, result.userIdInProvider);
assertEquals("1938468", result.userIdInProvider);


}
Expand Down

0 comments on commit e1b9dcf

Please sign in to comment.