review changes

Signed-off-by: Mauritz Uphoff <mauritz.uphoff@stackit.cloud>

Author: h3adex

stackit/internal/services/authorization/authorization_acc_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"maps"
+	"regexp"
 	"strings"
 	"testing"
 
@@ -25,11 +26,20 @@ var (
 	//go:embed testdata/resource-project-role-assignment.tf
 	resourceProjectRoleAssignment string
 
+	//go:embed testdata/resource-project-role-assignment-duplicate.tf
+	resourceProjectRoleAssignmentDuplicate string
+
 	//go:embed testdata/resource-folder-role-assignment.tf
 	resourceFolderRoleAssignment string
 
+	//go:embed testdata/resource-folder-role-assignment-duplicate.tf
+	resourceFolderRoleAssignmentDuplicate string
+
 	//go:embed testdata/resource-org-role-assignment.tf
 	resourceOrgRoleAssignment string
+
+	//go:embed testdata/resource-org-role-assignment-duplicate.tf
+	resourceOrgRoleAssignmentDuplicate string
 )
 
 var testProjectName = fmt.Sprintf("proj-%s", acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum))
@@ -155,6 +165,13 @@ func TestAccProjectRoleAssignmentResource(t *testing.T) {
 					resource.TestCheckResourceAttr("stackit_authorization_project_role_assignment.pra", "subject", testutil.ConvertConfigVariable(testConfigVarsProjectRoleAssignmentUpdated()["subject"])),
 				),
 			},
+			// Duplicate assignment should fail
+			{
+				ConfigVariables: testConfigVarsProjectRoleAssignmentUpdated(),
+				Config:          testutil.AuthorizationProviderConfig() + "\n" + resourceProjectRoleAssignmentDuplicate,
+				ExpectError:     regexp.MustCompile(`Error while checking for duplicate role assignments`),
+			},
+
 			// Deletion is done by the framework implicitly
 		},
 	})
@@ -234,6 +251,12 @@ func TestAccFolderRoleAssignmentResource(t *testing.T) {
 					resource.TestCheckResourceAttr("stackit_authorization_folder_role_assignment.fra", "subject", testutil.ConvertConfigVariable(testConfigVarsFolderRoleAssignmentUpdated()["subject"])),
 				),
 			},
+			// Duplicate assignment should fail
+			{
+				ConfigVariables: testConfigVarsFolderRoleAssignmentUpdated(),
+				Config:          testutil.AuthorizationProviderConfig() + "\n" + resourceFolderRoleAssignmentDuplicate,
+				ExpectError:     regexp.MustCompile(`Error while checking for duplicate role assignments`),
+			},
 			// Deletion is done by the framework implicitly
 		},
 	})
@@ -295,6 +318,12 @@ func TestAccOrgRoleAssignmentResource(t *testing.T) {
 					resource.TestCheckResourceAttr("stackit_authorization_organization_role_assignment.ora", "subject", testutil.ConvertConfigVariable(testConfigVarsOrgRoleAssignmentUpdated()["subject"])),
 				),
 			},
+			// Duplicate assignment should fail
+			{
+				ConfigVariables: testConfigVarsOrgRoleAssignmentUpdated(),
+				Config:          testutil.AuthorizationProviderConfig() + "\n" + resourceOrgRoleAssignmentDuplicate,
+				ExpectError:     regexp.MustCompile(`Error while checking for duplicate role assignments`),
+			},
 			// Deletion is done by the framework implicitly
 		},
 	})

stackit/internal/services/authorization/roleassignments/resource.go

