Fix segmentation fault when unassigning role assignments (#213)

client.go

@@ -13,8 +13,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"
-	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2"
 	"github.com/google/uuid"
 	"github.com/hashicorp/go-multierror"
@@ -214,11 +214,10 @@ func (c *client) unassignRoles(ctx context.Context, roleIDs []string) error {
 	var merr *multierror.Error
 
 	for _, id := range roleIDs {
-		var rawResponse *http.Response
-		ctxWithResp := policy.WithCaptureResponse(ctx, &rawResponse)
-		if _, err := c.provider.DeleteRoleAssignmentByID(ctxWithResp, id); err != nil {
-			// If a role was deleted manually then Azure returns a error and status 204
-			if rawResponse != nil && rawResponse.StatusCode == http.StatusNoContent || rawResponse.StatusCode == http.StatusNotFound {
+		if _, err := c.provider.DeleteRoleAssignmentByID(ctx, id); err != nil {
+			// If a role was deleted out-of-band then Azure returns an error and status 204
+			respErr := new(azcore.ResponseError)
+			if errors.As(err, &respErr) && (respErr.StatusCode == http.StatusNoContent || respErr.StatusCode == http.StatusNotFound) {
 				continue
 			}
 

path_service_principal_test.go

@@ -888,6 +888,64 @@ func TestRoleAssignmentWALRollback(t *testing.T) {
 	})
 }
 
+// TestUnassignRoleFailures ensures that the plugin can
+// cleanly resolve any errors received when unassigning roles
+func TestUnassignRoleFailures(t *testing.T) {
+	b, s := getTestBackendMocked(t, true)
+
+	mp := newMockProvider()
+	mp.(*mockProvider).failUnassignRoles = true
+	mp.(*mockProvider).unassignRolesFailureParams = failureParams{
+		expectError: true,
+	}
+	b.getProvider = func(context.Context, hclog.Logger, logical.SystemView, *clientSettings) (AzureProvider, error) {
+		return mp, nil
+	}
+
+	c, err := b.getClient(context.Background(), s)
+	if err != nil {
+		t.Fatalf("error getting client: %s", err.Error())
+	}
+
+	// verify error is received
+	t.Run("Role unassign fail", func(t *testing.T) {
+		testAssignmentIDs := []string{"test-1", "test-2"}
+		// Remove one of the RA IDs to simulate a failure to assign a role
+		if err := c.unassignRoles(context.Background(), testAssignmentIDs); err == nil {
+			t.Fatalf("expected error but got nil")
+		}
+	})
+
+	mp.(*mockProvider).unassignRolesFailureParams = failureParams{
+		expectError: false,
+		statusCode:  204,
+	}
+
+	// verify the error is properly handled and ignored in 204 case
+	t.Run("Role unassign error handled for 204", func(t *testing.T) {
+		testAssignmentIDs := []string{"test-1", "test-2"}
+		// Remove one of the RA IDs to simulate a failure to assign a role
+		if err := c.unassignRoles(context.Background(), testAssignmentIDs); err != nil {
+			t.Fatalf("error unassigning Role: %s", err.Error())
+		}
+	})
+
+	mp.(*mockProvider).unassignRolesFailureParams = failureParams{
+		expectError: false,
+		statusCode:  404,
+	}
+
+	// verify the error is properly handled and ignored in 404 case
+	t.Run("Role unassign error handled for 404", func(t *testing.T) {
+		testAssignmentIDs := []string{"test-1", "test-2"}
+		// Remove one of the RA IDs to simulate a failure to assign a role
+		if err := c.unassignRoles(context.Background(), testAssignmentIDs); err != nil {
+			t.Fatalf("error unassigning Role: %s", err.Error())
+		}
+	})
+
+}
+
 // This is an integration test against the live Azure service. It requires
 // valid, sufficiently-privileged Azure credentials in env variables.
 func TestCredentialInteg_msgraph(t *testing.T) {

provider_mock_test.go

@@ -12,6 +12,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2"
 	"github.com/google/uuid"
 
@@ -20,13 +21,20 @@ import (
 
 // mockProvider is a Provider that provides stubs and simple, deterministic responses.
 type mockProvider struct {
-	applications              map[string]string
-	servicePrincipals         map[string]bool
-	deletedObjects            map[string]bool
-	passwords                 map[string]string
-	failNextCreateApplication bool
-	ctxTimeout                time.Duration
-	lock                      sync.Mutex
+	applications               map[string]string
+	servicePrincipals          map[string]bool
+	deletedObjects             map[string]bool
+	passwords                  map[string]string
+	failNextCreateApplication  bool
+	failUnassignRoles          bool
+	unassignRolesFailureParams failureParams
+	ctxTimeout                 time.Duration
+	lock                       sync.Mutex
+}
+
+type failureParams struct {
+	statusCode  int
+	expectError bool
 }
 
 func newMockProvider() AzureProvider {
@@ -224,6 +232,21 @@ func (m *mockProvider) CreateRoleAssignment(_ context.Context, scope string, nam
 }
 
 func (m *mockProvider) DeleteRoleAssignmentByID(_ context.Context, _ string) (armauthorization.RoleAssignmentsClientDeleteByIDResponse, error) {
+	if m.failUnassignRoles {
+		if m.unassignRolesFailureParams.expectError {
+			// return empty response and no 200 status codes to throw error
+			return armauthorization.RoleAssignmentsClientDeleteByIDResponse{}, &azcore.ResponseError{
+				ErrorCode: "mock: fail to delete role assignment",
+			}
+		}
+
+		// return empty response and with status code; will ignore error and assume role
+		// assignment was manually deleted based on status code
+		return armauthorization.RoleAssignmentsClientDeleteByIDResponse{}, &azcore.ResponseError{
+			StatusCode: m.unassignRolesFailureParams.statusCode,
+			ErrorCode:  "mock: fail to delete role assignment",
+		}
+	}
 	return armauthorization.RoleAssignmentsClientDeleteByIDResponse{}, nil
 }