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.

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

feature: Allow sending static key/value pairs to the configured IdP #2397

Merged
merged 16 commits into from
Jul 17, 2023
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 @@ -90,6 +104,7 @@ 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 (this.additionalAuthzParameters != that.additionalAuthzParameters) return false;
strehle marked this conversation as resolved.
Show resolved Hide resolved
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());
strehle marked this conversation as resolved.
Show resolved Hide resolved
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,24 @@
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.List;
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 +31,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 (oidcIdentityProviderDefinition.getAdditionalAuthzParameters() != null &&
oAuthStandardParameters.stream().anyMatch(oidcIdentityProviderDefinition.getAdditionalAuthzParameters().keySet()::contains)) {
strehle marked this conversation as resolved.
Show resolved Hide resolved
errors.add("No OAuth standard parameters allowed in section additionalAuthzParameters");
}
}

Expand All @@ -56,4 +69,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