8224 report required permissions when authorization fails

contrib/auth/acl/service.go

@@ -895,8 +895,8 @@ func (s *AuthService) Authorize(ctx context.Context, req *auth.AuthorizationRequ
 	if err != nil {
 		return nil, err
 	}
-
-	allowed := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies)
+	perm := &auth.MissingPermissions{}
+	allowed := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm)
 
 	if allowed != auth.CheckAllow {
 		return &auth.AuthorizationResponse{

esti/auth_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"net/http"
 	"slices"
+	"strings"
 	"testing"
 	"time"
 
@@ -234,7 +235,7 @@ func TestCreateRepo_Unauthorized(t *testing.T) {
 	})
 	require.NoError(t, err)
 	require.NotNil(t, resp.JSON401)
-	if resp.JSON401.Message != auth.ErrInsufficientPermissions.Error() {
+	if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) {
 		t.Fatalf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message)
 	}
 }
@@ -255,15 +256,15 @@ func TestRepoMetadata_Unauthorized(t *testing.T) {
 		})
 		require.NoError(t, err)
 		require.NotNil(t, resp.JSON401)
-		if resp.JSON401.Message != auth.ErrInsufficientPermissions.Error() {
+		if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) {
 			t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message)
 		}
 	})
 	t.Run("delete", func(t *testing.T) {
 		resp, err := clt.DeleteRepositoryMetadataWithResponse(ctx, repo, apigen.DeleteRepositoryMetadataJSONRequestBody{Keys: []string{"foo"}})
 		require.NoError(t, err)
 		require.NotNil(t, resp.JSON401)
-		if resp.JSON401.Message != auth.ErrInsufficientPermissions.Error() {
+		if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) {
 			t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message)
 		}
 	})
@@ -272,7 +273,7 @@ func TestRepoMetadata_Unauthorized(t *testing.T) {
 		resp, err := clt.GetRepositoryMetadataWithResponse(ctx, repo)
 		require.NoError(t, err)
 		require.NotNil(t, resp.JSON401)
-		if resp.JSON401.Message != auth.ErrInsufficientPermissions.Error() {
+		if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) {
 			t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message)
 		}
 	})

pkg/api/controller_test.go

@@ -34,11 +34,13 @@ import (
 	"github.com/treeverse/lakefs/pkg/api/apigen"
 	"github.com/treeverse/lakefs/pkg/api/apiutil"
 	"github.com/treeverse/lakefs/pkg/auth"
+	"github.com/treeverse/lakefs/pkg/auth/model"
 	"github.com/treeverse/lakefs/pkg/block"
 	"github.com/treeverse/lakefs/pkg/catalog"
 	"github.com/treeverse/lakefs/pkg/config"
 	"github.com/treeverse/lakefs/pkg/graveler"
 	"github.com/treeverse/lakefs/pkg/httputil"
+	"github.com/treeverse/lakefs/pkg/permissions"
 	"github.com/treeverse/lakefs/pkg/stats"
 	"github.com/treeverse/lakefs/pkg/testutil"
 	"github.com/treeverse/lakefs/pkg/upload"
@@ -5355,6 +5357,152 @@ func TestController_CreateCommitRecord(t *testing.T) {
 	})
 }
 
