From 01b38909eabe6aa4dc8fa62e0a70c3aa15d570cc Mon Sep 17 00:00:00 2001 From: Bharath Sekar Date: Mon, 6 Jul 2020 01:48:15 -0700 Subject: [PATCH] PR refactoring --- .../buildServer/auth/oauth/OAuthUser.java | 39 ++++++++++++------- .../auth/oauth/ServerPrincipalFactory.java | 38 +++++++----------- .../OAuthAuthenticationSchemeTest.groovy | 8 ++-- .../auth/oauth/OAuthClientTest.groovy | 14 ++++++- .../auth/oauth/OAuthUserTest.groovy | 11 +++--- .../oauth/ServerPrincipalFactoryTest.groovy | 16 ++++---- src/test/resources/user/azure.json | 3 +- src/test/resources/user/bitbucket.json | 3 +- src/test/resources/user/github.json | 3 +- src/test/resources/user/google.json | 3 +- src/test/resources/user/okta.json | 21 ++++++++++ 11 files changed, 94 insertions(+), 65 deletions(-) create mode 100644 src/test/resources/user/okta.json diff --git a/src/main/java/jetbrains/buildServer/auth/oauth/OAuthUser.java b/src/main/java/jetbrains/buildServer/auth/oauth/OAuthUser.java index 220f7f4..1cde4f0 100644 --- a/src/main/java/jetbrains/buildServer/auth/oauth/OAuthUser.java +++ b/src/main/java/jetbrains/buildServer/auth/oauth/OAuthUser.java @@ -2,7 +2,13 @@ import org.json.simple.JSONArray; -import java.util.*; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; public class OAuthUser { private static final String[] IDS_LIST = new String[]{"login", "username", "id", "preferred_username"}; @@ -13,13 +19,13 @@ public class OAuthUser { private final String id; private final String name; private final String email; - private final List groups; + private final Set groups; public OAuthUser(String id) { this(id, null, null, null); } - public OAuthUser(String id, String name, String email, List groups) { + public OAuthUser(String id, String name, String email, Set groups) { this.id = id; this.name = name; this.email = email; @@ -30,8 +36,7 @@ public OAuthUser(Map userData) { this.id = getValueByKeys(userData, IDS_LIST); this.name = getValueByKeys(userData, NAMES_LIST); this.email = getValueByKeys(userData, EMAIL_LIST); - this.groups = getGroups(userData, GROUPS_KEY) == null ? new ArrayList<>() : Arrays.asList(getGroups(userData, - GROUPS_KEY)); + this.groups = parseGroups(userData, GROUPS_KEY); } private String getValueByKeys(Map userData, String[] keys) { @@ -48,13 +53,13 @@ private String getValueByKeys(Map userData, String[] keys) { return value; } - private String[] getGroups(Map userData, String key) { + private Set parseGroups(Map userData, String key) { if (userData == null) { - return null; + return new HashSet<>(); } Object groupsObject = userData.get(key); if (groupsObject == null) { - return null; + return new HashSet<>(); } JSONArray groupsJsonArray = (JSONArray) groupsObject; int groupSize = groupsJsonArray.size(); @@ -62,7 +67,7 @@ private String[] getGroups(Map userData, String key) { for (int i = 0; i < groupSize; i++) { groupsArray[i] = (String) groupsJsonArray.get(i); } - return groupsArray; + return new HashSet(Arrays.asList(groupsArray)); } public String getId() { @@ -77,6 +82,10 @@ public String getEmail() { return email; } + public Set getGroups() { + return groups; + } + public void validate(AuthenticationSchemeProperties properties) throws Exception { if (this.getId() == null) { throw new Exception("Unauthenticated since user endpoint does not return any login id"); @@ -100,9 +109,12 @@ public void validate(AuthenticationSchemeProperties properties) throws Exception @Override public String toString() { + String groupsText = (getGroups() != null && getGroups().size() >0) ? + ", groups='" + getGroups().toString() + '\'' : ""; return "OAuthUser{" + "id='" + getId() + '\'' + ", name='" + getName() + '\'' + ", email='" + getEmail() + '\'' + + groupsText + '}'; } @@ -117,15 +129,12 @@ public boolean equals(Object o) { OAuthUser oAuthUser = (OAuthUser) o; return Objects.equals(id, oAuthUser.id) && Objects.equals(name, oAuthUser.name) && - Objects.equals(email, oAuthUser.email); + Objects.equals(email, oAuthUser.email) && + Objects.equals(groups, oAuthUser.groups); } @Override public int hashCode() { - return Objects.hash(id, name, email); - } - - public List getGroups() { - return groups; + return Objects.hash(id, name, email, groups); } } diff --git a/src/main/java/jetbrains/buildServer/auth/oauth/ServerPrincipalFactory.java b/src/main/java/jetbrains/buildServer/auth/oauth/ServerPrincipalFactory.java index 2b9cd79..0f191b9 100644 --- a/src/main/java/jetbrains/buildServer/auth/oauth/ServerPrincipalFactory.java +++ b/src/main/java/jetbrains/buildServer/auth/oauth/ServerPrincipalFactory.java @@ -38,19 +38,20 @@ public Optional getServerPrincipal(@NotNull final OAuthUser use boolean allowCreatingNewUsersByLogin) { Optional existingUserOptional = findExistingUser(user.getId()); boolean syncGroups = properties.isSyncGroups(); - List groupsInToken = user.getGroups(); + Set groupsInToken = user.getGroups(); if (existingUserOptional.isPresent()) { LOG.info("Use existing user: " + user.getId()); SUser existingUser = existingUserOptional.get(); if (syncGroups) { //user group or all user groups? - List existingUserGroups = + Set existingUserGroups = existingUser.getUserGroups().stream().map(userGroup -> userGroup.getName()) - .collect(Collectors.toList()); - List additionalGroupsInToken = obtainGroupsDelta(groupsInToken, existingUserGroups); + .collect(Collectors.toSet()); + Set additionalGroupsInToken = new HashSet<>(groupsInToken); + additionalGroupsInToken.removeAll(existingUserGroups); addUserToGroups(existingUser, applyGroupWhitelist(additionalGroupsInToken)); - - List groupsNotPresentInToken = obtainGroupsDelta(existingUserGroups, groupsInToken); + Set groupsNotPresentInToken = new HashSet<>(existingUserGroups); + groupsNotPresentInToken.removeAll(groupsInToken); removeUserFromGroups(existingUser, applyGroupWhitelist(groupsNotPresentInToken)); } return Optional.of(new ServerPrincipal(PluginConstants.OAUTH_AUTH_SCHEME_NAME, existingUser.getUsername())); @@ -69,7 +70,7 @@ public Optional getServerPrincipal(@NotNull final OAuthUser use } } - private void removeUserFromGroups(SUser existingUser, List groupsToDelete) { + private void removeUserFromGroups(SUser existingUser, Set groupsToDelete) { for (String group : groupsToDelete) { SUserGroup userGroup = userGroupManager.findUserGroupByName(group); if (userGroup != null) { @@ -78,24 +79,24 @@ private void removeUserFromGroups(SUser existingUser, List groupsToDelet } } - private List applyGroupWhitelist(List groups) { - List finalGroupList = new ArrayList<>(); + private Set applyGroupWhitelist(Set groups) { + Set finalGroupSet = new HashSet<>(); Set whitelistedGroups = properties.getWhitelistedGroups(); if (whitelistedGroups != null) { for (String group : groups) { for (String whitelistedGroup : whitelistedGroups) { if (group.startsWith(whitelistedGroup)) { - finalGroupList.add(group); + finalGroupSet.add(group); break; } } } } - return finalGroupList; + return finalGroupSet; } - private void addUserToGroups(SUser created, List finalGroupList) { - for (String group : finalGroupList) { + private void addUserToGroups(SUser created, Set finalGroupSet) { + for (String group : finalGroupSet) { SUserGroup userGroup = userGroupManager.findUserGroupByName(group); if (userGroup != null) { userGroup.addUser(created); @@ -113,15 +114,4 @@ private Optional findExistingUser(@NotNull final String userName) { return Optional.empty(); } } - - private List obtainGroupsDelta(List minuend, List subtrahend) { - return minuend.stream() - .filter(filterOutEqualValuesCaseInsensitive(subtrahend)) - .collect(Collectors.toList()); - } - - private Predicate filterOutEqualValuesCaseInsensitive(Collection valuesToExclude) { - return valueUnderTest -> valuesToExclude.stream().map(String::toLowerCase) - .noneMatch(s -> s.equals(valueUnderTest.toLowerCase())); - } } diff --git a/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthAuthenticationSchemeTest.groovy b/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthAuthenticationSchemeTest.groovy index d2f2e4f..505186e 100644 --- a/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthAuthenticationSchemeTest.groovy +++ b/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthAuthenticationSchemeTest.groovy @@ -136,7 +136,7 @@ class OAuthAuthenticationSchemeTest extends Specification { getRequestedSessionId() >> "state" } client.getAccessToken("code") >> "token" - client.getUserData("token") >> new OAuthUser("testUser", "Test User", "test@example.com", Arrays.asList("dev-")) + client.getUserData("token") >> new OAuthUser("testUser", "Test User", "test@example.com", null) properties.getEmailDomains() >> Arrays.asList("example.com") when: HttpAuthenticationResult result = scheme.processAuthenticationRequest(req, res, [:]) @@ -153,7 +153,7 @@ class OAuthAuthenticationSchemeTest extends Specification { getRequestedSessionId() >> "state" } client.getAccessToken("code") >> "token" - client.getUserData("token") >> new OAuthUser("testUser", "Test User", "test@example.com", Arrays.asList("dev-")) + client.getUserData("token") >> new OAuthUser("testUser", "Test User", "test@example.com", null) properties.getEmailDomains() >> Arrays.asList("example.com", "@example1.com") when: HttpAuthenticationResult result = scheme.processAuthenticationRequest(req, res, [:]) @@ -170,7 +170,7 @@ class OAuthAuthenticationSchemeTest extends Specification { getRequestedSessionId() >> "state" } client.getAccessToken("code") >> "token" - client.getUserData("token") >> new OAuthUser("testUser", "Test User", "test@acme.com", Arrays.asList("dev-")) + client.getUserData("token") >> new OAuthUser("testUser", "Test User", "test@acme.com", null) properties.getEmailDomains() >> Arrays.asList("example.com") when: HttpAuthenticationResult result = scheme.processAuthenticationRequest(req, res, [:]) @@ -187,7 +187,7 @@ class OAuthAuthenticationSchemeTest extends Specification { getRequestedSessionId() >> "state" } client.getAccessToken("code") >> "token" - client.getUserData("token") >> new OAuthUser("testUser", "Test User", "test@acme.com", Arrays.asList("dev-")) + client.getUserData("token") >> new OAuthUser("testUser", "Test User", "test@acme.com", null) properties.getEmailDomains() >> Arrays.asList("example.com", "@example1.com") when: HttpAuthenticationResult result = scheme.processAuthenticationRequest(req, res, [:]) diff --git a/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthClientTest.groovy b/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthClientTest.groovy index c7fd9ba..c126908 100644 --- a/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthClientTest.groovy +++ b/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthClientTest.groovy @@ -86,11 +86,23 @@ class OAuthClientTest extends Specification { } def "should fetch user data"() { + setup: + def token = 'test_token' + server.enqueue(new MockResponse().setBody(JSONValue.toJSONString([id: 'user', name: 'userName', email: 'email']))) + expect: + client.getUserData(token) == new OAuthUser('user', 'userName', 'email', new HashSet()) + def req = server.takeRequest() + req.method == 'GET' + req.path == '/user' + req.headers.get('Authorization') == 'Bearer ' + token + } + + def "should fetch user data with groups"() { setup: def token = 'test_token' server.enqueue(new MockResponse().setBody(JSONValue.toJSONString([id: 'user', name: 'userName', email: 'email', groups: ['dev-']]))) expect: - client.getUserData(token) == new OAuthUser('user', 'userName', 'email', Arrays.asList("dev-")) + client.getUserData(token) == new OAuthUser('user', 'userName', 'email', new HashSet(Arrays.asList("dev-"))) def req = server.takeRequest() req.method == 'GET' req.path == '/user' diff --git a/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthUserTest.groovy b/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthUserTest.groovy index 3eb81ad..9642349 100644 --- a/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthUserTest.groovy +++ b/src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthUserTest.groovy @@ -24,14 +24,15 @@ class OAuthUserTest extends Specification { user.groups == expectedGroups where: location || expectedId | expectedName | expectedGroups - "classpath:user/github.json" || 'pwielgolaski' | 'Piotr Wielgolaski' | ["dev"] - "classpath:user/bitbucket.json" || 'pwielgolaski' | 'Piotr Wielgolaski' | ["dev"] - "classpath:user/google.json" || 'superemail@gmail.com' | 'Piotr Wielgołaski' | ["dev"] - "classpath:user/azure.json" || 'any-guid' | 'Piotr Wielgołaski' | ["dev"] + "classpath:user/github.json" || 'pwielgolaski' | 'Piotr Wielgolaski' | new HashSet<>() + "classpath:user/bitbucket.json" || 'pwielgolaski' | 'Piotr Wielgolaski' | new HashSet<>() + "classpath:user/google.json" || 'superemail@gmail.com' | 'Piotr Wielgołaski' | new HashSet<>() + "classpath:user/azure.json" || 'any-guid' | 'Piotr Wielgołaski' | new HashSet<>() + "classpath:user/okta.json" || 'user@guidewire.com' | 'Bharath Sekar' | new HashSet<>(["dev"].asList()) } def "should return name if id is not given"() { expect: - new OAuthUser(null, 'name', 'email@domain', Lists.newArrayList("dev")).id == 'email@domain' + new OAuthUser(null, 'name', 'email@domain', new HashSet(Arrays.asList("dev"))).id == 'email@domain' } } diff --git a/src/test/groovy/jetbrains/buildServer/auth/oauth/ServerPrincipalFactoryTest.groovy b/src/test/groovy/jetbrains/buildServer/auth/oauth/ServerPrincipalFactoryTest.groovy index 59a5f99..5996bd5 100644 --- a/src/test/groovy/jetbrains/buildServer/auth/oauth/ServerPrincipalFactoryTest.groovy +++ b/src/test/groovy/jetbrains/buildServer/auth/oauth/ServerPrincipalFactoryTest.groovy @@ -40,7 +40,7 @@ class ServerPrincipalFactoryTest extends Specification { def "read user from model with existing groups add user group membership"() { given: - def user = new OAuthUser("testUser", null, null, Arrays.asList("dev-team1", "dev-team2")) + def user = new OAuthUser("testUser", null, null, new HashSet(Arrays.asList("dev-team1", "dev-team2"))) userModel.findUserByUsername(_, _) >> this.teamcityUser this.teamcityUser.getUsername() >> user.id this.teamcityUser.getUserGroups() >> Arrays.asList(userGroup) @@ -59,7 +59,7 @@ class ServerPrincipalFactoryTest extends Specification { def "read user from model with existing groups remove user membership"() { given: - def user = new OAuthUser("testUser", null, null, Arrays.asList()) + def user = new OAuthUser("testUser", null, null, new HashSet()) userModel.findUserByUsername(_, _) >> this.teamcityUser this.teamcityUser.getUsername() >> user.id this.teamcityUser.getUserGroups() >> Arrays.asList(userGroup) @@ -87,7 +87,7 @@ class ServerPrincipalFactoryTest extends Specification { def "read user from model with existing groups add and remove user memberships"() { given: def user = new OAuthUser("testUser", null, null, - Arrays.asList("dev-team2", "dev-team3", "dev-team4", "dev-team5")) + new HashSet(Arrays.asList("dev-team2", "dev-team3", "dev-team4", "dev-team5"))) userModel.findUserByUsername(_, _) >> this.teamcityUser this.teamcityUser.getUsername() >> user.id UserGroup userGroup2 = Mock() @@ -163,7 +163,7 @@ class ServerPrincipalFactoryTest extends Specification { def "create user from model without sync of groups"() { given: - def user = new OAuthUser("testUser", null, null, Arrays.asList("dev-team1", "dev-team2")) + def user = new OAuthUser("testUser", null, null, new HashSet(Arrays.asList("dev-team1", "dev-team2"))) userModel.findUserByUsername(_, _) >> null userModel.createUserAccount(PluginConstants.OAUTH_AUTH_SCHEME_NAME, user.id) >> teamcityUser properties.isSyncGroups() >> false @@ -179,7 +179,7 @@ class ServerPrincipalFactoryTest extends Specification { def "create user from model with whitelisted groups"() { given: - def user = new OAuthUser("testUser", null, null, Arrays.asList("dev-team1", "dev-team2")) + def user = new OAuthUser("testUser", null, null, new HashSet(Arrays.asList("dev-team1", "dev-team2"))) userModel.findUserByUsername(_, _) >> null userModel.createUserAccount(PluginConstants.OAUTH_AUTH_SCHEME_NAME, user.id) >> teamcityUser properties.isSyncGroups() >> true @@ -196,7 +196,7 @@ class ServerPrincipalFactoryTest extends Specification { def "create user from model with whitelisted one group"() { given: - def user = new OAuthUser("testUser", null, null, Arrays.asList("dev-team1", "dev-team2")) + def user = new OAuthUser("testUser", null, null, new HashSet(Arrays.asList("dev-team1", "dev-team2"))) userModel.findUserByUsername(_, _) >> null userModel.createUserAccount(PluginConstants.OAUTH_AUTH_SCHEME_NAME, user.id) >> teamcityUser properties.isSyncGroups() >> true @@ -213,7 +213,7 @@ class ServerPrincipalFactoryTest extends Specification { def "create user from model with whitelisted groups and only one is existing in TC"() { given: - def user = new OAuthUser("testUser", null, null, Arrays.asList("dev-team1", "dev-team2")) + def user = new OAuthUser("testUser", null, null, new HashSet(Arrays.asList("dev-team1", "dev-team2"))) userModel.findUserByUsername(_, _) >> null userModel.createUserAccount(PluginConstants.OAUTH_AUTH_SCHEME_NAME, user.id) >> teamcityUser properties.isSyncGroups() >> true @@ -230,7 +230,7 @@ class ServerPrincipalFactoryTest extends Specification { def "create user from model with no whitelisted groups and one is existing in TC"() { given: - def user = new OAuthUser("testUser", null, null, Arrays.asList("dev-team1", "dev-team2")) + def user = new OAuthUser("testUser", null, null, new HashSet(Arrays.asList("dev-team1", "dev-team2"))) userModel.findUserByUsername(_, _) >> null userModel.createUserAccount(PluginConstants.OAUTH_AUTH_SCHEME_NAME, user.id) >> teamcityUser properties.isSyncGroups() >> true diff --git a/src/test/resources/user/azure.json b/src/test/resources/user/azure.json index 19ebeeb..d017d10 100644 --- a/src/test/resources/user/azure.json +++ b/src/test/resources/user/azure.json @@ -10,6 +10,5 @@ "officeLocation": null, "preferredLanguage": null, "surname": null, - "userPrincipalName": "piotr.wielgolaski_example.com#EXT#@any-domain.onmicrosoft.com", - "groups": ["dev"] + "userPrincipalName": "piotr.wielgolaski_example.com#EXT#@any-domain.onmicrosoft.com" } \ No newline at end of file diff --git a/src/test/resources/user/bitbucket.json b/src/test/resources/user/bitbucket.json index 57b4ef7..22fd9bd 100644 --- a/src/test/resources/user/bitbucket.json +++ b/src/test/resources/user/bitbucket.json @@ -7,6 +7,5 @@ "is_staff": false, "location": null, "type": "user", - "uuid": "{92fec1df-5d41-472d-a627-1976932cc4aa}", - "groups": ["dev"] + "uuid": "{92fec1df-5d41-472d-a627-1976932cc4aa}" } \ No newline at end of file diff --git a/src/test/resources/user/github.json b/src/test/resources/user/github.json index 8b6473e..6f8f111 100644 --- a/src/test/resources/user/github.json +++ b/src/test/resources/user/github.json @@ -17,6 +17,5 @@ "followers": 1, "following": 6, "created_at": "2013-02-25T17:44:41Z", - "updated_at": "2017-08-14T18:22:57Z", - "groups": ["dev"] + "updated_at": "2017-08-14T18:22:57Z" } \ No newline at end of file diff --git a/src/test/resources/user/google.json b/src/test/resources/user/google.json index 9dbaa61..226ca7a 100644 --- a/src/test/resources/user/google.json +++ b/src/test/resources/user/google.json @@ -7,6 +7,5 @@ "picture": "https://lh3.googleusercontent.com/-XdUIqdMkCWA/AAAAAAAAAAI/AAAAAAAAAAA/4252rscbv5M/photo.jpg", "email": "superemail@gmail.com", "email_verified": true, - "gender": "male", - "groups": ["dev"] + "gender": "male" } \ No newline at end of file diff --git a/src/test/resources/user/okta.json b/src/test/resources/user/okta.json new file mode 100644 index 0000000..e8f3600 --- /dev/null +++ b/src/test/resources/user/okta.json @@ -0,0 +1,21 @@ +{ + "sub": "00unddzs1234abcdmO0h7", + "name": "Bharath Sekar", + "ver": 1, + "iss": "https://mycompany.okta.com/oauth2/ausj1ftnbaOabCd2U3h4", + "aud": "0oajrcy6439lTEsAB0h7", + "iat": 1590611373, + "exp": 1590614973, + "jti": "ID.9iPRG40sXmFoqKmZG0t83qPpqDjTSnGC3k4pBYSv72Q", + "amr": [ + "otp", + "mfa", + "pwd" + ], + "preferred_username": "user@guidewire.com", + "auth_time": 1590605665, + "at_hash": "T7BDzcPMmBQnyD7UIiLaWg", + "groups": [ + "dev" + ] +} \ No newline at end of file