Refactor StorageId

Fixes #9031

server-spi/src/main/java/org/keycloak/storage/StorageId.java

@@ -21,35 +21,52 @@ import org.keycloak.models.ClientModel;
 import org.keycloak.models.UserModel;
 
 import java.io.Serializable;
+import java.util.Objects;
 
 /**
  * @author <a href="mailto:bill@burkecentral.com">Bill Burke</a>
  * @version $Revision: 1 $
  */
 public class StorageId implements Serializable {
-    private String id;
-    private String providerId;
-    private String externalId;
+    private final String providerId;
+    private final String externalId;
 
 
     public StorageId(String id) {
-        this.id = id;
         if (!id.startsWith("f:")) {
+            providerId = null;
             externalId = id;
-            return;
+        } else {
+            int providerIndex = id.indexOf(':', 2);
+            providerId = id.substring(2, providerIndex);
+            externalId = id.substring(providerIndex + 1);
         }
-        int providerIndex = id.indexOf(':', 2);
-        providerId = id.substring(2, providerIndex);
-        externalId = id.substring(providerIndex + 1);
-
     }
 
     public StorageId(String providerId, String externalId) {
-        this.id = "f:" + providerId + ":" + externalId;
+        if (providerId != null && providerId.contains(":")) {
+            throw new IllegalArgumentException("Provider must not contain a colon (:) character");
+        }
         this.providerId = providerId;
         this.externalId = externalId;
     }
 
+    public boolean isLocal() {
+        return getProviderId() == null;
+    }
+
+    public String getId() {
+        return providerId == null ? externalId : ("f:" + providerId + ":" + externalId);
+    }
+
+    public String getProviderId() {
+        return providerId;
+    }
+
+    public String getExternalId() {
+        return externalId;
+    }
+
     /**
      * generate the id string that should be returned by UserModel.getId()
      *
@@ -68,40 +85,67 @@ public class StorageId implements Serializable {
         return new StorageId(keycloakId).getProviderId();
     }
 
-
-
-    public static String resolveProviderId(UserModel user) {
-        return new StorageId(user.getId()).getProviderId();
-    }
-    public static boolean isLocalStorage(UserModel user) {
-        return new StorageId(user.getId()).getProviderId() == null;
-    }
     public static boolean isLocalStorage(String id) {
         return new StorageId(id).getProviderId() == null;
     }
 
+    /**
+     * @deprecated Use {@link #providerId(String)} instead.
+     */
+    public static String resolveProviderId(UserModel user) {
+        return providerId(user.getId());
+    }
+
+    /**
+     * @deprecated Use {@link #isLocalStorage(String)} instead.
+     */
+    public static boolean isLocalStorage(UserModel user) {
+        return isLocalStorage(user.getId());
+    }
+
+    /**
+     * @deprecated Use {@link #providerId(String)} instead.
+     */
     public static String resolveProviderId(ClientModel client) {
-        return new StorageId(client.getId()).getProviderId();
+        return providerId(client.getId());
     }
+
+    /**
+     * @deprecated Use {@link #isLocalStorage(String)} instead.
+     */
     public static boolean isLocalStorage(ClientModel client) {
-        return new StorageId(client.getId()).getProviderId() == null;
-    }
-    public boolean isLocal() {
-        return getProviderId() == null;
-
+        return isLocalStorage(client.getId());
     }
 
-    public String getId() {
-        return id;
+    @Override
+    public int hashCode() {
+        return Objects.hash(providerId, externalId);
     }
 
-    public String getProviderId() {
-        return providerId;
+    @Override
+    public boolean equals(Object obj) {
+        if (this == obj) {
+            return true;
+        }
+        if (obj == null) {
+            return false;
+        }
+        if (getClass() != obj.getClass()) {
+            return false;
+        }
+        final StorageId other = (StorageId) obj;
+        if ( ! Objects.equals(this.providerId, other.providerId)) {
+            return false;
+        }
+        if ( ! Objects.equals(this.externalId, other.externalId)) {
+            return false;
+        }
+        return true;
     }
 
-    public String getExternalId() {
-        return externalId;
+    @Override
+    public String toString() {
+        return getId();
     }
 
-
 }

server-spi/src/test/java/org/keycloak/storage/StorageIdTest.java

@@ -0,0 +1,88 @@
+/*
+ * Copyright 2021 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ * 
+ * Licensed 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.keycloak.storage;
+
+import org.keycloak.component.ComponentModel;
+import org.junit.Test;
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.CoreMatchers.notNullValue;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+/**
+ *
+ * @author hmlnarik
+ */
+public class StorageIdTest {
+
+    @Test
+    public void testStatic() {
+        final String localId = "123";
+        assertThat(StorageId.externalId(localId), is("123"));
+        assertThat(StorageId.providerId(localId), nullValue());
+        assertTrue(StorageId.isLocalStorage(localId));
+
+        final String remoteId = "f:abc:123";
+        assertThat(StorageId.externalId(remoteId), is("123"));
+        assertThat(StorageId.providerId(remoteId), is("abc"));
+        assertFalse(StorageId.isLocalStorage(remoteId));
+
+        final ComponentModel cm = new ComponentModel();
+        cm.setId("localId");
+        assertThat(StorageId.keycloakId(cm, localId), is("f:localId:123"));
+    }
+
+    @Test
+    public void testLocalId() {
+        StorageId id = new StorageId("123");
+        assertThat(id, notNullValue());
+        assertThat(id.getExternalId(), is("123"));
+        assertThat(id.getProviderId(), nullValue());
+        assertThat(id.getId(), is("123"));
+        assertTrue(id.isLocal());
+    }
+
+    @Test
+    public void testExternalIdString() {
+        StorageId id = new StorageId("f:abc:123");
+        assertThat(id, notNullValue());
+        assertThat(id.getExternalId(), is("123"));
+        assertThat(id.getProviderId(), is("abc"));
+        assertThat(id.getId(), is("f:abc:123"));
+        assertFalse(id.isLocal());
+    }
+
+    @Test
+    public void testExternalIdTwoStrings() {
+        StorageId id = new StorageId("abc", "123");
+        assertThat(id, notNullValue());
+        assertThat(id.getExternalId(), is("123"));
+        assertThat(id.getProviderId(), is("abc"));
+        assertThat(id.getId(), is("f:abc:123"));
+        assertFalse(id.isLocal());
+    }
+
+    @Test
+    public void testEquals() {
+        assertThat(new StorageId("abc", "123"), equalTo(new StorageId("f:abc:123")));
+        assertThat(new StorageId("abc", "123"), not(equalTo(new StorageId("f:abc:1234"))));
+    }
+}