SONAR-24336 Make GitHub login ready to scale with # of organizations

SonarSource · Feb 17, 2025 · e5466c0 · e5466c0
1 parent 2540165
commit e5466c0
Show file tree

Hide file tree

Showing 6 changed files with 176 additions and 74 deletions.
diff --git a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubIdentityProvider.java b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubIdentityProvider.java
@@ -25,7 +25,10 @@
 import java.io.IOException;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
+import java.util.stream.Collectors;
+import org.jetbrains.annotations.NotNull;
 import org.sonar.api.server.authentication.Display;
 import org.sonar.api.server.authentication.OAuth2IdentityProvider;
 import org.sonar.api.server.authentication.UnauthorizedException;
@@ -139,46 +142,45 @@ private void onCallback(CallbackContext context) throws InterruptedException, Ex
     context.redirectToRequestedPage();
   }
 
-  private void check(OAuth20Service scribe, OAuth2AccessToken accessToken, GsonUser user) throws InterruptedException, ExecutionException, IOException {
-    if (!isUserAuthorized(scribe, accessToken, user.getLogin())) {
+  private void check(OAuth20Service scribe, OAuth2AccessToken accessToken, GsonUser user) throws IOException {
+    if (!isUserAuthorized(scribe, accessToken)) {
       String message = settings.getOrganizations().isEmpty()
         ? format("'%s' must be a member of at least one organization which has installed the SonarQube GitHub app", user.getLogin())
         : format("'%s' must be a member of at least one organization: '%s'", user.getLogin(), String.join("', '", settings.getOrganizations().stream().sorted().toList()));
       throw new UnauthorizedException(message);
     }
   }
 
-  private boolean isUserAuthorized(OAuth20Service scribe, OAuth2AccessToken accessToken, String login) throws IOException, ExecutionException, InterruptedException {
+  private boolean isUserAuthorized(OAuth20Service scribe, OAuth2AccessToken accessToken) throws IOException {
+    Set<String> userOrganizationNames = getUserOrganizationNames(scribe, accessToken);
     if (isOrganizationMembershipRequired()) {
-      return isOrganizationsMember(scribe, accessToken, login);
+      return isOrganizationsMember(settings.getOrganizations(), userOrganizationNames);
     } else {
-      return isMemberOfInstallationOrganization(scribe, accessToken, login);
+      return isMemberOfInstallationOrganization(userOrganizationNames);
     }
   }
 
-  private boolean isOrganizationMembershipRequired() {
-    return !settings.getOrganizations().isEmpty();
+  private static boolean isOrganizationsMember(Set<String> organizations, Set<String> userOrganizationNames) {
+    return organizations.stream().anyMatch(userOrganizationNames::contains);
   }
 
-  private boolean isOrganizationsMember(OAuth20Service scribe, OAuth2AccessToken accessToken, String login) throws IOException, ExecutionException, InterruptedException {
-    for (String organization : settings.getOrganizations()) {
-      if (gitHubRestClient.isOrganizationMember(scribe, accessToken, organization, login)) {
-        return true;
-      }
-    }
-    return false;
+  @NotNull
+  private Set<String> getUserOrganizationNames(OAuth20Service scribe, OAuth2AccessToken accessToken) throws IOException {
+    return gitHubRestClient.getUserOrganizations(scribe, accessToken).stream()
+      .map(GsonOrganization::organization)
+      .collect(Collectors.toSet());
+  }
+
+  private boolean isOrganizationMembershipRequired() {
+    return !settings.getOrganizations().isEmpty();
   }
 
-  private boolean isMemberOfInstallationOrganization(OAuth20Service scribe, OAuth2AccessToken accessToken, String login)
-    throws IOException, ExecutionException, InterruptedException {
+  private boolean isMemberOfInstallationOrganization(Set<String> userOrganizationNames) {
     GithubAppConfiguration githubAppConfiguration = githubAppConfiguration();
     List<GithubAppInstallation> githubAppInstallations = githubAppClient.getWhitelistedGithubAppInstallations(githubAppConfiguration);
-    for (GithubAppInstallation installation : githubAppInstallations) {
-      if (gitHubRestClient.isOrganizationMember(scribe, accessToken, installation.organizationName(), login)) {
-        return true;
-      }
-    }
-    return false;
+    return githubAppInstallations.stream()
+      .map(GithubAppInstallation::organizationName)
+      .anyMatch(userOrganizationNames::contains);
   }
 
   private GithubAppConfiguration githubAppConfiguration() {

diff --git a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubRestClient.java b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubRestClient.java
@@ -62,36 +62,13 @@ public String getEmail(OAuth20Service scribe, OAuth2AccessToken accessToken) thr
       .orElse(null);
   }
 
-  public List<GsonTeam> getTeams(OAuth20Service scribe, OAuth2AccessToken accessToken) {
-    return executePaginatedRequest(settings.apiURL() + "user/teams", scribe, accessToken, GsonTeam::parse);
+  public List<GsonOrganization> getUserOrganizations(OAuth20Service scribe, OAuth2AccessToken accessToken) throws IOException {
+    String responseBody = executeRequest(settings.apiURL() + "user/orgs", scribe, accessToken).getBody();
+    LOGGER.info("Organizations response received : {}", responseBody);
+    return GsonOrganization.parse(responseBody);
   }
 
-  /**
-   * Check to see that login is a member of organization.
-   *
-   * A 204 response code indicates organization membership.  302 and 404 codes are not treated as exceptional,
-   * they indicate various ways in which a login is not a member of the organization.
-   *
-   * @see <a href="https://developer.github.com/v3/orgs/members/#response-if-requester-is-an-organization-member-and-user-is-a-member">GitHub members API</a>
-   */
-  public boolean isOrganizationMember(OAuth20Service scribe, OAuth2AccessToken accessToken, String organization, String login)
-    throws IOException, ExecutionException, InterruptedException {
-    String requestUrl = settings.apiURL() + format("orgs/%s/members/%s", organization, login);
-    OAuthRequest request = new OAuthRequest(Verb.GET, requestUrl);
-    scribe.signRequest(accessToken, request);
-
-    Response response = scribe.execute(request);
-    int code = response.getCode();
-    switch (code) {
-      case HttpURLConnection.HTTP_MOVED_TEMP, HttpURLConnection.HTTP_NOT_FOUND, HttpURLConnection.HTTP_NO_CONTENT:
-        LOGGER.trace("Orgs response received : {}", code);
-        return code == HttpURLConnection.HTTP_NO_CONTENT;
-      default:
-        throw unexpectedResponseCode(requestUrl, response);
-    }
-  }
-
-  private static IllegalStateException unexpectedResponseCode(String requestUrl, Response response) throws IOException {
-    return new IllegalStateException(format("Fail to execute request '%s'. HTTP code: %s, response: %s", requestUrl, response.getCode(), response.getBody()));
+  public List<GsonTeam> getTeams(OAuth20Service scribe, OAuth2AccessToken accessToken) {
+    return executePaginatedRequest(settings.apiURL() + "user/teams", scribe, accessToken, GsonTeam::parse);
   }
 }
diff --git a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GsonOrganization.java b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GsonOrganization.java
@@ -0,0 +1,33 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2025 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.auth.github;
+
+import com.google.gson.Gson;
+import com.google.gson.annotations.SerializedName;
+import com.google.gson.reflect.TypeToken;
+import java.util.List;
+
+public record GsonOrganization (@SerializedName("login") String organization) {
+  public static List<GsonOrganization> parse(String json) {
+    Gson gson = new Gson();
+    return gson.fromJson(json, new TypeToken<>() {
+    });
+  }
+}
diff --git a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubIdentityProviderTest.java b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubIdentityProviderTest.java
@@ -233,8 +233,7 @@ private CallbackContext mockUserBelongingToOrganization(UserIdentity userIdentit
 
     OAuth2AccessToken accessToken = mockAccessToken(scribeService, context, httpRequest, user);
 
-    when(gitHubRestClient.isOrganizationMember(scribeService, accessToken, "organization1", "login")).thenReturn(false);
-    when(gitHubRestClient.isOrganizationMember(scribeService, accessToken, "organization2", "login")).thenReturn(true);
+    when(gitHubRestClient.getUserOrganizations(scribeService, accessToken)).thenReturn(List.of(new GsonOrganization("organization2")));
 
     when(userIdentityFactory.create(user, "email", null)).thenReturn(userIdentity);
     return context;
@@ -249,8 +248,7 @@ private CallbackContext mockUserNotBelongingToOrganization(UserIdentity userIden
 
     OAuth2AccessToken accessToken = mockAccessToken(scribeService, context, httpRequest, user);
 
-    when(gitHubRestClient.isOrganizationMember(scribeService, accessToken, "organization1", "login")).thenReturn(false);
-    when(gitHubRestClient.isOrganizationMember(scribeService, accessToken, "organization2", "login")).thenReturn(false);
+    when(gitHubRestClient.getUserOrganizations(scribeService, accessToken)).thenReturn(List.of(new GsonOrganization("organization3")));
 
     when(userIdentityFactory.create(user, "email", null)).thenReturn(userIdentity);
     return context;

diff --git a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GsonOrganizationTest.java b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GsonOrganizationTest.java
@@ -0,0 +1,66 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2025 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.auth.github;
+
+import java.util.List;
+import org.junit.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class GsonOrganizationTest {
+
+  @Test
+  public void parse() {
+    List<GsonOrganization> underTest = GsonOrganization.parse("""
+      [
+        {
+          "login": "github",
+          "id": 1,
+          "node_id": "MDEyOk9yZ2FuaXphdGlvbjE=",
+          "url": "https://api.github.com/orgs/github",
+          "repos_url": "https://api.github.com/orgs/github/repos",
+          "events_url": "https://api.github.com/orgs/github/events",
+          "hooks_url": "https://api.github.com/orgs/github/hooks",
+          "issues_url": "https://api.github.com/orgs/github/issues",
+          "members_url": "https://api.github.com/orgs/github/members{/member}",
+          "public_members_url": "https://api.github.com/orgs/github/public_members{/member}",
+          "avatar_url": "https://github.com/images/error/octocat_happy.gif",
+          "description": "A great organization"
+        },
+        {
+          "login": "github-org2",
+          "id": 1,
+          "node_id": "MDEyOk9yZ2FuaXphdGlvbjE=",
+          "url": "https://api.github.com/orgs/github",
+          "repos_url": "https://api.github.com/orgs/github/repos",
+          "events_url": "https://api.github.com/orgs/github/events",
+          "hooks_url": "https://api.github.com/orgs/github/hooks",
+          "issues_url": "https://api.github.com/orgs/github/issues",
+          "members_url": "https://api.github.com/orgs/github/members{/member}",
+          "public_members_url": "https://api.github.com/orgs/github/public_members{/member}",
+          "avatar_url": "https://github.com/images/error/octocat_happy.gif",
+          "description": "An even greater organization"
+        }
+      ]""");
+    assertThat(underTest).hasSize(2);
+    assertThat(underTest).extracting(GsonOrganization::organization).containsExactly("github", "github-org2");
+  }
+
+}
diff --git a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/IntegrationTest.java b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/IntegrationTest.java
@@ -124,10 +124,13 @@ public void callback_on_successful_authentication() throws IOException, Interrup
     github.enqueue(newSuccessfulAccessTokenResponse());
     // response of api.github.com/user
     github.enqueue(new MockResponse().setBody("{\"id\":\"ABCD\", \"login\":\"octocat\", \"name\":\"monalisa octocat\",\"email\":\"[email protected]\"}"));
-    // response of api.github.com/orgs/first_org/members/user
-    github.enqueue(new MockResponse().setResponseCode(404));
-    // response of api.github.com/orgs/second_org/members/user
-    github.enqueue(new MockResponse().setResponseCode(204));
+    // response of api.github.com/user/orgs
+    github.enqueue(new MockResponse().setBody("""
+      [
+        {
+          "login": "second_org"
+        }
+      ]"""));
 
     HttpServletRequest request = newRequest("the-verifier-code");
     DumbCallbackContext callbackContext = new DumbCallbackContext(request);
@@ -161,8 +164,13 @@ public void should_retrieve_private_primary_verified_email_address() {
     github.enqueue(newSuccessfulAccessTokenResponse());
     // response of api.github.com/user
     github.enqueue(new MockResponse().setBody("{\"id\":\"ABCD\", \"login\":\"octocat\", \"name\":\"monalisa octocat\",\"email\":null}"));
-    // response of api.github.com/orgs/first_org/members/user
-    github.enqueue(new MockResponse().setResponseCode(204));
+    // response of api.github.com/user/orgs
+    github.enqueue(new MockResponse().setBody("""
+      [
+        {
+          "login": "second_org"
+        }
+      ]"""));
     // response of api.github.com/user/emails
     github.enqueue(new MockResponse().setBody("""
       [
@@ -198,7 +206,12 @@ public void should_not_fail_if_no_email() {
     // response of api.github.com/user
     github.enqueue(new MockResponse().setBody("{\"id\":\"ABCD\", \"login\":\"octocat\", \"name\":\"monalisa octocat\",\"email\":null}"));
     // response of api.github.com/orgs/first_org/members/user
-    github.enqueue(new MockResponse().setResponseCode(204));
+    github.enqueue(new MockResponse().setBody("""
+      [
+        {
+          "login": "second_org"
+        }
+      ]"""));
     // response of api.github.com/user/emails
     github.enqueue(new MockResponse().setBody("[]"));
 
@@ -238,8 +251,13 @@ public void callback_on_successful_authentication_with_group_sync() {
     github.enqueue(newSuccessfulAccessTokenResponse());
     // response of api.github.com/user
     github.enqueue(new MockResponse().setBody("{\"id\":\"ABCD\", \"login\":\"octocat\", \"name\":\"monalisa octocat\",\"email\":\"[email protected]\"}"));
-    // response of api.github.com/orgs/first_org/members/user
-    github.enqueue(new MockResponse().setResponseCode(204));
+    // response of api.github.com/user/orgs
+    github.enqueue(new MockResponse().setBody("""
+      [
+        {
+          "login": "second_org"
+        }
+      ]"""));
     // response of api.github.com/user/teams
     github.enqueue(new MockResponse().setBody("""
       [
@@ -267,8 +285,13 @@ public void callback_on_successful_authentication_with_group_sync_on_many_pages(
     github.enqueue(newSuccessfulAccessTokenResponse());
     // response of api.github.com/user
     github.enqueue(new MockResponse().setBody("{\"id\":\"ABCD\", \"login\":\"octocat\", \"name\":\"monalisa octocat\",\"email\":\"[email protected]\"}"));
-    // response of api.github.com/orgs/first_org/members/user
-    github.enqueue(new MockResponse().setResponseCode(204));
+    // response of api.github.com/user/orgs
+    github.enqueue(new MockResponse().setBody("""
+      [
+        {
+          "login": "second_org"
+        }
+      ]"""));
     // responses of api.github.com/user/teams
     github.enqueue(new MockResponse()
       .setHeader("Link", "<" + gitHubUrl + "/user/teams?per_page=100&page=2>; rel=\"next\", <" + gitHubUrl + "/user/teams?per_page=100&page=2>; rel=\"last\"")
@@ -324,8 +347,13 @@ public void callback_on_successful_authentication_with_organizations_with_member
     github.enqueue(newSuccessfulAccessTokenResponse());
     // response of api.github.com/user
     github.enqueue(new MockResponse().setBody("{\"id\":\"ABCD\", \"login\":\"octocat\", \"name\":\"monalisa octocat\",\"email\":\"[email protected]\"}"));
-    // response of api.github.com/orgs/example0/members/user
-    github.enqueue(new MockResponse().setResponseCode(204));
+    // response of api.github.com/user/orgs
+    github.enqueue(new MockResponse().setBody("""
+      [
+        {
+          "login": "example1"
+        }
+      ]"""));
 
     HttpServletRequest request = newRequest("the-verifier-code");
     DumbCallbackContext callbackContext = new DumbCallbackContext(request);
@@ -343,10 +371,8 @@ public void callback_on_successful_authentication_with_organizations_without_mem
     github.enqueue(newSuccessfulAccessTokenResponse());
     // response of api.github.com/user
     github.enqueue(new MockResponse().setBody("{\"id\":\"ABCD\", \"login\":\"octocat\", \"name\":\"monalisa octocat\",\"email\":\"[email protected]\"}"));
-    // response of api.github.com/orgs/first_org/members/user
-    github.enqueue(new MockResponse().setResponseCode(404).setBody("{}"));
-    // response of api.github.com/orgs/second_org/members/user
-    github.enqueue(new MockResponse().setResponseCode(404).setBody("{}"));
+    // response of api.github.com/user/orgs
+    github.enqueue(new MockResponse().setBody("[]"));
 
     HttpServletRequest request = newRequest("the-verifier-code");
     DumbCallbackContext callbackContext = new DumbCallbackContext(request);
@@ -381,7 +407,7 @@ public void callback_throws_ISE_if_error_when_checking_membership() {
     github.enqueue(newSuccessfulAccessTokenResponse());
     // response of api.github.com/user
     github.enqueue(new MockResponse().setBody("{\"id\":\"ABCD\", \"login\":\"octocat\", \"name\":\"monalisa octocat\",\"email\":\"[email protected]\"}"));
-    // crash of api.github.com/orgs/example/members/user
+    // crash of api.github.com/user/orgs
     github.enqueue(new MockResponse().setResponseCode(500).setBody("{error}"));
 
     HttpServletRequest request = newRequest("the-verifier-code");
@@ -390,7 +416,7 @@ public void callback_throws_ISE_if_error_when_checking_membership() {
       underTest.callback(callbackContext);
       fail("exception expected");
     } catch (IllegalStateException e) {
-      assertThat(e.getMessage()).isEqualTo("Fail to execute request '" + gitHubSettings.apiURL() + "orgs/example/members/octocat'. HTTP code: 500, response: {error}");
+      assertThat(e.getMessage()).isEqualTo("Fail to execute request '" + gitHubSettings.apiURL() + "user/orgs'. HTTP code: 500, response: {error}");
     }
   }