From 515f817c7ab34f7edf07abf96c54d223ec4cf2f3 Mon Sep 17 00:00:00 2001 From: Christian Roggia Date: Sat, 18 Sep 2021 13:08:37 +0200 Subject: [PATCH] feat: return NotFound if resource doesn't exist --- pkg/services/authorize.go | 26 ++-- pkg/services/authorize_integration_test.go | 137 ++++++++++----------- pkg/services/data/authorize.query.dql | 4 + 3 files changed, 86 insertions(+), 81 deletions(-) diff --git a/pkg/services/authorize.go b/pkg/services/authorize.go index f522c14..e8a921d 100644 --- a/pkg/services/authorize.go +++ b/pkg/services/authorize.go @@ -68,20 +68,21 @@ func (s *AccessControlServerImpl) TestIamPolicy(ctx context.Context, req *grbac. } // Ask in parallel whether the user is allowed or allUsers is allowed. - var isAllowed, isAllUsersAllowed bool + var isAllowed, isAllUsersAllowed, isFound bool group, ctx := errgroup.WithContext(ctx) group.Go(func() error { - allowed, err := s.testIamPolicy(ctx, m) + allowed, _, err := s.testIamPolicy(ctx, m) isAllowed = allowed return err }) group.Go(func() error { - allowed, err := s.testIamPolicy(ctx, allUsers) + allowed, found, err := s.testIamPolicy(ctx, allUsers) isAllUsersAllowed = allowed + isFound = found return err }) @@ -94,26 +95,27 @@ func (s *AccessControlServerImpl) TestIamPolicy(ctx context.Context, req *grbac. return &empty.Empty{}, nil } + if !isFound { + return nil, status.New(codes.NotFound, "resource not found").Err() + } + return nil, status.New(codes.PermissionDenied, "permission denied").Err() } -func (s *AccessControlServerImpl) testIamPolicy(ctx context.Context, m map[string]string) (bool, error) { +func (s *AccessControlServerImpl) testIamPolicy(ctx context.Context, m map[string]string) (bool, bool, error) { resp, err := s.cli.NewReadOnlyTxn().QueryWithVars(ctx, queryAuthorize, m) if err != nil { - return false, err + return false, false, err } payload := new(struct { - Ok []json.RawMessage `json:"ok"` + Ok []json.RawMessage `json:"ok"` + Found []json.RawMessage `json:"found"` }) if err := json.Unmarshal(resp.Json, &payload); err != nil { - return false, err - } - - if len(payload.Ok) == 0 { - return false, nil + return false, false, err } - return true, nil + return len(payload.Ok) != 0, len(payload.Found) != 0, nil } diff --git a/pkg/services/authorize_integration_test.go b/pkg/services/authorize_integration_test.go index 37f475a..76bf5d8 100644 --- a/pkg/services/authorize_integration_test.go +++ b/pkg/services/authorize_integration_test.go @@ -234,95 +234,95 @@ func TestIntegrationAuthorize(t *testing.T) { object string subject string relation string - allowed bool + status codes.Code } for _, i := range []*T{ // Test: authorization rule on non-existing resource should return permission denied. - {ResourceNotFound.Name, User0.Name, PermissionGet.Name, false}, - {ResourceNotFound.Name, Anonymous, PermissionGet.Name, false}, + {ResourceNotFound.Name, User0.Name, PermissionGet.Name, codes.NotFound}, + {ResourceNotFound.Name, Anonymous, PermissionGet.Name, codes.NotFound}, // Test: authorization rule on non-existing permission should return permission denied. - {Resource0.Name, User0.Name, PermissionNotFound.Name, false}, - {Resource0.Name, Anonymous, PermissionNotFound.Name, false}, + {Resource0.Name, User0.Name, PermissionNotFound.Name, codes.PermissionDenied}, + {Resource0.Name, Anonymous, PermissionNotFound.Name, codes.PermissionDenied}, // Test: only members of group-0 should be granted "grbac.test.create" permission on resource-0. - {Resource0.Name, User0.Name, PermissionCreate.Name, true}, - {Resource0.Name, ServiceAccount0.Name, PermissionCreate.Name, true}, + {Resource0.Name, User0.Name, PermissionCreate.Name, codes.OK}, + {Resource0.Name, ServiceAccount0.Name, PermissionCreate.Name, codes.OK}, - {Resource0.Name, User1.Name, PermissionCreate.Name, false}, - {Resource0.Name, User2.Name, PermissionCreate.Name, false}, - {Resource0.Name, UserNotFound.Name, PermissionCreate.Name, false}, - {Resource0.Name, ServiceAccount1.Name, PermissionCreate.Name, false}, - {Resource0.Name, ServiceAccount2.Name, PermissionCreate.Name, false}, - {Resource0.Name, ServiceAccountNotFound.Name, PermissionCreate.Name, false}, - {Resource0.Name, Anonymous, PermissionCreate.Name, false}, + {Resource0.Name, User1.Name, PermissionCreate.Name, codes.PermissionDenied}, + {Resource0.Name, User2.Name, PermissionCreate.Name, codes.PermissionDenied}, + {Resource0.Name, UserNotFound.Name, PermissionCreate.Name, codes.PermissionDenied}, + {Resource0.Name, ServiceAccount1.Name, PermissionCreate.Name, codes.PermissionDenied}, + {Resource0.Name, ServiceAccount2.Name, PermissionCreate.Name, codes.PermissionDenied}, + {Resource0.Name, ServiceAccountNotFound.Name, PermissionCreate.Name, codes.PermissionDenied}, + {Resource0.Name, Anonymous, PermissionCreate.Name, codes.PermissionDenied}, // Test: only members of group-0 should be granted "grbac.test.get" permission on resource-0. - {Resource0.Name, User0.Name, PermissionGet.Name, true}, - {Resource0.Name, ServiceAccount0.Name, PermissionGet.Name, true}, + {Resource0.Name, User0.Name, PermissionGet.Name, codes.OK}, + {Resource0.Name, ServiceAccount0.Name, PermissionGet.Name, codes.OK}, - {Resource0.Name, User1.Name, PermissionGet.Name, false}, - {Resource0.Name, User2.Name, PermissionGet.Name, false}, - {Resource0.Name, UserNotFound.Name, PermissionGet.Name, false}, - {Resource0.Name, ServiceAccount1.Name, PermissionGet.Name, false}, - {Resource0.Name, ServiceAccount2.Name, PermissionGet.Name, false}, - {Resource0.Name, ServiceAccountNotFound.Name, PermissionGet.Name, false}, - {Resource0.Name, Anonymous, PermissionGet.Name, false}, + {Resource0.Name, User1.Name, PermissionGet.Name, codes.PermissionDenied}, + {Resource0.Name, User2.Name, PermissionGet.Name, codes.PermissionDenied}, + {Resource0.Name, UserNotFound.Name, PermissionGet.Name, codes.PermissionDenied}, + {Resource0.Name, ServiceAccount1.Name, PermissionGet.Name, codes.PermissionDenied}, + {Resource0.Name, ServiceAccount2.Name, PermissionGet.Name, codes.PermissionDenied}, + {Resource0.Name, ServiceAccountNotFound.Name, PermissionGet.Name, codes.PermissionDenied}, + {Resource0.Name, Anonymous, PermissionGet.Name, codes.PermissionDenied}, // Test: nobody should be granted "grbac.test.delete" permission on resource-0. - {Resource0.Name, User0.Name, PermissionDelete.Name, false}, - {Resource0.Name, User1.Name, PermissionDelete.Name, false}, - {Resource0.Name, User2.Name, PermissionDelete.Name, false}, - {Resource0.Name, UserNotFound.Name, PermissionDelete.Name, false}, - {Resource0.Name, ServiceAccount0.Name, PermissionDelete.Name, false}, - {Resource0.Name, ServiceAccount1.Name, PermissionDelete.Name, false}, - {Resource0.Name, ServiceAccount2.Name, PermissionDelete.Name, false}, - {Resource0.Name, ServiceAccountNotFound.Name, PermissionDelete.Name, false}, - {Resource0.Name, Anonymous, PermissionDelete.Name, false}, + {Resource0.Name, User0.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource0.Name, User1.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource0.Name, User2.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource0.Name, UserNotFound.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource0.Name, ServiceAccount0.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource0.Name, ServiceAccount1.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource0.Name, ServiceAccount2.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource0.Name, ServiceAccountNotFound.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource0.Name, Anonymous, PermissionDelete.Name, codes.PermissionDenied}, // Test: all users should be granted "grbac.test.get" permission on resource-1. - {Resource1.Name, User0.Name, PermissionGet.Name, true}, - {Resource1.Name, User1.Name, PermissionGet.Name, true}, - {Resource1.Name, User2.Name, PermissionGet.Name, true}, - {Resource1.Name, UserNotFound.Name, PermissionGet.Name, true}, - {Resource1.Name, ServiceAccount0.Name, PermissionGet.Name, true}, - {Resource1.Name, ServiceAccount1.Name, PermissionGet.Name, true}, - {Resource1.Name, ServiceAccount2.Name, PermissionGet.Name, true}, - {Resource1.Name, ServiceAccountNotFound.Name, PermissionGet.Name, true}, - {Resource1.Name, Anonymous, PermissionGet.Name, true}, + {Resource1.Name, User0.Name, PermissionGet.Name, codes.OK}, + {Resource1.Name, User1.Name, PermissionGet.Name, codes.OK}, + {Resource1.Name, User2.Name, PermissionGet.Name, codes.OK}, + {Resource1.Name, UserNotFound.Name, PermissionGet.Name, codes.OK}, + {Resource1.Name, ServiceAccount0.Name, PermissionGet.Name, codes.OK}, + {Resource1.Name, ServiceAccount1.Name, PermissionGet.Name, codes.OK}, + {Resource1.Name, ServiceAccount2.Name, PermissionGet.Name, codes.OK}, + {Resource1.Name, ServiceAccountNotFound.Name, PermissionGet.Name, codes.OK}, + {Resource1.Name, Anonymous, PermissionGet.Name, codes.OK}, // Test: only members of group-0 should be granted "grbac.test.delete" permission on resource-1. - {Resource1.Name, User0.Name, PermissionDelete.Name, true}, - {Resource1.Name, ServiceAccount0.Name, PermissionDelete.Name, true}, + {Resource1.Name, User0.Name, PermissionDelete.Name, codes.OK}, + {Resource1.Name, ServiceAccount0.Name, PermissionDelete.Name, codes.OK}, - {Resource1.Name, User1.Name, PermissionDelete.Name, false}, - {Resource1.Name, User2.Name, PermissionDelete.Name, false}, - {Resource1.Name, UserNotFound.Name, PermissionDelete.Name, false}, - {Resource1.Name, ServiceAccount1.Name, PermissionDelete.Name, false}, - {Resource1.Name, ServiceAccount2.Name, PermissionDelete.Name, false}, - {Resource1.Name, ServiceAccountNotFound.Name, PermissionDelete.Name, false}, - {Resource1.Name, Anonymous, PermissionDelete.Name, false}, + {Resource1.Name, User1.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource1.Name, User2.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource1.Name, UserNotFound.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource1.Name, ServiceAccount1.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource1.Name, ServiceAccount2.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource1.Name, ServiceAccountNotFound.Name, PermissionDelete.Name, codes.PermissionDenied}, + {Resource1.Name, Anonymous, PermissionDelete.Name, codes.PermissionDenied}, // Test: only members of group-0 (inherited) and group-1 should be granted "grbac.test.create" permission on resource-1. - {Resource1.Name, User0.Name, PermissionCreate.Name, true}, - {Resource1.Name, User1.Name, PermissionCreate.Name, true}, - {Resource1.Name, ServiceAccount0.Name, PermissionCreate.Name, true}, - {Resource1.Name, ServiceAccount1.Name, PermissionCreate.Name, true}, + {Resource1.Name, User0.Name, PermissionCreate.Name, codes.OK}, + {Resource1.Name, User1.Name, PermissionCreate.Name, codes.OK}, + {Resource1.Name, ServiceAccount0.Name, PermissionCreate.Name, codes.OK}, + {Resource1.Name, ServiceAccount1.Name, PermissionCreate.Name, codes.OK}, - {Resource1.Name, User2.Name, PermissionCreate.Name, false}, - {Resource1.Name, ServiceAccount2.Name, PermissionCreate.Name, false}, - {Resource1.Name, Anonymous, PermissionCreate.Name, false}, + {Resource1.Name, User2.Name, PermissionCreate.Name, codes.PermissionDenied}, + {Resource1.Name, ServiceAccount2.Name, PermissionCreate.Name, codes.PermissionDenied}, + {Resource1.Name, Anonymous, PermissionCreate.Name, codes.PermissionDenied}, // Test: only members of group-0 and group-1 should be granted "grbac.test.get" permission on resource-2. - {Resource2.Name, User0.Name, PermissionGet.Name, true}, - {Resource2.Name, User1.Name, PermissionGet.Name, true}, - {Resource2.Name, ServiceAccount0.Name, PermissionGet.Name, true}, - {Resource2.Name, ServiceAccount1.Name, PermissionGet.Name, true}, - - {Resource2.Name, User2.Name, PermissionGet.Name, false}, - {Resource2.Name, ServiceAccount2.Name, PermissionGet.Name, false}, - {Resource2.Name, Anonymous, PermissionGet.Name, false}, + {Resource2.Name, User0.Name, PermissionGet.Name, codes.OK}, + {Resource2.Name, User1.Name, PermissionGet.Name, codes.OK}, + {Resource2.Name, ServiceAccount0.Name, PermissionGet.Name, codes.OK}, + {Resource2.Name, ServiceAccount1.Name, PermissionGet.Name, codes.OK}, + + {Resource2.Name, User2.Name, PermissionGet.Name, codes.PermissionDenied}, + {Resource2.Name, ServiceAccount2.Name, PermissionGet.Name, codes.PermissionDenied}, + {Resource2.Name, Anonymous, PermissionGet.Name, codes.PermissionDenied}, } { subject := i.subject if isUser(i.subject) { @@ -338,13 +338,12 @@ func TestIntegrationAuthorize(t *testing.T) { }, }) - if i.allowed { + if err == nil { + assert.Equal(t, i.status, codes.OK) assert.NoError(t, err, "[%s:%s:%s]", i.object, i.relation, i.subject) } else { + assert.Equal(t, i.status, status.Code(err)) assert.Error(t, err, "[%s:%s:%s]", i.object, i.relation, i.subject) - if err != nil { - assert.Equal(t, codes.PermissionDenied, status.Code(err), "[%s:%s:%s]", i.object, i.relation, i.subject) - } } } } diff --git a/pkg/services/data/authorize.query.dql b/pkg/services/data/authorize.query.dql index 57c5a6f..2184ff8 100644 --- a/pkg/services/data/authorize.query.dql +++ b/pkg/services/data/authorize.query.dql @@ -14,4 +14,8 @@ query queryAuthorize($principal: string, $resource: string, $permission: string) ok(func: uid(path), first:1) { uid } + + found(func:uid(object), first: 1) { + uid + } } \ No newline at end of file