Fix duplicate role definition check (#16)

The combination of (role_id + scope) must be unique, not role_id alone.
hashicorp · Oct 24, 2018 · 824f07d · 824f07d
1 parent 3dfddbe
commit 824f07d
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 12 deletions.
diff --git a/path_roles.go b/path_roles.go
@@ -166,7 +166,7 @@ func (b *azureSecretBackend) pathRoleUpdate(ctx context.Context, req *logical.Re
 		role.AzureRoles = parsedRoles
 	}
 
-	roleIDs := make(map[string]bool)
+	roleSet := make(map[string]bool)
 	for _, r := range role.AzureRoles {
 		var roleDef authorization.RoleDefinition
 		if r.RoleID != "" {
@@ -192,12 +192,14 @@ func (b *azureSecretBackend) pathRoleUpdate(ctx context.Context, req *logical.Re
 
 		roleDefID := to.String(roleDef.ID)
 		roleDefName := to.String(roleDef.RoleName)
-		if roleIDs[roleDefID] {
-			return logical.ErrorResponse(fmt.Sprintf("duplicate role_id: '%s'", *roleDef.ID)), nil
-		}
-		roleIDs[roleDefID] = true
 
 		r.RoleName, r.RoleID = roleDefName, roleDefID
+
+		rsKey := r.RoleID + "||" + r.Scope
+		if roleSet[rsKey] {
+			return logical.ErrorResponse(fmt.Sprintf("duplicate role_id and scope: '%s', '%s'", r.RoleID, r.Scope)), nil
+		}
+		roleSet[rsKey] = true
 	}
 
 	if role.ApplicationObjectID == "" && len(role.AzureRoles) == 0 {

diff --git a/path_roles_test.go b/path_roles_test.go
@@ -181,7 +181,7 @@ func TestRoleCreate(t *testing.T) {
 		})
 		nilErr(t, err)
 		if resp.IsError() {
-			t.Fatal("received unxpected error response")
+			t.Fatalf("received unexpected error response: %v", resp.Error())
 		}
 
 		resp, err = testRoleRead(t, b, s, name)
@@ -193,6 +193,64 @@ func TestRoleCreate(t *testing.T) {
 		equal(t, "/subscriptions/FAKE_SUB_ID/providers/Microsoft.Authorization/roleDefinitions/FAKE_ROLE-Contributor", roles[1].RoleID)
 	})
 
+	t.Run("Duplicate role name and scope", func(t *testing.T) {
+		b, s := getTestBackend(t, true)
+
+		var role = map[string]interface{}{
+			"azure_roles": compactJSON(`[
+				{
+					"role_name": "Owner",
+					"scope":  "test_scope_1"
+				},
+				{
+					"role_name": "Owner",
+					"scope":  "test_scope_1"
+				}
+			]`),
+		}
+
+		name := generateUUID()
+		resp, err := b.HandleRequest(context.Background(), &logical.Request{
+			Operation: logical.CreateOperation,
+			Path:      "roles/" + name,
+			Data:      role,
+			Storage:   s,
+		})
+		nilErr(t, err)
+		if !resp.IsError() {
+			t.Fatal("expected error response for duplicate role & scope")
+		}
+	})
+
+	t.Run("Duplicate role name, different scope", func(t *testing.T) {
+		b, s := getTestBackend(t, true)
+
+		var role = map[string]interface{}{
+			"azure_roles": compactJSON(`[
+				{
+					"role_name": "Owner",
+					"scope":  "test_scope_1"
+				},
+				{
+					"role_name": "Owner",
+					"scope":  "test_scope_2"
+				}
+			]`),
+		}
+
+		name := generateUUID()
+		resp, err := b.HandleRequest(context.Background(), &logical.Request{
+			Operation: logical.CreateOperation,
+			Path:      "roles/" + name,
+			Data:      role,
+			Storage:   s,
+		})
+		nilErr(t, err)
+		if resp.IsError() {
+			t.Fatalf("received unexpected error response: %v", resp.Error())
+		}
+	})
+
 	t.Run("Role name lookup (multiple match)", func(t *testing.T) {
 		b, s := getTestBackend(t, true)
 

diff --git a/path_service_principal_test.go b/path_service_principal_test.go
@@ -348,12 +348,20 @@ func TestCredentialInteg(t *testing.T) {
 		nilErr(t, err)
 
 		// Add a Vault role that will provide creds with Azure "Reader" permissions
+		// Resources groups "vault-azure-secrets-test1" and "vault-azure-secrets-test2"
+		// should already exist in the test infrastructure. (The test can be simplified
+		// to just use scope "/subscriptions/%s" if need be.)
 		rolename := "test_role"
 		role := map[string]interface{}{
-			"azure_roles": fmt.Sprintf(`[{
-			"role_name": "Reader",
-			"scope":  "/subscriptions/%s"
-		}]`, subscriptionID),
+			"azure_roles": fmt.Sprintf(`[
+			{
+				"role_name": "Reader",
+				"scope":  "/subscriptions/%s/resourceGroups/vault-azure-secrets-test1"
+			},
+			{
+				"role_name": "Reader",
+				"scope":  "/subscriptions/%s/resourceGroups/vault-azure-secrets-test2"
+			}]`, subscriptionID, subscriptionID),
 		}
 		resp, err := b.HandleRequest(context.Background(), &logical.Request{
 			Operation: logical.CreateOperation,
@@ -411,10 +419,10 @@ func TestCredentialInteg(t *testing.T) {
 			t.Fatalf("Expected nil error on GET of new SP, got: %#v", err)
 		}
 
-		// Verify that a role assignment was created. Get the assignment
+		// Verify that the role assignments were created. Get the assignment
 		// info from Azure and verify it matches the Reader role.
 		raIDs := resp.Secret.InternalData["role_assignment_ids"].([]string)
-		equal(t, 1, len(raIDs))
+		equal(t, 2, len(raIDs))
 
 		ra, err := provider.raClient.GetByID(context.Background(), raIDs[0])
 		nilErr(t, err)