From 13870f3a6992b11c9b77afe873242f3bc91728f7 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Tue, 20 Jun 2023 15:14:53 +0200 Subject: [PATCH] Improve error management in the github provider Closes https://github.com/keycloak/keycloak/issues/9429 --- .../social/github/GitHubIdentityProvider.java | 62 +++++++++++++------ .../testsuite/broker/SocialLoginTest.java | 2 + 2 files changed, 44 insertions(+), 20 deletions(-) diff --git a/services/src/main/java/org/keycloak/social/github/GitHubIdentityProvider.java b/services/src/main/java/org/keycloak/social/github/GitHubIdentityProvider.java index d612ed01c4..4d80d9b15d 100755 --- a/services/src/main/java/org/keycloak/social/github/GitHubIdentityProvider.java +++ b/services/src/main/java/org/keycloak/social/github/GitHubIdentityProvider.java @@ -18,7 +18,7 @@ package org.keycloak.social.github; import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.node.ArrayNode; +import jakarta.ws.rs.core.Response; import java.util.Iterator; import org.keycloak.broker.oidc.AbstractOAuth2IdentityProvider; import org.keycloak.broker.oidc.OAuth2IdentityProviderConfig; @@ -137,36 +137,58 @@ public class GitHubIdentityProvider extends AbstractOAuth2IdentityProvider imple @Override protected BrokeredIdentityContext doGetFederatedIdentity(String accessToken) { - try { - JsonNode profile = SimpleHttp.doGet(profileUrl, session).header("Authorization", "Bearer " + accessToken).asJson(); + try (SimpleHttp.Response response = SimpleHttp.doGet(profileUrl, session) + .header("Authorization", "Bearer " + accessToken) + .header("Accept", "application/json") + .asResponse()) { - BrokeredIdentityContext user = extractIdentityFromProfile(null, profile); + if (Response.Status.fromStatusCode(response.getStatus()).getFamily() != Response.Status.Family.SUCCESSFUL) { + logger.warnf("Profile endpoint returned an error (%d): %s", response.getStatus(), response.asString()); + throw new IdentityBrokerException("Profile could not be retrieved from the github endpoint"); + } - if (user.getEmail() == null) { - user.setEmail(searchEmail(accessToken)); - } + JsonNode profile = response.asJson(); + logger.tracef("profile retrieved from github: %s", profile); + BrokeredIdentityContext user = extractIdentityFromProfile(null, profile); - return user; + if (user.getEmail() == null) { + user.setEmail(searchEmail(accessToken)); + } + + return user; } catch (Exception e) { - throw new IdentityBrokerException("Could not obtain user profile from github.", e); + throw new IdentityBrokerException("Profile could not be retrieved from the github endpoint", e); } } private String searchEmail(String accessToken) { - try { - ArrayNode emails = (ArrayNode) SimpleHttp.doGet(emailUrl, session).header("Authorization", "Bearer " + accessToken).asJson(); + try (SimpleHttp.Response response = SimpleHttp.doGet(emailUrl, session) + .header("Authorization", "Bearer " + accessToken) + .header("Accept", "application/json") + .asResponse()) { - Iterator loop = emails.elements(); - while (loop.hasNext()) { - JsonNode mail = loop.next(); - if (mail.get("primary").asBoolean()) { - return getJsonProperty(mail, "email"); - } - } + if (Response.Status.fromStatusCode(response.getStatus()).getFamily() != Response.Status.Family.SUCCESSFUL) { + logger.warnf("Primary email endpoint returned an error (%d): %s", response.getStatus(), response.asString()); + throw new IdentityBrokerException("Primary email could not be retrieved from the github endpoint"); + } + + JsonNode emails = response.asJson(); + logger.tracef("emails retrieved from github: %s", emails); + if (emails.isArray()) { + Iterator loop = emails.elements(); + while (loop.hasNext()) { + JsonNode mail = loop.next(); + JsonNode primary = mail.get("primary"); + if (primary != null && primary.asBoolean()) { + return getJsonProperty(mail, "email"); + } + } + } + + throw new IdentityBrokerException("Primary email from github is not found in the user's email list."); } catch (Exception e) { - throw new IdentityBrokerException("Could not obtain user email from github.", e); + throw new IdentityBrokerException("Primary email could not be retrieved from the github endpoint", e); } - throw new IdentityBrokerException("Primary email from github is not found."); } @Override diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/SocialLoginTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/SocialLoginTest.java index f95d66602c..8a897c2ed7 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/SocialLoginTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/SocialLoginTest.java @@ -29,6 +29,7 @@ import org.keycloak.representations.idm.authorization.DecisionStrategy; import org.keycloak.services.resources.admin.permissions.AdminPermissionManagement; import org.keycloak.services.resources.admin.permissions.AdminPermissions; import org.keycloak.testsuite.AbstractKeycloakTest; +import org.keycloak.testsuite.arquillian.annotation.EnableFeature; import org.keycloak.testsuite.arquillian.annotation.UncaughtServerErrorExpected; import org.keycloak.testsuite.auth.page.login.UpdateAccount; import org.keycloak.testsuite.pages.AppPage; @@ -97,6 +98,7 @@ import com.google.common.collect.ImmutableMap; * @author Stian Thorgersen * @author Vaclav Muzikar */ +@EnableFeature(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) public class SocialLoginTest extends AbstractKeycloakTest { public static final String SOCIAL_CONFIG = "social.config";