2nd alternative fix: allow to change or delete a relyingPartySecret o…

…n IdP (#2896) * WIP: idp secret * feature: delete secret on existing IdP - allow to delete a relyingPartySecret on IdP - Filter IdP list by origin - Return the auth_method to show current configured client authentication method * Documentation * fix names * sonar * sonar * Add patch call to change a secret from an external IdP * Alias handling * 2nd alternative fix: allow to change or delete a relyingPartySecret on IdP Alternative to PR: #2882, #2887 and PR: #2885 Only one call in backend, but more input on client side. authMethod is used to make this change non-breakable. Because before the update prevented the removal of the relyingPartySecret. No if authMethod is not client_secret_basic or client_secret_post, then the relyingPartySecret can be overwritten with null. * sonar * Tests added * Sonar smell * Review * more checks for edge cases * more tests to cover edge cases * Review Again fixed an edge case CLIENT_SECRET_BASIC, CLIENT_SECRET_POST both same method, therefore treat them equal * Review Again fixed an edge case Found during tests * Test refactored * small doc fixes - some clarification & formatting - no need to call out what the default is in the description because the `.optional("client_secret_basic")` syntax would automatically add that language. * doc: clarify when external OIDC client auth requirements - clarify when config.jwtClientAuthentication and config.relyingPartySecret are required in relation to the new field config.authMethod * Review, removed auth_method which is not used, but we ue authMethod field only * revert this * remove deprecated --------- Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
cloudfoundry · May 23, 2024 · 5812d75 · 5812d75
1 parent b382c9f
commit 5812d75
Show file tree

Hide file tree

Showing 12 changed files with 331 additions and 28 deletions.
diff --git a/model/src/main/java/org/cloudfoundry/identity/uaa/account/OpenIdConfiguration.java b/model/src/main/java/org/cloudfoundry/identity/uaa/account/OpenIdConfiguration.java
@@ -3,6 +3,7 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 import lombok.Data;
 import lombok.NoArgsConstructor;
+import org.cloudfoundry.identity.uaa.constants.ClientAuthentication;
 
 @Data
 @NoArgsConstructor
@@ -18,7 +19,7 @@ public class OpenIdConfiguration {
     private String tokenUrl;
 
     @JsonProperty("token_endpoint_auth_methods_supported")
-    private String[] tokenAMR = new String[]{"client_secret_basic", "client_secret_post", "private_key_jwt"};
+    private String[] tokenAMR = new String[]{ClientAuthentication.CLIENT_SECRET_BASIC, ClientAuthentication.CLIENT_SECRET_POST, ClientAuthentication.PRIVATE_KEY_JWT};
 
     @JsonProperty("token_endpoint_auth_signing_alg_values_supported")
     private String[] tokenEndpointAuthSigningValues = new String[]{"RS256", "HS256"};

diff --git a/model/src/main/java/org/cloudfoundry/identity/uaa/constants/ClientAuthentication.java b/model/src/main/java/org/cloudfoundry/identity/uaa/constants/ClientAuthentication.java
@@ -0,0 +1,55 @@
+package org.cloudfoundry.identity.uaa.constants;
+
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * ClientAuthentication constants are defined in OIDC core and discovery standard, e.g. https://openid.net/specs/openid-connect-registration-1_0.html
+ * OIDC possible values are: client_secret_post, client_secret_basic, client_secret_jwt, private_key_jwt, and none
+ * UAA knows only: client_secret_post, client_secret_basic, private_key_jwt, and none
+ *
+ * Planned: tls_client_auth
+ */
+public final class ClientAuthentication {
+
+  private ClientAuthentication() {}
+
+  public static final String CLIENT_SECRET_BASIC = "client_secret_basic";
+  public static final String CLIENT_SECRET_POST = "client_secret_post";
+  public static final String PRIVATE_KEY_JWT = "private_key_jwt";
+  public static final String NONE = "none";
+
+  public static final List<String> UAA_SUPPORTED_METHODS = List.of(CLIENT_SECRET_BASIC, CLIENT_SECRET_POST, NONE, PRIVATE_KEY_JWT);
+
+  public static boolean secretNeeded(String method) {
+    return method == null || CLIENT_SECRET_POST.equals(method) || CLIENT_SECRET_BASIC.equals(method);
+  }
+
+  public static boolean isMethodSupported(String method) {
+    return Optional.ofNullable(method).map(UAA_SUPPORTED_METHODS::contains).orElse(true);
+  }
+
+  public static boolean isValidMethod(String method, boolean hasSecret, boolean hasKeyConfiguration) {
+    return (isMethodSupported(method) && secretNeeded(method) && hasSecret && !hasKeyConfiguration  ||
+            isMethodSupported(method) && !secretNeeded(method) && !hasSecret ||
+            (method == null && (!hasSecret || !hasKeyConfiguration)));
+  }
+
+  public static boolean isAuthMethodEqual(String method1, String method2) {
+    return secretNeeded(method1) && secretNeeded(method2) || Objects.equals(method1, method2);
+  }
+
+  public static String getCalculatedMethod(String method, boolean hasSecret, boolean hasKeyConfiguration) {
+    if (method != null && isMethodSupported(method)) {
+      return method;
+    } else {
+      if (hasSecret) {
+        return CLIENT_SECRET_BASIC;
+      } else if (hasKeyConfiguration) {
+        return PRIVATE_KEY_JWT;
+      } else
+        return NONE;
+    }
+  }
+}
diff --git a/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/TokenConstants.java b/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/TokenConstants.java
@@ -15,6 +15,8 @@
 
 package org.cloudfoundry.identity.uaa.oauth.token;
 
+import org.cloudfoundry.identity.uaa.constants.ClientAuthentication;
+
 import java.util.Arrays;
 import java.util.List;
 import java.util.stream.Collectors;
@@ -62,9 +64,9 @@ static public List<String> getStringValues() {
     public static final String GRANT_TYPE_AUTHORIZATION_CODE = "authorization_code";
     public static final String GRANT_TYPE_IMPLICIT = "implicit";
 
-    public static final String CLIENT_AUTH_NONE = "none";
+    public static final String CLIENT_AUTH_NONE = ClientAuthentication.NONE;
     public static final String CLIENT_AUTH_EMPTY = "empty";
-    public static final String CLIENT_AUTH_PRIVATE_KEY_JWT = "private_key_jwt";
+    public static final String CLIENT_AUTH_PRIVATE_KEY_JWT = ClientAuthentication.PRIVATE_KEY_JWT;
 
     public static final String ID_TOKEN_HINT_PROMPT = "prompt";
     public static final String ID_TOKEN_HINT_PROMPT_NONE = "none";

diff --git a/...g/cloudfoundry/identity/uaa/provider/AbstractExternalOAuthIdentityProviderDefinition.java b/...g/cloudfoundry/identity/uaa/provider/AbstractExternalOAuthIdentityProviderDefinition.java
@@ -51,6 +51,8 @@ public enum OAuthGroupMappingMode {
     private OAuthGroupMappingMode groupMappingMode;
     private boolean pkce = true;
     private boolean performRpInitiatedLogout = true;
+    @JsonInclude(JsonInclude.Include.NON_NULL)
+    private String authMethod;
 
     public T setAuthUrl(URL authUrl) {
         this.authUrl = authUrl;
@@ -149,6 +151,14 @@ public void setPerformRpInitiatedLogout(boolean performRpInitiatedLogout) {
         this.performRpInitiatedLogout = performRpInitiatedLogout;
     }
 
+    public String getAuthMethod() {
+        return this.authMethod;
+    }
+
+    public void setAuthMethod(final String authMethod) {
+        this.authMethod = authMethod;
+    }
+
     @JsonIgnore
     public Class getParameterizedClass() {
         ParameterizedType parameterizedType =
@@ -183,6 +193,7 @@ public boolean equals(Object o) {
         if (!Objects.equals(groupMappingMode, that.groupMappingMode)) return false;
         if (pkce != that.pkce) return false;
         if (performRpInitiatedLogout != that.performRpInitiatedLogout) return false;
+        if (!Objects.equals(authMethod, that.authMethod)) return false;
         return Objects.equals(responseType, that.responseType);
 
     }
@@ -208,6 +219,7 @@ public int hashCode() {
         result = 31 * result + (responseType != null ? responseType.hashCode() : 0);
         result = 31 * result + (pkce ? 1 : 0);
         result = 31 * result + (performRpInitiatedLogout ? 1 : 0);
+        result = 31 * result + (authMethod != null ? authMethod.hashCode() : 0);
         return result;
     }
 }
diff --git a/model/src/test/java/org/cloudfoundry/identity/uaa/constants/ClientAuthenticationTest.java b/model/src/test/java/org/cloudfoundry/identity/uaa/constants/ClientAuthenticationTest.java
@@ -0,0 +1,83 @@
+package org.cloudfoundry.identity.uaa.constants;
+
+import org.junit.jupiter.api.Test;
+
+import static org.cloudfoundry.identity.uaa.constants.ClientAuthentication.CLIENT_SECRET_BASIC;
+import static org.cloudfoundry.identity.uaa.constants.ClientAuthentication.CLIENT_SECRET_POST;
+import static org.cloudfoundry.identity.uaa.constants.ClientAuthentication.NONE;
+import static org.cloudfoundry.identity.uaa.constants.ClientAuthentication.PRIVATE_KEY_JWT;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class ClientAuthenticationTest {
+
+  @Test
+  void secretNeeded() {
+    assertTrue(ClientAuthentication.secretNeeded(CLIENT_SECRET_POST));
+    assertTrue(ClientAuthentication.secretNeeded(CLIENT_SECRET_BASIC));
+    assertFalse(ClientAuthentication.secretNeeded(NONE));
+    assertFalse(ClientAuthentication.secretNeeded(PRIVATE_KEY_JWT));
+  }
+
+  @Test
+  void isMethodSupported() {
+    assertTrue(ClientAuthentication.isMethodSupported(CLIENT_SECRET_POST));
+    assertFalse(ClientAuthentication.isMethodSupported("foo"));
+  }
+
+  @Test
+  void isValidMethodTrue() {
+    assertTrue(ClientAuthentication.isValidMethod(NONE, false, false));
+    assertTrue(ClientAuthentication.isValidMethod(PRIVATE_KEY_JWT, false, true));
+    assertTrue(ClientAuthentication.isValidMethod(CLIENT_SECRET_POST, true, false));
+    assertTrue(ClientAuthentication.isValidMethod(CLIENT_SECRET_BASIC, true, false));
+    // legacy checks, no method passed
+    assertTrue(ClientAuthentication.isValidMethod(null, false, false));
+    assertTrue(ClientAuthentication.isValidMethod(null, true, false));
+    assertTrue(ClientAuthentication.isValidMethod(null, false, true));
+
+  }
+
+  @Test
+  void isValidMethodFalse() {
+    assertFalse(ClientAuthentication.isValidMethod(CLIENT_SECRET_BASIC, false, false));
+    assertFalse(ClientAuthentication.isValidMethod(CLIENT_SECRET_POST, false, false));
+    assertFalse(ClientAuthentication.isValidMethod(NONE, true, false));
+    assertFalse(ClientAuthentication.isValidMethod(PRIVATE_KEY_JWT, true, true));
+    assertFalse(ClientAuthentication.isValidMethod(CLIENT_SECRET_BASIC, true, true));
+    // legacy checks, no method passed
+    assertFalse(ClientAuthentication.isValidMethod(null, true, true));
+  }
+
+  @Test
+  void getCalculatedMethod() {
+    assertEquals(NONE, ClientAuthentication.getCalculatedMethod(NONE, false, false));
+    assertEquals(NONE, ClientAuthentication.getCalculatedMethod(null, false, false));
+    assertEquals(PRIVATE_KEY_JWT, ClientAuthentication.getCalculatedMethod(PRIVATE_KEY_JWT, false, true));
+    assertEquals(PRIVATE_KEY_JWT, ClientAuthentication.getCalculatedMethod(null, false, true));
+    assertEquals(CLIENT_SECRET_BASIC, ClientAuthentication.getCalculatedMethod(CLIENT_SECRET_BASIC, true, false));
+    assertEquals(CLIENT_SECRET_BASIC, ClientAuthentication.getCalculatedMethod(null, true, false));
+  }
+
+  @Test
+  void isAuthMethodEqualTrue() {
+    assertTrue(ClientAuthentication.isAuthMethodEqual(NONE, NONE));
+    assertTrue(ClientAuthentication.isAuthMethodEqual(CLIENT_SECRET_BASIC, CLIENT_SECRET_POST));
+    assertTrue(ClientAuthentication.isAuthMethodEqual(CLIENT_SECRET_POST, CLIENT_SECRET_BASIC));
+    assertTrue(ClientAuthentication.isAuthMethodEqual(CLIENT_SECRET_BASIC, CLIENT_SECRET_BASIC));
+    assertTrue(ClientAuthentication.isAuthMethodEqual(CLIENT_SECRET_POST, CLIENT_SECRET_POST));
+    assertTrue(ClientAuthentication.isAuthMethodEqual(PRIVATE_KEY_JWT, PRIVATE_KEY_JWT));
+    assertTrue(ClientAuthentication.isAuthMethodEqual(null, null));
+    assertTrue(ClientAuthentication.isAuthMethodEqual(null, CLIENT_SECRET_BASIC));
+    assertTrue(ClientAuthentication.isAuthMethodEqual(null, CLIENT_SECRET_POST));
+    assertTrue(ClientAuthentication.isAuthMethodEqual(CLIENT_SECRET_BASIC, null));
+  }
+
+  @Test
+  void isAuthMethodEqualFalse() {
+    assertFalse(ClientAuthentication.isAuthMethodEqual(PRIVATE_KEY_JWT, null));
+    assertFalse(ClientAuthentication.isAuthMethodEqual(PRIVATE_KEY_JWT, CLIENT_SECRET_BASIC));
+    assertFalse(ClientAuthentication.isAuthMethodEqual(PRIVATE_KEY_JWT, NONE));
+  }
+}
diff --git a/model/src/test/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderTest.java b/model/src/test/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderTest.java
@@ -58,6 +58,7 @@ void testEqualsAndHashCode() {
         idp1.setIdentityZoneId(UAA);
         final OIDCIdentityProviderDefinition config1 = new OIDCIdentityProviderDefinition();
         config1.setIssuer("issuer");
+        config1.setAuthMethod("none");
         idp1.setConfig(config1);
 
         final IdentityProvider<OIDCIdentityProviderDefinition> idp2 = new IdentityProvider<>();
@@ -70,6 +71,7 @@ void testEqualsAndHashCode() {
         idp2.setIdentityZoneId(UAA);
         final OIDCIdentityProviderDefinition config2 = new OIDCIdentityProviderDefinition();
         config2.setIssuer("issuer");
+        config2.setAuthMethod("none");
         idp2.setConfig(config2);
 
         idp2.setCreated(idp1.getCreated());

diff --git a/...test/java/org/cloudfoundry/identity/uaa/provider/OIDCIdentityProviderDefinitionTests.java b/...test/java/org/cloudfoundry/identity/uaa/provider/OIDCIdentityProviderDefinitionTests.java
@@ -80,5 +80,6 @@ public void serialize_jwtClientAuthentication() {
         String json = JsonUtils.writeValueAsString(def);
         def = JsonUtils.readValue(json, OIDCIdentityProviderDefinition.class);
         assertEquals(settings, def.getJwtClientAuthentication());
+        assertNull(def.getAuthMethod());
     }
 }
diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderEndpoints.java
@@ -43,6 +43,8 @@
 import org.cloudfoundry.identity.uaa.audit.event.EntityDeletedEvent;
 import org.cloudfoundry.identity.uaa.authentication.manager.DynamicLdapAuthenticationManager;
 import org.cloudfoundry.identity.uaa.authentication.manager.LdapLoginAuthenticationManager;
+import org.cloudfoundry.identity.uaa.constants.ClientAuthentication;
+import org.cloudfoundry.identity.uaa.provider.oauth.ExternalOAuthIdentityProviderConfigValidator;
 import org.cloudfoundry.identity.uaa.provider.saml.SamlIdentityProviderConfigurator;
 import org.cloudfoundry.identity.uaa.scim.ScimGroupExternalMembershipManager;
 import org.cloudfoundry.identity.uaa.scim.ScimGroupProvisioning;
@@ -168,6 +170,7 @@ public ResponseEntity<IdentityProvider> createIdentityProvider(@RequestBody Iden
             return new ResponseEntity<>(body, UNPROCESSABLE_ENTITY);
         }
         createdIdp.setSerializeConfigRaw(rawConfig);
+        setAuthMethod(createdIdp);
         redactSensitiveData(createdIdp);
 
         return new ResponseEntity<>(createdIdp, CREATED);
@@ -194,6 +197,7 @@ public ResponseEntity<IdentityProvider> deleteIdentityProvider(@PathVariable Str
         // delete the IdP
         existing.setSerializeConfigRaw(rawConfig);
         publisher.publishEvent(new EntityDeletedEvent<>(existing, authentication, identityZoneId));
+        setAuthMethod(existing);
         redactSensitiveData(existing);
 
         // delete the alias IdP if present
@@ -275,6 +279,7 @@ public ResponseEntity<IdentityProvider> updateIdentityProvider(@PathVariable Str
             return new ResponseEntity<>(body, UNPROCESSABLE_ENTITY);
         }
         updatedIdp.setSerializeConfigRaw(rawConfig);
+        setAuthMethod(updatedIdp);
         redactSensitiveData(updatedIdp);
 
         return new ResponseEntity<>(updatedIdp, OK);
@@ -314,6 +319,7 @@ public ResponseEntity<List<IdentityProvider>> retrieveIdentityProviders(@Request
         List<IdentityProvider> identityProviderList = identityProviderProvisioning.retrieveAll(retrieveActiveOnly, identityZoneManager.getCurrentIdentityZoneId());
         for(IdentityProvider idp : identityProviderList) {
             idp.setSerializeConfigRaw(rawConfig);
+            setAuthMethod(idp);
             redactSensitiveData(idp);
         }
         return new ResponseEntity<>(identityProviderList, OK);
@@ -323,6 +329,7 @@ public ResponseEntity<List<IdentityProvider>> retrieveIdentityProviders(@Request
     public ResponseEntity<IdentityProvider> retrieveIdentityProvider(@PathVariable String id, @RequestParam(required = false, defaultValue = "false") boolean rawConfig) {
         IdentityProvider identityProvider = identityProviderProvisioning.retrieve(id, identityZoneManager.getCurrentIdentityZoneId());
         identityProvider.setSerializeConfigRaw(rawConfig);
+        setAuthMethod(identityProvider);
         redactSensitiveData(identityProvider);
         return new ResponseEntity<>(identityProvider, OK);
     }
@@ -422,16 +429,13 @@ protected void patchSensitiveData(String id, IdentityProvider provider) {
             }
             case OAUTH20 :
             case OIDC10 : {
-                if (provider.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition) {
-                    AbstractExternalOAuthIdentityProviderDefinition definition = (AbstractExternalOAuthIdentityProviderDefinition) provider.getConfig();
-                    if (definition.getRelyingPartySecret() == null) {
-                        IdentityProvider existing = identityProviderProvisioning.retrieve(id, zoneId);
-                        if (existing!=null &&
-                            existing.getConfig()!=null &&
-                            existing.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition) {
-                            AbstractExternalOAuthIdentityProviderDefinition existingDefinition = (AbstractExternalOAuthIdentityProviderDefinition)existing.getConfig();
-                            definition.setRelyingPartySecret(existingDefinition.getRelyingPartySecret());
-                        }
+                if (provider.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition definition && definition.getRelyingPartySecret() == null) {
+                    IdentityProvider existing = identityProviderProvisioning.retrieve(id, zoneId);
+                    if (existing!=null &&
+                        existing.getConfig()!=null &&
+                        existing.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition existingDefinition &&
+                        secretNeeded(definition)) {
+                        definition.setRelyingPartySecret(existingDefinition.getRelyingPartySecret());
                     }
                 }
                 break;
@@ -470,4 +474,18 @@ protected void redactSensitiveData(IdentityProvider provider) {
         }
     }
 
+    protected boolean secretNeeded(AbstractExternalOAuthIdentityProviderDefinition abstractExternalOAuthIdentityProviderDefinition) {
+        boolean needSecret = true;
+        if (abstractExternalOAuthIdentityProviderDefinition.getAuthMethod() != null) {
+            return ClientAuthentication.secretNeeded(abstractExternalOAuthIdentityProviderDefinition.getAuthMethod());
+        }
+        return needSecret;
+    }
+
+    protected void setAuthMethod(IdentityProvider<?> provider) {
+        if (provider.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition<?> definition) {
+            definition.setAuthMethod(ExternalOAuthIdentityProviderConfigValidator.getAuthMethod(definition));
+        }
+    }
+
 }