Skip to content

Commit

Permalink
8224 report required permissions when authorization fails (#8314)
Browse files Browse the repository at this point in the history 
In case of unauthorized request, an error including denied/missing permissions will be raised
  • Loading branch information
@ItamarYuran
ItamarYuran authored Nov 12, 2024
1 parent 65ad3e5 commit 3d0db30
Show file tree
Hide file tree
Showing 4 changed files with 188 additions and 16 deletions.
6 changes: 3 additions & 3 deletions contrib/auth/acl/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -895,13 +895,13 @@ 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)
permAudit := &auth.MissingPermissions{}
allowed := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, permAudit)

if allowed != auth.CheckAllow {
return &auth.AuthorizationResponse{
Allowed: false,
Error: auth.ErrInsufficientPermissions,
Error: fmt.Errorf("%w: %s", auth.ErrInsufficientPermissions, permAudit.String()),
}, nil
}

Expand Down
9 changes: 5 additions & 4 deletions esti/auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"net/http"
"slices"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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)
}
})
Expand All @@ -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)
}
})
Expand Down
148 changes: 148 additions & 0 deletions pkg/api/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()
Expand Down
41 changes: 32 additions & 9 deletions pkg/auth/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down

0 comments on commit 3d0db30

Please sign in to comment.