diff --git a/weed/admin/dash/policies_management.go b/weed/admin/dash/policies_management.go index 8f8d6dc27..8a736d499 100644 --- a/weed/admin/dash/policies_management.go +++ b/weed/admin/dash/policies_management.go @@ -2,7 +2,9 @@ package dash import ( "context" + "errors" "fmt" + "strings" "time" "github.com/seaweedfs/seaweedfs/weed/credential" @@ -10,6 +12,10 @@ import ( "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" ) +// ErrPolicyStillAttached is returned when deleting a managed policy that is +// still attached to one or more users or groups. +var ErrPolicyStillAttached = errors.New("policy is still attached") + type IAMPolicy struct { Name string `json:"name"` Document policy_engine.PolicyDocument `json:"document"` @@ -146,7 +152,10 @@ func (s *AdminServer) UpdatePolicy(name string, document policy_engine.PolicyDoc return policyManager.UpdatePolicy(ctx, name, document) } -// DeletePolicy deletes an IAM policy +// DeletePolicy deletes an IAM policy. Deletion is rejected while the policy is +// still attached to any user or group, matching AWS IAM behavior and the IAM +// API handler, so a deleted policy name never lingers in an attached policy +// list. func (s *AdminServer) DeletePolicy(name string) error { policyManager := s.GetPolicyManager() if policyManager == nil { @@ -154,9 +163,67 @@ func (s *AdminServer) DeletePolicy(name string) error { } ctx := context.Background() + attached, err := s.IsPolicyAttached(ctx, name) + if err != nil { + return fmt.Errorf("failed to check policy attachments: %w", err) + } + if len(attached) > 0 { + return fmt.Errorf("policy %s is still attached to: %s: %w", name, strings.Join(attached, ", "), ErrPolicyStillAttached) + } + return policyManager.DeletePolicy(ctx, name) } +// IsPolicyAttached returns the names of users and groups that still have the +// given managed policy attached. The returned entries are prefixed with +// "user:" or "group:". Returns nil when the policy is not attached anywhere. +func (s *AdminServer) IsPolicyAttached(ctx context.Context, policyName string) ([]string, error) { + if s.credentialManager == nil { + return nil, fmt.Errorf("credential manager not available") + } + + var attached []string + + usernames, err := s.credentialManager.ListUsers(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list users: %w", err) + } + for _, username := range usernames { + policies, err := s.credentialManager.ListAttachedUserPolicies(ctx, username) + if err != nil { + return nil, fmt.Errorf("failed to list policies for user %s: %w", username, err) + } + for _, p := range policies { + if p == policyName { + attached = append(attached, "user:"+username) + break + } + } + } + + groupNames, err := s.credentialManager.ListGroups(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list groups: %w", err) + } + for _, groupName := range groupNames { + group, err := s.credentialManager.GetGroup(ctx, groupName) + if errors.Is(err, credential.ErrGroupNotFound) { + continue + } + if err != nil { + return nil, fmt.Errorf("failed to get group %s: %w", groupName, err) + } + for _, p := range group.PolicyNames { + if p == policyName { + attached = append(attached, "group:"+groupName) + break + } + } + } + + return attached, nil +} + // GetPolicy retrieves a specific IAM policy func (s *AdminServer) GetPolicy(name string) (*IAMPolicy, error) { policyManager := s.GetPolicyManager() diff --git a/weed/admin/dash/policies_management_test.go b/weed/admin/dash/policies_management_test.go new file mode 100644 index 000000000..b92de340f --- /dev/null +++ b/weed/admin/dash/policies_management_test.go @@ -0,0 +1,160 @@ +package dash + +import ( + "context" + "errors" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/credential" + _ "github.com/seaweedfs/seaweedfs/weed/credential/memory" // register memory store + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" +) + +func newAdminServerWithMemoryStore(t *testing.T) *AdminServer { + t.Helper() + cm, err := credential.NewCredentialManagerWithDefaults(credential.StoreTypeMemory) + if err != nil { + t.Fatalf("failed to create credential manager: %v", err) + } + return &AdminServer{credentialManager: cm} +} + +func samplePolicyDocument() policy_engine.PolicyDocument { + return policy_engine.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy_engine.PolicyStatement{{ + Effect: policy_engine.PolicyEffectAllow, + Action: policy_engine.NewStringOrStringSlice("s3:GetObject"), + Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::test/*"), + }}, + } +} + +func TestIsPolicyAttached(t *testing.T) { + server := newAdminServerWithMemoryStore(t) + ctx := context.Background() + const policyName = "policy_a" + + if err := server.CreatePolicy(policyName, samplePolicyDocument()); err != nil { + t.Fatalf("CreatePolicy: %v", err) + } + + if attached, err := server.IsPolicyAttached(ctx, policyName); err != nil { + t.Fatalf("IsPolicyAttached: %v", err) + } else if len(attached) != 0 { + t.Fatalf("expected no attachments, got %v", attached) + } + + if err := server.credentialManager.CreateUser(ctx, &iam_pb.Identity{Name: "alice"}); err != nil { + t.Fatalf("CreateUser: %v", err) + } + if err := server.credentialManager.AttachUserPolicy(ctx, "alice", policyName); err != nil { + t.Fatalf("AttachUserPolicy: %v", err) + } + + attached, err := server.IsPolicyAttached(ctx, policyName) + if err != nil { + t.Fatalf("IsPolicyAttached: %v", err) + } + if len(attached) != 1 || attached[0] != "user:alice" { + t.Fatalf("expected [user:alice], got %v", attached) + } + + if err := server.credentialManager.CreateGroup(ctx, &iam_pb.Group{Name: "devs", PolicyNames: []string{policyName}}); err != nil { + t.Fatalf("CreateGroup: %v", err) + } + attached, err = server.IsPolicyAttached(ctx, policyName) + if err != nil { + t.Fatalf("IsPolicyAttached: %v", err) + } + if len(attached) != 2 { + t.Fatalf("expected 2 attachments, got %v", attached) + } +} + +func TestDeletePolicyRejectsWhenAttachedToUser(t *testing.T) { + server := newAdminServerWithMemoryStore(t) + ctx := context.Background() + const policyName = "policy_u" + + if err := server.CreatePolicy(policyName, samplePolicyDocument()); err != nil { + t.Fatalf("CreatePolicy: %v", err) + } + if err := server.credentialManager.CreateUser(ctx, &iam_pb.Identity{Name: "bob"}); err != nil { + t.Fatalf("CreateUser: %v", err) + } + if err := server.credentialManager.AttachUserPolicy(ctx, "bob", policyName); err != nil { + t.Fatalf("AttachUserPolicy: %v", err) + } + + if err := server.DeletePolicy(policyName); !errors.Is(err, ErrPolicyStillAttached) { + t.Fatalf("expected ErrPolicyStillAttached, got %v", err) + } + + if p, err := server.GetPolicy(policyName); err != nil { + t.Fatalf("policy should still exist after rejected deletion: %v", err) + } else if p == nil { + t.Fatal("policy should still exist after rejected deletion, got nil") + } + + attached, err := server.credentialManager.ListAttachedUserPolicies(ctx, "bob") + if err != nil { + t.Fatalf("ListAttachedUserPolicies: %v", err) + } + if len(attached) != 1 || attached[0] != policyName { + t.Fatalf("expected policy %q to remain attached, got %v", policyName, attached) + } + + if err := server.credentialManager.DetachUserPolicy(ctx, "bob", policyName); err != nil { + t.Fatalf("DetachUserPolicy: %v", err) + } + if err := server.DeletePolicy(policyName); err != nil { + t.Fatalf("DeletePolicy after detach failed: %v", err) + } + if p, err := server.GetPolicy(policyName); err != nil { + t.Fatalf("GetPolicy after detach-delete: %v", err) + } else if p != nil { + t.Fatalf("policy should be gone after deletion, got %v", p) + } +} + +func TestDeletePolicyRejectsWhenAttachedToGroup(t *testing.T) { + server := newAdminServerWithMemoryStore(t) + ctx := context.Background() + const policyName = "policy_g" + + if err := server.CreatePolicy(policyName, samplePolicyDocument()); err != nil { + t.Fatalf("CreatePolicy: %v", err) + } + if err := server.credentialManager.CreateGroup(ctx, &iam_pb.Group{Name: "team_g", PolicyNames: []string{policyName}}); err != nil { + t.Fatalf("CreateGroup: %v", err) + } + + if err := server.DeletePolicy(policyName); !errors.Is(err, ErrPolicyStillAttached) { + t.Fatalf("expected ErrPolicyStillAttached, got %v", err) + } + + if p, err := server.GetPolicy(policyName); err != nil { + t.Fatalf("policy should still exist after rejected deletion: %v", err) + } else if p == nil { + t.Fatal("policy should still exist after rejected deletion, got nil") + } +} + +func TestDeletePolicySucceedsWhenNotAttached(t *testing.T) { + server := newAdminServerWithMemoryStore(t) + const policyName = "policy_free" + + if err := server.CreatePolicy(policyName, samplePolicyDocument()); err != nil { + t.Fatalf("CreatePolicy: %v", err) + } + if err := server.DeletePolicy(policyName); err != nil { + t.Fatalf("DeletePolicy for unattached policy failed: %v", err) + } + if p, err := server.GetPolicy(policyName); err != nil { + t.Fatalf("GetPolicy after delete: %v", err) + } else if p != nil { + t.Fatalf("policy should be gone after deletion, got %v", p) + } +} diff --git a/weed/admin/handlers/policy_handlers.go b/weed/admin/handlers/policy_handlers.go index d9a6d786e..117145acb 100644 --- a/weed/admin/handlers/policy_handlers.go +++ b/weed/admin/handlers/policy_handlers.go @@ -1,6 +1,7 @@ package handlers import ( + "errors" "fmt" "net/http" "time" @@ -186,8 +187,13 @@ func (h *PolicyHandlers) DeletePolicy(w http.ResponseWriter, r *http.Request) { // Delete the policy err = h.adminServer.DeletePolicy(policyName) if err != nil { - glog.Errorf("Failed to delete policy %s: %v", policyName, err) - writeJSONError(w, http.StatusInternalServerError, "Failed to delete policy: "+err.Error()) + status := http.StatusInternalServerError + if errors.Is(err, dash.ErrPolicyStillAttached) { + status = http.StatusConflict + } else { + glog.Errorf("Failed to delete policy %s: %v", policyName, err) + } + writeJSONError(w, status, "Failed to delete policy: "+err.Error()) return }