Improve the parsing and validation of UserSecretReferenceUrns (#1840)

This change addresses all the TODOs found the org.polaris.core.secrets package. 
Main changes:

- Create a helper to parse, validate and build the URN strings. 
- Use Regex instead of `String.split()`.
- Add Precondition checks to ensure that the URN is valid and the UserSecretManager matches the expected type. 
- Remove the now unused `GLOBAL_INSTANCE` of the UnsafeInMemorySecretsManager.

Testing 
- Existing `UnsafeInMemorySecretsManagerTest` captures most of the functional changes. 
- Added `UserSecretReferenceUrnHelperTest` to capture the utilities exposed.

Author: poojanilangekar

polaris-core/src/main/java/org/apache/polaris/core/secrets/UnsafeInMemorySecretsManager.java

@@ -18,6 +18,7 @@
  */
 package org.apache.polaris.core.secrets;
 
+import com.google.common.base.Preconditions;
 import jakarta.annotation.Nonnull;
 import java.nio.charset.StandardCharsets;
 import java.security.SecureRandom;
@@ -34,9 +35,6 @@
  * development purposes.
  */
 public class UnsafeInMemorySecretsManager implements UserSecretsManager {
-  // TODO: Remove this and wire into QuarkusProducers; just a placeholder for now to get the
-  // rest of the logic working.
-  public static final UserSecretsManager GLOBAL_INSTANCE = new UnsafeInMemorySecretsManager();
 
   private final Map<String, String> rawSecretStore = new ConcurrentHashMap<>();
   private final SecureRandom rand = new SecureRandom();
@@ -45,6 +43,8 @@ public class UnsafeInMemorySecretsManager implements UserSecretsManager {
   private static final String CIPHERTEXT_HASH = "ciphertext-hash";
   private static final String ENCRYPTION_KEY = "encryption-key";
 
+  public static final String SECRET_MANAGER_TYPE = "unsafe-in-memory";
+
   /** {@inheritDoc} */
   @Override
   @Nonnull
@@ -73,10 +73,8 @@ public UserSecretReference writeSecret(
 
     String secretUrn;
     for (int secretOrdinal = 0; ; ++secretOrdinal) {
-      secretUrn =
-          String.format(
-              "urn:polaris-secret:unsafe-in-memory:%d:%d", forEntity.getId(), secretOrdinal);
-
+      String typeSpecificIdentifier = forEntity.getId() + ":" + secretOrdinal;
+      secretUrn = buildUrn(SECRET_MANAGER_TYPE, typeSpecificIdentifier);
       // Store the base64-encoded encrypted ciphertext in the simulated "secret store".
       String existingSecret =
           rawSecretStore.putIfAbsent(secretUrn, encryptedSecretCipherTextBase64);
@@ -107,7 +105,14 @@ public UserSecretReference writeSecret(
   @Override
   @Nonnull
   public String readSecret(@Nonnull UserSecretReference secretReference) {
-    // TODO: Precondition checks and/or wire in PolarisDiagnostics
+    String secretManagerType = secretReference.getUserSecretManagerType();
+    Preconditions.checkState(
+        secretManagerType.equals(SECRET_MANAGER_TYPE),
+        "Invalid secret manager type, expected: "
+            + SECRET_MANAGER_TYPE
+            + " got: "
+            + secretManagerType);
+
     String encryptedSecretCipherTextBase64 = rawSecretStore.get(secretReference.getUrn());
     if (encryptedSecretCipherTextBase64 == null) {
       // Secret at this URN no longer exists.

polaris-core/src/main/java/org/apache/polaris/core/secrets/UserSecretReference.java

@@ -27,6 +27,8 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 /**
  * Represents a "wrapped reference" to a user-owned secret that holds an identifier to retrieve
@@ -56,6 +58,43 @@ public class UserSecretReference {
   @JsonProperty(value = "referencePayload")
   private final Map<String, String> referencePayload;
 
+  private static final String URN_SCHEME = "urn";
+  private static final String URN_NAMESPACE = "polaris-secret";
+  private static final String SECRET_MANAGER_TYPE_REGEX = "([a-zA-Z0-9_-]+)";
+  private static final String TYPE_SPECIFIC_IDENTIFIER_REGEX =
+      "([a-zA-Z0-9_-]+(?::[a-zA-Z0-9_-]+)*)";
+
+  /**
+   * Precompiled regex pattern for validating the secret manager type and type-specific identifier.
+   */
+  private static final Pattern SECRET_MANAGER_TYPE_PATTERN =
+      Pattern.compile("^" + SECRET_MANAGER_TYPE_REGEX + "$");
+
+  private static final Pattern TYPE_SPECIFIC_IDENTIFIER_PATTERN =
+      Pattern.compile("^" + TYPE_SPECIFIC_IDENTIFIER_REGEX + "$");
+
+  /**
+   * Precompiled regex pattern for validating and parsing UserSecretReference URNs. Expected format:
+   * urn:polaris-secret:<secret-manager-type>:<identifier1>(:<identifier2>:...).
+   *
+   * <p>Groups:
+   *
+   * <p>Group 1: secret-manager-type (alphanumeric, hyphens, underscores).
+   *
+   * <p>Group 2: type-specific-identifier (one or more colon-separated alphanumeric components).
+   */
+  private static final Pattern URN_PATTERN =
+      Pattern.compile(
+          "^"
+              + URN_SCHEME
+              + ":"
+              + URN_NAMESPACE
+              + ":"
+              + SECRET_MANAGER_TYPE_REGEX
+              + ":"
+              + TYPE_SPECIFIC_IDENTIFIER_REGEX
+              + "$");
+
   /**
    * @param urn A string which should be self-sufficient to retrieve whatever secret material that
    *     is stored in the remote secret store and also to identify an implementation of the
@@ -70,26 +109,83 @@ public class UserSecretReference {
   public UserSecretReference(
       @JsonProperty(value = "urn", required = true) @Nonnull String urn,
       @JsonProperty(value = "referencePayload") @Nullable Map<String, String> referencePayload) {
-    // TODO: Add better/standardized parsing and validation of URN syntax
     Preconditions.checkArgument(
-        urn.startsWith("urn:polaris-secret:") && urn.split(":").length >= 4,
-        "Invalid secret URN '%s'; must be of the form "
-            + "'urn:polaris-secret:<secret-manager-type>:<type-specific-identifier>'",
-        urn);
+        urnIsValid(urn),
+        "Invalid secret URN: " + urn + "; must be of the form: " + URN_PATTERN.toString());
     this.urn = urn;
     this.referencePayload = Objects.requireNonNullElse(referencePayload, new HashMap<>());
   }
 
+  /**
+   * Validates whether the given URN string matches the expected format for UserSecretReference
+   * URNs.
+   *
+   * @param urn The URN string to validate.
+   * @return true if the URN is valid, false otherwise.
+   */
+  private static boolean urnIsValid(@Nonnull String urn) {
+    return urn.trim().isEmpty() ? false : URN_PATTERN.matcher(urn).matches();
+  }
+
+  /**
+   * Builds a URN string from the given secret manager type and type-specific identifier. Validates
+   * the inputs to ensure they conform to the expected pattern.
+   *
+   * @param secretManagerType The secret manager type (alphanumeric, hyphens, underscores).
+   * @param typeSpecificIdentifier The type-specific identifier (colon-separated alphanumeric
+   *     components).
+   * @return The constructed URN string.
+   */
+  @Nonnull
+  public static String buildUrnString(
+      @Nonnull String secretManagerType, @Nonnull String typeSpecificIdentifier) {
+
+    Preconditions.checkArgument(
+        !secretManagerType.trim().isEmpty(), "Secret manager type cannot be empty");
+    Preconditions.checkArgument(
+        SECRET_MANAGER_TYPE_PATTERN.matcher(secretManagerType).matches(),
+        "Invalid secret manager type '%s'; must contain only alphanumeric characters, hyphens, and underscores",
+        secretManagerType);
+
+    Preconditions.checkArgument(
+        !typeSpecificIdentifier.trim().isEmpty(), "Type-specific identifier cannot be empty");
+    Preconditions.checkArgument(
+        TYPE_SPECIFIC_IDENTIFIER_PATTERN.matcher(typeSpecificIdentifier).matches(),
+        "Invalid type-specific identifier '%s'; must be colon-separated alphanumeric components (hyphens and underscores allowed)",
+        typeSpecificIdentifier);
+
+    return URN_SCHEME
+        + ":"
+        + URN_NAMESPACE
+        + ":"
+        + secretManagerType
+        + ":"
+        + typeSpecificIdentifier;
+  }
+
   /**
    * Since UserSecretReference objects are specific to UserSecretManager implementations, the
    * "secret-manager-type" portion of the URN should be used to validate that a URN is valid for a
    * given implementation and to dispatch to the correct implementation at runtime if multiple
    * concurrent implementations are possible in a given runtime environment.
    */
   @JsonIgnore
-  public String getUserSecretManagerTypeFromUrn() {
-    // TODO: Add better/standardized parsing and validation of URN syntax
-    return urn.split(":")[2];
+  public String getUserSecretManagerType() {
+    Matcher matcher = URN_PATTERN.matcher(urn);
+    Preconditions.checkState(matcher.matches(), "Invalid secret URN: " + urn);
+    return matcher.group(1);
+  }
+
+  /**
+   * Returns the type-specific identifier from the URN. Since the format is specific to the
+   * UserSecretManager implementation, this method does not validate the identifier. It is the
+   * responsibility of the caller to validate it.
+   */
+  @JsonIgnore
+  public String getTypeSpecificIdentifier() {
+    Matcher matcher = URN_PATTERN.matcher(urn);
+    Preconditions.checkState(matcher.matches(), "Invalid secret URN: " + urn);
+    return matcher.group(2);
   }
 
   public @Nonnull String getUrn() {

polaris-core/src/main/java/org/apache/polaris/core/secrets/UserSecretsManager.java

@@ -63,4 +63,17 @@ public interface UserSecretsManager {
    * @param secretReference Reference object for retrieving the original secret
    */
   void deleteSecret(@Nonnull UserSecretReference secretReference);
+
+  /**
+   * Builds a URN string from the given secret manager type and type-specific identifier.
+   *
+   * @param typeSpecificIdentifier The type-specific identifier (colon-separated alphanumeric
+   *     components with underscores and hyphens).
+   * @return The constructed URN string.
+   */
+  @Nonnull
+  default String buildUrn(
+      @Nonnull String secretManagerType, @Nonnull String typeSpecificIdentifier) {
+    return UserSecretReference.buildUrnString(secretManagerType, typeSpecificIdentifier);
+  }
 }

polaris-core/src/test/java/org/apache/polaris/core/secrets/UserSecretReferenceTest.java

@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.secrets;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+public class UserSecretReferenceTest {
+
+  @ParameterizedTest
+  @ValueSource(
+      strings = {
+        "urn:polaris-secret:unsafe-in-memory:key1",
+        "urn:polaris-secret:unsafe-in-memory:key1:value1",
+        "urn:polaris-secret:aws_secrets-manager:my-key_123",
+        "urn:polaris-secret:vault:project:env:service:key"
+      })
+  public void testValidUrns(String validUrn) {
+    assertThat(new UserSecretReference(validUrn, null)).isNotNull();
+  }
+
+  @ParameterizedTest
+  @ValueSource(
+      strings = {
+        "",
+        "   ",
+        "not-a-urn",
+        "urn:",
+        "urn:polaris-secret:",
+        "urn:polaris-secret:type:",
+        "wrong:polaris-secret:type:key:",
+        "urn:polaris-secret:type with spaces:key",
+        "urn:polaris-secret:type@invalid:key",
+        "urn:polaris-secret:unsafe-in-memory:key::"
+      })
+  public void testInvalidUrns(String invalidUrn) {
+    assertThatThrownBy(() -> new UserSecretReference(invalidUrn, null))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining("Invalid secret URN: " + invalidUrn);
+  }
+
+  @Test
+  public void tesGetUrnComponents() {
+    String urn = "urn:polaris-secret:unsafe-in-memory:key1:value1";
+    UserSecretReference reference = new UserSecretReference(urn, null);
+
+    assertThat(reference.getUserSecretManagerType()).isEqualTo("unsafe-in-memory");
+    assertThat(reference.getTypeSpecificIdentifier()).isEqualTo("key1:value1");
+  }
+
+  @Test
+  public void testBuildUrn() {
+    String urn = UserSecretReference.buildUrnString("aws-secrets", "my-key");
+    assertThat(urn).isEqualTo("urn:polaris-secret:aws-secrets:my-key");
+
+    String urnWithMultipleIdentifiers =
+        UserSecretReference.buildUrnString("vault", "project:service");
+    assertThat(urnWithMultipleIdentifiers).isEqualTo("urn:polaris-secret:vault:project:service");
+
+    String urnWithNumbers = UserSecretReference.buildUrnString("type_123", "456:789");
+    assertThat(urnWithNumbers).isEqualTo("urn:polaris-secret:type_123:456:789");
+  }
+}