+func TestCheckPermissions_UnpermittedRequests(t *testing.T) {
+	ctx := context.Background()
+	testCases := []struct {
+		name     string
+		node     permissions.Node
+		username string
+		policies []*model.Policy
+		expected string
+	}{
+		{
+			name: "deny single action",
+			node: permissions.Node{
+				Type: permissions.NodeTypeNode,
+				Permission: permissions.Permission{
+					Action:   "fs:DeleteRepository",
+					Resource: "arn:lakefs:fs:::repository/repo1",
+				},
+			},
+			username: "user1",
+			policies: []*model.Policy{
+				{
+					Statement: []model.Statement{
+						{
+							Action:   []string{"fs:DeleteRepository"},
+							Resource: "arn:lakefs:fs:::repository/repo1",
+							Effect:   model.StatementEffectDeny,
+						},
+					},
+				},
+			},
+			expected: "denied permission to fs:DeleteRepository",
+		},
+		{
+			name: "deny multiple actions, one concerning the request",
+			node: permissions.Node{
+				Type: permissions.NodeTypeNode,
+				Permission: permissions.Permission{
+					Action:   "fs:DeleteRepository",
+					Resource: "arn:lakefs:fs:::repository/repo1",
+				},
+			},
+			username: "user1",
+			policies: []*model.Policy{
+				{
+					Statement: []model.Statement{
+						{
+							Action:   []string{"fs:DeleteRepository", "fs:CreateRepository"},
+							Resource: "arn:lakefs:fs:::repository/repo1",
+							Effect:   model.StatementEffectDeny,
+						},
+					},
+				},
+			},
+			expected: "denied permission to fs:DeleteRepository",
+		}, {
+			name: "neutral action",
+			node: permissions.Node{
+				Type: permissions.NodeTypeNode,
+				Permission: permissions.Permission{
+					Action:   "fs:ReadRepository",
+					Resource: "arn:lakefs:fs:::repository/repo1",
+				},
+			},
+			username: "user1",
+			policies: []*model.Policy{
+				{
+					Statement: []model.Statement{
+						{
+							Action:   []string{"fs:DeleteRepository"},
+							Resource: "arn:lakefs:fs:::repository/repo1",
+							Effect:   model.StatementEffectDeny,
+						},
+					},
+				},
+			},
+			expected: "not allowed to fs:ReadRepository",
+		}, {
+			name: "nodeAnd no policy, returns first missing one",
+			node: permissions.Node{
+				Type: permissions.NodeTypeAnd,
+				Nodes: []permissions.Node{
+					{
+						Type: permissions.NodeTypeNode,
+						Permission: permissions.Permission{
+							Action:   "fs:CreateRepository",
+							Resource: "*",
+						},
+					},
+					{
+						Type: permissions.NodeTypeNode,
+						Permission: permissions.Permission{
+							Action:   "fs:AttachStorageNamespace",
+							Resource: "*",
+						},
+					},
+				},
+			},
+			username: "user1",
+			expected: "not allowed to fs:CreateRepository",
+		}, {
+			name: "nodeAnd one policy, returns first missing policy",
+			node: permissions.Node{
+				Type: permissions.NodeTypeAnd,
+				Nodes: []permissions.Node{
+					{
+						Type: permissions.NodeTypeNode,
+						Permission: permissions.Permission{
+							Action:   "fs:CreateRepository",
+							Resource: "*",
+						},
+					},
+					{
+						Type: permissions.NodeTypeNode,
+						Permission: permissions.Permission{
+							Action:   "fs:AttachStorageNamespace",
+							Resource: "*",
+						},
+					},
+				},
+			},
+			username: "user1",
+			policies: []*model.Policy{
+				{
+					Statement: []model.Statement{
+						{
+							Action:   []string{"fs:CreateRepository"},
+							Resource: "*",
+							Effect:   model.StatementEffectAllow,
+						},
+					},
+				},
+			},
+			expected: "not allowed to fs:AttachStorageNamespace",
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			perm := &auth.MissingPermissions{}
+			result := auth.CheckPermissions(ctx, tc.node, tc.username, tc.policies, perm)
+			fmt.Println("expected:\n" + tc.expected)
+			fmt.Println("got:\n" + perm.String())
+			fmt.Println(result)
+			require.Equal(t, tc.expected, perm.String())
+		})
+	}
+}
 func TestController_CreatePullRequest(t *testing.T) {
 	clt, deps := setupClientWithAdmin(t)
 	ctx := context.Background()

pkg/auth/service.go

@@ -44,12 +44,20 @@ type AuthorizationResponse struct {
 	Error   error
 }
 
+type MissingPermissions struct {
+	// Denied is a list of actions the user was denied for the attempt.
+	Denied []string
+	// Unauthorized is a list of actions the user did not have for the attempt.
+	Unauthorized []string
+}
+
 // CheckResult - the final result for the authorization is accepted only if it's CheckAllow
 type CheckResult int
 
 const (
-	InvalidUserID = ""
-	MaxPage       = 1000
+	UserNotAllowed = "not allowed"
+	InvalidUserID  = ""
+	MaxPage        = 1000
 	// CheckAllow Permission allowed
 	CheckAllow CheckResult = iota
 	// CheckNeutral Permission neither allowed nor denied
@@ -918,13 +926,13 @@ func (a *APIAuthService) Authorize(ctx context.Context, req *AuthorizationReques
 	if err != nil {
 		return nil, err
 	}
-
-	allowed := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies)
+	permAudit := &MissingPermissions{}
+	allowed := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, permAudit)
 
 	if allowed != CheckAllow {
 		return &AuthorizationResponse{
 			Allowed: false,
-			Error:   ErrInsufficientPermissions,
+			Error:   fmt.Errorf("%w: %s", ErrInsufficientPermissions, permAudit.String()),
 		}, nil
 	}
 
@@ -1147,10 +1155,21 @@ func NewAPIAuthServiceWithClient(client ClientWithResponsesInterface, externalPr
 	}, nil
 }
 
-func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy) CheckResult {
+func (n *MissingPermissions) String() string {
+	if len(n.Denied) != 0 {
+		return fmt.Sprintf("denied permission to %s", strings.Join(n.Denied, ","))
+	}
+	if len(n.Unauthorized) != 0 {
+		return fmt.Sprintf("not allowed to %s", strings.Join(n.Unauthorized, ","))
+	}
+	return UserNotAllowed
+}
+
+func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, permAudit *MissingPermissions) CheckResult {
 	allowed := CheckNeutral
 	switch node.Type {
 	case permissions.NodeTypeNode:
+		hasPermission := false
 		// check whether the permission is allowed, denied or natural (not allowed and not denied)
 		for _, policy := range policies {
 			for _, stmt := range policy.Statement {
@@ -1165,21 +1184,25 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin
 
 					if stmt.Effect == model.StatementEffectDeny {
 						// this is a "Deny" and it takes precedence
+						permAudit.Denied = append(permAudit.Denied, action)
 						return CheckDeny
 					}
-
+					hasPermission = true
 					allowed = CheckAllow
 				}
 			}
 		}
+		if !hasPermission {
+			permAudit.Unauthorized = append(permAudit.Unauthorized, node.Permission.Action)
+		}
 
 	case permissions.NodeTypeOr:
 		// returns:
 		// Allowed - at least one of the permissions is allowed and no one is denied
 		// Denied - one of the permissions is Deny
 		// Natural - otherwise
 		for _, node := range node.Nodes {
-			result := CheckPermissions(ctx, node, username, policies)
+			result := CheckPermissions(ctx, node, username, policies, permAudit)
 			if result == CheckDeny {
 				return CheckDeny
 			}
@@ -1194,7 +1217,7 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin
 		// Denied - one of the permissions is Deny
 		// Natural - otherwise
 		for _, node := range node.Nodes {
-			result := CheckPermissions(ctx, node, username, policies)
+			result := CheckPermissions(ctx, node, username, policies, permAudit)
 			if result == CheckNeutral || result == CheckDeny {
 				return result
 			}