Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update BaseSubjectSet to support caveat expressions #932

Merged
merged 1 commit into from
Nov 7, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 53 additions & 49 deletions .golangci.yaml
Original file line number Diff line number Diff line change
@@ -1,69 +1,73 @@
---
run:
timeout: "5m"
timeout: '5m'
output:
sort-results: true
linters-settings:
goimports:
local-prefixes: "github.com/authzed/spicedb"
local-prefixes: 'github.com/authzed/spicedb'
rowserrcheck:
packages:
- "github.com/jmoiron/sqlx"
- "github.com/jackc/pgx"
- 'github.com/jmoiron/sqlx'
- 'github.com/jackc/pgx'
gosec:
excludes:
- "G404" # Allow the usage of math/rand
- 'G404' # Allow the usage of math/rand
linters:
enable:
- "bidichk"
- "bodyclose"
- "deadcode"
- "errcheck"
- "errname"
- "errorlint"
- "gofumpt"
- "goimports"
- "goprintffuncname"
- "gosec"
- "gosimple"
- "govet"
- "importas"
- "ineffassign"
- "makezero"
- "prealloc"
- "predeclared"
- "promlinter"
- "revive"
- "rowserrcheck"
- "staticcheck"
- "structcheck"
- "stylecheck"
- "tenv"
- "typecheck"
- "unconvert"
- "unused"
- "varcheck"
- "wastedassign"
- "whitespace"
- 'bidichk'
- 'bodyclose'
- 'deadcode'
- 'errcheck'
- 'errname'
- 'errorlint'
- 'gofumpt'
- 'goimports'
- 'goprintffuncname'
- 'gosec'
- 'gosimple'
- 'govet'
- 'importas'
- 'ineffassign'
- 'makezero'
- 'prealloc'
- 'predeclared'
- 'promlinter'
- 'revive'
- 'rowserrcheck'
- 'staticcheck'
- 'structcheck'
- 'stylecheck'
- 'tenv'
- 'typecheck'
- 'unconvert'
- 'unused'
- 'varcheck'
- 'wastedassign'
- 'whitespace'
issues:
exclude-rules:
- text: "tx.Rollback()"
- text: 'tx.Rollback()'
linters:
- "errcheck"
- 'errcheck'
# NOTE: temporarily disable deprecation checks for v0.
- path: "internal/services/"
- path: 'internal/services/'
linters:
- "staticcheck"
text: "SA1019"
- path: "internal/middleware/consistency/"
- 'staticcheck'
text: 'SA1019'
- path: 'internal/middleware/consistency/'
linters:
- "staticcheck"
text: "SA1019"
- path: "pkg/proto/core/v1/core.pb.validate.manual.go" # Ignore manual definition of metadata map
- 'staticcheck'
text: 'SA1019'
- path: 'pkg/proto/core/v1/core.pb.validate.manual.go' # Ignore manual definition of metadata map
linters:
- "stylecheck"
text: "ST1003"
- path: "pkg/proto/core/v1/core.pb.validate.manual.go" # Ignore manual definition of metadata map
- 'stylecheck'
text: 'ST1003'
- path: 'pkg/proto/core/v1/core.pb.validate.manual.go' # Ignore manual definition of metadata map
linters:
- "revive"
text: "var-naming"
- 'revive'
text: 'var-naming'
# Ignore receiver errors for generic types not understood by the linter.
- linters:
- 'revive'
text: 'receiver-naming: receiver name \S+ should be consistent with previous receiver name \S+ for invalid-type'
34 changes: 17 additions & 17 deletions buf.gen.yaml
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
#!/usr/bin/env -S buf generate -o pkg/proto proto/internal --template
---
version: "v1"
version: 'v1'
managed:
enabled: true
go_package_prefix:
default: "github.com/authzed/spicedb/pkg/proto"
default: 'github.com/authzed/spicedb/pkg/proto'
except:
- "buf.build/envoyproxy/protoc-gen-validate"
- "buf.build/authzed/api"
- "buf.build/googleapis/googleapis"
- 'buf.build/envoyproxy/protoc-gen-validate'
- 'buf.build/authzed/api'
- 'buf.build/googleapis/googleapis'
plugins:
- name: "go"
out: "."
opt: "paths=source_relative"
- name: "go-grpc"
out: "."
opt: "paths=source_relative"
- name: "go-vtproto"
out: "."
- name: 'go'
out: '.'
opt: 'paths=source_relative'
- name: 'go-grpc'
out: '.'
opt: 'paths=source_relative'
- name: 'go-vtproto'
out: '.'
# To generate pooling methods, you must add an additional `pool=fully/qualified.ProtoMessageType`
opt: "paths=source_relative,features=marshal+unmarshal+size+clone+pool"
- name: "validate"
out: "."
opt: "paths=source_relative,lang=go"
opt: 'paths=source_relative,features=marshal+unmarshal+size+clone+pool+equal'
- name: 'validate'
out: '.'
opt: 'paths=source_relative,lang=go'
2 changes: 1 addition & 1 deletion internal/dispatch/graph/lookupsubjects_test.go
Original file line number Diff line number Diff line change
@@ -142,7 +142,7 @@ func TestSimpleLookupSubjects(t *testing.T) {
foundSubjectIds := []string{}
for _, result := range stream.Results() {
for _, found := range result.FoundSubjects {
if len(found.ExcludedSubjectIds) > 0 {
if len(found.ExcludedSubjects) > 0 {
continue
}

50 changes: 39 additions & 11 deletions internal/membership/foundsubject.go
Original file line number Diff line number Diff line change
@@ -6,13 +6,14 @@ import (
"strings"

core "github.com/authzed/spicedb/pkg/proto/core/v1"
v1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"

"github.com/authzed/spicedb/pkg/tuple"
)

// NewFoundSubject creates a new FoundSubject for a subject and a set of its resources.
func NewFoundSubject(subject *core.ObjectAndRelation, resources ...*core.ObjectAndRelation) FoundSubject {
return FoundSubject{subject, nil, tuple.NewONRSet(resources...)}
return FoundSubject{subject, nil, nil, tuple.NewONRSet(resources...)}
}

// FoundSubject contains a single found subject and all the relationships in which that subject
@@ -22,7 +23,10 @@ type FoundSubject struct {
subject *core.ObjectAndRelation

// excludedSubjects are any subjects excluded. Only should be set if subject is a wildcard.
excludedSubjectIds []string
excludedSubjects []FoundSubject

// caveatExpression is the conditional expression on the found subject.
caveatExpression *v1.CaveatExpression

// relations are the relations under which the subject lives that informed the locating
// of this subject for the root ONR.
@@ -36,8 +40,12 @@ func (fs FoundSubject) GetSubjectId() string {
return fs.subject.ObjectId
}

func (fs FoundSubject) GetExcludedSubjectIds() []string {
return fs.excludedSubjectIds
func (fs FoundSubject) GetCaveatExpression() *v1.CaveatExpression {
return fs.caveatExpression
}

func (fs FoundSubject) GetExcludedSubjects() []FoundSubject {
return fs.excludedSubjects
}

// Subject returns the Subject of the FoundSubject.
@@ -58,13 +66,14 @@ func (fs FoundSubject) WildcardType() (string, bool) {
// If not a wildcard subject, returns false.
func (fs FoundSubject) ExcludedSubjectsFromWildcard() ([]*core.ObjectAndRelation, bool) {
if fs.subject.ObjectId == tuple.PublicWildcard {
excludedSubjects := make([]*core.ObjectAndRelation, 0, len(fs.excludedSubjectIds))
for _, excludedID := range fs.excludedSubjectIds {
excludedSubjects = append(excludedSubjects, &core.ObjectAndRelation{
Namespace: fs.subject.Namespace,
ObjectId: excludedID,
Relation: fs.subject.Relation,
})
excludedSubjects := make([]*core.ObjectAndRelation, 0, len(fs.excludedSubjects))
for _, excludedSubject := range fs.excludedSubjects {
// TODO(jschorr): Fix once we add caveats support to debug tooling
if excludedSubject.caveatExpression != nil {
panic("not yet supported")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't panic-ing here prevent us from cutting an release without the debug tooling implemented?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! That's intended; I want to do something here, even if it is just returning "(unsupported)", but I don't know exactly what yet, so I added in panics

}

excludedSubjects = append(excludedSubjects, excludedSubject.subject)
}

return excludedSubjects, true
@@ -73,6 +82,20 @@ func (fs FoundSubject) ExcludedSubjectsFromWildcard() ([]*core.ObjectAndRelation
return []*core.ObjectAndRelation{}, false
}

func (fs FoundSubject) excludedSubjectIDs() []string {
excludedSubjects := make([]string, 0, len(fs.excludedSubjects))
for _, excludedSubject := range fs.excludedSubjects {
// TODO(jschorr): Fix once we add caveats support to debug tooling
if excludedSubject.caveatExpression != nil {
panic("not yet supported")
}

excludedSubjects = append(excludedSubjects, excludedSubject.subject.ObjectId)
}

return excludedSubjects
}

// Relationships returns all the relationships in which the subject was found as per the expand.
func (fs FoundSubject) Relationships() []*core.ObjectAndRelation {
return fs.relationships.AsSlice()
@@ -81,6 +104,11 @@ func (fs FoundSubject) Relationships() []*core.ObjectAndRelation {
// ToValidationString returns the FoundSubject in a format that is consumable by the validationfile
// package.
func (fs FoundSubject) ToValidationString() string {
if fs.caveatExpression != nil {
// TODO(jschorr): Implement once we have a format for this.
panic("conditional found subjects not yet supported")
}

onrString := tuple.StringONR(fs.Subject())
excluded, isWildcard := fs.ExcludedSubjectsFromWildcard()
if isWildcard && len(excluded) > 0 {
28 changes: 19 additions & 9 deletions internal/membership/trackingsubjectset.go
Original file line number Diff line number Diff line change
@@ -4,7 +4,10 @@ import (
"fmt"
"strings"

"github.com/authzed/spicedb/pkg/tuple"

core "github.com/authzed/spicedb/pkg/proto/core/v1"
v1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"

"github.com/authzed/spicedb/internal/util"
)
@@ -62,24 +65,21 @@ func (tss *TrackingSubjectSet) getSetForKey(key string) util.BaseSubjectSet[Foun
parts := strings.Split(key, "#")

created := util.NewBaseSubjectSet[FoundSubject](
func(subjectID string, excludedSubjectIDs []string, sources ...FoundSubject) FoundSubject {
func(subjectID string, caveatExpression *v1.CaveatExpression, excludedSubjects []FoundSubject, sources ...FoundSubject) FoundSubject {
fs := NewFoundSubject(&core.ObjectAndRelation{
Namespace: parts[0],
ObjectId: subjectID,
Relation: parts[1],
})
fs.excludedSubjectIds = excludedSubjectIDs
fs.excludedSubjects = excludedSubjects
fs.caveatExpression = caveatExpression
for _, source := range sources {
fs.relationships.UpdateFrom(source.relationships)
if source.relationships != nil {
fs.relationships.UpdateFrom(source.relationships)
}
}
return fs
},
func(existing FoundSubject, added FoundSubject) FoundSubject {
fs := NewFoundSubject(existing.subject)
fs.excludedSubjectIds = existing.excludedSubjectIds
fs.relationships = existing.relationships.Union(added.relationships)
return fs
},
)
tss.setByType[key] = created
return created
@@ -151,6 +151,16 @@ func (tss TrackingSubjectSet) removeExact(subjects ...*core.ObjectAndRelation) {
}
}

func (tss TrackingSubjectSet) getSubjects() []string {
var subjects []string
for _, subjectSet := range tss.setByType {
for _, foundSubject := range subjectSet.AsSlice() {
subjects = append(subjects, tuple.StringONR(foundSubject.subject))
}
}
return subjects
}

// ToSlice returns a slice of all subjects found in the set.
func (tss TrackingSubjectSet) ToSlice() []FoundSubject {
subjects := []FoundSubject{}
17 changes: 11 additions & 6 deletions internal/membership/trackingsubjectset_test.go
Original file line number Diff line number Diff line change
@@ -45,10 +45,15 @@ func subtract(firstSet *TrackingSubjectSet, sets ...*TrackingSubjectSet) *Tracki
}

func fs(subjectType string, subjectID string, subjectRel string, excludedSubjectIDs ...string) FoundSubject {
excludedSubjects := make([]FoundSubject, 0, len(excludedSubjectIDs))
for _, excludedSubjectID := range excludedSubjectIDs {
excludedSubjects = append(excludedSubjects, FoundSubject{subject: ONR(subjectType, excludedSubjectID, subjectRel)})
}

return FoundSubject{
subject: ONR(subjectType, subjectID, subjectRel),
excludedSubjectIds: excludedSubjectIDs,
relationships: tuple.NewONRSet(),
subject: ONR(subjectType, subjectID, subjectRel),
excludedSubjects: excludedSubjects,
relationships: tuple.NewONRSet(),
}
}

@@ -330,8 +335,8 @@ func TestTrackingSubjectSet(t *testing.T) {
found, ok := tc.set.Get(fs.subject)
require.True(ok, "missing expected subject %s", fs.subject)

expectedExcluded := util.NewSet[string](fs.excludedSubjectIds...)
foundExcluded := util.NewSet[string](found.excludedSubjectIds...)
expectedExcluded := util.NewSet[string](fs.excludedSubjectIDs()...)
foundExcluded := util.NewSet[string](found.excludedSubjectIDs()...)
require.Len(expectedExcluded.Subtract(foundExcluded).AsSlice(), 0, "mismatch on excluded subjects on %s: expected: %s, found: %s", fs.subject, expectedExcluded, foundExcluded)
require.Len(foundExcluded.Subtract(expectedExcluded).AsSlice(), 0, "mismatch on excluded subjects on %s: expected: %s, found: %s", fs.subject, expectedExcluded, foundExcluded)
} else {
@@ -340,7 +345,7 @@ func TestTrackingSubjectSet(t *testing.T) {
tc.set.removeExact(fs.subject)
}

require.True(tc.set.IsEmpty())
require.True(tc.set.IsEmpty(), "Found remaining: %v", tc.set.getSubjects())
})
}
}
7 changes: 6 additions & 1 deletion internal/services/v1/permissions.go
Original file line number Diff line number Diff line change
@@ -430,9 +430,14 @@ func (ps *permissionServer) LookupSubjects(req *v1.LookupSubjectsRequest, resp v

stream := dispatchpkg.NewHandlingDispatchStream(ctx, func(result *dispatch.DispatchLookupSubjectsResponse) error {
for _, foundSubject := range result.FoundSubjects {
excludedSubjectIDs := make([]string, 0, len(foundSubject.ExcludedSubjects))
for _, excludedSubject := range foundSubject.ExcludedSubjects {
excludedSubjectIDs = append(excludedSubjectIDs, excludedSubject.SubjectId)
}

err := resp.Send(&v1.LookupSubjectsResponse{
SubjectObjectId: foundSubject.SubjectId,
ExcludedSubjectIds: foundSubject.ExcludedSubjectIds,
ExcludedSubjectIds: excludedSubjectIDs,
LookedUpAt: revisionReadAt,
})
if err != nil {
888 changes: 732 additions & 156 deletions internal/util/basesubjectset.go

Large diffs are not rendered by default.

19 changes: 9 additions & 10 deletions internal/util/subjectset.go
Original file line number Diff line number Diff line change
@@ -15,16 +15,7 @@ type SubjectSet struct {
// NewSubjectSet creates and returns a new subject set.
func NewSubjectSet() SubjectSet {
return SubjectSet{
BaseSubjectSet: BaseSubjectSet[*v1.FoundSubject]{
values: map[string]*v1.FoundSubject{},
constructor: func(subjectID string, excludedSubjectIDs []string, sources ...*v1.FoundSubject) *v1.FoundSubject {
return &v1.FoundSubject{
SubjectId: subjectID,
ExcludedSubjectIds: excludedSubjectIDs,
}
},
combiner: nil,
},
BaseSubjectSet: NewBaseSubjectSet[*v1.FoundSubject](subjectSetConstructor),
}
}

@@ -39,3 +30,11 @@ func (ss SubjectSet) IntersectionDifference(other SubjectSet) {
func (ss SubjectSet) UnionWithSet(other SubjectSet) {
ss.BaseSubjectSet.UnionWithSet(other.BaseSubjectSet)
}

func subjectSetConstructor(subjectID string, caveatExpression *v1.CaveatExpression, excludedSubjects []*v1.FoundSubject, sources ...*v1.FoundSubject) *v1.FoundSubject {
return &v1.FoundSubject{
SubjectId: subjectID,
CaveatExpression: caveatExpression,
ExcludedSubjects: excludedSubjects,
}
}
2,742 changes: 2,682 additions & 60 deletions internal/util/subjectset_test.go

Large diffs are not rendered by default.

976 changes: 976 additions & 0 deletions pkg/proto/core/v1/core_vtproto.pb.go

Large diffs are not rendered by default.

361 changes: 361 additions & 0 deletions pkg/proto/developer/v1/developer_vtproto.pb.go
306 changes: 161 additions & 145 deletions pkg/proto/dispatch/v1/dispatch.pb.go

Large diffs are not rendered by default.

63 changes: 63 additions & 0 deletions pkg/proto/dispatch/v1/dispatch.pb.validate.go
747 changes: 726 additions & 21 deletions pkg/proto/dispatch/v1/dispatch_vtproto.pb.go

Large diffs are not rendered by default.

313 changes: 313 additions & 0 deletions pkg/proto/impl/v1/impl_vtproto.pb.go
3 changes: 2 additions & 1 deletion proto/internal/dispatch/v1/dispatch.proto
Original file line number Diff line number Diff line change
@@ -161,7 +161,8 @@ message DispatchLookupSubjectsRequest {

message FoundSubject {
string subject_id = 1;
repeated string excluded_subject_ids = 2;
CaveatExpression caveat_expression = 2;
repeated FoundSubject excluded_subjects = 3;
}

message DispatchLookupSubjectsResponse {