refactor user specific validation to user classes

src/main/java/jetbrains/buildServer/auth/oauth/GithubUser.java

@@ -1,46 +1,63 @@
 package jetbrains.buildServer.auth.oauth;
 
-import okhttp3.OkHttpClient;
-import okhttp3.Request;
-import org.json.simple.JSONValue;
+import com.intellij.openapi.util.text.StringUtil;
+import org.apache.log4j.Logger;
 import org.json.simple.JSONArray;
 import org.json.simple.JSONObject;
+import org.json.simple.JSONValue;
 
-import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
-import java.io.IOException;
+import java.util.Objects;
+import java.util.Set;
+import java.util.function.Supplier;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 public class GithubUser extends OAuthUser {
 
-  private static final String ORGANIZATION_ENDPOINT = "https://api.github.com/user/orgs";
-  private final Map<Boolean, OkHttpClient> httpClients = new HashMap<>();
-  private final String[] organizations;
-
-  public GithubUser(Map userData, String token, Boolean allowInsecureHttps) throws IOException {
-      super(userData);
-      this.organizations = fetchUserOrganizations(token, allowInsecureHttps);
-  }
-
-  public String[] getOrganizations() {
-      return organizations;
-  }
-
-  private String[] fetchUserOrganizations(String token, Boolean allowInsecureHttps) throws IOException { 
-      Request orgRequest = new Request.Builder()
-            .url(ORGANIZATION_ENDPOINT)
-            .addHeader("Authorization","Bearer " + token)
-            .build();
-      String response = getHttpClient(allowInsecureHttps).newCall(orgRequest).execute().body().string();
-      JSONArray parsedResponse = (JSONArray) JSONValue.parse(response);
-      String[] orgs = new String[parsedResponse.size()];
-      for(int i = 0; i < parsedResponse.size(); i++) {
-        JSONObject value = (JSONObject) parsedResponse.get(i);
-        orgs[i] = (String) value.get("login");
-      }
-      return orgs;
-  }
-
-  private OkHttpClient getHttpClient(Boolean allowInsecureHttps) {
-      return httpClients.computeIfAbsent(allowInsecureHttps, HttpClientFactory::createClient);
-  }
+    private static final Logger log = Logger.getLogger(GithubUser.class);
+    public static final String ORGANIZATION_ENDPOINT = "https://api.github.com/user/orgs";
+    private final Supplier<String> organizationSupplier;
+
+    public GithubUser(Map userData, Supplier<String> organizationSupplier) {
+        super(userData);
+        this.organizationSupplier = organizationSupplier;
+    }
+
+    private Set<String> fetchUserOrganizations() {
+        String response = organizationSupplier.get();
+        log.debug("Fetched user org data: " + response);
+        Object parsedResponse = JSONValue.parse(response);
+        if (parsedResponse instanceof JSONArray) {
+            return ((List<Object>) parsedResponse)
+                    .stream()
+                    .filter(item -> item instanceof JSONObject)
+                    .map(item -> ((JSONObject) item).get("login"))
+                    .filter(Objects::nonNull)
+                    .map(Object::toString)
+                    .collect(Collectors.toSet());
+        } else {
+            String message = ((JSONObject) parsedResponse).getOrDefault("message", "Incorrect response:" + response ).toString();
+            throw new IllegalStateException(message);
+        }
+    }
+
+    @Override
+    public void validate(AuthenticationSchemeProperties properties) throws Exception {
+        super.validate(properties);
+        // Check the organizations that the user belongs to for Github Oauth
+        String orgs = properties.getOrganizations();
+        if (StringUtil.isNotEmpty(orgs)) {
+            Set<String> userOrganizations = this.fetchUserOrganizations();
+            Set<String> configuredOrganizations = Stream.of(orgs.split(","))
+                    .map(String::trim)
+                    .collect(Collectors.toSet());
+
+            configuredOrganizations.retainAll(userOrganizations);
+            if (configuredOrganizations.isEmpty()) {
+                throw new Exception("User's organization does not match with the ones specified in the oAuth settings");
+            }
+        }
+    }
 }

src/main/java/jetbrains/buildServer/auth/oauth/OAuthAuthenticationScheme.java

@@ -90,40 +90,12 @@ public HttpAuthenticationResult processAuthenticationRequest(@NotNull HttpServle
         }
 
         OAuthUser user = authClient.getUserData(token);
