[VAULT-3347] Ensure Deduplication in Provider and Client APIs in OIDC…

… Provider (#12460) * add deduplication for Provider * add deduplication to provider client API * add changelog * delete changelog * update comments * update test names
hashicorp · Aug 30, 2021 · 09909e7 · 09909e7
1 parent a302aaf
commit 09909e7
Show file tree

Hide file tree

Showing 2 changed files with 115 additions and 0 deletions.
diff --git a/vault/identity_store_oidc_provider.go b/vault/identity_store_oidc_provider.go
@@ -653,6 +653,10 @@ func (i *IdentityStore) pathOIDCCreateUpdateClient(ctx context.Context, req *log
 		client.Assignments = d.Get("assignments").([]string)
 	}
 
+	// remove duplicate assignments and redirect URIs
+	client.Assignments = strutil.RemoveDuplicates(client.Assignments, false)
+	client.RedirectURIs = strutil.RemoveDuplicates(client.RedirectURIs, false)
+
 	// enforce assignment existence
 	for _, assignment := range client.Assignments {
 		entry, err := req.Storage.Get(ctx, assignmentPath+assignment)
@@ -825,6 +829,10 @@ func (i *IdentityStore) pathOIDCCreateUpdateProvider(ctx context.Context, req *l
 		provider.Scopes = d.GetDefaultOrZero("scopes").([]string)
 	}
 
+	// remove duplicate allowed client IDs and scopes
+	provider.AllowedClientIDs = strutil.RemoveDuplicates(provider.AllowedClientIDs, false)
+	provider.Scopes = strutil.RemoveDuplicates(provider.Scopes, false)
+
 	if provider.Issuer != "" {
 		// verify that issuer is the correct format:
 		//   - http or https

diff --git a/vault/identity_store_oidc_provider_test.go b/vault/identity_store_oidc_provider_test.go
@@ -259,6 +259,65 @@ func TestOIDC_Path_OIDC_ProviderClient(t *testing.T) {
 	}
 }
 
+// TestOIDC_Path_OIDC_ProviderClient_DeDuplication tests that a
+// client doesn't have duplicate redirect URIs or Assignments
+func TestOIDC_Path_OIDC_ProviderClient_Deduplication(t *testing.T) {
+	c, _, _ := TestCoreUnsealed(t)
+	ctx := namespace.RootContext(nil)
+	storage := &logical.InmemStorage{}
+
+	// Create a test key "test-key"
+	c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/key/test-key",
+		Operation: logical.CreateOperation,
+		Data: map[string]interface{}{
+			"verification_ttl": "2m",
+			"rotation_period":  "2m",
+		},
+		Storage: storage,
+	})
+
+	// Create a test assignment "test-assignment1" -- should succeed
+	c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/assignment/test-assignment1",
+		Operation: logical.CreateOperation,
+		Storage:   storage,
+	})
+
+	// Create a test client "test-client" -- should succeed
+	resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/client/test-client",
+		Operation: logical.CreateOperation,
+		Storage:   storage,
+		Data: map[string]interface{}{
+			"key":           "test-key",
+			"assignments":   []string{"test-assignment1", "test-assignment1"},
+			"redirect_uris": []string{"http://example.com", "http://notduplicate.com", "http://example.com"},
+		},
+	})
+	expectSuccess(t, resp, err)
+
+	// Read "test-client" and validate
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/client/test-client",
+		Operation: logical.ReadOperation,
+		Storage:   storage,
+	})
+	expectSuccess(t, resp, err)
+	expected := map[string]interface{}{
+		"redirect_uris":    []string{"http://example.com", "http://notduplicate.com"},
+		"assignments":      []string{"test-assignment1"},
+		"key":              "test-key",
+		"id_token_ttl":     0,
+		"access_token_ttl": 0,
+		"client_id":        resp.Data["client_id"],
+		"client_secret":    resp.Data["client_secret"],
+	}
+	if diff := deep.Equal(expected, resp.Data); diff != nil {
+		t.Fatal(diff)
+	}
+}
+
 // TestOIDC_Path_OIDC_ProviderClient_Update tests Update operations for clients
 func TestOIDC_Path_OIDC_ProviderClient_Update(t *testing.T) {
 	c, _, _ := TestCoreUnsealed(t)
@@ -1395,6 +1454,54 @@ func TestOIDC_Path_OIDCProvider_DuplicateTemplateKeys(t *testing.T) {
 	expectSuccess(t, resp, err)
 }
 
+// TestOIDC_Path_OIDCProvider_DeDuplication tests that a
+// provider doensn't have duplicate scopes or client IDs
+func TestOIDC_Path_OIDCProvider_Deduplication(t *testing.T) {
+	c, _, _ := TestCoreUnsealed(t)
+	ctx := namespace.RootContext(nil)
+	storage := &logical.InmemStorage{}
+
+	// Create a test scope "test-scope1" -- should succeed
+	resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/scope/test-scope1",
+		Operation: logical.CreateOperation,
+		Data: map[string]interface{}{
+			"template":    `{"groups": {{identity.entity.groups.names}} }`,
+			"description": "desc1",
+		},
+		Storage: storage,
+	})
+	expectSuccess(t, resp, err)
+
+	// Create a test provider "test-provider" with duplicates
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/provider/test-provider",
+		Operation: logical.CreateOperation,
+		Data: map[string]interface{}{
+			"scopes":             []string{"test-scope1", "test-scope1"},
+			"allowed_client_ids": []string{"test-id1", "test-id2", "test-id1"},
+		},
+		Storage: storage,
+	})
+	expectSuccess(t, resp, err)
+
+	// Read "test-provider" again and validate
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "oidc/provider/test-provider",
+		Operation: logical.ReadOperation,
+		Storage:   storage,
+	})
+	expectSuccess(t, resp, err)
+	expected := map[string]interface{}{
+		"issuer":             "",
+		"allowed_client_ids": []string{"test-id1", "test-id2"},
+		"scopes":             []string{"test-scope1"},
+	}
+	if diff := deep.Equal(expected, resp.Data); diff != nil {
+		t.Fatal(diff)
+	}
+}
+
 // TestOIDC_Path_OIDCProvider_Update tests Update operations for providers
 func TestOIDC_Path_OIDCProvider_Update(t *testing.T) {
 	c, _, _ := TestCoreUnsealed(t)