From 56b9424ddcc88c3386ad08dfac83f2e012f487ff Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Wed, 10 Aug 2022 08:56:54 -0700 Subject: [PATCH 01/16] Major Rewrite - Nimbus OIDC and MS Graph Massive rewrite of the plugin code internals. Remove ADAL4J in favor of Nimbus OAuth 2.0 SDK. Move to MS Graph for group info using the MS Graph SDK. --- pom.xml | 36 +-- .../org/almrangers/auth/aad/AadGroup.java | 62 ---- .../auth/aad/AadIdentityProvider.java | 288 ++++++++++-------- .../org/almrangers/auth/aad/AadSettings.java | 37 +-- .../org/almrangers/auth/aad/AadUserInfo.java | 184 +++++++++++ .../almrangers/auth/aad/AuthAadPlugin.java | 2 +- .../almrangers/auth/aad/HttpClientHelper.java | 99 ------ .../org/almrangers/auth/aad/JSONHelper.java | 84 ----- .../org/almrangers/auth/aad/package-info.java | 2 +- .../org/sonar/l10n/authaad.properties | 6 - .../auth/aad/AadIdentityProviderTest.java | 167 +++------- .../almrangers/auth/aad/AadSettingsTest.java | 30 +- .../almrangers/auth/aad/AadUserInfoTest.java | 101 ++++++ .../auth/aad/AuthAadPluginTest.java | 4 +- 14 files changed, 534 insertions(+), 568 deletions(-) delete mode 100644 src/main/java/org/almrangers/auth/aad/AadGroup.java create mode 100644 src/main/java/org/almrangers/auth/aad/AadUserInfo.java delete mode 100644 src/main/java/org/almrangers/auth/aad/HttpClientHelper.java delete mode 100644 src/main/java/org/almrangers/auth/aad/JSONHelper.java create mode 100644 src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java diff --git a/pom.xml b/pom.xml index 3874374..1c432c7 100644 --- a/pom.xml +++ b/pom.xml @@ -4,7 +4,7 @@ org.almrangers.auth.aad sonar-auth-aad-plugin - 1.3.1 + 2.0.0-SNAPSHOT sonar-plugin Azure Active Directory (AAD) Authentication Plug-in for SonarQube @@ -89,24 +89,23 @@ javax.servlet javax.servlet-api - 3.0.1 + 4.0.1 provided - com.microsoft.azure - adal4j - 1.6.7 + com.nimbusds + oauth2-oidc-sdk + 10.1 - org.json - json - 20220320 + com.squareup.okhttp3 + okhttp + 4.10.0 - org.slf4j - slf4j-log4j12 - 1.7.5 - provided + com.microsoft.graph + microsoft-graph + 5.39.0 com.google.code.findbugs @@ -114,11 +113,6 @@ 3.0.2 provided - - org.apache.commons - commons-text - 1.9 - @@ -136,7 +130,7 @@ org.mockito mockito-core - 4.6.1 + 4.8.1 test @@ -145,12 +139,6 @@ - - com.squareup.okhttp3 - mockwebserver - 4.10.0 - test - diff --git a/src/main/java/org/almrangers/auth/aad/AadGroup.java b/src/main/java/org/almrangers/auth/aad/AadGroup.java deleted file mode 100644 index b7be4c8..0000000 --- a/src/main/java/org/almrangers/auth/aad/AadGroup.java +++ /dev/null @@ -1,62 +0,0 @@ -/** - * Azure Active Directory Authentication Plugin for SonarQube - *

- * Copyright (c) 2016 Microsoft Corporation - * All rights reserved. - *

- * The MIT License (MIT) - *

- * Permission is hereby granted, free of charge, to any person obtaining a copy - * of this software and associated documentation files (the "Software"), to deal - * in the Software without restriction, including without limitation the rights - * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell - * copies of the Software, and to permit persons to whom the Software is - * furnished to do so, subject to the following conditions: - *

- * The above copyright notice and this permission notice shall be included in all - * copies or substantial portions of the Software. - *

- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, - * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE - * SOFTWARE. - */ -package org.almrangers.auth.aad; - -public class AadGroup { - private String objectId; - private String objectType; - private String displayName; - - public String getObjectId() { - return objectId; - } - - public void setObjectId(String objectId) { - this.objectId = objectId; - } - - public String getObjectType() { - return objectType; - } - - public void setObjectType(String objectType) { - this.objectType = objectType; - } - - public String getDisplayName() { - return displayName; - } - - public void setDisplayName(String displayName) { - this.displayName = displayName; - } - - public boolean isValid() { - return displayName != null && !displayName.isEmpty(); - } - -} diff --git a/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java b/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java index cfede93..f15d919 100644 --- a/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java +++ b/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java @@ -2,7 +2,7 @@ * Azure Active Directory Authentication Plugin for SonarQube *

* Copyright (c) 2016 Microsoft Corporation - * All rights reserved. + * Copyright (c) 2022 Michael Johnson *

* The MIT License (MIT) *