-        if (user.getId() == null) {
-            return sendUnauthorizedRequest(request, response, "Unauthenticated since user endpoint does not return any login id");
-        }
-        String emailDomain = properties.getEmailDomain();
-        if (!(emailDomain == null || emailDomain.isEmpty())) {
-            if (!emailDomain.startsWith("@")) {
-                emailDomain = "@" + emailDomain;
-            }
-            if (user.getEmail() == null || !user.getEmail().endsWith(emailDomain)) {
-                return sendUnauthorizedRequest(request, response, "Unauthenticated since user email is not " + emailDomain);
-            }
-        }
-        // Check the organizations that the user belongs to for Github Oauth
-        if (GithubUser.class.isInstance(user) ) {
-          String orgs = properties.getOrganizations();
-          if(!(orgs == null || orgs.isEmpty())) {
-            String[] configuredOrganizations = orgs.split(",");
-            Arrays.parallelSetAll(configuredOrganizations, (i) -> configuredOrganizations[i].trim());
-            String[] userOrganizations = ((GithubUser) user).getOrganizations();
-
-            Set<String> userOrganizationsSet = new HashSet<String>(Arrays.asList(userOrganizations));
-            Set<String> configuredOrganizationsSet = new HashSet<String>(Arrays.asList(configuredOrganizations));
-            Set<String> matchedOrganizationsSet = new HashSet<String>();
-
-            for (String organization:configuredOrganizationsSet) {
-              if (userOrganizationsSet.contains(organization)) {
-                matchedOrganizationsSet.add(organization);
-              }
-            }
-            if(matchedOrganizationsSet == null || matchedOrganizationsSet.size() == 0) {
-              return sendUnauthorizedRequest(request, response, "User's organization does not match with the ones specified in the Oauth settings");
-            }
-          }
+        try {
+            user.validate(properties);
+        } catch (Exception ex) {
+            return sendUnauthorizedRequest(request, response, ex.getMessage());
         }
+
         boolean allowCreatingNewUsersByLogin = AuthModuleUtil.allowCreatingNewUsersByLogin(schemeProperties, DEFAULT_ALLOW_CREATING_NEW_USERS_BY_LOGIN);
         final Optional<ServerPrincipal> principal = principalFactory.getServerPrincipal(user, allowCreatingNewUsersByLogin);
 

src/main/java/jetbrains/buildServer/auth/oauth/OAuthClient.java

@@ -8,6 +8,7 @@
 import okhttp3.RequestBody;
 import okhttp3.Response;
 import org.apache.log4j.Logger;
+import org.jetbrains.annotations.NotNull;
 import org.json.simple.JSONValue;
 import org.springframework.http.MediaType;
 
@@ -62,18 +63,31 @@ public String getAccessToken(String code) throws IOException {
         return (String) jsonResponse.get("access_token");
     }
 
-    public OAuthUser getUserData(String token) throws IOException {
-        Request request = new Request.Builder()
-                .url(properties.getUserEndpoint())
-                .addHeader("Authorization","Bearer " + token)
-                .build();
-        String response = getHttpClient().newCall(request).execute().body().string();
+    public OAuthUser getUserData(String token) {
+        String response = authenticatedGETCall(properties.getUserEndpoint(), token);
         log.debug("Fetched user data: " + response);
         Map parsedResponse = (Map) JSONValue.parse(response);
-        if ( (properties.getPreset()).equals("github")) {
-          return new GithubUser(parsedResponse, token, properties.isAllowInsecureHttps());
+        return createUser(token, parsedResponse);
+    }
+
+    private String authenticatedGETCall(String endpoint, String token)  {
+        try {
+            Request request = new Request.Builder()
+                    .url(endpoint)
+                    .addHeader("Authorization", "Bearer " + token)
+                    .build();
+            return getHttpClient().newCall(request).execute().body().string();
+        } catch (IOException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    @NotNull
+    private OAuthUser createUser(String token, Map parsedResponse) {
+        if ("github".equals(properties.getPreset())) {
+            return new GithubUser(parsedResponse, () -> authenticatedGETCall(GithubUser.ORGANIZATION_ENDPOINT, token));
         } else {
-          return new OAuthUser(parsedResponse);
+            return new OAuthUser(parsedResponse);
         }
     }
 }

src/main/java/jetbrains/buildServer/auth/oauth/OAuthUser.java

@@ -1,5 +1,7 @@
 package jetbrains.buildServer.auth.oauth;
 
+import com.intellij.openapi.util.text.StringUtil;
+
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -54,6 +56,21 @@ public String getEmail() {
         return email;
     }
 
+    public void validate(AuthenticationSchemeProperties properties) throws Exception {
+        if (this.getId() == null) {
+            throw new Exception("Unauthenticated since user endpoint does not return any login id");
+        }
+        String emailDomain = properties.getEmailDomain();
+        if (StringUtil.isNotEmpty(emailDomain)) {
+            if (!emailDomain.startsWith("@")) {
+                emailDomain = "@" + emailDomain;
+            }
+            if (this.getEmail() == null || !this.getEmail().endsWith(emailDomain)) {
+                throw new Exception("Unauthenticated since user email is not " + emailDomain);
+            }
+        }
+    }
+
     @Override
     public String toString() {
         return "OAuthUser{" + "id='" + getId() + '\'' +

src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthAuthenticationSchemeTest.groovy

@@ -180,15 +180,13 @@ class OAuthAuthenticationSchemeTest extends Specification {
     }
 
     def "authenticate user when user's organization match the specified organization"() {
-      given:
+        given:
         HttpServletRequest req = Mock() {
             getParameter(OAuthAuthenticationScheme.CODE) >> "code"
             getParameter(OAuthAuthenticationScheme.STATE) >> "state"
             getRequestedSessionId() >> "state"
         }
-        GithubUser githubUser = Mock()
-        githubUser.getId() >> "sample-test"
-        githubUser.getOrganizations() >> ["test-org"]
+        GithubUser githubUser = new GithubUser([id: "sample-test"], { '[ {"login": "test-org"} ]' })
 
         client.getUserData("token") >> githubUser
         client.getAccessToken("code") >> "token"
@@ -201,15 +199,13 @@ class OAuthAuthenticationSchemeTest extends Specification {
     }
 
     def "authenticate user when no organization is specified in the settings"() {
-      given:
+        given:
         HttpServletRequest req = Mock() {
             getParameter(OAuthAuthenticationScheme.CODE) >> "code"
             getParameter(OAuthAuthenticationScheme.STATE) >> "state"
             getRequestedSessionId() >> "state"
         }
-        GithubUser githubUser = Mock()
-        githubUser.getId() >> "sample-test"
-        githubUser.getOrganizations() >> ["test-org"]
+        GithubUser githubUser = new GithubUser([id: "sample-test"], { '[ {"login": "test-org"} ]' })
 
         client.getUserData("token") >> githubUser
         client.getAccessToken("code") >> "token"
@@ -222,15 +218,13 @@ class OAuthAuthenticationSchemeTest extends Specification {
 
 
     def "dont authenticate user when user's organization does not match the specified organization"() {
-      given:
+        given:
         HttpServletRequest req = Mock() {
             getParameter(OAuthAuthenticationScheme.CODE) >> "code"
             getParameter(OAuthAuthenticationScheme.STATE) >> "state"
             getRequestedSessionId() >> "state"
         }
-        GithubUser githubUser = Mock()
-        githubUser.getId() >> "sample-test"
-        githubUser.getOrganizations() >> ["test-org"]
+        GithubUser githubUser = new GithubUser([id: "sample-test"], { '[ {"login": "test-org"} ]' })
 
         client.getUserData("token") >> githubUser
         client.getAccessToken("code") >> "token"
@@ -240,7 +234,7 @@ class OAuthAuthenticationSchemeTest extends Specification {
         HttpAuthenticationResult result = scheme.processAuthenticationRequest(req, res, [:])
         then:
         result.type == HttpAuthenticationResult.Type.UNAUTHENTICATED
-        1 * res.sendError(401, "User's organization does not match with the ones specified in the Oauth settings")
+        1 * res.sendError(401, "User's organization does not match with the ones specified in the oAuth settings")
     }
 
 }

src/test/groovy/jetbrains/buildServer/auth/oauth/OAuthUserTest.groovy

@@ -6,15 +6,15 @@ import org.springframework.core.io.DefaultResourceLoader
 import spock.lang.Specification
 import spock.lang.Unroll
 
-import java.nio.charset.Charset
+import java.nio.charset.StandardCharsets
 
 class OAuthUserTest extends Specification {
 
     @Unroll("should read user details #location")
     def "should read user details"() {
         setup:
         def file = new DefaultResourceLoader().getResource(location).file
-        def content = Files.contentOf(file, Charset.defaultCharset())
+        def content = Files.contentOf(file, StandardCharsets.UTF_8)
         when:
         def user = new OAuthUser((Map) JSONValue.parse(content))
         then: