Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ELY-2574] Add ability to configure additional scope for authentication request #1925

Merged
merged 1 commit into from Mar 22, 2024
Merged

[ELY-2574] Add ability to configure additional scope for authentication request #1925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfigurationBuilder.java
View file
Open in desktop
Expand Up @@ -100,6 +100,9 @@ protected OidcClientConfiguration internalBuild(final OidcJsonConfiguration oidc
if (oidcJsonConfiguration.getTokenCookiePath() != null) {
oidcClientConfiguration.setOidcStateCookiePath(oidcJsonConfiguration.getTokenCookiePath());
}
if (oidcJsonConfiguration.getScope() != null) {
oidcClientConfiguration.setScope(oidcJsonConfiguration.getScope());
}
if (oidcJsonConfiguration.getPrincipalAttribute() != null) oidcClientConfiguration.setPrincipalAttribute(oidcJsonConfiguration.getPrincipalAttribute());

oidcClientConfiguration.setResourceCredentials(oidcJsonConfiguration.getCredentials());
Expand Down
12 changes: 11 additions & 1 deletion http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java
View file
Open in desktop
Expand Up @@ -46,7 +46,7 @@
"register-node-at-startup", "register-node-period", "token-store", "adapter-state-cookie-path", "principal-attribute",
"proxy-url", "turn-off-change-session-id-on-login", "token-minimum-time-to-live",
"min-time-between-jwks-requests", "public-key-cache-ttl",
"ignore-oauth-query-parameter", "verify-token-audience", "token-signature-algorithm"
"ignore-oauth-query-parameter", "verify-token-audience", "token-signature-algorithm", "scope"
})
public class OidcJsonConfiguration {

Expand Down Expand Up @@ -140,6 +140,9 @@ public class OidcJsonConfiguration {
@JsonProperty("token-signature-algorithm")
protected String tokenSignatureAlgorithm = DEFAULT_TOKEN_SIGNATURE_ALGORITHM;

@JsonProperty("scope")
protected String scope;
fjuma marked this conversation as resolved.
Show resolved Hide resolved

/**
* The Proxy url to use for requests to the auth-server, configurable via the adapter config property {@code proxy-url}.
*/
Expand Down Expand Up @@ -511,5 +514,12 @@ public void setTokenSignatureAlgorithm(String tokenSignatureAlgorithm) {
this.tokenSignatureAlgorithm = tokenSignatureAlgorithm;
}

public String getScope() {
return scope;
}

public void setScope(String scope) {
fjuma marked this conversation as resolved.
Show resolved Hide resolved
this.scope = scope;
}
}

28 changes: 27 additions & 1 deletion http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java
View file
Open in desktop
Expand Up @@ -45,8 +45,10 @@
import java.net.URL;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

import org.apache.http.HttpStatus;
import org.apache.http.NameValuePair;
Expand Down Expand Up @@ -166,10 +168,13 @@ protected String getRedirectUri(String state) {

List<String> forwardableQueryParams = Arrays.asList(LOGIN_HINT, DOMAIN_HINT, KC_IDP_HINT, PROMPT, MAX_AGE, UI_LOCALES, SCOPE);
List<NameValuePair> forwardedQueryParams = new ArrayList<>(forwardableQueryParams.size());
Set<String> allScopes = new HashSet<>();
addScopes(deployment.getScope(), allScopes);

for (String paramName : forwardableQueryParams) {
String paramValue = getQueryParamValue(facade, paramName);
if (SCOPE.equals(paramName)) {
paramValue = addOidcScopeIfNeeded(paramValue);
paramValue = combineAndReorderScopes(allScopes, paramValue);
}
fjuma marked this conversation as resolved.
Show resolved Hide resolved
if (paramValue != null && !paramValue.isEmpty()) {
forwardedQueryParams.add(new BasicNameValuePair(paramName, paramValue));
Expand All @@ -180,6 +185,7 @@ protected String getRedirectUri(String state) {
if (deployment.getAuthUrl() == null) {
return null;
}

URIBuilder redirectUriBuilder = new URIBuilder(deployment.getAuthUrl())
.addParameter(RESPONSE_TYPE, CODE)
.addParameter(CLIENT_ID, deployment.getResourceName())
Expand Down Expand Up @@ -416,4 +422,24 @@ private static boolean hasScope(String scopeParam, String targetScope) {
}
return false;
}

private String combineAndReorderScopes(Set<String> allScopes, String paramValue) {
StringBuilder combinedScopes = new StringBuilder();
addScopes(paramValue, allScopes);

//some OpenID providers require openid scope to be added in the beginning
combinedScopes.append(OIDC_SCOPE);
for (String scope : allScopes) {
if (!scope.equals(OIDC_SCOPE)) {
combinedScopes.append(" ").append(scope);
}
}
return combinedScopes.toString();
}

private void addScopes(String scopes, Set<String> allScopes) {
if (scopes != null && !scopes.isEmpty()) {
allScopes.addAll(Arrays.asList(scopes.split("\\s+")));
}
}
}
29 changes: 22 additions & 7 deletions http/oidc/src/test/java/org/wildfly/security/http/oidc/KeycloakConfiguration.java
View file
Open in desktop
Expand Up @@ -33,6 +33,8 @@

import io.restassured.RestAssured;

import static org.wildfly.security.http.oidc.Oidc.OIDC_SCOPE;

/**
* Keycloak configuration for testing.
*
Expand All @@ -47,6 +49,7 @@ public class KeycloakConfiguration {
private static final String BOB = "bob";
private static final String BOB_PASSWORD = "bob123+";
public static final String ALLOWED_ORIGIN = "http://somehost";
public static final boolean EMAIL_VERIFIED = false;

/**
* Configure RealmRepresentation as follows:
Expand All @@ -60,8 +63,8 @@ public class KeycloakConfiguration {
* </ul>
*/
public static RealmRepresentation getRealmRepresentation(final String realmName, String clientId, String clientSecret,
String clientHostName, int clientPort, String clientApp) {
return createRealm(realmName, clientId, clientSecret, clientHostName, clientPort, clientApp);
String clientHostName, int clientPort, String clientApp, boolean configureClientScopes) {
return createRealm(realmName, clientId, clientSecret, clientHostName, clientPort, clientApp, configureClientScopes);
}

public static RealmRepresentation getRealmRepresentation(final String realmName, String clientId, String clientSecret,
Expand Down Expand Up @@ -101,15 +104,22 @@ public static String getAccessToken(String authServerUrl, String realmName, Stri
.as(AccessTokenResponse.class).getToken();
}

private static RealmRepresentation createRealm(final String realmName, String clientId, String clientSecret,
String clientHostName, int clientPort, String clientApp,
boolean directAccessGrantEnabled, String bearerOnlyClientId,
String corsClientId) {
return createRealm(realmName, clientId, clientSecret, clientHostName, clientPort, clientApp, directAccessGrantEnabled, bearerOnlyClientId, corsClientId, false);
}

private static RealmRepresentation createRealm(String name, String clientId, String clientSecret,
String clientHostName, int clientPort, String clientApp) {
return createRealm(name, clientId, clientSecret, clientHostName, clientPort, clientApp, false, null, null);
String clientHostName, int clientPort, String clientApp, boolean configureClientScopes) {
return createRealm(name, clientId, clientSecret, clientHostName, clientPort, clientApp, false, null, null, configureClientScopes);
}

private static RealmRepresentation createRealm(String name, String clientId, String clientSecret,
String clientHostName, int clientPort, String clientApp,
boolean directAccessGrantEnabled, String bearerOnlyClientId,
String corsClientId) {
String corsClientId, boolean configureClientScopes) {
RealmRepresentation realm = new RealmRepresentation();

realm.setRealm(name);
Expand All @@ -127,8 +137,12 @@ private static RealmRepresentation createRealm(String name, String clientId, Str

realm.getRoles().getRealm().add(new RoleRepresentation("user", null, false));
realm.getRoles().getRealm().add(new RoleRepresentation("admin", null, false));

realm.getClients().add(createWebAppClient(clientId, clientSecret, clientHostName, clientPort, clientApp, directAccessGrantEnabled));
ClientRepresentation webAppClient = createWebAppClient(clientId, clientSecret, clientHostName, clientPort, clientApp, directAccessGrantEnabled);
if (configureClientScopes) {
webAppClient.setDefaultClientScopes(Collections.singletonList(OIDC_SCOPE));
webAppClient.setOptionalClientScopes(Arrays.asList("phone", "email", "profile"));
}
realm.getClients().add(webAppClient);

if (bearerOnlyClientId != null) {
realm.getClients().add(createBearerOnlyClient(bearerOnlyClientId));
Expand Down Expand Up @@ -178,6 +192,7 @@ private static UserRepresentation createUser(String username, String password, L
user.setCredentials(new ArrayList<>());
user.setRealmRoles(realmRoles);
user.setEmail(username + "@gmail.com");
user.setEmailVerified(EMAIL_VERIFIED);

CredentialRepresentation credential = new CredentialRepresentation();
credential.setType(CredentialRepresentation.PASSWORD);
Expand Down
35 changes: 33 additions & 2 deletions http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcBaseTest.java
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@
package org.wildfly.security.http.oidc;

import static org.junit.Assert.assertEquals;
import static org.wildfly.common.Assert.assertTrue;

import java.io.IOException;
import java.net.URI;
Expand All @@ -29,6 +30,9 @@
import javax.security.auth.callback.UnsupportedCallbackException;
import javax.security.sasl.AuthorizeCallback;

import org.jose4j.jwt.JwtClaims;
import org.jose4j.jwt.consumer.InvalidJwtException;
import org.jose4j.jwt.consumer.JwtConsumerBuilder;
import org.junit.AfterClass;
import org.keycloak.representations.idm.RealmRepresentation;
import org.testcontainers.DockerClientFactory;
Expand All @@ -37,6 +41,8 @@
import org.wildfly.security.auth.callback.IdentityCredentialCallback;
import org.wildfly.security.auth.callback.SecurityIdentityCallback;
import org.wildfly.security.auth.server.SecurityDomain;
import org.wildfly.security.credential.BearerTokenCredential;
import org.wildfly.security.credential.Credential;
import org.wildfly.security.evidence.Evidence;
import org.wildfly.security.http.HttpServerAuthenticationMechanism;
import org.wildfly.security.http.HttpServerAuthenticationMechanismFactory;
Expand Down Expand Up @@ -76,6 +82,7 @@ public class OidcBaseTest extends AbstractBaseHttpTest {
public static final String CLIENT_PAGE_TEXT = "Welcome page!";
public static final String CLIENT_HOST_NAME = "localhost";
public static MockWebServer client; // to simulate the application being secured
public static final Boolean CONFIGURE_CLIENT_SCOPES = true; // to simulate the application being secured

protected HttpServerAuthenticationMechanismFactory oidcFactory;

Expand Down Expand Up @@ -117,8 +124,11 @@ protected static boolean isDockerAvailable() {
return false;
}
}

protected CallbackHandler getCallbackHandler() {
return getCallbackHandler(false, null);
}

protected CallbackHandler getCallbackHandler(boolean checkScope, String expectedScopes) {
return callbacks -> {
for(Callback callback : callbacks) {
if (callback instanceof EvidenceVerifyCallback) {
Expand All @@ -127,7 +137,13 @@ protected CallbackHandler getCallbackHandler() {
} else if (callback instanceof AuthenticationCompleteCallback) {
// NO-OP
} else if (callback instanceof IdentityCredentialCallback) {
// NO-OP
if (checkScope) {
try {
checkForScopeClaims(callback, expectedScopes);
} catch (InvalidJwtException e) {
throw new RuntimeException(e);
}
}
} else if (callback instanceof AuthorizeCallback) {
((AuthorizeCallback) callback).setAuthorized(true);
} else if (callback instanceof SecurityIdentityCallback) {
Expand Down Expand Up @@ -181,6 +197,7 @@ protected HtmlInput loginToKeycloak(String username, String password, URI reques
webClient.addCookie(getCookieString(cookie), requestUri.toURL(), null);
}
}

HtmlPage keycloakLoginPage = webClient.getPage(location);
HtmlForm loginForm = keycloakLoginPage.getForms().get(0);
loginForm.getInputByName(KEYCLOAK_USERNAME).setValueAttribute(username);
Expand Down Expand Up @@ -215,4 +232,18 @@ protected String getCookieString(HttpServerCookie cookie) {
return header.toString();
}

protected void checkForScopeClaims(Callback callback, String expectedScopes) throws InvalidJwtException {
Credential credential = ((IdentityCredentialCallback)callback).getCredential();
String token = ((BearerTokenCredential) credential).getToken();
JwtClaims jwtClaims = new JwtConsumerBuilder().setSkipSignatureVerification().setSkipAllValidators().build().processToClaims(token);

if (expectedScopes != null) {
if (expectedScopes.contains("email")) {
assertTrue(jwtClaims.getClaimValueAsString("email_verified").contains(String.valueOf(KeycloakConfiguration.EMAIL_VERIFIED)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that Keycloak will set the email and profile scopes by default so just wondering if checking only email_verified and profile are actually enough to successfully test that the configured scopes have been set properly. As an example, I tried tweaking testMultipleScopeValue locally to only set the openid scope. I would have expected the test to fail in this case however it actually ended up passing. Is there a claim that we could check that would only be set if a specific scope other than the default ones has been configured?

Copy link
Contributor

@fjuma fjuma Mar 20, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the address claim is a candidate for a claim that would confirm that the configured scopes are being used as expected, i.e., we could update tests to also specify the address scope. Then, the JWT claims will include the address claim as well. This claim isn't included by default so this would confirm that the configured scopes are actually getting used as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can do address, because org.keycloak.representations.idm.UserRepresentation
does not have a way to set that. Plus when I added address as a scope, the request timed out.
I saw this earlier with this API as well when I wanted to set offline_access as one of the scopes. It is kind of limiting in that sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I can change the default list of scopes for the client to just OpenID, so that the results of those scopes are only included if the config asks for it.

Copy link
Contributor

@fjuma fjuma Mar 21, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried adding address locally and was able to see the address added in the claims. So even if we can't actually set a value for it, we can still verify that it's present in the claims. This allows us to verify that setting the scope value actually resulted in the correct behaviour. With the test as is right now, we can't be sure that setting the scope actually had an effect since the current claims that are being tested will be present regardless of the scope configuration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I can change the default list of scopes for the client to just OpenID, so that the results of those scopes are only included if the config asks for it.

Yes another option would be to change the default list of scopes if that's easy to do. I quickly tried that yesterday but wasn't able to get things working as expected but maybe I was missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I am trying that now and I am getting a class cast exception.
I am looking more into it to see what is causing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to fix this. I added openid as the default scope and the others needed to be added as optional scopes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this update! The tests look great now!

One last comment (that won't block merging either) is that right now we're checking if the scope from the JWT claims contains a certain value and then checking that value. Instead, it would be better to use the expectedScope String instead. It should be possible to do that by passing the expectedScope to getCallbackHandler instead of passing the boolean value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

}
if (expectedScopes.contains("profile")) {
assertTrue(jwtClaims.getClaimValueAsString("preferred_username").contains(KeycloakConfiguration.ALICE));
}
}
}
}