@@ -27,27 +27,30 @@ package org.almrangers.auth.aad; -import com.microsoft.aad.adal4j.AuthenticationContext; -import com.microsoft.aad.adal4j.AuthenticationResult; -import com.microsoft.aad.adal4j.ClientCredential; -import com.microsoft.aad.adal4j.UserInfo; -import java.net.HttpURLConnection; -import java.net.MalformedURLException; -import java.net.URI; -import java.net.URL; -import java.util.Base64; +import java.net.*; +import java.util.Arrays; import java.util.HashSet; -import java.util.Optional; -import java.util.Set; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.Future; -import javax.annotation.Nullable; import javax.servlet.http.HttpServletRequest; -import org.apache.commons.lang3.StringUtils; -import org.json.JSONArray; -import org.json.JSONObject; +import com.nimbusds.jose.JOSEException; +import com.nimbusds.jose.JWSAlgorithm; +import com.nimbusds.jose.jwk.source.JWKSource; +import com.nimbusds.jose.jwk.source.RemoteJWKSet; +import com.nimbusds.jose.proc.*; +import com.nimbusds.jwt.JWT; +import com.nimbusds.jwt.proc.ConfigurableJWTProcessor; +import com.nimbusds.jwt.proc.DefaultJWTClaimsVerifier; +import com.nimbusds.jwt.proc.DefaultJWTProcessor; +import com.nimbusds.oauth2.sdk.*; +import com.nimbusds.oauth2.sdk.auth.ClientSecretBasic; +import com.nimbusds.oauth2.sdk.auth.Secret; +import com.nimbusds.oauth2.sdk.http.HTTPResponse; +import com.nimbusds.oauth2.sdk.id.ClientID; +import com.nimbusds.oauth2.sdk.id.State; +import com.nimbusds.oauth2.sdk.token.AccessToken; +import com.nimbusds.openid.connect.sdk.OIDCTokenResponse; +import com.nimbusds.openid.connect.sdk.token.OIDCTokens; import org.sonar.api.server.ServerSide; import org.sonar.api.server.authentication.Display; import org.sonar.api.server.authentication.OAuth2IdentityProvider; @@ -56,18 +59,11 @@ import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; -import static java.lang.String.format; -import static org.almrangers.auth.aad.AadSettings.AUTH_REQUEST_FORMAT; -import static org.almrangers.auth.aad.AadSettings.LOGIN_STRATEGY_PROVIDER_ID; -import static org.almrangers.auth.aad.AadSettings.LOGIN_STRATEGY_UNIQUE; - @ServerSide public class AadIdentityProvider implements OAuth2IdentityProvider { private static final String KEY = "aad"; private static final String NAME = "Microsoft"; - private static final String NAME_CLAIM = "name"; - private static final int JWT_PAYLOAD_PART_INDEX = 1; private static final Logger LOGGER = Loggers.get(AadIdentityProvider.class); private final AadSettings settings; @@ -106,9 +102,30 @@ public boolean allowsUsersToSignUp() { @Override public void init(InitContext context) { - String state = context.generateCsrfState(); - String authUrl = String.format(AUTH_REQUEST_FORMAT, settings.authorizationUrl(), settings.clientId().orElse(null), context.getCallbackUrl(), state); - context.redirectTo(authUrl); + String sqState = context.generateCsrfState(); + + State state = new State(sqState); + ClientID clientId = new ClientID(settings.clientId().orElse(null)); + Scope scope = Scope.parse("openid profile email User.Read"); + + try { + AuthorizationRequest authReq = new AuthorizationRequest( + new URI(settings.authorizationUrl()), + ResponseType.CODE, + ResponseMode.QUERY, + clientId, + new URI(context.getCallbackUrl()), + scope, + state); + + URI authUrl = authReq.toURI(); + + context.redirectTo(authUrl.toString()); + } catch (URISyntaxException e) { + LOGGER.error(e.toString()); + } + + } @Override @@ -116,49 +133,98 @@ public void callback(CallbackContext context) { try { onCallback(context); } catch (Exception e) { - LOGGER.error("Exception:" + e.toString()); - throw new IllegalStateException(e); + LOGGER.error("Exception:" + e); + throw new UnauthorizedException(e.getMessage()); } } - private void onCallback(CallbackContext context) { + private void onCallback(CallbackContext context) throws UnauthorizedException { context.verifyCsrfState(); HttpServletRequest request = context.getRequest(); - String code = request.getParameter("code"); - AuthenticationContext authContext; - AuthenticationResult result; + AuthorizationCode code = new AuthorizationCode(request.getParameter("code")); ExecutorService service = null; - Set userGroups; try { - service = Executors.newFixedThreadPool(1); - authContext = new AuthenticationContext(settings.authorityUrl(), false, service); - URI url = new URI(context.getCallbackUrl()); - ClientCredential clientCred = new ClientCredential(settings.clientId().orElse(null), settings.clientSecret().orElse(null)); - Future future = authContext.acquireTokenByAuthorizationCode( - code, url, clientCred, settings.getGraphURL(), null); - result = future.get(); - - UserInfo aadUser = result.getUserInfo(); - UserIdentity.Builder userIdentityBuilder = UserIdentity.builder() - .setProviderLogin(aadUser.getDisplayableId()) - .setLogin(getLogin(aadUser)) - .setName(getUserName(result)) - .setEmail(aadUser.getDisplayableId()); - if (settings.enableGroupSync()) { - if (settings.enableClientCredential()) { - Future clientFuture = authContext.acquireToken(settings.getGraphURL(), clientCred, null); - result = clientFuture.get(); + TokenRequest tokenReq = new TokenRequest( + new URI(settings.authorityUrl()), + new ClientSecretBasic( + new ClientID(settings.clientId().orElse(null)), + new Secret(settings.clientSecret().orElse(""))), + new AuthorizationCodeGrant(code, new URI(context.getCallbackUrl())) + ); + + HTTPResponse tokenHTTPResp = tokenReq.toHTTPRequest().send(); + + // Parse and check response + OIDCTokenResponse tokenResponse; + + try { + tokenResponse = OIDCTokenResponse.parse(tokenHTTPResp); + } catch (ParseException e) { + //If we got an error in the token process, this will catch it and log the details. + if(!tokenHTTPResp.indicatesSuccess()) { + TokenErrorResponse tokenErrorResponse = TokenResponse.parse(tokenHTTPResp).toErrorResponse(); + + LOGGER.error("Issue getting authentication token. Returned error: " + + tokenErrorResponse.getErrorObject().getDescription()); + + throw new UnauthorizedException("Error when authenticating user. Please check the logs for more details."); + } else { + // Some other error happened, throw the error message directly. + throw new UnauthorizedException(e.getMessage()); + } + } + + OIDCTokens accessTokens = tokenResponse.getOIDCTokens(); + + JWT idToken = accessTokens.getIDToken(); + AccessToken userAccessToken = accessTokens.getAccessToken(); + + AadUserInfo aadUser; + AccessToken accessToken = userAccessToken; + + if (validateIdToken(idToken)) { + // If group sync is enabled and client credential flow is enabled, + // get a client access token we will use to fetch group membership + if(settings.enableGroupSync() && settings.enableClientCredential()) { + TokenRequest clientRequest = new TokenRequest( + new URI(settings.authorityUrl()), + new ClientSecretBasic( + new ClientID(settings.clientId().orElse("")), + new Secret(settings.clientSecret().orElse("")) + ), + new ClientCredentialsGrant(), + new Scope(settings.getGraphURL() + "/.default")); + + TokenResponse clientResponse = TokenResponse.parse(clientRequest.toHTTPRequest().send()); + + // Client token request failed, log the error + if (!clientResponse.indicatesSuccess()) { + TokenErrorResponse errorResponse = clientResponse.toErrorResponse(); + LOGGER.error("Issue in getting client token for group sync. Returned error: " + + errorResponse.getErrorObject().getDescription()); + } else { + AccessTokenResponse successResponse = clientResponse.toSuccessResponse(); + accessToken = successResponse.getTokens().getAccessToken(); + } } - userGroups = getUserGroupsMembership(result.getAccessToken(), aadUser.getUniqueId()); - userIdentityBuilder.setGroups(userGroups); + + // NOTE: The Access token IS EITHER: + // The client credential token if client credential flow is enabled **OR** + // The user's token if client credential flow fails or client flow is disabled + aadUser = new AadUserInfo(idToken, accessToken, settings.enableGroupSync()); + + UserIdentity.Builder userIdentity = parseUserId(aadUser); + + context.authenticate(userIdentity.build()); + + context.redirectToRequestedPage(); } - context.authenticate(userIdentityBuilder.build()); - context.redirectToRequestedPage(); } catch (Exception e) { - LOGGER.error("Exception:" + e.toString()); + LOGGER.error("Exception:" + e); + throw new UnauthorizedException(e.getMessage()); } finally { if (service != null) { service.shutdown(); @@ -166,79 +232,53 @@ private void onCallback(CallbackContext context) { } } - String getUserName(AuthenticationResult result) { - UserInfo userInfo = result.getUserInfo(); - if (userInfo.getGivenName() != null && userInfo.getFamilyName() != null) { - return userInfo.getGivenName() + " " + userInfo.getFamilyName(); - } + private boolean validateIdToken(JWT idToken) throws MalformedURLException, BadJOSEException, JOSEException { - if (result.getIdToken() != null) { - String base64EncodedJWTPayload = result.getIdToken().split("\\.")[JWT_PAYLOAD_PART_INDEX]; - JSONObject token = new JSONObject(new String(Base64.getDecoder().decode(base64EncodedJWTPayload))); - if (token.has(NAME_CLAIM)) { - return token.getString(NAME_CLAIM); - } - } - LOGGER.warn(String.format("User's name not found from authentication token for user %s", userInfo.getUniqueId())); - return userInfo.getDisplayableId(); - } + // Create a JWT processor for the access tokens + ConfigurableJWTProcessor jwtProcessor = + new DefaultJWTProcessor<>(); - private String getLogin(UserInfo aadUser) { - Optional loginStrategy = settings.loginStrategy(); - if(loginStrategy.isPresent()) { - if (LOGIN_STRATEGY_UNIQUE.equals(loginStrategy.get())) { - return generateUniqueLogin(aadUser); - } else if (LOGIN_STRATEGY_PROVIDER_ID.equals(loginStrategy.get())) { - return aadUser.getDisplayableId(); - } else { - throw new UnauthorizedException(format("Login strategy not found : %s", loginStrategy)); - } - } else { - throw new UnauthorizedException("Login strategy value is not set/present."); - } - } + JWKSource keySource = + new RemoteJWKSet<>(new URL(settings.jwkKeysUrl())); - URL getUrl(String userId, @Nullable String nextPage) throws MalformedURLException { - String url = String.format(settings.getGraphMembershipUrl(), settings.tenantId().orElse("common"), userId); - if (null != nextPage) { - url = nextPage; - } - return new URL(url); + //MS uses RSA 256 to sign their JWTs + JWSAlgorithm expectedJWSAlg = JWSAlgorithm.RS256; + + JWSKeySelector keySelector = + new JWSVerificationKeySelector<>(expectedJWSAlg, keySource); + + jwtProcessor.setJWSKeySelector(keySelector); + + // Verify the specific claims in the ID token. Confirm the audience matches + // the expected value (our Client ID), and that specific attributes are present. + // Note that this also automatically validates the various timestamp values + // to ensure the token is still valid. + jwtProcessor.setJWTClaimsSetVerifier( + new DefaultJWTClaimsVerifier<>( + settings.clientId().orElse(null), + null, + new HashSet<>(Arrays.asList("iss", "iat", "nbf", "exp", "oid", "name", "preferred_username", "sub", "tid")) + ) + ); + + // Don't capture the output. This will throw an error if the token doesn't + // validate instead of returning true. + jwtProcessor.process(idToken, null); + + return true; // If there was no exception thrown, then the token is valid } - public Set getUserGroupsMembership(String accessToken, String userId) { - Set userGroups = new HashSet<>(); - String nextPage = null; - try { - do { - URL url = getUrl(userId, nextPage); - HttpURLConnection connection = (HttpURLConnection) url.openConnection(); - connection.setRequestProperty("Authorization", accessToken); - connection.setRequestProperty("Accept", "application/json;odata.metadata=minimal"); - String goodRespStr = HttpClientHelper.getResponseStringFromConn(connection, true); - int responseCode = connection.getResponseCode(); - JSONObject response = HttpClientHelper.processGoodRespStr(responseCode, goodRespStr); - JSONArray groups; - groups = JSONHelper.fetchDirectoryObjectJSONArray(response); - AadGroup group; - for (int i = 0; i < groups.length(); i++) { - JSONObject thisUserJSONObject = groups.optJSONObject(i); - group = new AadGroup(); - JSONHelper.convertJSONObjectToDirectoryObject(thisUserJSONObject, group); - if (group.isValid()) { - userGroups.add(group.getDisplayName()); - } - } - nextPage = JSONHelper.fetchNextPageLink(response); - } while (StringUtils.isNotEmpty(nextPage)); - } catch (Exception e) { - LOGGER.error(e.toString()); + private UserIdentity.Builder parseUserId(AadUserInfo aadUser) { + UserIdentity.Builder userIdentityBuilder = UserIdentity.builder() + .setProviderLogin(aadUser.getDisplayId()) + .setName(aadUser.getDisplayName()) + .setEmail(aadUser.getUserEmail()); + + if (settings.enableGroupSync()) { + userIdentityBuilder.setGroups(aadUser.getUserGroups()); } - return userGroups; - } - private String generateUniqueLogin(UserInfo aadUser) { - return String.format("%s@%s", aadUser.getDisplayableId(), getKey()); + return userIdentityBuilder; } } diff --git a/src/main/java/org/almrangers/auth/aad/AadSettings.java b/src/main/java/org/almrangers/auth/aad/AadSettings.java index c50eae7..88f9127 100644 --- a/src/main/java/org/almrangers/auth/aad/AadSettings.java +++ b/src/main/java/org/almrangers/auth/aad/AadSettings.java @@ -2,7 +2,7 @@ * Azure Active Directory Authentication Plugin for SonarQube *

* Copyright (c) 2016 Microsoft Corporation - * All rights reserved. + * Copyright (c) 2022 Michael Johnson *

* The MIT License (MIT) *

@@ -34,7 +34,6 @@ import org.sonar.api.config.PropertyDefinition; import org.sonar.api.server.ServerSide; -import static java.lang.String.format; import static java.lang.String.valueOf; import static org.sonar.api.PropertyType.*; @@ -51,10 +50,6 @@ public class AadSettings { protected static final String DIRECTORY_LOC_CN = "Azure AD China"; protected static final String ENABLE_GROUPS_SYNC = "sonar.auth.aad.enableGroupsSync"; protected static final String ENABLE_CLIENT_CRED = "sonar.auth.aad.enableClientCredential"; - protected static final String LOGIN_STRATEGY = "sonar.auth.aad.loginStrategy"; - protected static final String LOGIN_STRATEGY_UNIQUE = "Unique"; - protected static final String LOGIN_STRATEGY_PROVIDER_ID = "Same as Azure AD login"; - protected static final String LOGIN_STRATEGY_DEFAULT_VALUE = LOGIN_STRATEGY_UNIQUE; protected static final String MULTI_TENANT = "sonar.auth.aad.multiTenant"; protected static final String CATEGORY = "aad"; @@ -65,14 +60,13 @@ public class AadSettings { protected static final String LOGIN_URL = "https://login.microsoftonline.com"; protected static final String LOGIN_URL_USGOV = "https://login.microsoftonline.us"; protected static final String LOGIN_URL_CN = "https://login.chinacloudapi.cn"; - protected static final String AUTHORIZATION_URL = "oauth2/authorize"; - protected static final String AUTHORITY_URL = "oauth2/token"; + protected static final String AUTHORIZATION_URL = "oauth2/v2.0/authorize"; + protected static final String AUTHORITY_URL = "oauth2/v2.0/token"; protected static final String COMMON_URL = "common"; protected static final String GRAPH_URL = "https://graph.microsoft.com"; protected static final String GRAPH_URL_USGOV = "https://graph.microsoft.com"; protected static final String GRAPH_URL_CN = "https://microsoftgraph.chinacloudapi.cn"; - protected static final String AUTH_REQUEST_FORMAT = "%s?client_id=%s&response_type=code&redirect_uri=%s&state=%s&scope=openid"; protected static final String GROUPS_REQUEST_FORMAT = "/v1.0/%s/users/%s/transitiveMemberOf"; private final Configuration config; @@ -128,28 +122,20 @@ public static List definitions() { .defaultValue(valueOf(false)) .index(1) .build(), - PropertyDefinition.builder(LOGIN_STRATEGY) - .category(CATEGORY) - .subCategory(SUBCATEGORY_ADVANCED) - .type(SINGLE_SELECT_LIST) - .defaultValue(LOGIN_STRATEGY_DEFAULT_VALUE) - .options(LOGIN_STRATEGY_UNIQUE, LOGIN_STRATEGY_PROVIDER_ID) - .index(2) - .build(), PropertyDefinition.builder(DIRECTORY_LOCATION) .category(CATEGORY) .subCategory(SUBCATEGORY_ADVANCED) .type(SINGLE_SELECT_LIST) .defaultValue(DIRECTORY_LOC_GLOBAL) .options(DIRECTORY_LOC_GLOBAL, DIRECTORY_LOC_USGOV, DIRECTORY_LOC_CN) - .index(3) + .index(2) .build(), PropertyDefinition.builder(ENABLE_CLIENT_CRED) .category(CATEGORY) .subCategory(SUBCATEGORY_ADVANCED) .type(BOOLEAN) .defaultValue(valueOf(false)) - .index(4) + .index(3) .build() ); } @@ -183,7 +169,7 @@ public Optional clientSecret() { } public boolean isEnabled() { - return config.getBoolean(ENABLED).orElse(Boolean.FALSE) && clientId().isPresent() && clientSecret().isPresent() && loginStrategy().isPresent(); + return config.getBoolean(ENABLED).orElse(Boolean.FALSE) && clientId().isPresent() && clientSecret().isPresent(); } private String getEndpoint() { @@ -209,6 +195,13 @@ private String getLoginHost() { return LOGIN_URL; } + // I don't like hard-coding a key URL, but there's not a good way I can find + // to load quickly from the OIDC discovery URL. This should work for + // both national clouds and the general cloud for both tenant and common. + public String jwkKeysUrl() { + return String.format("%s/%s/discovery/keys", getLoginHost(), getEndpoint()); + } + public String authorizationUrl() { return String.format("%s/%s/%s", getLoginHost(), getEndpoint(), AUTHORIZATION_URL); } @@ -235,8 +228,4 @@ public String getGraphURL() { public String getGraphMembershipUrl() { return getGraphURL() + GROUPS_REQUEST_FORMAT; } - - public Optional loginStrategy() { - return config.get(LOGIN_STRATEGY); - } } diff --git a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java new file mode 100644 index 0000000..b4c5414 --- /dev/null +++ b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java @@ -0,0 +1,184 @@ +/** + * Azure Active Directory Authentication Plugin for SonarQube + *

+ * Copyright (c) 2022 Michael Johnson + *

+ * The MIT License (MIT) + *

+ * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + *

+ * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + *

+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package org.almrangers.auth.aad; + +import com.microsoft.graph.authentication.IAuthenticationProvider; +import com.microsoft.graph.http.GraphServiceException; +import com.microsoft.graph.models.DirectoryObject; +import com.microsoft.graph.models.Group; +import com.microsoft.graph.requests.DirectoryObjectCollectionWithReferencesPage; +import com.microsoft.graph.requests.DirectoryObjectCollectionWithReferencesRequestBuilder; +import com.microsoft.graph.requests.GraphServiceClient; +import com.nimbusds.jwt.JWT; +import com.nimbusds.jwt.JWTClaimsSet; +import com.nimbusds.oauth2.sdk.token.AccessToken; +import okhttp3.Request; +import org.sonar.api.utils.log.Logger; +import org.sonar.api.utils.log.Loggers; +import reactor.util.annotation.NonNull; + +import java.net.URL; +import java.text.ParseException; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.concurrent.CompletableFuture; + +public class AadUserInfo { + private String userOid; + private String displayId; + private String displayName; + private String userEmail; + + //Initialized to an empty set so if group sync is enabled and no groups are + //returned from the MS Graph call, user will be removed from all SQ groups. + private Set userGroups = Collections.emptySet(); + + private static final Logger LOGGER = Loggers.get(AadUserInfo.class); + + public AadUserInfo(JWT idToken, AccessToken accessToken, Boolean wantGroups) throws ParseException { + parseToken(idToken); + + if(Boolean.TRUE.equals(wantGroups)) { + processGroups(accessToken.getValue()); + } + } + + private void parseToken(JWT idToken) throws ParseException { + JWTClaimsSet claims = idToken.getJWTClaimsSet(); + + // User's OID. Used for grabbing group membership if that feature is enabled + if(!claims.getStringClaim("oid").isEmpty()) { + this.userOid = claims.getStringClaim("oid"); + } + + // Display ID + // Tries the "preferred username" first, and falls back to email + if(!claims.getStringClaim("preferred_username").isEmpty()) { + this.displayId = claims.getStringClaim("preferred_username"); + } else if(!claims.getStringClaim("email").isEmpty()) { + this.displayId = claims.getStringClaim("email"); + } + + // Display Name + // Attempts to get the user's name from the name claim. AAD requires + // this, so it can't be blank. To be safe, we still set a display + // name if that claim isn't in the token for some reason. + if(!claims.getStringClaim("name").isEmpty()) { + this.displayName = claims.getStringClaim("name"); + } else { + this.displayName = "No name provided"; + } + + // Email + // Tries email first, and falls back to "preferred_username" if empty. This should work for most AAD installs. + if(!claims.getStringClaim("email").isEmpty()) { + this.userEmail = claims.getStringClaim("email"); + } else if(!claims.getStringClaim("preferred_username").isEmpty()) { + this.userEmail = claims.getStringClaim("preferred_username"); + } + } + + private void processGroups(String accessToken) { + Set parsedUserGroups = new HashSet<>(); + + // We already have the auth code, so create a custom provider that will + // return the code to our graph client. + IAuthenticationProvider graphAuthProvider = new IAuthenticationProvider() { + @NonNull + @Override + public CompletableFuture getAuthorizationTokenAsync(URL requestUrl) { + CompletableFuture future = new CompletableFuture<>(); + future.complete(accessToken); + return future; + } + }; + + try { + final GraphServiceClient graphClient = + GraphServiceClient + .builder() + .authenticationProvider(graphAuthProvider) + .buildClient(); + + DirectoryObjectCollectionWithReferencesPage memberGroupCollection = + graphClient + .users(userOid).transitiveMemberOf() + .buildRequest() + .select("id,displayName") + .top(999) // Maximum page size of 999 to reduce number of requests. + .get(); + + while(memberGroupCollection != null) { + final List groupList = memberGroupCollection.getCurrentPage(); + + for ( DirectoryObject group : groupList) { + String groupDisplayName = ((Group) group).displayName; + + // Don't add the group if the display name is null + if(groupDisplayName != null) { + parsedUserGroups.add(groupDisplayName); + } + } + + final DirectoryObjectCollectionWithReferencesRequestBuilder nextPage = memberGroupCollection.getNextPage(); + if (nextPage == null) { + break; + } else { + memberGroupCollection = nextPage.buildRequest().get(); + } + } + + // Log a warning if we were asked to parse groups but couldn't + if(parsedUserGroups.isEmpty()) { + LOGGER.warn("Group list was empty. Maybe your AAD permissions aren't set correctly?"); + } + + userGroups = parsedUserGroups; + } catch (GraphServiceException e) { + // Post the error to the logs, don't consider this fatal (fail auth) + LOGGER.error("Group Membership Request failed with error: " + e.getMessage()); + } + } + + public String getDisplayId() { + return displayId; + } + + public String getDisplayName() { + return displayName; + } + + public String getUserEmail() { + return userEmail; + } + + public Set getUserGroups() { + return userGroups; + } +} diff --git a/src/main/java/org/almrangers/auth/aad/AuthAadPlugin.java b/src/main/java/org/almrangers/auth/aad/AuthAadPlugin.java index a0eef14..ec4aeaf 100644 --- a/src/main/java/org/almrangers/auth/aad/AuthAadPlugin.java +++ b/src/main/java/org/almrangers/auth/aad/AuthAadPlugin.java @@ -2,7 +2,7 @@ * Azure Active Directory Authentication Plugin for SonarQube *

* Copyright (c) 2016 Microsoft Corporation - * All rights reserved. + * Copyright (c) 2022 Michael Johnson *

* The MIT License (MIT) *

diff --git a/src/main/java/org/almrangers/auth/aad/HttpClientHelper.java b/src/main/java/org/almrangers/auth/aad/HttpClientHelper.java deleted file mode 100644 index 8d7320a..0000000 --- a/src/main/java/org/almrangers/auth/aad/HttpClientHelper.java +++ /dev/null @@ -1,99 +0,0 @@ -/******************************************************************************* - * Copyright © Microsoft Open Technologies, Inc. - * - * All Rights Reserved - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * THIS CODE IS PROVIDED *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS - * OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION - * ANY IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A - * PARTICULAR PURPOSE, MERCHANTABILITY OR NON-INFRINGEMENT. - * - * See the Apache License, Version 2.0 for the specific language - * governing permissions and limitations under the License. - ******************************************************************************/ - -package org.almrangers.auth.aad; - -import java.io.BufferedReader; -import java.io.IOException; -import java.io.InputStreamReader; -import java.net.HttpURLConnection; -import org.json.JSONException; -import org.json.JSONObject; - -public class HttpClientHelper { - - private HttpClientHelper() { - // Static methods - } - - public static String getResponseStringFromConn(HttpURLConnection conn, boolean isSuccess) throws IOException { - - BufferedReader reader; - if (isSuccess) { - reader = new BufferedReader(new InputStreamReader(conn.getInputStream())); - } else { - reader = new BufferedReader(new InputStreamReader(conn.getErrorStream())); - } - StringBuilder stringBuilder = new StringBuilder(); - String line = ""; - while ((line = reader.readLine()) != null) { - stringBuilder.append(line); - } - - return stringBuilder.toString(); - } - - /** - * for bad response, whose responseCode is not 200 level - * - * @param responseCode - * @param goodRespStr - * @return - * @throws JSONException - */ - public static JSONObject processGoodRespStr(int responseCode, String goodRespStr) { - JSONObject response = new JSONObject(); - response.put("responseCode", responseCode); - if (goodRespStr.equalsIgnoreCase("")) { - response.put("responseMsg", ""); - } else { - response.put("responseMsg", new JSONObject(goodRespStr)); - } - - return response; - } - - /** - * for good response - * - * @param responseCode - * @param responseMsg - * @return - * @throws JSONException - */ - public static JSONObject processBadRespStr(int responseCode, String responseMsg) { - - JSONObject response = new JSONObject(); - response.put("responseCode", responseCode); - if (responseMsg.equalsIgnoreCase("")) { // good response is empty string - response.put("responseMsg", ""); - } else { // bad response is json string - JSONObject errorObject = new JSONObject(responseMsg).optJSONObject("odata.error"); - - String errorCode = errorObject.optString("code"); - String errorMsg = errorObject.optJSONObject("message").optString("value"); - response.put("responseCode", responseCode); - response.put("errorCode", errorCode); - response.put("errorMsg", errorMsg); - } - - return response; - } -} diff --git a/src/main/java/org/almrangers/auth/aad/JSONHelper.java b/src/main/java/org/almrangers/auth/aad/JSONHelper.java deleted file mode 100644 index 5667b02..0000000 --- a/src/main/java/org/almrangers/auth/aad/JSONHelper.java +++ /dev/null @@ -1,84 +0,0 @@ -/******************************************************************************* - * Copyright © Microsoft Open Technologies, Inc. - * - * All Rights Reserved - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * THIS CODE IS PROVIDED *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS - * OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION - * ANY IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A - * PARTICULAR PURPOSE, MERCHANTABILITY OR NON-INFRINGEMENT. - * - * See the Apache License, Version 2.0 for the specific language - * governing permissions and limitations under the License. - ******************************************************************************/ -package org.almrangers.auth.aad; - -import java.lang.reflect.Field; -import org.apache.commons.text.WordUtils; -import org.json.JSONArray; -import org.json.JSONObject; - -public class JSONHelper { - - private JSONHelper() { - // Utility class - } - - /** - * This method parses an JSON Array out of a collection of JSON Objects - * within a string. - * - * @param jsonObject The JSON String that holds the collection. - * @return An JSON Array that would contains all the collection object. - * @throws Exception - */ - public static JSONArray fetchDirectoryObjectJSONArray(JSONObject jsonObject) { - return jsonObject.optJSONObject("responseMsg").optJSONArray("value"); - } - - /** - * This method parses a JSON field out of a json object - * - * @param jsonObject The JSON String that holds the collection. - * @return next page link - * @throws Exception - */ - public static String fetchNextPageLink(JSONObject jsonObject) { - return jsonObject.optJSONObject("responseMsg").has("@odata.nextLink") ? jsonObject.optJSONObject("responseMsg").get("@odata.nextLink").toString() : null; - } - - /** - * This is a generic method that copies the simple attribute values from an - * argument jsonObject to an argument generic object. - * - * @param jsonObject The jsonObject from where the attributes are to be copied. - * @param destObject The object where the attributes should be copied into. - * @throws Exception Throws a Exception when the operation are unsuccessful. - */ - public static void convertJSONObjectToDirectoryObject(JSONObject jsonObject, T destObject) throws Exception { - - // Get the list of all the field names. - Field[] fieldList = destObject.getClass().getDeclaredFields(); - - // For all the declared field. - for (int i = 0; i < fieldList.length; i++) { - // If the field is of type String, that is - // if it is a simple attribute. - if (fieldList[i].getType().equals(String.class)) { - // Invoke the corresponding set method of the destObject using - // the argument taken from the jsonObject. - destObject - .getClass() - .getMethod(String.format("set%s", WordUtils.capitalize(fieldList[i].getName())), - String.class) - .invoke(destObject, jsonObject.optString(fieldList[i].getName())); - } - } - } -} diff --git a/src/main/java/org/almrangers/auth/aad/package-info.java b/src/main/java/org/almrangers/auth/aad/package-info.java index 06e7664..9811d7f 100644 --- a/src/main/java/org/almrangers/auth/aad/package-info.java +++ b/src/main/java/org/almrangers/auth/aad/package-info.java @@ -2,7 +2,7 @@ * Azure Active Directory Authentication Plugin for SonarQube *

* Copyright (c) 2016 Microsoft Corporation - * All rights reserved. + * Copyright (c) 2022 Michael Johnson *

* The MIT License (MIT) *

diff --git a/src/main/resources/org/sonar/l10n/authaad.properties b/src/main/resources/org/sonar/l10n/authaad.properties index 786a15d..8c04f0d 100644 --- a/src/main/resources/org/sonar/l10n/authaad.properties +++ b/src/main/resources/org/sonar/l10n/authaad.properties @@ -40,12 +40,6 @@ property.sonar.auth.aad.tenantId.description=Azure AD Tenant ID. property.sonar.auth.aad.allowUsersToSignUp.name=Allow users to sign-up property.sonar.auth.aad.allowUsersToSignUp.description=Allow new users to authenticate. When set to 'false', only existing users will be able to authenticate to the server. -property.sonar.auth.aad.loginStrategy.name=Login generation strategy -property.sonar.auth.aad.loginStrategy.description=Logins will be set the following way:\ -

    \ -
  • Unique: the user's login will be auto-generated the first time so that it is unique.
    • \ -
  • Same as Azure AD login: the user's login will be the Azure AD login.
- property.sonar.auth.aad.directoryLocation.name=Directory Location property.sonar.auth.aad.directoryLocation.description=The location of the Azure installation. You normally won't need to change this. diff --git a/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java b/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java index 6cfa4b9..0a08ed0 100644 --- a/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java +++ b/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java @@ -2,7 +2,7 @@ * Azure Active Directory Authentication Plugin for SonarQube *

* Copyright (c) 2016 Microsoft Corporation - * All rights reserved. + * Copyright (c) 2022 Michael Johnson *

* The MIT License (MIT) *

@@ -27,35 +27,22 @@ package org.almrangers.auth.aad; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.Assert.assertEquals; -import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import java.io.IOException; -import java.net.HttpURLConnection; -import java.net.MalformedURLException; -import java.net.URL; -import java.util.Base64; - -import com.microsoft.aad.adal4j.AuthenticationResult; -import com.microsoft.aad.adal4j.UserInfo; +import okhttp3.HttpUrl; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import org.mockito.ArgumentCaptor; import org.sonar.api.config.internal.MapSettings; import org.sonar.api.server.authentication.OAuth2IdentityProvider; -public class AadIdentityProviderTest { +import java.util.HashMap; - private static final String GIVEN_NAME = "GivenName"; - private static final String FAMILY_NAME = "FamilyName"; - private static final String DISPLAYABLE_ID = "DisplayableId"; - private static final String EXPECTED_NAME = GIVEN_NAME + " " + FAMILY_NAME; - private static final String JSON_WITH_NAME = "{\"name\": \"" + EXPECTED_NAME + "\"}"; - private static final String EMPTY_JSON = "{}"; +public class AadIdentityProviderTest { @Rule public ExpectedException thrown = ExpectedException.none(); @@ -63,43 +50,6 @@ public class AadIdentityProviderTest { AadSettings aadSettings = new AadSettings(settings.asConfig()); AadIdentityProvider underTest = spy(new AadIdentityProvider(aadSettings)); - @Test - public void shouldHandleGetMembershipsPagination() throws IOException { - - URL mockUrl = mock(URL.class); - HttpURLConnection mockConnection = mock(HttpURLConnection.class); - - doReturn(mockUrl) - .when(underTest) - .getUrl("userId", null); - - doReturn (mockConnection) - .when(mockUrl) - .openConnection(); - - doReturn(ClassLoader.class.getResourceAsStream("/get-members-page1.json")) - .when(mockConnection) - .getInputStream(); - - - URL mockUrl2 = mock(URL.class); - HttpURLConnection mockConnection2 = mock(HttpURLConnection.class); - - doReturn(mockUrl2) - .when(underTest) - .getUrl("userId", "https://graph.microsoft.com/v1.0/536e97e9-0d29-43ec-b8d5-a505d3ee6a8f/users/abc.xyz@example.com/memberOf?$skiptoken=RANDOMTOKEN"); - - doReturn (mockConnection2) - .when(mockUrl2) - .openConnection(); - - doReturn(ClassLoader.class.getResourceAsStream("/get-members-page2.json")) - .when(mockConnection2) - .getInputStream(); - - assertEquals(5, underTest.getUserGroupsMembership("token", "userId").size()); - } - @Test public void check_fields() { assertThat(underTest.getKey()).isEqualTo("aad"); @@ -117,14 +67,48 @@ public void init() { underTest.init(context); - verify(context).redirectTo("https://login.microsoftonline.com/null/oauth2/authorize?client_id=id&response_type=code&redirect_uri=http://localhost/callback&state=state&scope=openid"); + ArgumentCaptor redirectUrl = ArgumentCaptor.forClass(String.class); + verify(context).redirectTo(redirectUrl.capture()); + + // The redirect URL may not always have the query parameters in the same order + // depending on how we're testing. The mess below is to test the individual + // portions of the URL to make sure they're equivalent. + + HttpUrl expectedUrl = HttpUrl.parse("https://login.microsoftonline.com/null/oauth2/v2.0/authorize?response_type=code&redirect_uri=http%3A%2F%2Flocalhost%2Fcallback&state=state&client_id=id&response_mode=query&scope=openid+profile+email+User.Read"); + HttpUrl actualUrl = HttpUrl.parse(redirectUrl.getValue()); + + // Split the expected URL into "path" and "query" parts to test them independently. + assert expectedUrl != null; + String expectedUrlPath = expectedUrl.toString().split("\\?")[0]; + + HashMap expectedUrlQuery = new HashMap<>(); + for(int i = 0, size = expectedUrl.querySize(); i < size; i++) { + expectedUrlQuery.put( + expectedUrl.queryParameterName(i), + expectedUrl.queryParameterValue(i) + ); + } + + // Split the actual URL into "path" and "query" parts to test them independently. + assert actualUrl != null; + String actualUrlPath = actualUrl.toString().split("\\?")[0]; + + HashMap actualUrlQuery = new HashMap<>(); + for(int i = 0, size = actualUrl.querySize(); i < size; i++) { + actualUrlQuery.put( + actualUrl.queryParameterName(i), + actualUrl.queryParameterValue(i) + ); + } + + assertThat(actualUrlPath).isEqualTo(expectedUrlPath); + assertThat(actualUrlQuery).isEqualTo(expectedUrlQuery); } @Test public void is_enabled() { settings.setProperty("sonar.auth.aad.clientId.secured", "id"); settings.setProperty("sonar.auth.aad.clientSecret.secured", "secret"); - settings.setProperty("sonar.auth.aad.loginStrategy", AadSettings.LOGIN_STRATEGY_DEFAULT_VALUE); settings.setProperty("sonar.auth.aad.enabled", true); assertThat(underTest.isEnabled()).isTrue(); @@ -141,79 +125,10 @@ public void allow_signups() { assertThat(underTest.allowsUsersToSignUp()).isFalse(); } - @Test - public void page_url() throws MalformedURLException { - URL testUrl; - - testUrl = underTest.getUrl("123e4567-e89b-12d3-a456-426655440000", null); - assertEquals("https://graph.microsoft.com/v1.0/common/users/123e4567-e89b-12d3-a456-426655440000/transitiveMemberOf", testUrl.toString()); - - testUrl = underTest.getUrl("123e4567-e89b-12d3-a456-426655440000", "https://graph.microsoft.com/v1.0/536e97e9-0d29-43ec-b8d5-a505d3ee6a8f/users/abc.xyz@example.com/memberOf?$skiptoken=RANDOMTOKEN"); - assertEquals("https://graph.microsoft.com/v1.0/536e97e9-0d29-43ec-b8d5-a505d3ee6a8f/users/abc.xyz@example.com/memberOf?$skiptoken=RANDOMTOKEN", testUrl.toString()); - } - - @Test - public void shouldParseUsersNameFromUserInfoIfNotNull() { - UserInfo mockUserInfo = mock(UserInfo.class); - doReturn(GIVEN_NAME).when(mockUserInfo).getGivenName(); - doReturn(FAMILY_NAME).when(mockUserInfo).getFamilyName(); - AuthenticationResult mockResult = mock(AuthenticationResult.class); - doReturn(mockUserInfo).when(mockResult).getUserInfo(); - - assertEquals(GIVEN_NAME + " " + FAMILY_NAME, underTest.getUserName(mockResult)); - } - - @Test - public void shouldParseUsersNameFromIdTokenIfUserInfoNameNull() { - UserInfo mockUserInfo = mock(UserInfo.class); - doReturn(null).when(mockUserInfo).getGivenName(); - doReturn(null).when(mockUserInfo).getFamilyName(); - AuthenticationResult mockResult = mock(AuthenticationResult.class); - doReturn(mockUserInfo).when(mockResult).getUserInfo(); - doReturn(getMockJWT(EMPTY_JSON, JSON_WITH_NAME, EMPTY_JSON)).when(mockResult).getIdToken(); - - assertEquals(EXPECTED_NAME, underTest.getUserName(mockResult)); - } - - @Test - public void shouldFallBackToAnonymousIfNoNameFoundForUser() { - UserInfo mockUserInfo = mock(UserInfo.class); - doReturn(null).when(mockUserInfo).getGivenName(); - doReturn(null).when(mockUserInfo).getFamilyName(); - doReturn(DISPLAYABLE_ID).when(mockUserInfo).getDisplayableId(); - AuthenticationResult mockResult = mock(AuthenticationResult.class); - doReturn(mockUserInfo).when(mockResult).getUserInfo(); - doReturn(getMockJWT(EMPTY_JSON, EMPTY_JSON, EMPTY_JSON)).when(mockResult).getIdToken(); - - assertEquals(DISPLAYABLE_ID, underTest.getUserName(mockResult)); - } - - @Test - public void shouldHandleNullIdToken() { - UserInfo mockUserInfo = mock(UserInfo.class); - doReturn(null).when(mockUserInfo).getGivenName(); - doReturn(null).when(mockUserInfo).getFamilyName(); - doReturn(DISPLAYABLE_ID).when(mockUserInfo).getDisplayableId(); - AuthenticationResult mockResult = mock(AuthenticationResult.class); - doReturn(mockUserInfo).when(mockResult).getUserInfo(); - doReturn(null).when(mockResult).getIdToken(); - - assertEquals(DISPLAYABLE_ID, underTest.getUserName(mockResult)); - } - - private String getMockJWT(String header, String payload, String signature) { - return Base64.getEncoder().encodeToString(header.getBytes()) - + "." - + Base64.getEncoder().encodeToString(payload.getBytes()) - + "." - + Base64.getEncoder().encodeToString(signature.getBytes()); - } - private void setSettings(boolean enabled) { if (enabled) { settings.setProperty("sonar.auth.aad.clientId.secured", "id"); settings.setProperty("sonar.auth.aad.clientSecret.secured", "secret"); - settings.setProperty("sonar.auth.aad.loginStrategy", AadSettings.LOGIN_STRATEGY_DEFAULT_VALUE); settings.setProperty("sonar.auth.aad.directoryLocation", AadSettings.DIRECTORY_LOC_GLOBAL); settings.setProperty("sonar.auth.aad.enabled", true); } else { diff --git a/src/test/java/org/almrangers/auth/aad/AadSettingsTest.java b/src/test/java/org/almrangers/auth/aad/AadSettingsTest.java index cf64f49..4be1507 100644 --- a/src/test/java/org/almrangers/auth/aad/AadSettingsTest.java +++ b/src/test/java/org/almrangers/auth/aad/AadSettingsTest.java @@ -2,7 +2,7 @@ * Azure Active Directory Authentication Plugin for SonarQube *

* Copyright (c) 2016 Microsoft Corporation - * All rights reserved. + * Copyright (c) 2022 Michael Johnson *

* The MIT License (MIT) *

@@ -42,7 +42,6 @@ public class AadSettingsTest { public void is_enabled() { settings.setProperty("sonar.auth.aad.clientId.secured", "id"); settings.setProperty("sonar.auth.aad.clientSecret.secured", "secret"); - settings.setProperty("sonar.auth.aad.loginStrategy", LOGIN_STRATEGY_DEFAULT_VALUE); settings.setProperty("sonar.auth.aad.enabled", true); assertThat(underTest.isEnabled()).isTrue(); @@ -55,33 +54,39 @@ public void is_enabled() { public void return_authorization_authority_url_for_single_tenant_azureAd_app() { settings.setProperty("sonar.auth.aad.multiTenant", "false"); settings.setProperty("sonar.auth.aad.tenantId", "tenantId"); - assertThat(underTest.authorizationUrl()).isEqualTo("https://login.microsoftonline.com/tenantId/oauth2/authorize"); - assertThat(underTest.authorityUrl()).isEqualTo("https://login.microsoftonline.com/tenantId/oauth2/token"); + assertThat(underTest.authorizationUrl()).isEqualTo("https://login.microsoftonline.com/tenantId/oauth2/v2.0/authorize"); + assertThat(underTest.authorityUrl()).isEqualTo("https://login.microsoftonline.com/tenantId/oauth2/v2.0/token"); } @Test public void return_authorization_authority_url_for_multi_tenant_azureAd_app() { settings.setProperty("sonar.auth.aad.multiTenant", "true"); - assertThat(underTest.authorizationUrl()).isEqualTo("https://login.microsoftonline.com/common/oauth2/authorize"); - assertThat(underTest.authorityUrl()).isEqualTo("https://login.microsoftonline.com/common/oauth2/token"); + assertThat(underTest.authorizationUrl()).isEqualTo("https://login.microsoftonline.com/common/oauth2/v2.0/authorize"); + assertThat(underTest.authorityUrl()).isEqualTo("https://login.microsoftonline.com/common/oauth2/v2.0/token"); } @Test public void return_correct_urls() { + // This tenant ID is used for the JWK Keys URL. The below GUID was generated randomly and has no meaning outside testing. + settings.setProperty("sonar.auth.aad.tenantId", "6664a665-d25b-40ac-8ab4-e27b014c2464"); + //Azure Default "Global" settings.setProperty("sonar.auth.aad.directoryLocation", DIRECTORY_LOC_GLOBAL); assertThat(underTest.authorizationUrl()).startsWith("https://login.microsoftonline.com"); assertThat(underTest.getGraphURL()).startsWith("https://graph.microsoft.com"); + assertThat(underTest.jwkKeysUrl()).isEqualTo("https://login.microsoftonline.com/6664a665-d25b-40ac-8ab4-e27b014c2464/discovery/keys"); //Azure US Gov settings.setProperty("sonar.auth.aad.directoryLocation", DIRECTORY_LOC_USGOV); assertThat(underTest.authorizationUrl()).startsWith("https://login.microsoftonline.us"); assertThat(underTest.getGraphURL()).startsWith("https://graph.microsoft.com"); + assertThat(underTest.jwkKeysUrl()).isEqualTo("https://login.microsoftonline.us/6664a665-d25b-40ac-8ab4-e27b014c2464/discovery/keys"); //Azure China settings.setProperty("sonar.auth.aad.directoryLocation", DIRECTORY_LOC_CN); assertThat(underTest.authorizationUrl()).startsWith("https://login.chinacloudapi.cn"); assertThat(underTest.getGraphURL()).startsWith("https://microsoftgraph.chinacloudapi.cn"); + assertThat(underTest.jwkKeysUrl()).isEqualTo("https://login.chinacloudapi.cn/6664a665-d25b-40ac-8ab4-e27b014c2464/discovery/keys"); } @Test @@ -93,9 +98,9 @@ public void return_graph_membership_url() { @Test public void is_enabled_always_return_false_when_client_id_is_null() { settings.setProperty("sonar.auth.aad.enabled", true); + //noinspection ConstantConditions settings.setProperty("sonar.auth.aad.clientId.secured", (String) null); settings.setProperty("sonar.auth.aad.clientSecret.secured", "secret"); - settings.setProperty("sonar.auth.aad.loginStrategy", LOGIN_STRATEGY_DEFAULT_VALUE); assertThat(underTest.isEnabled()).isFalse(); } @@ -104,17 +109,12 @@ public void is_enabled_always_return_false_when_client_id_is_null() { public void is_enabled_always_return_false_when_client_secret_is_null() { settings.setProperty("sonar.auth.aad.enabled", true); settings.setProperty("sonar.auth.aad.clientId.secured", "id"); + //noinspection ConstantConditions settings.setProperty("sonar.auth.aad.clientSecret.secured", (String) null); - settings.setProperty("sonar.auth.aad.loginStrategy", LOGIN_STRATEGY_DEFAULT_VALUE); assertThat(underTest.isEnabled()).isFalse(); } - @Test - public void default_login_strategy_is_unique_login() { - assertThat(underTest.loginStrategy().orElse(null)).isEqualTo(AadSettings.LOGIN_STRATEGY_UNIQUE); - } - @Test public void return_client_id() { settings.setProperty("sonar.auth.aad.clientId.secured", "id"); @@ -160,7 +160,7 @@ public void return_authority_url() { // Just do a quick test with the global location to verify the URL is built properly settings.setProperty("sonar.auth.aad.directoryLocation", "Azure AD (Global)"); settings.setProperty("sonar.auth.aad.tenantId", " 123e4567-e89b-12d3-a456-426655440000"); - assertThat(underTest.authorityUrl()).isEqualTo("https://login.microsoftonline.com/123e4567-e89b-12d3-a456-426655440000/oauth2/token"); + assertThat(underTest.authorityUrl()).isEqualTo("https://login.microsoftonline.com/123e4567-e89b-12d3-a456-426655440000/oauth2/v2.0/token"); } @Test @@ -174,6 +174,6 @@ public void allow_users_to_sign_up() { @Test public void definitions() { - assertThat(AadSettings.definitions()).hasSize(10); + assertThat(AadSettings.definitions()).hasSize(9); } } diff --git a/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java b/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java new file mode 100644 index 0000000..a3c37fe --- /dev/null +++ b/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java @@ -0,0 +1,101 @@ +/** + * Azure Active Directory Authentication Plugin for SonarQube + *

+ * Copyright (c) 2022 Michael Johnson + *

+ * The MIT License (MIT) + *

+ * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + *

+ * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + *

+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ +package org.almrangers.auth.aad; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.text.ParseException; +import java.util.*; + +import com.nimbusds.jwt.JWTClaimsSet; +import com.nimbusds.jwt.PlainJWT; +import com.nimbusds.oauth2.sdk.token.BearerAccessToken; +import org.junit.Before; +import org.junit.Test; + +public class AadUserInfoTest { + + String testTennantId = "ff4d5470-f7f3-4603-900d-cb291dc340bd"; + String testAudience = "fbfc665d-79c1-45b6-aa56-d66c3d64f63c"; + String testSubject = "0.AQPR2ObdWaKTqn-ixSd3lY55gUGQJKwcOixphE53Gb9sTyiO2sv."; + String testOid = "377ae852-940f-4e0a-b154-563b7427a3dc"; + String testUserMail = "john.doe@example.com"; + String testUserName = "John Doe"; + + PlainJWT testIdToken; + AadUserInfo userInfo; + + @Before + public void setUp() { + // Get current date/time for the test token + Calendar testCalendar = Calendar.getInstance(); + Date currentTestDate = testCalendar.getTime(); + + // Set up a 1 hour expiration for the test token + testCalendar.add(Calendar.HOUR, 1); + Date expirationTestDate = testCalendar.getTime(); + + + // This token is based on the ID token returned from MS ID Platform OAuth v2.0 + // see: https://github.com/MicrosoftDocs/azure-docs/blob/main/articles/active-directory/develop/id-tokens.md + testIdToken = new PlainJWT( + new JWTClaimsSet.Builder() + .audience(testAudience) + .issuer("https://login.microsoftonline.com/" + testTennantId + "/v2.0") + .issueTime(currentTestDate) + .notBeforeTime(currentTestDate) + .expirationTime(expirationTestDate) + .claim("email", testUserMail) + .claim("name", testUserName) + .claim("oid", testOid) + .claim("preferred_username", testUserMail) + .claim("rh", "ignored") + .subject(testSubject) + .claim("tid", testTennantId) + .claim("uti", "8UZ2eBjwiUDNU_LQSTJWAA") + .claim("ver", "2.0") + .build()); + } + + @Test + public void test_token_parsing() throws ParseException { + userInfo = new AadUserInfo(testIdToken, new BearerAccessToken(), false); + + assertThat(userInfo).isInstanceOf(AadUserInfo.class); + } + + @Test + public void test_token_claims() throws ParseException { + userInfo = new AadUserInfo(testIdToken, new BearerAccessToken(), false); + + assertThat(userInfo.getDisplayId()).isEqualTo(testUserMail); + assertThat(userInfo.getDisplayName()).isEqualTo(testUserName); + assertThat(userInfo.getUserEmail()).isEqualTo(testUserMail); + + // No groups were parsed, so we should get an empty set + assertThat(userInfo.getUserGroups()).isEqualTo(Collections.emptySet()); + } +} diff --git a/src/test/java/org/almrangers/auth/aad/AuthAadPluginTest.java b/src/test/java/org/almrangers/auth/aad/AuthAadPluginTest.java index a842c48..bd6e036 100644 --- a/src/test/java/org/almrangers/auth/aad/AuthAadPluginTest.java +++ b/src/test/java/org/almrangers/auth/aad/AuthAadPluginTest.java @@ -2,7 +2,7 @@ * Azure Active Directory Authentication Plugin for SonarQube *

* Copyright (c) 2016 Microsoft Corporation - * All rights reserved. + * Copyright (c) 2022 Michael Johnson *

* The MIT License (MIT) *

@@ -41,7 +41,7 @@ public class AuthAadPluginTest { @Test public void test_extensions() { - assertThat(this.context.getExtensions()).hasSize(12); + assertThat(this.context.getExtensions()).hasSize(11); } public AuthAadPluginTest() { From 475122bc0bffdb921bf8d302be8ea50b9b51fa7d Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Wed, 2 Nov 2022 13:58:43 -0700 Subject: [PATCH 02/16] Add SonarCloud Scanning Action Add a GitHub action to scan the application on SonarCloud. Update the JaCoCo config to output the report to the default location that SonarQube expects. --- .github/workflows/sonarcloud.yml | 27 +++++++++++++++++++++++++++ pom.xml | 9 ++------- 2 files changed, 29 insertions(+), 7 deletions(-) create mode 100644 .github/workflows/sonarcloud.yml diff --git a/.github/workflows/sonarcloud.yml b/.github/workflows/sonarcloud.yml new file mode 100644 index 0000000..69ef0ba --- /dev/null +++ b/.github/workflows/sonarcloud.yml @@ -0,0 +1,27 @@ +name: Scan with SonarCloud + +on: + push: + +jobs: + build: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v3 + with: + fetch-depth: 0 + - name: Set up AdoptOpenJDK 11 + uses: actions/setup-java@v2 + with: + distribution: 'zulu' + java-version: '11' + - name: Scan with SonarCloud + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + mvn --batch-mode --update-snapshots clean verify sonar:sonar \ + -Dsonar.projectKey="${{ secrets.SONARCLOUD_KEY }}" \ + -Dsonar.organization="${{ secrets.SONARCLOUD_ORG }}" \ + -Dsonar.host.url=https://sonarcloud.io \ + -Dsonar.login="${{ secrets.SONARCLOUD_TOKEN }}" diff --git a/pom.xml b/pom.xml index 1c432c7..f5f4f33 100644 --- a/pom.xml +++ b/pom.xml @@ -146,7 +146,7 @@ org.jacoco jacoco-maven-plugin - 0.8.7 + 0.8.8 prepare-agent @@ -155,15 +155,10 @@ - post-unit-test - test + report report - - target/jacoco.exec - target/jacoco-ut - From 94ffaa38968be9b2a433fa5d2abad0088d94d22e Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Thu, 3 Nov 2022 09:31:00 -0700 Subject: [PATCH 03/16] Update GH Actions Update the GitHub action definitions to use newer JDK and action versions. Adjust the code coverage report location. --- .github/workflows/maven.yml | 17 +++++++++-------- .github/workflows/pr-unit-tests.yml | 13 +++++++------ 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index 6afb917..339687b 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -8,20 +8,21 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - name: Set up JDK 1.8 - uses: actions/setup-java@v1 + - uses: actions/checkout@v3 + - name: Set up AdoptOpenJDK 11 + uses: actions/setup-java@v2 with: - java-version: 1.8 + distribution: 'zulu' + java-version: '11' - name: Build and Test with Maven - run: mvn --batch-mode --update-snapshots package + run: mvn --batch-mode --update-snapshots clean verify package - name: Upload Packaged JAR - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v3 with: name: plugin-binary path: target/*.jar - name: Upload Code Coverage Report - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v3 with: name: code-coverage-report - path: target/jacoco-ut/ + path: target/site/jacoco/ diff --git a/.github/workflows/pr-unit-tests.yml b/.github/workflows/pr-unit-tests.yml index 8dd63ac..b29ae19 100644 --- a/.github/workflows/pr-unit-tests.yml +++ b/.github/workflows/pr-unit-tests.yml @@ -8,15 +8,16 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - name: Set up JDK 1.8 - uses: actions/setup-java@v1 + - uses: actions/checkout@v3 + - name: Set up AdoptOpenJDK 11 + uses: actions/setup-java@v2 with: - java-version: 1.8 + distribution: 'zulu' + java-version: '11' - name: Unit Tests run: mvn --batch-mode --update-snapshots verify - name: Upload Code Coverage Report - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v3 with: name: code-coverage-report - path: target/jacoco-ut/ + path: target/site/jacoco/ From 2709908f9b59618cd3d7a49e6a9bdb6accd2d0df Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Fri, 4 Nov 2022 12:45:02 -0700 Subject: [PATCH 04/16] Fix NPE in ID Token Parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getStringClaim returns null if the claim doesn’t exist. Convert isEmpty check to a null-safe version and check explicitly for null as well. This covers when a claim doesn’t exist (null) and when it exists, but is empty for some reason. --- .../org/almrangers/auth/aad/AadUserInfo.java | 10 +- .../almrangers/auth/aad/AadUserInfoTest.java | 110 +++++++++++++++--- 2 files changed, 98 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java index b4c5414..a166c17 100644 --- a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java +++ b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java @@ -73,13 +73,13 @@ private void parseToken(JWT idToken) throws ParseException { JWTClaimsSet claims = idToken.getJWTClaimsSet(); // User's OID. Used for grabbing group membership if that feature is enabled - if(!claims.getStringClaim("oid").isEmpty()) { + if(claims.getStringClaim("oid") != null) { this.userOid = claims.getStringClaim("oid"); } // Display ID // Tries the "preferred username" first, and falls back to email - if(!claims.getStringClaim("preferred_username").isEmpty()) { + if(!"".equals(claims.getStringClaim("preferred_username")) && claims.getStringClaim("preferred_username") != null) { this.displayId = claims.getStringClaim("preferred_username"); } else if(!claims.getStringClaim("email").isEmpty()) { this.displayId = claims.getStringClaim("email"); @@ -89,7 +89,7 @@ private void parseToken(JWT idToken) throws ParseException { // Attempts to get the user's name from the name claim. AAD requires // this, so it can't be blank. To be safe, we still set a display // name if that claim isn't in the token for some reason. - if(!claims.getStringClaim("name").isEmpty()) { + if(!"".equals(claims.getStringClaim("name")) && claims.getStringClaim("name") != null) { this.displayName = claims.getStringClaim("name"); } else { this.displayName = "No name provided"; @@ -97,9 +97,9 @@ private void parseToken(JWT idToken) throws ParseException { // Email // Tries email first, and falls back to "preferred_username" if empty. This should work for most AAD installs. - if(!claims.getStringClaim("email").isEmpty()) { + if(!"".equals(claims.getStringClaim("email")) && claims.getStringClaim("email") != null) { this.userEmail = claims.getStringClaim("email"); - } else if(!claims.getStringClaim("preferred_username").isEmpty()) { + } else if(claims.getStringClaim("preferred_username") != null) { this.userEmail = claims.getStringClaim("preferred_username"); } } diff --git a/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java b/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java index a3c37fe..a214b0a 100644 --- a/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java +++ b/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java @@ -43,11 +43,51 @@ public class AadUserInfoTest { String testSubject = "0.AQPR2ObdWaKTqn-ixSd3lY55gUGQJKwcOixphE53Gb9sTyiO2sv."; String testOid = "377ae852-940f-4e0a-b154-563b7427a3dc"; String testUserMail = "john.doe@example.com"; + String testUserUsername = "john.doe@example.net"; String testUserName = "John Doe"; + // All the different tokens we need to test. + // The "No*" tokens are to test specific logic in the token parsing for user info. PlainJWT testIdToken; + PlainJWT testIdTokenNoName; + PlainJWT testIdTokenNoMail; + PlainJWT testIdTokenNoUsername; AadUserInfo userInfo; + @Test + public void test_token_parsing() throws ParseException { + userInfo = new AadUserInfo(testIdToken, new BearerAccessToken(), false); + + assertThat(userInfo).isInstanceOf(AadUserInfo.class); + } + + @Test + public void test_token_claims() throws ParseException { + userInfo = new AadUserInfo(testIdToken, new BearerAccessToken(), false); + + assertThat(userInfo.getDisplayId()).isEqualTo(testUserUsername); + assertThat(userInfo.getDisplayName()).isEqualTo(testUserName); + assertThat(userInfo.getUserEmail()).isEqualTo(testUserMail); + + // No groups were parsed, so we should get an empty set + assertThat(userInfo.getUserGroups()).isEqualTo(Collections.emptySet()); + + // Test for the "no name claim" scenario + userInfo = new AadUserInfo(testIdTokenNoName, new BearerAccessToken(), false); + + assertThat(userInfo.getDisplayName()).isEqualTo("No name provided"); + + // Test for the "no email claim" scenario + userInfo = new AadUserInfo(testIdTokenNoMail, new BearerAccessToken(), false); + + assertThat(userInfo.getUserEmail()).isEqualTo(testUserUsername); + + // Test for the "no username claim" scenario + userInfo = new AadUserInfo(testIdTokenNoUsername, new BearerAccessToken(), false); + + assertThat(userInfo.getDisplayId()).isEqualTo(testUserMail); + } + @Before public void setUp() { // Get current date/time for the test token @@ -71,31 +111,67 @@ public void setUp() { .claim("email", testUserMail) .claim("name", testUserName) .claim("oid", testOid) - .claim("preferred_username", testUserMail) + .claim("preferred_username", testUserUsername) .claim("rh", "ignored") .subject(testSubject) .claim("tid", testTennantId) .claim("uti", "8UZ2eBjwiUDNU_LQSTJWAA") .claim("ver", "2.0") .build()); - } - @Test - public void test_token_parsing() throws ParseException { - userInfo = new AadUserInfo(testIdToken, new BearerAccessToken(), false); - - assertThat(userInfo).isInstanceOf(AadUserInfo.class); - } - - @Test - public void test_token_claims() throws ParseException { - userInfo = new AadUserInfo(testIdToken, new BearerAccessToken(), false); + // This is to specifically test when no name is passed in the ID token + // (This should be impossible, as name is required for accounts.) + testIdTokenNoName = new PlainJWT( + new JWTClaimsSet.Builder() + .audience(testAudience) + .issuer("https://login.microsoftonline.com/" + testTennantId + "/v2.0") + .issueTime(currentTestDate) + .notBeforeTime(currentTestDate) + .expirationTime(expirationTestDate) + .claim("email", testUserMail) + .claim("oid", testOid) + .claim("preferred_username", testUserUsername) + .claim("rh", "ignored") + .subject(testSubject) + .claim("tid", testTennantId) + .claim("uti", "8UZ2eBjwiUDNU_LQSTJWAA") + .claim("ver", "2.0") + .build()); - assertThat(userInfo.getDisplayId()).isEqualTo(testUserMail); - assertThat(userInfo.getDisplayName()).isEqualTo(testUserName); - assertThat(userInfo.getUserEmail()).isEqualTo(testUserMail); + // This is to test when no email claim is passed in the ID token. + testIdTokenNoMail = new PlainJWT( + new JWTClaimsSet.Builder() + .audience(testAudience) + .issuer("https://login.microsoftonline.com/" + testTennantId + "/v2.0") + .issueTime(currentTestDate) + .notBeforeTime(currentTestDate) + .expirationTime(expirationTestDate) + .claim("name", testUserName) + .claim("oid", testOid) + .claim("preferred_username", testUserUsername) + .claim("rh", "ignored") + .subject(testSubject) + .claim("tid", testTennantId) + .claim("uti", "8UZ2eBjwiUDNU_LQSTJWAA") + .claim("ver", "2.0") + .build()); - // No groups were parsed, so we should get an empty set - assertThat(userInfo.getUserGroups()).isEqualTo(Collections.emptySet()); + // This is to test when the preferred_username claim isn't present in the ID token. + testIdTokenNoUsername = new PlainJWT( + new JWTClaimsSet.Builder() + .audience(testAudience) + .issuer("https://login.microsoftonline.com/" + testTennantId + "/v2.0") + .issueTime(currentTestDate) + .notBeforeTime(currentTestDate) + .expirationTime(expirationTestDate) + .claim("email", testUserMail) + .claim("name", testUserName) + .claim("oid", testOid) + .claim("rh", "ignored") + .subject(testSubject) + .claim("tid", testTennantId) + .claim("uti", "8UZ2eBjwiUDNU_LQSTJWAA") + .claim("ver", "2.0") + .build()); } } From abf81d53fb8e9a6356c0afeeeac3e30ac0e5914c Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Tue, 8 Nov 2022 14:11:26 -0800 Subject: [PATCH 05/16] Refactor for Better Testing Break apart some of the more complex code into individual methods to improve readability/understandability and help with unit testing. --- pom.xml | 8 +- .../auth/aad/AadIdentityProvider.java | 104 ++--------- .../almrangers/auth/aad/AadTokenHelper.java | 118 ++++++++++++ .../org/almrangers/auth/aad/AadUserInfo.java | 42 +++-- .../auth/aad/AadIdentityProviderTest.java | 5 +- .../auth/aad/AadTokenHelperTest.java | 169 ++++++++++++++++++ .../almrangers/auth/aad/AadUserInfoTest.java | 15 ++ 7 files changed, 356 insertions(+), 105 deletions(-) create mode 100644 src/main/java/org/almrangers/auth/aad/AadTokenHelper.java create mode 100644 src/test/java/org/almrangers/auth/aad/AadTokenHelperTest.java diff --git a/pom.xml b/pom.xml index f5f4f33..568228f 100644 --- a/pom.xml +++ b/pom.xml @@ -139,6 +139,12 @@ + + com.squareup.okhttp3 + mockwebserver + 4.10.0 + test + @@ -165,7 +171,7 @@ org.sonarsource.sonar-packaging-maven-plugin sonar-packaging-maven-plugin - 1.20.0.405 + 1.21.0.505 true org.almrangers.auth.aad.AuthAadPlugin diff --git a/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java b/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java index f15d919..621a7f0 100644 --- a/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java +++ b/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java @@ -28,34 +28,23 @@ package org.almrangers.auth.aad; import java.net.*; -import java.util.Arrays; -import java.util.HashSet; import java.util.concurrent.ExecutorService; import javax.servlet.http.HttpServletRequest; -import com.nimbusds.jose.JOSEException; -import com.nimbusds.jose.JWSAlgorithm; -import com.nimbusds.jose.jwk.source.JWKSource; -import com.nimbusds.jose.jwk.source.RemoteJWKSet; -import com.nimbusds.jose.proc.*; import com.nimbusds.jwt.JWT; -import com.nimbusds.jwt.proc.ConfigurableJWTProcessor; -import com.nimbusds.jwt.proc.DefaultJWTClaimsVerifier; -import com.nimbusds.jwt.proc.DefaultJWTProcessor; import com.nimbusds.oauth2.sdk.*; import com.nimbusds.oauth2.sdk.auth.ClientSecretBasic; import com.nimbusds.oauth2.sdk.auth.Secret; -import com.nimbusds.oauth2.sdk.http.HTTPResponse; import com.nimbusds.oauth2.sdk.id.ClientID; import com.nimbusds.oauth2.sdk.id.State; import com.nimbusds.oauth2.sdk.token.AccessToken; +import com.nimbusds.oauth2.sdk.token.BearerAccessToken; import com.nimbusds.openid.connect.sdk.OIDCTokenResponse; import com.nimbusds.openid.connect.sdk.token.OIDCTokens; import org.sonar.api.server.ServerSide; import org.sonar.api.server.authentication.Display; import org.sonar.api.server.authentication.OAuth2IdentityProvider; import org.sonar.api.server.authentication.UnauthorizedException; -import org.sonar.api.server.authentication.UserIdentity; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; @@ -154,39 +143,22 @@ private void onCallback(CallbackContext context) throws UnauthorizedException { new AuthorizationCodeGrant(code, new URI(context.getCallbackUrl())) ); - HTTPResponse tokenHTTPResp = tokenReq.toHTTPRequest().send(); - // Parse and check response - OIDCTokenResponse tokenResponse; - - try { - tokenResponse = OIDCTokenResponse.parse(tokenHTTPResp); - } catch (ParseException e) { - //If we got an error in the token process, this will catch it and log the details. - if(!tokenHTTPResp.indicatesSuccess()) { - TokenErrorResponse tokenErrorResponse = TokenResponse.parse(tokenHTTPResp).toErrorResponse(); - - LOGGER.error("Issue getting authentication token. Returned error: " - + tokenErrorResponse.getErrorObject().getDescription()); - - throw new UnauthorizedException("Error when authenticating user. Please check the logs for more details."); - } else { - // Some other error happened, throw the error message directly. - throw new UnauthorizedException(e.getMessage()); - } - } + OIDCTokenResponse tokenResponse = AadTokenHelper.extractTokenResponse(tokenReq.toHTTPRequest().send()); OIDCTokens accessTokens = tokenResponse.getOIDCTokens(); JWT idToken = accessTokens.getIDToken(); - AccessToken userAccessToken = accessTokens.getAccessToken(); AadUserInfo aadUser; - AccessToken accessToken = userAccessToken; + AccessToken accessToken; // The user's auth token as the default. + + if (AadTokenHelper.validateIdToken(idToken, settings)) { - if (validateIdToken(idToken)) { - // If group sync is enabled and client credential flow is enabled, - // get a client access token we will use to fetch group membership + // Decide if we are going to use a user auth token or the client auth + // token. This is used for group sync for access to MS Graph. If client + // credential is enabled along with group sync, try to grab a client auth + // token. Otherwise we just set the token to the user token. if(settings.enableGroupSync() && settings.enableClientCredential()) { TokenRequest clientRequest = new TokenRequest( new URI(settings.authorityUrl()), @@ -197,6 +169,7 @@ private void onCallback(CallbackContext context) throws UnauthorizedException { new ClientCredentialsGrant(), new Scope(settings.getGraphURL() + "/.default")); + // Parse and check response TokenResponse clientResponse = TokenResponse.parse(clientRequest.toHTTPRequest().send()); // Client token request failed, log the error @@ -204,10 +177,14 @@ private void onCallback(CallbackContext context) throws UnauthorizedException { TokenErrorResponse errorResponse = clientResponse.toErrorResponse(); LOGGER.error("Issue in getting client token for group sync. Returned error: " + errorResponse.getErrorObject().getDescription()); + accessToken = new BearerAccessToken(); // Empty access token so we pass _something_. } else { AccessTokenResponse successResponse = clientResponse.toSuccessResponse(); accessToken = successResponse.getTokens().getAccessToken(); } + } else { + // Use the user's access token + accessToken = accessTokens.getAccessToken(); } // NOTE: The Access token IS EITHER: @@ -215,9 +192,7 @@ private void onCallback(CallbackContext context) throws UnauthorizedException { // The user's token if client credential flow fails or client flow is disabled aadUser = new AadUserInfo(idToken, accessToken, settings.enableGroupSync()); - UserIdentity.Builder userIdentity = parseUserId(aadUser); - - context.authenticate(userIdentity.build()); + context.authenticate(aadUser.buildUserId(settings.enableGroupSync()).build()); context.redirectToRequestedPage(); } @@ -232,53 +207,4 @@ private void onCallback(CallbackContext context) throws UnauthorizedException { } } - private boolean validateIdToken(JWT idToken) throws MalformedURLException, BadJOSEException, JOSEException { - - // Create a JWT processor for the access tokens - ConfigurableJWTProcessor jwtProcessor = - new DefaultJWTProcessor<>(); - - JWKSource keySource = - new RemoteJWKSet<>(new URL(settings.jwkKeysUrl())); - - //MS uses RSA 256 to sign their JWTs - JWSAlgorithm expectedJWSAlg = JWSAlgorithm.RS256; - - JWSKeySelector keySelector = - new JWSVerificationKeySelector<>(expectedJWSAlg, keySource); - - jwtProcessor.setJWSKeySelector(keySelector); - - // Verify the specific claims in the ID token. Confirm the audience matches - // the expected value (our Client ID), and that specific attributes are present. - // Note that this also automatically validates the various timestamp values - // to ensure the token is still valid. - jwtProcessor.setJWTClaimsSetVerifier( - new DefaultJWTClaimsVerifier<>( - settings.clientId().orElse(null), - null, - new HashSet<>(Arrays.asList("iss", "iat", "nbf", "exp", "oid", "name", "preferred_username", "sub", "tid")) - ) - ); - - // Don't capture the output. This will throw an error if the token doesn't - // validate instead of returning true. - jwtProcessor.process(idToken, null); - - return true; // If there was no exception thrown, then the token is valid - } - - private UserIdentity.Builder parseUserId(AadUserInfo aadUser) { - UserIdentity.Builder userIdentityBuilder = UserIdentity.builder() - .setProviderLogin(aadUser.getDisplayId()) - .setName(aadUser.getDisplayName()) - .setEmail(aadUser.getUserEmail()); - - if (settings.enableGroupSync()) { - userIdentityBuilder.setGroups(aadUser.getUserGroups()); - } - - return userIdentityBuilder; - } - } diff --git a/src/main/java/org/almrangers/auth/aad/AadTokenHelper.java b/src/main/java/org/almrangers/auth/aad/AadTokenHelper.java new file mode 100644 index 0000000..4b4ff79 --- /dev/null +++ b/src/main/java/org/almrangers/auth/aad/AadTokenHelper.java @@ -0,0 +1,118 @@ +/** + * Azure Active Directory Authentication Plugin for SonarQube + *

+ * Copyright (c) 2022 Michael Johnson + *

+ * The MIT License (MIT) + *

+ * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + *

+ * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + *

+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ +package org.almrangers.auth.aad; + +import com.nimbusds.jose.JOSEException; +import com.nimbusds.jose.JWSAlgorithm; +import com.nimbusds.jose.jwk.source.JWKSource; +import com.nimbusds.jose.jwk.source.RemoteJWKSet; +import com.nimbusds.jose.proc.BadJOSEException; +import com.nimbusds.jose.proc.JWSKeySelector; +import com.nimbusds.jose.proc.JWSVerificationKeySelector; +import com.nimbusds.jose.proc.SecurityContext; +import com.nimbusds.jwt.JWT; +import com.nimbusds.jwt.proc.ConfigurableJWTProcessor; +import com.nimbusds.jwt.proc.DefaultJWTClaimsVerifier; +import com.nimbusds.jwt.proc.DefaultJWTProcessor; +import com.nimbusds.oauth2.sdk.ParseException; +import com.nimbusds.oauth2.sdk.TokenErrorResponse; +import com.nimbusds.oauth2.sdk.TokenResponse; +import com.nimbusds.oauth2.sdk.http.HTTPResponse; +import com.nimbusds.openid.connect.sdk.OIDCTokenResponse; +import org.sonar.api.server.authentication.UnauthorizedException; +import org.sonar.api.utils.log.Logger; +import org.sonar.api.utils.log.Loggers; + +import java.net.MalformedURLException; +import java.net.URL; +import java.util.Arrays; +import java.util.HashSet; + +public class AadTokenHelper { + + private static final Logger LOGGER = Loggers.get(AadTokenHelper.class); + + AadTokenHelper() { + throw new IllegalStateException("This is a utility class, do not instantiate it."); + } + + public static boolean validateIdToken(JWT idToken, AadSettings settings) throws MalformedURLException, BadJOSEException, JOSEException { + + // Create a JWT processor for the access tokens + ConfigurableJWTProcessor jwtProcessor = + new DefaultJWTProcessor<>(); + + JWKSource keySource = + new RemoteJWKSet<>(new URL(settings.jwkKeysUrl())); + + //MS uses RSA 256 to sign their JWTs + JWSAlgorithm expectedJWSAlg = JWSAlgorithm.RS256; + + JWSKeySelector keySelector = + new JWSVerificationKeySelector<>(expectedJWSAlg, keySource); + + jwtProcessor.setJWSKeySelector(keySelector); + + // Verify the specific claims in the ID token. Confirm the audience matches + // the expected value (our Client ID), and that specific attributes are present. + // Note that this also automatically validates the various timestamp values + // to ensure the token is still valid. + jwtProcessor.setJWTClaimsSetVerifier( + new DefaultJWTClaimsVerifier<>( + settings.clientId().orElse(null), + null, + new HashSet<>(Arrays.asList("iss", "iat", "nbf", "exp", "oid", "name", "preferred_username", "sub", "tid")) + ) + ); + + // Don't capture the output. This will throw an error if the token doesn't + // validate instead of returning true. + jwtProcessor.process(idToken, null); + + return true; // If there was no exception thrown, then the token is valid + } + + public static OIDCTokenResponse extractTokenResponse(HTTPResponse tokenHTTPResp) { + OIDCTokenResponse tokenResponse; + + try { + //If we got an error in the token process, this will catch it and log the details. + if(!tokenHTTPResp.indicatesSuccess()) { + TokenErrorResponse tokenErrorResponse = TokenResponse.parse(tokenHTTPResp).toErrorResponse(); + + LOGGER.error("Issue getting authentication token. Returned error: " + + tokenErrorResponse.getErrorObject().getDescription()); + } + + tokenResponse = OIDCTokenResponse.parse(tokenHTTPResp); + + } catch (ParseException e) { + throw new UnauthorizedException("Error when authenticating user. Please check the logs for more details."); + } + + return tokenResponse; + } +} diff --git a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java index a166c17..d1a72d8 100644 --- a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java +++ b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java @@ -37,6 +37,7 @@ import com.nimbusds.jwt.JWTClaimsSet; import com.nimbusds.oauth2.sdk.token.AccessToken; import okhttp3.Request; +import org.sonar.api.server.authentication.UserIdentity; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; import reactor.util.annotation.NonNull; @@ -50,6 +51,10 @@ import java.util.concurrent.CompletableFuture; public class AadUserInfo { + private final String USERNAME_CLAIM = "preferred_username"; + private final String EMAIL_CLAIM = "email"; + private final String DISPLAYNAME_CLAIM = "name"; + private String userOid; private String displayId; private String displayName; @@ -79,31 +84,46 @@ private void parseToken(JWT idToken) throws ParseException { // Display ID // Tries the "preferred username" first, and falls back to email - if(!"".equals(claims.getStringClaim("preferred_username")) && claims.getStringClaim("preferred_username") != null) { - this.displayId = claims.getStringClaim("preferred_username"); - } else if(!claims.getStringClaim("email").isEmpty()) { - this.displayId = claims.getStringClaim("email"); + if(!"".equals(claims.getStringClaim(USERNAME_CLAIM)) && claims.getStringClaim(USERNAME_CLAIM) != null) { + this.displayId = claims.getStringClaim(USERNAME_CLAIM); + } else if(!claims.getStringClaim(EMAIL_CLAIM).isEmpty()) { + this.displayId = claims.getStringClaim(EMAIL_CLAIM); } // Display Name // Attempts to get the user's name from the name claim. AAD requires // this, so it can't be blank. To be safe, we still set a display // name if that claim isn't in the token for some reason. - if(!"".equals(claims.getStringClaim("name")) && claims.getStringClaim("name") != null) { - this.displayName = claims.getStringClaim("name"); + if(!"".equals(claims.getStringClaim(DISPLAYNAME_CLAIM)) && claims.getStringClaim(DISPLAYNAME_CLAIM) != null) { + this.displayName = claims.getStringClaim(DISPLAYNAME_CLAIM); } else { this.displayName = "No name provided"; } // Email - // Tries email first, and falls back to "preferred_username" if empty. This should work for most AAD installs. - if(!"".equals(claims.getStringClaim("email")) && claims.getStringClaim("email") != null) { - this.userEmail = claims.getStringClaim("email"); - } else if(claims.getStringClaim("preferred_username") != null) { - this.userEmail = claims.getStringClaim("preferred_username"); + // Tries email first, and falls back to "preferred_username" if empty. + // This should work for most AAD installs. + if(!"".equals(claims.getStringClaim(EMAIL_CLAIM)) && claims.getStringClaim(EMAIL_CLAIM) != null) { + this.userEmail = claims.getStringClaim(EMAIL_CLAIM); + } else if(claims.getStringClaim(USERNAME_CLAIM) != null) { + this.userEmail = claims.getStringClaim(USERNAME_CLAIM); } } + public UserIdentity.Builder buildUserId(boolean includeGroups) { + UserIdentity.Builder userIdentityBuilder = UserIdentity.builder() + .setProviderLogin(getDisplayId()) + .setName(getDisplayName()) + .setEmail(getUserEmail()); + + if (includeGroups) { + userIdentityBuilder.setGroups(getUserGroups()); + } + + return userIdentityBuilder; + } + + private void processGroups(String accessToken) { Set parsedUserGroups = new HashSet<>(); diff --git a/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java b/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java index 0a08ed0..99e38ec 100644 --- a/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java +++ b/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java @@ -27,10 +27,7 @@ package org.almrangers.auth.aad; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; import okhttp3.HttpUrl; import org.junit.Rule; diff --git a/src/test/java/org/almrangers/auth/aad/AadTokenHelperTest.java b/src/test/java/org/almrangers/auth/aad/AadTokenHelperTest.java new file mode 100644 index 0000000..640fd8d --- /dev/null +++ b/src/test/java/org/almrangers/auth/aad/AadTokenHelperTest.java @@ -0,0 +1,169 @@ +/** + * Azure Active Directory Authentication Plugin for SonarQube + *

+ * Copyright (c) 2022 Michael Johnson + *

+ * The MIT License (MIT) + *

+ * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + *

+ * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + *

+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ +package org.almrangers.auth.aad; + +import com.nimbusds.jose.JOSEException; +import com.nimbusds.jose.JWSAlgorithm; +import com.nimbusds.jose.JWSHeader; +import com.nimbusds.jose.crypto.RSASSASigner; +import com.nimbusds.jose.jwk.RSAKey; +import com.nimbusds.jose.jwk.gen.RSAKeyGenerator; +import com.nimbusds.jose.proc.BadJOSEException; +import com.nimbusds.jwt.JWTClaimsSet; +import com.nimbusds.jwt.SignedJWT; +import com.nimbusds.oauth2.sdk.http.HTTPResponse; +import com.nimbusds.oauth2.sdk.token.AccessToken; +import com.nimbusds.oauth2.sdk.token.BearerAccessToken; +import com.nimbusds.oauth2.sdk.token.RefreshToken; +import com.nimbusds.openid.connect.sdk.OIDCTokenResponse; +import com.nimbusds.openid.connect.sdk.token.OIDCTokens; +import okhttp3.HttpUrl; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import org.junit.Before; +import org.junit.Test; +import org.sonar.api.config.internal.MapSettings; +import org.sonar.api.server.authentication.UnauthorizedException; + +import java.io.IOException; +import java.text.ParseException; +import java.util.Calendar; +import java.util.Date; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.spy; + +public class AadTokenHelperTest { + + SignedJWT testIdToken; + AccessToken testAccessToken = new BearerAccessToken(); + MapSettings settings = new MapSettings(); + AadSettings aadSettings = new AadSettings(settings.asConfig()); + RSAKey rsaPublicJwk; + + @Test + public void exception_on_instantiate() { + assertThrows(IllegalStateException.class, AadTokenHelper::new); + } + + @Test + public void extract_oidc_tokens_success() { + // Test OIDC Tokens + OIDCTokens oidcTokens = new OIDCTokens(testIdToken, testAccessToken, new RefreshToken()); + + // "OK" token response + HTTPResponse goodHttpResponse = new HTTPResponse(200); + goodHttpResponse.setHeader("Content-Type", "application/json"); + goodHttpResponse.setContent(oidcTokens.toString()); + + assertThat(AadTokenHelper.extractTokenResponse(goodHttpResponse)).isInstanceOf(OIDCTokenResponse.class); + } + + @Test + public void extract_oidc_tokens_failure() { + // "Bad" token response + HTTPResponse badHttpResponse = new HTTPResponse(400); + badHttpResponse.setHeader("Content-Type", "application/json"); + badHttpResponse.setContent("{\"error\": \"invalid_request\"}"); + + UnauthorizedException unauthorizedException = assertThrows(UnauthorizedException.class, + () -> AadTokenHelper.extractTokenResponse(badHttpResponse)); + + assertTrue(unauthorizedException.getMessage().contains("Error when authenticating user")); + } + + @Test + public void validate_id_token() throws IOException, BadJOSEException, JOSEException { + MockWebServer mockWebServer = new MockWebServer(); + mockWebServer.enqueue( + new MockResponse().setBody("{\"keys\": [" + + rsaPublicJwk.toJSONString() + + "]}") + ); + + mockWebServer.start(); + + HttpUrl baseUrl = mockWebServer.url("/common/discovery/keys"); + + AadSettings spySettings = spy(aadSettings); + doReturn(baseUrl.toString()).when(spySettings).jwkKeysUrl(); + + assertThat(AadTokenHelper.validateIdToken(testIdToken, spySettings)).isTrue(); + + mockWebServer.close(); + } + + @Before + public void setUp() throws JOSEException { + // Some needed settings used by the tests + settings.setProperty("sonar.auth.aad.tenantId", "common"); + + // Get current date/time for the test token + Calendar testCalendar = Calendar.getInstance(); + Date currentTestDate = testCalendar.getTime(); + + // Set up a 1 hour expiration for the test token + testCalendar.add(Calendar.HOUR, 1); + Date expirationTestDate = testCalendar.getTime(); + + RSAKey rsaKey = new RSAKeyGenerator(2048) + .keyID("123") + .generate(); + + // Save the public key for verification + rsaPublicJwk = rsaKey.toPublicJWK(); + + RSASSASigner rsaSigner = new RSASSASigner(rsaKey); + + // This token is based on the ID token returned from MS ID Platform OAuth v2.0 + // see: https://github.com/MicrosoftDocs/azure-docs/blob/main/articles/active-directory/develop/id-tokens.md + testIdToken = new SignedJWT( + new JWSHeader.Builder(JWSAlgorithm.RS256) + .build(), + new JWTClaimsSet.Builder() + .audience("testAudience") + .issuer("https://login.microsoftonline.com/" + "testTennantId" + "/v2.0") + .issueTime(currentTestDate) + .notBeforeTime(currentTestDate) + .expirationTime(expirationTestDate) + .claim("email", "testUserMail") + .claim("name", "testUserName") + .claim("oid", "testOid") + .claim("preferred_username", "testUserUsername") + .claim("rh", "ignored") + .subject("testSubject") + .claim("tid", "testTennantId") + .claim("uti", "8UZ2eBjwiUDNU_LQSTJWAA") + .claim("ver", "2.0") + .build() + ); + + testIdToken.sign(rsaSigner); + } +} diff --git a/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java b/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java index a214b0a..53b72bf 100644 --- a/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java +++ b/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java @@ -35,6 +35,7 @@ import com.nimbusds.oauth2.sdk.token.BearerAccessToken; import org.junit.Before; import org.junit.Test; +import org.sonar.api.server.authentication.UserIdentity; public class AadUserInfoTest { @@ -88,6 +89,20 @@ public void test_token_claims() throws ParseException { assertThat(userInfo.getDisplayId()).isEqualTo(testUserMail); } + @Test + public void returns_user_id_builder() throws ParseException { + userInfo = new AadUserInfo(testIdToken, new BearerAccessToken(), false); + + UserIdentity userId = userInfo.buildUserId(true).build(); + + assertThat(userId.getEmail()).isEqualTo(testUserMail); + assertThat(userId.getName()).isEqualTo(testUserName); + + // Since no groups were passed in to build the user, + // getting the groups should return an empty set. + assertThat(userId.getGroups()).isEqualTo(Collections.emptySet()); + } + @Before public void setUp() { // Get current date/time for the test token From b0df1fe24489d7103d00c1e3dcf32ad0a97a85c6 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Tue, 15 Nov 2022 11:37:27 -0800 Subject: [PATCH 06/16] More Unit Tests Add unit testing for the group parsing code. --- .../org/almrangers/auth/aad/AadUserInfo.java | 50 ++++++++------- .../almrangers/auth/aad/AadUserInfoTest.java | 61 +++++++++++++++++++ 2 files changed, 89 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java index d1a72d8..b629bcd 100644 --- a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java +++ b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java @@ -41,6 +41,7 @@ import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; import reactor.util.annotation.NonNull; +import reactor.util.annotation.Nullable; import java.net.URL; import java.text.ParseException; @@ -125,8 +126,6 @@ public UserIdentity.Builder buildUserId(boolean includeGroups) { private void processGroups(String accessToken) { - Set parsedUserGroups = new HashSet<>(); - // We already have the auth code, so create a custom provider that will // return the code to our graph client. IAuthenticationProvider graphAuthProvider = new IAuthenticationProvider() { @@ -154,27 +153,8 @@ public CompletableFuture getAuthorizationTokenAsync(URL requestUrl) { .top(999) // Maximum page size of 999 to reduce number of requests. .get(); - while(memberGroupCollection != null) { - final List groupList = memberGroupCollection.getCurrentPage(); - - for ( DirectoryObject group : groupList) { - String groupDisplayName = ((Group) group).displayName; - - // Don't add the group if the display name is null - if(groupDisplayName != null) { - parsedUserGroups.add(groupDisplayName); - } - } - - final DirectoryObjectCollectionWithReferencesRequestBuilder nextPage = memberGroupCollection.getNextPage(); - if (nextPage == null) { - break; - } else { - memberGroupCollection = nextPage.buildRequest().get(); - } - } + Set parsedUserGroups = processMemberGroupCollection(memberGroupCollection); - // Log a warning if we were asked to parse groups but couldn't if(parsedUserGroups.isEmpty()) { LOGGER.warn("Group list was empty. Maybe your AAD permissions aren't set correctly?"); } @@ -186,6 +166,32 @@ public CompletableFuture getAuthorizationTokenAsync(URL requestUrl) { } } + Set processMemberGroupCollection(@Nullable DirectoryObjectCollectionWithReferencesPage memberGroupCollection) { + Set parsedUserGroups = new HashSet<>(); + + while(memberGroupCollection != null) { + final List groupList = memberGroupCollection.getCurrentPage(); + + for ( DirectoryObject group : groupList) { + String groupDisplayName = ((Group) group).displayName; + + // Don't add the group if the display name is null + if(groupDisplayName != null) { + parsedUserGroups.add(groupDisplayName); + } + } + + final DirectoryObjectCollectionWithReferencesRequestBuilder nextPage = memberGroupCollection.getNextPage(); + if (nextPage == null) { + break; + } else { + memberGroupCollection = nextPage.buildRequest().get(); + } + } + + return parsedUserGroups; + } + public String getDisplayId() { return displayId; } diff --git a/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java b/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java index 53b72bf..6b72683 100644 --- a/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java +++ b/src/test/java/org/almrangers/auth/aad/AadUserInfoTest.java @@ -30,6 +30,12 @@ import java.text.ParseException; import java.util.*; +import com.google.gson.Gson; +import com.google.gson.JsonObject; +import com.microsoft.graph.logger.DefaultLogger; +import com.microsoft.graph.requests.DirectoryObjectCollectionResponse; +import com.microsoft.graph.requests.DirectoryObjectCollectionWithReferencesPage; +import com.microsoft.graph.serializer.DefaultSerializer; import com.nimbusds.jwt.JWTClaimsSet; import com.nimbusds.jwt.PlainJWT; import com.nimbusds.oauth2.sdk.token.BearerAccessToken; @@ -55,6 +61,9 @@ public class AadUserInfoTest { PlainJWT testIdTokenNoUsername; AadUserInfo userInfo; + // A group object for testing the group parser + DirectoryObjectCollectionWithReferencesPage memberGroupCollection; + @Test public void test_token_parsing() throws ParseException { userInfo = new AadUserInfo(testIdToken, new BearerAccessToken(), false); @@ -103,8 +112,32 @@ public void returns_user_id_builder() throws ParseException { assertThat(userId.getGroups()).isEqualTo(Collections.emptySet()); } + @Test + // This tests that parsing will still work without errors in the event that the + // group fetch doesn't work. For this test, it's because of an invalid token. + public void test_empty_group_parsing() throws ParseException { + userInfo = new AadUserInfo(testIdToken, new BearerAccessToken(), true); + + assertThat(userInfo).isInstanceOf(AadUserInfo.class); + assertThat(userInfo.getUserGroups()).isEqualTo(Collections.emptySet()); + } + + @Test + public void parse_member_groups() throws ParseException { + Set expectedGroups = new HashSet<>(Arrays.asList("Administrators", "Developers")); + + userInfo = new AadUserInfo(testIdToken, new BearerAccessToken(), false); + + Set memberships = userInfo.processMemberGroupCollection(memberGroupCollection); + + assertThat(memberships).isEqualTo(expectedGroups); + } + @Before public void setUp() { + // Build the object for parsing AAD Group Responses + createAadGroupResponse(); + // Get current date/time for the test token Calendar testCalendar = Calendar.getInstance(); Date currentTestDate = testCalendar.getTime(); @@ -189,4 +222,32 @@ public void setUp() { .claim("ver", "2.0") .build()); } + + void createAadGroupResponse() { + // This object is a direct copy of the MS Graph response with the ID + // being changed from the real value. + JsonObject jsonObj = new Gson().fromJson("{\n" + + " \"@odata.context\": \"https://graph.microsoft.com/v1.0/$metadata#directoryObjects(id,displayName)\",\n" + + " \"value\": [\n" + + " {\n" + + " \"@odata.type\": \"#microsoft.graph.group\",\n" + + " \"id\": \"89fb503f-134f-43cd-aaa7-f21facb2eca3\",\n" + + " \"displayName\": \"Developers\"\n" + + " },\n" + + " {\n" + + " \"@odata.type\": \"#microsoft.graph.group\",\n" + + " \"id\": \"d595c0e2-28f4-4a52-8ec5-58eab17309f8\",\n" + + " \"displayName\": \"Administrators\"\n" + + " }\n" + + " ]\n" + + "}", + JsonObject.class); + + DefaultSerializer serializer = new DefaultSerializer(new DefaultLogger()); + + DirectoryObjectCollectionResponse memberResponse = serializer.deserializeObject(jsonObj, DirectoryObjectCollectionResponse.class); + + assert memberResponse != null; + memberGroupCollection = new DirectoryObjectCollectionWithReferencesPage(memberResponse, null); + } } From 8f9f47e07a6e18c2f92b4ab6269ad9961edcb8e3 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Tue, 15 Nov 2022 11:38:14 -0800 Subject: [PATCH 07/16] Code Cleanup Remove some unused imports and do some minor adjustments to clear issues in SonarQube. --- src/main/java/org/almrangers/auth/aad/AadUserInfo.java | 8 +++++--- .../java/org/almrangers/auth/aad/AadTokenHelperTest.java | 1 - 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java index b629bcd..7eb0d1b 100644 --- a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java +++ b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java @@ -52,9 +52,6 @@ import java.util.concurrent.CompletableFuture; public class AadUserInfo { - private final String USERNAME_CLAIM = "preferred_username"; - private final String EMAIL_CLAIM = "email"; - private final String DISPLAYNAME_CLAIM = "name"; private String userOid; private String displayId; @@ -76,6 +73,11 @@ public AadUserInfo(JWT idToken, AccessToken accessToken, Boolean wantGroups) thr } private void parseToken(JWT idToken) throws ParseException { + // These are the names of the ID token claims we use below. + final String USERNAME_CLAIM = "preferred_username"; + final String EMAIL_CLAIM = "email"; + final String DISPLAYNAME_CLAIM = "name"; + JWTClaimsSet claims = idToken.getJWTClaimsSet(); // User's OID. Used for grabbing group membership if that feature is enabled diff --git a/src/test/java/org/almrangers/auth/aad/AadTokenHelperTest.java b/src/test/java/org/almrangers/auth/aad/AadTokenHelperTest.java index 640fd8d..fa35682 100644 --- a/src/test/java/org/almrangers/auth/aad/AadTokenHelperTest.java +++ b/src/test/java/org/almrangers/auth/aad/AadTokenHelperTest.java @@ -49,7 +49,6 @@ import org.sonar.api.server.authentication.UnauthorizedException; import java.io.IOException; -import java.text.ParseException; import java.util.Calendar; import java.util.Date; From bacdeac14bcd7d4005afee778f3ce8be96ff074b Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Wed, 16 Nov 2022 12:48:43 -0800 Subject: [PATCH 08/16] Unit Testing Additions Update the onCallback method in AadIdentityProvider to default access so we can unit test it directly. Add test to verify that login will fail if given an invalid code. --- pom.xml | 2 +- .../auth/aad/AadIdentityProvider.java | 22 ++++++++--------- .../auth/aad/AadIdentityProviderTest.java | 24 +++++++++++++++---- 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/pom.xml b/pom.xml index 568228f..e89ed5f 100644 --- a/pom.xml +++ b/pom.xml @@ -130,7 +130,7 @@ org.mockito mockito-core - 4.8.1 + 4.9.0 test diff --git a/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java b/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java index 621a7f0..e7b834e 100644 --- a/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java +++ b/src/main/java/org/almrangers/auth/aad/AadIdentityProvider.java @@ -127,7 +127,7 @@ public void callback(CallbackContext context) { } } - private void onCallback(CallbackContext context) throws UnauthorizedException { + void onCallback(CallbackContext context) throws UnauthorizedException { context.verifyCsrfState(); HttpServletRequest request = context.getRequest(); @@ -135,16 +135,16 @@ private void onCallback(CallbackContext context) throws UnauthorizedException { ExecutorService service = null; try { - TokenRequest tokenReq = new TokenRequest( - new URI(settings.authorityUrl()), - new ClientSecretBasic( - new ClientID(settings.clientId().orElse(null)), - new Secret(settings.clientSecret().orElse(""))), - new AuthorizationCodeGrant(code, new URI(context.getCallbackUrl())) - ); - - // Parse and check response - OIDCTokenResponse tokenResponse = AadTokenHelper.extractTokenResponse(tokenReq.toHTTPRequest().send()); + TokenRequest tokenReq = new TokenRequest( + new URI(settings.authorityUrl()), + new ClientSecretBasic( + new ClientID(settings.clientId().orElse(null)), + new Secret(settings.clientSecret().orElse(""))), + new AuthorizationCodeGrant(code, new URI(context.getCallbackUrl())) + ); + + // Parse and check response + OIDCTokenResponse tokenResponse = AadTokenHelper.extractTokenResponse(tokenReq.toHTTPRequest().send()); OIDCTokens accessTokens = tokenResponse.getOIDCTokens(); diff --git a/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java b/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java index 99e38ec..e245e16 100644 --- a/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java +++ b/src/test/java/org/almrangers/auth/aad/AadIdentityProviderTest.java @@ -27,22 +27,21 @@ package org.almrangers.auth.aad; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertThrows; import static org.mockito.Mockito.*; import okhttp3.HttpUrl; -import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.mockito.ArgumentCaptor; import org.sonar.api.config.internal.MapSettings; import org.sonar.api.server.authentication.OAuth2IdentityProvider; +import org.sonar.api.server.authentication.UnauthorizedException; +import javax.servlet.http.HttpServletRequest; import java.util.HashMap; public class AadIdentityProviderTest { - @Rule - public ExpectedException thrown = ExpectedException.none(); MapSettings settings = new MapSettings(); AadSettings aadSettings = new AadSettings(settings.asConfig()); AadIdentityProvider underTest = spy(new AadIdentityProvider(aadSettings)); @@ -55,6 +54,23 @@ public void check_fields() { assertThat(underTest.getDisplay().getBackgroundColor()).isEqualTo("#2F2F2F"); } + @Test + // This test simulates trying to log in but getting a bad "code" from the + // auth process, causing the request for auth and id tokens to fail. + public void fail_login_on_bad_auth_code() { + setSettings(true); + OAuth2IdentityProvider.CallbackContext context = mock(OAuth2IdentityProvider.CallbackContext.class); + HttpServletRequest request = mock(HttpServletRequest.class); + + when(request.getParameter("code")).thenReturn("9Q4mHqIAmAHORqpwwUaAxnGh"); + when(context.getRequest()).thenReturn(request); + + assertThrows(UnauthorizedException.class, + () -> underTest.onCallback(context) + ); + + } + @Test public void init() { setSettings(true); From 2ab650cdada92230bec8ef3be960e67b31cb0ebe Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Wed, 16 Nov 2022 12:52:17 -0800 Subject: [PATCH 09/16] Upgrade MS Graph SDK Upgrade the MS Graph SDK dependency. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index e89ed5f..0d67aae 100644 --- a/pom.xml +++ b/pom.xml @@ -105,7 +105,7 @@ com.microsoft.graph microsoft-graph - 5.39.0 + 5.41.0 com.google.code.findbugs From 00574bdf30cf439a2b91c6db01366de4d7fe4ec7 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Thu, 17 Nov 2022 10:47:02 -0800 Subject: [PATCH 10/16] Update to Build with Java 11 As the minimum supported JRE for SonarQube 8.9 and up is 11, bump the source and target versions to that version. --- pom.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pom.xml b/pom.xml index 0d67aae..4785d03 100644 --- a/pom.xml +++ b/pom.xml @@ -75,8 +75,8 @@ UTF-8 - 1.8 - 1.8 + 11 + 11 From fe6571a9677d4f6af4841a055ab1a4ce4d4471a5 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Thu, 17 Nov 2022 12:18:06 -0800 Subject: [PATCH 11/16] Update Sonar API to 8.9 Update the SonarQube API to version 8.9, which is the lowest version we are now supporting. Adjust the tests to work with the newer API version. --- pom.xml | 8 +++++++- .../java/org/almrangers/auth/aad/AadSettingsTest.java | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/pom.xml b/pom.xml index 4785d03..b093c8f 100644 --- a/pom.xml +++ b/pom.xml @@ -83,7 +83,7 @@ org.sonarsource.sonarqube sonar-plugin-api - 7.9 + 8.9.0.43852 provided @@ -115,6 +115,12 @@ + + org.sonarsource.sonarqube + sonar-plugin-api-impl + 8.9.0.43852 + test + org.assertj assertj-core diff --git a/src/test/java/org/almrangers/auth/aad/AadSettingsTest.java b/src/test/java/org/almrangers/auth/aad/AadSettingsTest.java index 4be1507..805e83a 100644 --- a/src/test/java/org/almrangers/auth/aad/AadSettingsTest.java +++ b/src/test/java/org/almrangers/auth/aad/AadSettingsTest.java @@ -29,12 +29,13 @@ import org.junit.Test; import org.sonar.api.config.PropertyDefinitions; import org.sonar.api.config.internal.MapSettings; +import org.sonar.api.utils.System2; import static org.almrangers.auth.aad.AadSettings.*; import static org.assertj.core.api.Assertions.assertThat; public class AadSettingsTest { - MapSettings settings = new MapSettings(new PropertyDefinitions(AadSettings.definitions())); + MapSettings settings = new MapSettings(new PropertyDefinitions(new System2(), AadSettings.definitions())); AadSettings underTest = new AadSettings(settings.asConfig()); From 5ca09fd5d7452ee34ca522afb3ffa90b3d2d3eff Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Fri, 18 Nov 2022 10:28:17 -0800 Subject: [PATCH 12/16] File Cleanup Remove unused Travis CI config. Remove old group test files, and replace Travis CI status badge with GitHub action badge. --- .travis.yml | 28 ----------------------- README.md | 4 ++-- src/test/resources/get-members-page1.json | 10 -------- src/test/resources/get-members-page2.json | 6 ----- 4 files changed, 2 insertions(+), 46 deletions(-) delete mode 100644 .travis.yml delete mode 100644 src/test/resources/get-members-page1.json delete mode 100644 src/test/resources/get-members-page2.json diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index bd48163..0000000 --- a/.travis.yml +++ /dev/null @@ -1,28 +0,0 @@ -dist: trusty -language: java -sudo: false -install: true - -addons: - sonarcloud: - organization: "srvrguy-github" - -jdk: - - oraclejdk8 - -script: - - | - if [ "${TRAVIS_SECURE_ENV_VARS}" == "true" ]; then - echo; - echo "Running Build with SonarCloud Scanning"; - mvn clean org.jacoco:jacoco-maven-plugin:prepare-agent install sonar:sonar $MAVEN_OPTIONS; - else - echo; - echo "Running a normal test suite with no SonarCloud"; - mvn clean install; - fi; - -cache: - directories: - - '$HOME/.m2/repository' - - '$HOME/.sonar/cache' diff --git a/README.md b/README.md index 1106707..b24e613 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,5 @@ # Azure Active Directory (AAD) Authentication Plug-in for SonarQube -[![Build Status](https://travis-ci.org/hkamel/sonar-auth-aad.svg?branch=master)](https://travis-ci.org/hkamel/sonar-auth-aad) [![Quality Gate Status](https://sonarcloud.io/api/project_badges/measure?project=org.almrangers.auth.aad%3Asonar-auth-aad-plugin&metric=alert_status)](https://sonarcloud.io/dashboard?id=org.almrangers.auth.aad%3Asonar-auth-aad-plugin) +[![Test and Package](https://github.com/hkamel/sonar-auth-aad/actions/workflows/maven.yml/badge.svg?branch=1.3.0)](https://github.com/hkamel/sonar-auth-aad/actions/workflows/maven.yml) [![Quality Gate Status](https://sonarcloud.io/api/project_badges/measure?project=org.almrangers.auth.aad%3Asonar-auth-aad-plugin&metric=alert_status)](https://sonarcloud.io/dashboard?id=org.almrangers.auth.aad%3Asonar-auth-aad-plugin) This plugin for SonarQube allows the use of Azure Active Directory (AAD) as an authentication option. @@ -10,4 +10,4 @@ as an authentication option. ## Installation, Setup, and Documentation Information for installation and configuration of this plugin is located -at https://github.com/hkamel/sonar-auth-aad/wiki \ No newline at end of file +at https://github.com/hkamel/sonar-auth-aad/wiki diff --git a/src/test/resources/get-members-page1.json b/src/test/resources/get-members-page1.json deleted file mode 100644 index 90e3cd0..0000000 --- a/src/test/resources/get-members-page1.json +++ /dev/null @@ -1,10 +0,0 @@ - -{ - "@odata.context": "https://graph.microsoft.com/v1.0/$metadata#directoryObjects", - "@odata.nextLink": "https://graph.microsoft.com/v1.0/536e97e9-0d29-43ec-b8d5-a505d3ee6a8f/users/abc.xyz@example.com/memberOf?$skiptoken=RANDOMTOKEN", - "value": [ - { "displayName": "Group1" }, - { "displayName": "Group2" }, - { "displayName": "Group3" } - ] -} diff --git a/src/test/resources/get-members-page2.json b/src/test/resources/get-members-page2.json deleted file mode 100644 index ff6a560..0000000 --- a/src/test/resources/get-members-page2.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "value": [ - { "displayName": "Group4" }, - { "displayName": "Group5" } - ] -} \ No newline at end of file From ba272cf8e0b159a40504b6afccfd264093816d09 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Tue, 6 Dec 2022 14:30:23 -0800 Subject: [PATCH 13/16] Fix MS Graph Group Sync Fix group parsing to only run on returned Group objects as the Graph call was also returning directory roles and admin units. re: #120 --- .../java/org/almrangers/auth/aad/AadUserInfo.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java index 7eb0d1b..4926dc3 100644 --- a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java +++ b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java @@ -174,12 +174,14 @@ Set processMemberGroupCollection(@Nullable DirectoryObjectCollectionWith while(memberGroupCollection != null) { final List groupList = memberGroupCollection.getCurrentPage(); - for ( DirectoryObject group : groupList) { - String groupDisplayName = ((Group) group).displayName; - - // Don't add the group if the display name is null - if(groupDisplayName != null) { - parsedUserGroups.add(groupDisplayName); + for (DirectoryObject group : groupList) { + if (group instanceof Group) { + String groupDisplayName = ((Group) group).displayName; + + // Don't add the group if the display name is null + if (groupDisplayName != null) { + parsedUserGroups.add(groupDisplayName); + } } } From 12e0241caff4a6f98587f75dc41c912d511e84b5 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Thu, 5 Jan 2023 12:02:00 -0800 Subject: [PATCH 14/16] Lowercase the Display ID (Provider Login) Based on behavior of the prior version and how data was provided in the older v1 tokens, force the provider login to lower case. re: #144 --- src/main/java/org/almrangers/auth/aad/AadUserInfo.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java index 4926dc3..a0d7d27 100644 --- a/src/main/java/org/almrangers/auth/aad/AadUserInfo.java +++ b/src/main/java/org/almrangers/auth/aad/AadUserInfo.java @@ -85,12 +85,12 @@ private void parseToken(JWT idToken) throws ParseException { this.userOid = claims.getStringClaim("oid"); } - // Display ID + // Display ID (Used as the "Provider Login") // Tries the "preferred username" first, and falls back to email if(!"".equals(claims.getStringClaim(USERNAME_CLAIM)) && claims.getStringClaim(USERNAME_CLAIM) != null) { - this.displayId = claims.getStringClaim(USERNAME_CLAIM); + this.displayId = claims.getStringClaim(USERNAME_CLAIM).toLowerCase(); } else if(!claims.getStringClaim(EMAIL_CLAIM).isEmpty()) { - this.displayId = claims.getStringClaim(EMAIL_CLAIM); + this.displayId = claims.getStringClaim(EMAIL_CLAIM).toLowerCase(); } // Display Name From 1539f4fa1b81ddbce2b0793e16d9f51c0485619c Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Fri, 24 Feb 2023 12:33:09 -0800 Subject: [PATCH 15/16] Update Library Dependencies Update the dependencies in the POM. This also resolves some security issues in the version of microsoft-graph that we were previously using. --- pom.xml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pom.xml b/pom.xml index b093c8f..4aaf8c5 100644 --- a/pom.xml +++ b/pom.xml @@ -95,7 +95,7 @@ com.nimbusds oauth2-oidc-sdk - 10.1 + 10.7 com.squareup.okhttp3 @@ -105,7 +105,7 @@ com.microsoft.graph microsoft-graph - 5.41.0 + 5.47.0 com.google.code.findbugs @@ -124,7 +124,7 @@ org.assertj assertj-core - 3.23.1 + 3.24.2 test @@ -136,7 +136,7 @@ org.mockito mockito-core - 4.9.0 + 4.11.0 test From 8daac9a8ccc2c721143bab5beb947c2cfcafd366 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Thu, 30 Mar 2023 11:29:14 -0700 Subject: [PATCH 16/16] Update MS Graph Update the MS Graph dependency. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 4aaf8c5..1c562af 100644 --- a/pom.xml +++ b/pom.xml @@ -105,7 +105,7 @@ com.microsoft.graph microsoft-graph - 5.47.0 + 5.49.0 com.google.code.findbugs