Skip to content

Commit

Permalink
feature: Allow sending static key/value pairs to the configured IdP (#…
Browse files Browse the repository at this point in the history
…2397)

* Feature: Allow sending static key/value pairs to the configured external IdP for authorization code flow

Add parameter additionalAuthzParameters (additional authorization parameters) to requests for auth code flow.
The usage is: OIDC IdPs have additional parameters, e.g. UAA itself has token_format to get special features.
The new parameter allows to customize the IdP with own parameters

* Documentation added

* tests

* more tests

* review

* add test

* cleanup

* test converage

* Feature: Allow sending static key/value pairs to the configured external IdP for authorization code flow

Add parameter additionalAuthzParameters (additional authorization parameters) to requests for auth code flow.
The usage is: OIDC IdPs have additional parameters, e.g. UAA itself has token_format to get special features.
The new parameter allows to customize the IdP with own parameters

* sonar

* rebase

* review

* review

* review
  • Loading branch information
strehle authored Jul 17, 2023
1 parent eb008a5 commit e3367a0
Show file tree
Hide file tree
Showing 11 changed files with 198 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@
import org.cloudfoundry.identity.uaa.login.Prompt;

import java.net.URL;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;

import static java.util.Collections.emptyMap;

@JsonIgnoreProperties(ignoreUnknown = true)
public class OIDCIdentityProviderDefinition extends AbstractExternalOAuthIdentityProviderDefinition<OIDCIdentityProviderDefinition>
Expand All @@ -31,6 +35,8 @@ public class OIDCIdentityProviderDefinition extends AbstractExternalOAuthIdentit
private List<Prompt> prompts = null;
@JsonInclude(JsonInclude.Include.NON_NULL)
private Object jwtClientAuthentication;
@JsonInclude(JsonInclude.Include.NON_NULL)
private Map<String, String> additionalAuthzParameters = null;

