Skip to content

Commit

Permalink
PR refactoring
Browse files Browse the repository at this point in the history
  • Loading branch information
@bsekar @pwielgolaski
bsekar authored and pwielgolaski committed Jul 11, 2020
1 parent 2c8a9cf commit 01b3890
Show file tree
Hide file tree
Showing 11 changed files with 94 additions and 65 deletions.
39 changes: 24 additions & 15 deletions src/main/java/jetbrains/buildServer/auth/oauth/OAuthUser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"};
Expand All @@ -13,13 +19,13 @@ public class OAuthUser {
private final String id;
private final String name;
private final String email;
private final List<String> groups;
private final Set<String> groups;

public OAuthUser(String id) {
this(id, null, null, null);
}

public OAuthUser(String id, String name, String email, List<String> groups) {
public OAuthUser(String id, String name, String email, Set<String> groups) {
this.id = id;
this.name = name;
this.email = email;
Expand All @@ -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) {
Expand All @@ -48,21 +53,21 @@ private String getValueByKeys(Map userData, String[] keys) {
return value;
}

private String[] getGroups(Map userData, String key) {
private Set<String> 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();
String[] groupsArray = new String[groupSize];
for (int i = 0; i < groupSize; i++) {
groupsArray[i] = (String) groupsJsonArray.get(i);
}
return groupsArray;
return new HashSet(Arrays.asList(groupsArray));
}

public String getId() {
Expand All @@ -77,6 +82,10 @@ public String getEmail() {
return email;
}

public Set<String> 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");
Expand All @@ -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 +
'}';
}

Expand All @@ -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<String> getGroups() {
return groups;
return Objects.hash(id, name, email, groups);
}
}
38 changes: 14 additions & 24 deletions src/main/java/jetbrains/buildServer/auth/oauth/ServerPrincipalFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,20 @@ public Optional<ServerPrincipal> getServerPrincipal(@NotNull final OAuthUser use
boolean allowCreatingNewUsersByLogin) {
Optional<SUser> existingUserOptional = findExistingUser(user.getId());
boolean syncGroups = properties.isSyncGroups();
List<String> groupsInToken = user.getGroups();
Set<String> 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<String> existingUserGroups =
Set<String> existingUserGroups =
existingUser.getUserGroups().stream().map(userGroup -> userGroup.getName())
.collect(Collectors.toList());
List<String> additionalGroupsInToken = obtainGroupsDelta(groupsInToken, existingUserGroups);
.collect(Collectors.toSet());
Set<String> additionalGroupsInToken = new HashSet<>(groupsInToken);
additionalGroupsInToken.removeAll(existingUserGroups);
addUserToGroups(existingUser, applyGroupWhitelist(additionalGroupsInToken));

List<String> groupsNotPresentInToken = obtainGroupsDelta(existingUserGroups, groupsInToken);
Set<String> groupsNotPresentInToken = new HashSet<>(existingUserGroups);
groupsNotPresentInToken.removeAll(groupsInToken);
removeUserFromGroups(existingUser, applyGroupWhitelist(groupsNotPresentInToken));
}
return Optional.of(new ServerPrincipal(PluginConstants.OAUTH_AUTH_SCHEME_NAME, existingUser.getUsername()));
Expand All @@ -69,7 +70,7 @@ public Optional<ServerPrincipal> getServerPrincipal(@NotNull final OAuthUser use
}
}

private void removeUserFromGroups(SUser existingUser, List<String> groupsToDelete) {
private void removeUserFromGroups(SUser existingUser, Set<String> groupsToDelete) {
for (String group : groupsToDelete) {
SUserGroup userGroup = userGroupManager.findUserGroupByName(group);
if (userGroup != null) {
Expand All @@ -78,24 +79,24 @@ private void removeUserFromGroups(SUser existingUser, List<String> groupsToDelet
}
}

private List<String> applyGroupWhitelist(List<String> groups) {
List<String> finalGroupList = new ArrayList<>();
private Set<String> applyGroupWhitelist(Set<String> groups) {
Set<String> finalGroupSet = new HashSet<>();
Set<String> 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<String> finalGroupList) {
for (String group : finalGroupList) {
private void addUserToGroups(SUser created, Set<String> finalGroupSet) {
for (String group : finalGroupSet) {
SUserGroup userGroup = userGroupManager.findUserGroupByName(group);
if (userGroup != null) {
userGroup.addUser(created);
Expand All @@ -113,15 +114,4 @@ private Optional<SUser> findExistingUser(@NotNull final String userName) {
return Optional.empty();
}
}

private List<String> obtainGroupsDelta(List<String> minuend, List<String> subtrahend) {
return minuend.stream()
.filter(filterOutEqualValuesCaseInsensitive(subtrahend))
.collect(Collectors.toList());
}

private Predicate<String> filterOutEqualValuesCaseInsensitive(Collection<String> valuesToExclude) {
return valueUnderTest -> valuesToExclude.stream().map(String::toLowerCase)
.noneMatch(s -> s.equals(valueUnderTest.toLowerCase()));
}
}
8 changes: 4 additions & 4 deletions src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthAuthenticationSchemeTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, [:])
Expand All @@ -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, [:])
Expand All @@ -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, [:])
Expand All @@ -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, [:])
Expand Down
14 changes: 13 additions & 1 deletion src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthClientTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>())
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'
Expand Down
11 changes: 6 additions & 5 deletions src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthUserTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>(Arrays.asList("dev"))).id == 'email@domain'
}
}
16 changes: 8 additions & 8 deletions src/test/groovy/jetbrains/buildServer/auth/oauth/ServerPrincipalFactoryTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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<String>())
userModel.findUserByUsername(_, _) >> this.teamcityUser
this.teamcityUser.getUsername() >> user.id
this.teamcityUser.getUserGroups() >> Arrays.asList(userGroup)
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
3 changes: 1 addition & 2 deletions src/test/resources/user/azure.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
3 changes: 1 addition & 2 deletions src/test/resources/user/bitbucket.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
}
3 changes: 1 addition & 2 deletions src/test/resources/user/github.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
3 changes: 1 addition & 2 deletions src/test/resources/user/google.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
Loading

0 comments on commit 01b3890

Please sign in to comment.