Skip to content

Commit

Permalink
ensure permissions checks are fully consistent (#141)
Browse files Browse the repository at this point in the history
Signed-off-by: Mike Mason <[email protected]>
  • Loading branch information
mikemrm authored Jul 14, 2023
1 parent 972fec5 commit 211c29f
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 12 deletions.
2 changes: 1 addition & 1 deletion internal/api/permissions.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (r *Router) checkAction(c echo.Context) error {
}

// Check the permissions
err = r.engine.SubjectHasPermission(ctx, subjectResource, action, resource, "")
err = r.engine.SubjectHasPermission(ctx, subjectResource, action, resource)
if err != nil && errors.Is(err, query.ErrActionNotAssigned) {
msg := fmt.Sprintf("subject '%s' does not have permission to perform action '%s' on resource '%s'",
subject, action, resourceIDStr)
Expand Down
2 changes: 1 addition & 1 deletion internal/query/mock/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func (e *Engine) GetResourceType(name string) *types.ResourceType {
}

// SubjectHasPermission returns nil to satisfy the Engine interface.
func (e *Engine) SubjectHasPermission(ctx context.Context, subject types.Resource, action string, resource types.Resource, queryToken string) error {
func (e *Engine) SubjectHasPermission(ctx context.Context, subject types.Resource, action string, resource types.Resource) error {
e.Called()

return nil
Expand Down
15 changes: 8 additions & 7 deletions internal/query/relations.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,21 @@ func resourceToSpiceDBRef(namespace string, r types.Resource) *pb.ObjectReferenc
}

// SubjectHasPermission checks if the given subject can do the given action on the given resource
func (e *engine) SubjectHasPermission(ctx context.Context, subject types.Resource, action string, resource types.Resource, queryToken string) error {
func (e *engine) SubjectHasPermission(ctx context.Context, subject types.Resource, action string, resource types.Resource) error {
req := &pb.CheckPermissionRequest{
Consistency: &pb.Consistency{
Requirement: &pb.Consistency_FullyConsistent{
FullyConsistent: true,
},
},
Resource: resourceToSpiceDBRef(e.namespace, resource),
Permission: action,
Subject: &pb.SubjectReference{
Object: resourceToSpiceDBRef(e.namespace, subject),
},
}

return e.checkPermission(ctx, req, queryToken)
return e.checkPermission(ctx, req)
}

// AssignSubjectRole assigns the given role to the given subject.
Expand Down Expand Up @@ -170,11 +175,7 @@ func (e *engine) subjectRoleRelDelete(subject types.Resource, role types.Role) *
}
}

func (e *engine) checkPermission(ctx context.Context, req *pb.CheckPermissionRequest, queryToken string) error {
if queryToken != "" {
req.Consistency = &pb.Consistency{Requirement: &pb.Consistency_AtLeastAsFresh{AtLeastAsFresh: &pb.ZedToken{Token: queryToken}}}
}

func (e *engine) checkPermission(ctx context.Context, req *pb.CheckPermissionRequest) error {
resp, err := e.client.CheckPermission(ctx, req)
if err != nil {
return err
Expand Down
4 changes: 2 additions & 2 deletions internal/query/relations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ func TestSubjectActions(t *testing.T) {
},
)
assert.NoError(t, err)
queryToken, err := e.AssignSubjectRole(ctx, subjRes, role)
_, err = e.AssignSubjectRole(ctx, subjRes, role)
assert.NoError(t, err)

type testInput struct {
Expand Down Expand Up @@ -565,7 +565,7 @@ func TestSubjectActions(t *testing.T) {
}

testFn := func(ctx context.Context, input testInput) testingx.TestResult[any] {
err := e.SubjectHasPermission(ctx, subjRes, input.action, input.resource, queryToken)
err := e.SubjectHasPermission(ctx, subjRes, input.action, input.resource)

return testingx.TestResult[any]{
Err: err,
Expand Down
2 changes: 1 addition & 1 deletion internal/query/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ type Engine interface {
DeleteRelationships(ctx context.Context, resource types.Resource) (string, error)
NewResourceFromID(id gidx.PrefixedID) (types.Resource, error)
GetResourceType(name string) *types.ResourceType
SubjectHasPermission(ctx context.Context, subject types.Resource, action string, resource types.Resource, queryToken string) error
SubjectHasPermission(ctx context.Context, subject types.Resource, action string, resource types.Resource) error
}

type engine struct {
Expand Down

0 comments on commit 211c29f

Please sign in to comment.