public URL getDiscoveryUrl() {
return discoveryUrl;
Expand Down Expand Up @@ -74,6 +80,14 @@ public void setJwtClientAuthentication(final Object jwtClientAuthentication) {
this.jwtClientAuthentication = jwtClientAuthentication;
}

public Map<String, String> getAdditionalAuthzParameters() {
return this.additionalAuthzParameters != null ? Collections.unmodifiableMap(this.additionalAuthzParameters) : null;
}

public void setAdditionalAuthzParameters(final Map<String, String> additonalAuthzParameters) {
this.additionalAuthzParameters = new HashMap<>(additonalAuthzParameters!=null?additonalAuthzParameters: emptyMap());
}

@Override
public Object clone() throws CloneNotSupportedException {
return super.clone();
Expand All @@ -89,7 +103,8 @@ public boolean equals(Object o) {

if (this.passwordGrantEnabled != that.passwordGrantEnabled) return false;
if (this.setForwardHeader != that.setForwardHeader) return false;
if (this.jwtClientAuthentication != that.jwtClientAuthentication) return false;
if (!Objects.equals(this.jwtClientAuthentication, that.jwtClientAuthentication)) return false;
if (!Objects.equals(this.additionalAuthzParameters, that.additionalAuthzParameters)) return false;
return Objects.equals(discoveryUrl, that.discoveryUrl);

}
Expand All @@ -101,6 +116,7 @@ public int hashCode() {
result = 31 * result + (passwordGrantEnabled ? 1 : 0);
result = 31 * result + (setForwardHeader ? 1 : 0);
result = 31 * result + (jwtClientAuthentication != null ? jwtClientAuthentication.hashCode() : 0);
result = 31 * result + (additionalAuthzParameters != null ? additionalAuthzParameters.hashCode() : 0);
return result;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;

import static org.junit.Assert.assertTrue;

public class OIDCIdentityProviderDefinitionTests {

private final String defaultJson = "{\"emailDomain\":null,\"additionalConfiguration\":null,\"providerDescription\":null,\"externalGroupsWhitelist\":[],\"attributeMappings\":{},\"addShadowUserOnLogin\":true,\"storeCustomAttributes\":false,\"authUrl\":null,\"tokenUrl\":null,\"tokenKeyUrl\":null,\"tokenKey\":null,\"linkText\":null,\"showLinkText\":true,\"skipSslValidation\":false,\"relyingPartyId\":null,\"relyingPartySecret\":null,\"scopes\":null,\"issuer\":null,\"responseType\":\"code\",\"userInfoUrl\":null,\"jwtClientAuthentication\":false}";
private final String defaultJson = "{\"emailDomain\":null,\"additionalConfiguration\":null,\"providerDescription\":null,\"externalGroupsWhitelist\":[],\"attributeMappings\":{},\"addShadowUserOnLogin\":true,\"storeCustomAttributes\":false,\"authUrl\":null,\"tokenUrl\":null,\"tokenKeyUrl\":null,\"tokenKey\":null,\"linkText\":null,\"showLinkText\":true,\"skipSslValidation\":false,\"relyingPartyId\":null,\"relyingPartySecret\":null,\"scopes\":null,\"issuer\":null,\"responseType\":\"code\",\"userInfoUrl\":null,\"jwtClientAuthentication\":false,\"additionalAuthzParameters\":{\"token_format\":\"jwt\"}}";
String url = "https://accounts.google.com/.well-known/openid-configuration";

@Test
Expand All @@ -44,6 +44,17 @@ public void serialize_discovery_url() throws MalformedURLException {
String json = JsonUtils.writeValueAsString(def);
def = JsonUtils.readValue(json, OIDCIdentityProviderDefinition.class);
assertEquals(url, def.getDiscoveryUrl().toString());
assertEquals("jwt", def.getAdditionalAuthzParameters().get("token_format"));
}

@Test
public void testSerializableObjectCalls() throws CloneNotSupportedException {
OIDCIdentityProviderDefinition def = JsonUtils.readValue(defaultJson, OIDCIdentityProviderDefinition.class);
OIDCIdentityProviderDefinition def2 = (OIDCIdentityProviderDefinition) def.clone();
assertTrue(def.equals(def2));
assertEquals(def.hashCode(), def2.hashCode());
assertEquals(1, def2.getAdditionalAuthzParameters().size());
assertEquals("jwt", def2.getAdditionalAuthzParameters().get("token_format"));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,15 @@ private String getTokenFromCode(ExternalOAuthCodeToken codeToken, AbstractExtern
logger.debug("Adding new client_id and client_secret for token exchange");
body.add("client_id", config.getRelyingPartyId());

if (config instanceof OIDCIdentityProviderDefinition) {
OIDCIdentityProviderDefinition oidcIdentityProviderDefinition = (OIDCIdentityProviderDefinition) config;
if (oidcIdentityProviderDefinition.getAdditionalAuthzParameters() != null){
for (Map.Entry<String, String> entry : oidcIdentityProviderDefinition.getAdditionalAuthzParameters().entrySet()) {
body.add(entry.getKey(), entry.getValue());
}
}
}

HttpHeaders headers = new HttpHeaders();

// no client-secret, switch to PKCE
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,26 @@
package org.cloudfoundry.identity.uaa.provider.oauth;

import org.cloudfoundry.identity.uaa.provider.AbstractIdentityProviderDefinition;
import org.cloudfoundry.identity.uaa.provider.AbstractExternalOAuthIdentityProviderDefinition;
import org.cloudfoundry.identity.uaa.provider.AbstractIdentityProviderDefinition;
import org.cloudfoundry.identity.uaa.provider.BaseIdentityProviderValidator;
import org.cloudfoundry.identity.uaa.provider.OIDCIdentityProviderDefinition;
import org.springframework.stereotype.Component;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.Set;

import static org.springframework.util.StringUtils.hasText;

@Component
public class ExternalOAuthIdentityProviderConfigValidator extends BaseIdentityProviderValidator {

private static final Set<String> oAuthStandardParameters = Set.of("redirect_uri", "code", "client_id", "client_secret", "response_type",
"grant_type", "code_verifier", "client_assertion", "client_assertion_type", "code_challenge", "code_challenge_method", "nonce", "state",
"scope", "assertion", "subject_token", "actor_token", "username", "password");

@Override
public void validate(AbstractIdentityProviderDefinition definition) {
if (definition == null) {
Expand All @@ -26,19 +33,27 @@ public void validate(AbstractIdentityProviderDefinition definition) {
AbstractExternalOAuthIdentityProviderDefinition def = (AbstractExternalOAuthIdentityProviderDefinition) definition;

List<String> errors = new ArrayList<>();
if (def instanceof OIDCIdentityProviderDefinition && ((OIDCIdentityProviderDefinition) definition).getDiscoveryUrl() != null) {
//we don't require auth/token url or keys/key url
} else {
if (def.getAuthUrl() == null) {
errors.add("Authorization URL must be a valid URL");
}
if (def instanceof OIDCIdentityProviderDefinition) {
OIDCIdentityProviderDefinition oidcIdentityProviderDefinition = (OIDCIdentityProviderDefinition) def;
if (oidcIdentityProviderDefinition.getDiscoveryUrl() != null) {
//we don't require auth/token url or keys/key url
} else {
if (def.getAuthUrl() == null) {
errors.add("Authorization URL must be a valid URL");
}

if (def.getTokenUrl() == null) {
errors.add("Token URL must be a valid URL");
if (def.getTokenUrl() == null) {
errors.add("Token URL must be a valid URL");
}

if (!hasText(def.getTokenKey()) && def.getTokenKeyUrl() == null) {
errors.add("Either token key or token key URL must be specified");
}
}

if (!hasText(def.getTokenKey()) && def.getTokenKeyUrl() == null) {
errors.add("Either token key or token key URL must be specified");
if (Optional.ofNullable(oidcIdentityProviderDefinition.getAdditionalAuthzParameters()).orElse(Collections.emptyMap())
.keySet().stream().anyMatch(ExternalOAuthIdentityProviderConfigValidator::isOAuthStandardParameter)) {
errors.add("No OAuth standard parameters allowed in section additionalAuthzParameters");
}
}

Expand All @@ -56,4 +71,8 @@ public void validate(AbstractIdentityProviderDefinition definition) {
throw new IllegalArgumentException("Invalid config for Identity Provider " + errorMessages);
}
}

protected static boolean isOAuthStandardParameter(String value) {
return oAuthStandardParameters.contains(value);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import static java.util.Collections.emptyList;
import static java.util.Collections.emptyMap;
import static java.util.Optional.ofNullable;
import static org.cloudfoundry.identity.uaa.constants.OriginKeys.OAUTH20;
import static org.cloudfoundry.identity.uaa.constants.OriginKeys.OIDC10;
Expand Down Expand Up @@ -92,6 +94,9 @@ public String getIdpAuthenticationUrl(
if (OIDCIdentityProviderDefinition.class.equals(definition.getParameterizedClass())) {
var nonceGenerator = new RandomValueStringGenerator(12);
uriBuilder.queryParam("nonce", nonceGenerator.generate());

Map<String, String> additionalParameters = ofNullable(((OIDCIdentityProviderDefinition) definition).getAdditionalAuthzParameters()).orElse(emptyMap());
additionalParameters.keySet().stream().forEach(e -> uriBuilder.queryParam(e, additionalParameters.get(e)));
}

return uriBuilder.build().toUriString();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Locale;
import java.util.Map;

import static org.cloudfoundry.identity.uaa.constants.OriginKeys.OAUTH20;
Expand Down Expand Up @@ -142,9 +143,17 @@ protected void setCommonProperties(Map<String, Object> idpDefinitionMap, Abstrac
}
String discoveryUrl = (String) idpDefinitionMap.get("discoveryUrl");
try {
if (hasText(discoveryUrl) && idpDefinition instanceof OIDCIdentityProviderDefinition) {
((OIDCIdentityProviderDefinition) idpDefinition).setDiscoveryUrl(new URL(discoveryUrl));
} else {
OIDCIdentityProviderDefinition oidcIdentityProviderDefinition = null;
if (idpDefinition instanceof OIDCIdentityProviderDefinition) {
oidcIdentityProviderDefinition = (OIDCIdentityProviderDefinition) idpDefinition;
oidcIdentityProviderDefinition.setAdditionalAuthzParameters(parseAdditionalParameters(idpDefinitionMap));

if (hasText(discoveryUrl)) {
oidcIdentityProviderDefinition.setDiscoveryUrl(new URL(discoveryUrl));
}
}

if (oidcIdentityProviderDefinition == null || !hasText(discoveryUrl)) {
idpDefinition.setAuthUrl(new URL((String) idpDefinitionMap.get("authUrl")));
idpDefinition.setTokenKeyUrl(idpDefinitionMap.get("tokenKeyUrl") == null ? null : new URL((String) idpDefinitionMap.get("tokenKeyUrl")));
idpDefinition.setTokenUrl(new URL((String) idpDefinitionMap.get("tokenUrl")));
Expand All @@ -159,6 +168,29 @@ protected void setCommonProperties(Map<String, Object> idpDefinitionMap, Abstrac
}
}

private static Map<String, String> parseAdditionalParameters(Map<String, Object> idpDefinitionMap) {
Map<String, Object> additionalParameters = (Map<String, Object>) idpDefinitionMap.get("additionalAuthzParameters");
if (additionalParameters != null) {
Map<String,String> additionalQueryParameters = new HashMap<>();
for (Map.Entry<String, Object> entry : additionalParameters.entrySet()) {
String keyEntry = entry.getKey().toLowerCase(Locale.ROOT);
String value = null;
if (entry.getValue() instanceof Integer) {
value = String.valueOf(entry.getValue());
} else if (entry.getValue() instanceof String) {
value = (String) entry.getValue();
}
// accept only custom parameters, filter out standard parameters
if (value == null || ExternalOAuthIdentityProviderConfigValidator.isOAuthStandardParameter(keyEntry)) {
continue;
}
additionalQueryParameters.put(entry.getKey(), value);
}
return additionalQueryParameters;
}
return null;
}

/* parse with null check because default should be null */
private AbstractExternalOAuthIdentityProviderDefinition.OAuthGroupMappingMode parseExternalGroupMappingMode(Object mode) {
if (mode instanceof String) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,23 @@ void pkceWithJwtClientAuthInBody_is_used() {
mockUaaServer.verify();
}

@Test
void testAdditionalParameterClientAuthInBody_is_used() {
config.setClientAuthInBody(true);
config.setAdditionalAuthzParameters(Map.of("token_format", "opaque"));
mockUaaServer.expect(requestTo(config.getTokenUrl().toString()))
.andExpect(request -> assertThat("Check Auth header not present", request.getHeaders().get("Authorization"), nullValue()))
.andExpect(content().string(containsString("token_format=opaque")))
.andExpect(content().string(containsString("client_id=" + config.getRelyingPartyId())))
.andRespond(withStatus(OK).contentType(APPLICATION_JSON).body(getIdTokenResponse()));
IdentityProvider<AbstractExternalOAuthIdentityProviderDefinition> identityProvider = getProvider();
when(provisioning.retrieveByOrigin(eq(ORIGIN), anyString())).thenReturn(identityProvider);
Map<String, Object> idToken = externalOAuthAuthenticationManager.getClaimsFromToken(xCodeToken, config);
assertNotNull(idToken);

mockUaaServer.verify();
}

@Test
void idToken_In_Redirect_Should_Use_it() {
mockToken();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

import java.net.MalformedURLException;
import java.net.URL;
import java.util.Collections;
import java.util.Map;

public class ExternalOAuthIdentityProviderConfigValidatorTest {
private AbstractExternalOAuthIdentityProviderDefinition definition;
Expand Down Expand Up @@ -102,4 +104,26 @@ public void tokenKeyUrl_orTokenKeyMustBeSpecified() {
definition.setTokenKeyUrl(null);
validator.validate(definition);
}

@Test
public void testAdditionalParametersAdd() {
OIDCIdentityProviderDefinition oidcIdentityProviderDefinition = (OIDCIdentityProviderDefinition) definition;
// nothing
oidcIdentityProviderDefinition.setAdditionalAuthzParameters(null);
validator.validate(definition);
// empty
oidcIdentityProviderDefinition.setAdditionalAuthzParameters(Collections.emptyMap());
validator.validate(definition);
// list
oidcIdentityProviderDefinition.setAdditionalAuthzParameters(Map.of("token_format", "jwt", "token_key", "any"));
validator.validate(definition);
}

@Test(expected = IllegalArgumentException.class)
public void testAdditionalParametersError() {
OIDCIdentityProviderDefinition oidcIdentityProviderDefinition = (OIDCIdentityProviderDefinition) definition;
// one standard parameter, should fail
oidcIdentityProviderDefinition.setAdditionalAuthzParameters(Map.of("token_format", "jwt", "code", "1234"));
validator.validate(definition);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ void setup() throws MalformedURLException {
def.setRelyingPartySecret("clientSecret");
}
oidc.setResponseType("id_token code");
oidc.setAdditionalAuthzParameters(Map.of("token_format", "jwt"));
oauth.setResponseType("code");

configurator = spy(new ExternalOAuthProviderConfigurator(
Expand Down Expand Up @@ -330,4 +331,13 @@ void excludeUnreachableOidcProvider() throws OidcMetadataFetchingException {
assertEquals(oauthProvider.getName(), providers.get(0).getName());
verify(configurator, times(1)).overlay(eq(config));
}

@Test
void testGetIdpAuthenticationUrlAndCheckTokenFormatParameter() {
String authzUri = configurator.getIdpAuthenticationUrl(oidc, OIDC10, mockHttpServletRequest);

Map<String, String> queryParams =
UriComponentsBuilder.fromUriString(authzUri).build().getQueryParams().toSingleValueMap();
assertThat(queryParams, hasEntry("token_format", "jwt"));
}
}
Loading

0 comments on commit e3367a0

Please sign in to comment.