@@ -36,7 +36,8 @@ var (
 	_ resource.ResourceWithConfigure   = &roleAssignmentResource{}
 	_ resource.ResourceWithImportState = &roleAssignmentResource{}
 
-	errRoleAssignmentNotFound = errors.New("response members did not contain expected role assignment")
+	errRoleAssignmentNotFound       = errors.New("response members did not contain expected role assignment")
+	errRoleAssignmentDuplicateFound = errors.New("found a duplicate role assignment")
 )
 
 type Model struct {
@@ -153,6 +154,17 @@ func (r *roleAssignmentResource) Create(ctx context.Context, req resource.Create
 	ctx = tflog.SetField(ctx, "role", model.Role.ValueString())
 	ctx = tflog.SetField(ctx, "resource_type", r.apiName)
 
+	listMemberResp, err := r.authorizationClient.ListMembers(ctx, r.apiName, model.ResourceId.ValueString()).Subject(model.Subject.ValueString()).Execute()
+	if err != nil {
+		core.LogAndAddError(ctx, &resp.Diagnostics, "Error listing current resource members", fmt.Sprintf("Calling API: %v", err))
+		return
+	}
+
+	if err := checkDuplicate(model, listMemberResp); err != nil {
+		core.LogAndAddError(ctx, &resp.Diagnostics, "Error while checking for duplicate role assignments", err.Error())
+		return
+	}
+
 	// Create new project role assignment
 	payload, err := toCreatePayload(&model, &r.apiName)
 	if err != nil {
@@ -338,3 +350,18 @@ func mapListMembersResponse(resp *authorization.ListMembersResponse, model *Mode
 
 	return errRoleAssignmentNotFound
 }
+
+// Prevent creating duplicate <resource_id, role, subject> assignments.
+// Duplicates lead to inconsistent state when one is removed (API removes the role entirely, Terraform plan then fails).
+func checkDuplicate(model Model, listMemberResp *authorization.ListMembersResponse) error { //nolint:gocritic // A read only copy is required since an api response is parsed into the model and this check should not affect the model parameter
+	err := mapListMembersResponse(listMemberResp, &model)
+
+	if err != nil {
+		if errors.Is(err, errRoleAssignmentNotFound) {
+			return nil
+		}
+		return err
+	}
+
+	return errRoleAssignmentDuplicateFound
+}

stackit/internal/services/authorization/roleassignments/resource_test.go

@@ -1,6 +1,7 @@
 package roleassignments
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -210,9 +211,7 @@ func TestMapListMembersResponse(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			model := tt.inputModel // copy pointer to avoid overriding test data
-
-			err := mapListMembersResponse(tt.resp, model)
+			err := mapListMembersResponse(tt.resp, tt.inputModel)
 
 			if tt.expectError && err == nil {
 				t.Fatalf("Expected error but got none")
@@ -222,10 +221,138 @@ func TestMapListMembersResponse(t *testing.T) {
 			}
 
 			if !tt.expectError {
-				if diff := cmp.Diff(tt.expected, model); diff != "" {
+				if diff := cmp.Diff(tt.expected, tt.inputModel); diff != "" {
 					t.Errorf("Mapped model mismatch (-want +got):\n%s", diff)
 				}
 			}
 		})
 	}
 }
+
+func TestCheckDuplicate(t *testing.T) {
+	role := "editor"
+	subject := "foo.bar@stackit.cloud"
+	resourceID := "project"
+
+	tests := []struct {
+		name        string
+		resp        *authorization.ListMembersResponse
+		inputModel  Model
+		expectError bool
+		expectedErr error
+	}{
+		{
+			name: "no members => no duplicate",
+			resp: &authorization.ListMembersResponse{
+				ResourceId: &resourceID,
+				Members:    &[]authorization.Member{},
+			},
+			inputModel: Model{
+				ResourceId: types.StringValue(resourceID),
+				Role:       types.StringValue(role),
+				Subject:    types.StringValue(subject),
+			},
+			expectError: false,
+		},
+		{
+			name: "members but no matching role/subject => no duplicate",
+			resp: &authorization.ListMembersResponse{
+				ResourceId: &resourceID,
+				Members: &[]authorization.Member{
+					{
+						Role:    utils.Ptr("reader"),
+						Subject: utils.Ptr("foo.bar@stackit.cloud"),
+					},
+					{
+						Role:    utils.Ptr("editor"),
+						Subject: utils.Ptr("someoneelse@stackit.cloud"),
+					},
+				},
+			},
+			inputModel: Model{
+				ResourceId: types.StringValue(resourceID),
+				Role:       types.StringValue(role),
+				Subject:    types.StringValue(subject),
+			},
+			expectError: false,
+		},
+		{
+			name: "matching role/subject exists => duplicate error",
+			resp: &authorization.ListMembersResponse{
+				ResourceId: &resourceID,
+				Members: &[]authorization.Member{
+					{
+						Role:    &role,
+						Subject: &subject,
+					},
+				},
+			},
+			inputModel: Model{
+				ResourceId: types.StringValue(resourceID),
+				Role:       types.StringValue(role),
+				Subject:    types.StringValue(subject),
+			},
+			expectError: true,
+			expectedErr: errRoleAssignmentDuplicateFound,
+		},
+		{
+			name: "nil response input => propagated error",
+			resp: nil,
+			inputModel: Model{
+				ResourceId: types.StringValue(resourceID),
+				Role:       types.StringValue(role),
+				Subject:    types.StringValue(subject),
+			},
+			expectError: true,
+			// we don't compare by value here, just expect some error
+		},
+		{
+			name: "nil members input => propagated error",
+			resp: &authorization.ListMembersResponse{
+				ResourceId: &resourceID,
+				Members:    nil,
+			},
+			inputModel: Model{
+				ResourceId: types.StringValue(resourceID),
+				Role:       types.StringValue(role),
+				Subject:    types.StringValue(subject),
+			},
+			expectError: true,
+		},
+		{
+			name: "nil resource_id input => propagated error",
+			resp: &authorization.ListMembersResponse{
+				ResourceId: nil,
+				Members: &[]authorization.Member{
+					{
+						Role:    &role,
+						Subject: &subject,
+					},
+				},
+			},
+			inputModel: Model{
+				ResourceId: types.StringValue(resourceID),
+				Role:       types.StringValue(role),
+				Subject:    types.StringValue(subject),
+			},
+			expectError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := checkDuplicate(tt.inputModel, tt.resp)
+
+			if tt.expectError && err == nil {
+				t.Fatalf("Expected error but got none")
+			}
+			if !tt.expectError && err != nil {
+				t.Fatalf("Expected no error, but got: %v", err)
+			}
+
+			if tt.expectError && tt.expectedErr != nil && !errors.Is(err, tt.expectedErr) {
+				t.Fatalf("Expected error %v, but got: %v", tt.expectedErr, err)
+			}
+		})
+	}
+}

stackit/internal/services/authorization/testdata/resource-folder-role-assignment-duplicate.tf

@@ -0,0 +1,24 @@
+variable "name" {}
+variable "role" {}
+variable "owner_email" {}
+variable "subject" {}
+variable "parent_container_id" {}
+
+resource "stackit_resourcemanager_folder" "folder" {
+  name                = var.name
+  owner_email         = var.owner_email
+  parent_container_id = var.parent_container_id
+}
+
+resource "stackit_authorization_folder_role_assignment" "fra" {
+  resource_id = stackit_resourcemanager_folder.folder.folder_id
+  role        = var.role
+  subject     = var.subject
+}
+
+# Second assignment – duplicates stackit_authorization_folder_role_assignment.fra (same resource_id, role, subject)
+resource "stackit_authorization_folder_role_assignment" "fra2" {
+  resource_id = stackit_resourcemanager_folder.folder.folder_id
+  role        = var.role
+  subject     = var.subject
+}

stackit/internal/services/authorization/testdata/resource-folder-role-assignment.tf

@@ -13,5 +13,5 @@ resource "stackit_resourcemanager_folder" "folder" {
 resource "stackit_authorization_folder_role_assignment" "fra" {
   resource_id = stackit_resourcemanager_folder.folder.folder_id
   role        = var.role
-  subject     = var.owner_email
+  subject     = var.subject
 }

stackit/internal/services/authorization/testdata/resource-org-role-assignment-duplicate.tf

@@ -0,0 +1,9 @@
+variable "role" {}
+variable "subject" {}
+variable "parent_container_id" {}
+
+resource "stackit_authorization_organization_role_assignment" "ora" {
+  resource_id = var.parent_container_id
+  role        = var.role
+  subject     = var.subject
+}

stackit/internal/services/authorization/testdata/resource-project-role-assignment-duplicate.tf

@@ -0,0 +1,24 @@
+variable "name" {}
+variable "role" {}
+variable "owner_email" {}
+variable "subject" {}
+variable "parent_container_id" {}
+
+resource "stackit_resourcemanager_project" "project" {
+  name                = var.name
+  owner_email         = var.owner_email
+  parent_container_id = var.parent_container_id
+}
+
+resource "stackit_authorization_project_role_assignment" "pra" {
+  resource_id = stackit_resourcemanager_project.project.project_id
+  role        = var.role
+  subject     = var.subject
+}
+
+# Second assignment – duplicates stackit_authorization_project_role_assignment.pra (same resource_id, role, subject)
+resource "stackit_authorization_project_role_assignment" "pra2" {
+  resource_id = stackit_resourcemanager_project.project.project_id
+  role        = var.role
+  subject     = var.subject
+}

stackit/internal/services/authorization/testdata/resource-project-role-assignment.tf

@@ -13,5 +13,5 @@ resource "stackit_resourcemanager_project" "project" {
 resource "stackit_authorization_project_role_assignment" "pra" {
   resource_id = stackit_resourcemanager_project.project.project_id
   role        = var.role
-  subject     = var.owner_email
+  subject     = var.subject
 }