Skip to content

Commit 5b08612

Browse files
EmyrkEmyrk
committed
Add a unit test that prevents regressions of rego input
The optimized input is always compared to the normal json marshal parser.
1 parent 22c6f50 commit 5b08612

File tree

5 files changed

+124
-39
lines changed

5 files changed

+124
-39
lines changed

‎coderd/rbac/astvalue.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ import (
55
"golang.org/x/xerrors"
66
)
77

8+
// regoInputValue returns a rego input value for the given subject, action, and
9+
// object. This rego input is already parsed and can be used directly in a
10+
// rego query.
811
func regoInputValue(subject Subject, action Action, object Object) (ast.Value, error) {
912
regoSubj, err := subject.regoValue()
1013
if err != nil {
@@ -29,6 +32,7 @@ func regoInputValue(subject Subject, action Action, object Object) (ast.Value, e
2932
return input, nil
3033
}
3134

35+
// regoValue returns the ast.Object representation of the subject.
3236
func (s Subject) regoValue() (ast.Value, error) {
3337
subjRoles, err := s.Roles.Expand()
3438
if err != nil {
@@ -153,6 +157,8 @@ type regoValue interface {
153157
regoValue() ast.Value
154158
}
155159

160+
// regoSlice returns the ast.Array representation of the slice.
161+
// The slice must contain only types that implement the regoValue interface.
156162
func regoSlice[T regoValue](slice []T) *ast.Array {
157163
terms := make([]*ast.Term, len(slice))
158164
for i, v := range slice {

‎coderd/rbac/authz.go

Lines changed: 0 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"sync"
77
"time"
88

9-
"github.com/open-policy-agent/opa/ast"
109
"github.com/open-policy-agent/opa/rego"
1110
"github.com/prometheus/client_golang/prometheus"
1211
"github.com/prometheus/client_golang/prometheus/promauto"
@@ -252,43 +251,6 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subject Subject, action A
252251
return nil
253252
}
254253

255-
type inputType struct {
256-
Subject authSubject `json:"subject"`
257-
Action Action `json:"action"`
258-
Object Object `json:"object"`
259-
}
260-
261-
func (i inputType) Value() (ast.Value, error) {
262-
s := [2]*ast.Term{
263-
ast.StringTerm("subject"),
264-
ast.NewTerm(ast.NewObject([2]*ast.Term{
265-
// ID
266-
ast.StringTerm("id"),
267-
ast.NewTerm(ast.NewObject([2]*ast.Term{
268-
ast.StringTerm("id"),
269-
ast.StringTerm(i.Subject.ID),
270-
})),
271-
////
272-
//ast.NewTerm(ast.NewObject([2]*ast.Term{
273-
// ast.StringTerm("groups"),
274-
// ast.NewTerm(ast.NewArray(i.Subject.Groups)),
275-
//})),
276-
})),
277-
}
278-
o := [2]*ast.Term{
279-
ast.StringTerm("subject"),
280-
ast.StringTerm(i.Subject.ID),
281-
}
282-
a := [2]*ast.Term{
283-
ast.StringTerm("action"),
284-
ast.StringTerm(string(i.Action)),
285-
}
286-
287-
input := ast.NewObject(s, a, o)
288-
289-
return input, nil
290-
}
291-
292254
// authorize is the internal function that does the actual authorization.
293255
// It is a different function so the exported one can add tracing + metrics.
294256
// That code tends to clutter up the actual logic, so it's separated out.

‎coderd/rbac/builtin.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package rbac
22

33
import (
4+
"sort"
45
"strings"
56

67
"github.com/google/uuid"
@@ -79,6 +80,8 @@ var (
7980
Site: permissions(map[string][]Action{
8081
ResourceWildcard.Type: {WildcardSymbol},
8182
}),
83+
Org: map[string][]Permission{},
84+
User: []Permission{},
8285
}
8386
},
8487

@@ -94,6 +97,7 @@ var (
9497
// All users can see the provisioner daemons.
9598
ResourceProvisionerDaemon.Type: {ActionRead},
9699
}),
100+
Org: map[string][]Permission{},
97101
User: permissions(map[string][]Action{
98102
ResourceWildcard.Type: {WildcardSymbol},
99103
}),
@@ -113,6 +117,8 @@ var (
113117
ResourceTemplate.Type: {ActionRead},
114118
ResourceAuditLog.Type: {ActionRead},
115119
}),
120+
Org: map[string][]Permission{},
121+
User: []Permission{},
116122
}
117123
},
118124

@@ -128,6 +134,8 @@ var (
128134
// CRUD to provisioner daemons for now.
129135
ResourceProvisionerDaemon.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
130136
}),
137+
Org: map[string][]Permission{},
138+
User: []Permission{},
131139
}
132140
},
133141

@@ -142,6 +150,8 @@ var (
142150
ResourceOrganizationMember.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
143151
ResourceGroup.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
144152
}),
153+
Org: map[string][]Permission{},
154+
User: []Permission{},
145155
}
146156
},
147157

@@ -151,6 +161,7 @@ var (
151161
return Role{
152162
Name: roleName(orgAdmin, organizationID),
153163
DisplayName: "Organization Admin",
164+
Site: []Permission{},
154165
Org: map[string][]Permission{
155166
organizationID: {
156167
{
@@ -160,6 +171,7 @@ var (
160171
},
161172
},
162173
},
174+
User: []Permission{},
163175
}
164176
},
165177

@@ -169,6 +181,7 @@ var (
169181
return Role{
170182
Name: roleName(orgMember, organizationID),
171183
DisplayName: "",
184+
Site: []Permission{},
172185
Org: map[string][]Permission{
173186
organizationID: {
174187
{
@@ -192,6 +205,7 @@ var (
192205
},
193206
},
194207
},
208+
User: []Permission{},
195209
}
196210
},
197211
}
@@ -422,5 +436,9 @@ func permissions(perms map[string][]Action) []Permission {
422436
})
423437
}
424438
}
439+
// Deterministic ordering of permissions
440+
sort.Slice(list, func(i, j int) bool {
441+
return list[i].ResourceType < list[j].ResourceType
442+
})
425443
return list
426444
}

‎coderd/rbac/builtin_internal_test.go

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,14 @@ import (
88
"github.com/stretchr/testify/require"
99
)
1010

11+
// BenchmarkRBACValueAllocation benchmarks the cost of allocating a rego input
12+
// value. By default, `ast.InterfaceToValue` is used to convert the input,
13+
// which uses json marshalling under the hood.
14+
//
15+
// Currently ast.Object.insert() is the slowest part of the process and allocates
16+
// the most amount of bytes. This general approach copies all of our struct
17+
// data and uses a lot of extra memory for handling things like sort order.
18+
// A possible large improvement would be to implement the ast.Value interface directly.
1119
func BenchmarkRBACValueAllocation(b *testing.B) {
1220
actor := Subject{
1321
Roles: RoleNames{RoleOrgMember(uuid.New()), RoleOrgAdmin(uuid.New()), RoleMember()},
@@ -47,7 +55,98 @@ func BenchmarkRBACValueAllocation(b *testing.B) {
4755
require.NoError(b, err)
4856
}
4957
})
58+
}
59+
60+
// TestRegoInputValue ensures the custom rego input parser returns the
61+
// same value as the default json parser. The json parser is always correct,
62+
// and the custom parser is used to reduce allocations. This optimization
63+
// should yield the same results. Anything different is a bug.
64+
func TestRegoInputValue(t *testing.T) {
65+
actor := Subject{
66+
Roles: RoleNames{RoleOrgMember(uuid.New()), RoleOrgAdmin(uuid.New()), RoleMember()},
67+
ID: uuid.NewString(),
68+
Scope: ScopeAll,
69+
Groups: []string{uuid.NewString(), uuid.NewString(), uuid.NewString()},
70+
}
71+
72+
obj := ResourceTemplate.
73+
WithID(uuid.New()).
74+
InOrg(uuid.New()).
75+
WithOwner(uuid.NewString()).
76+
WithGroupACL(map[string][]Action{
77+
uuid.NewString(): {ActionRead, ActionCreate},
78+
uuid.NewString(): {ActionRead, ActionCreate},
79+
uuid.NewString(): {ActionRead, ActionCreate},
80+
}).WithACLUserList(map[string][]Action{
81+
uuid.NewString(): {ActionRead, ActionCreate},
82+
uuid.NewString(): {ActionRead, ActionCreate},
83+
})
84+
85+
action := ActionRead
86+
87+
// This is the input that would be passed to the rego policy.
88+
jsonInput := map[string]interface{}{
89+
"subject": authSubject{
90+
ID: actor.ID,
91+
Roles: must(actor.Roles.Expand()),
92+
Groups: actor.Groups,
93+
Scope: must(actor.Scope.Expand()),
94+
},
95+
"action": action,
96+
"object": obj,
97+
}
98+
99+
manual, err := regoInputValue(actor, action, obj)
100+
require.NoError(t, err)
101+
102+
general, err := ast.InterfaceToValue(jsonInput)
103+
require.NoError(t, err)
104+
105+
// The custom parser does not set these fields because they are not needed.
106+
// To ensure the outputs are identical, intentionally overwrite all names
107+
// to the same values.
108+
ignoreNames(t, manual)
109+
ignoreNames(t, general)
110+
111+
cmp := manual.Compare(general)
112+
require.Equal(t, 0, cmp, "manual and general input values should be equal")
113+
}
114+
115+
// ignoreNames sets all names to "ignore" to ensure the values are identical.
116+
func ignoreNames(t *testing.T, value ast.Value) {
117+
t.Helper()
118+
119+
// Override the names of the roles
120+
ref := ast.Ref{
121+
ast.StringTerm("subject"),
122+
ast.StringTerm("roles"),
123+
}
124+
roles, err := value.Find(ref)
125+
require.NoError(t, err)
126+
127+
rolesArray, ok := roles.(*ast.Array)
128+
require.True(t, ok, "roles is expected to be an array")
129+
130+
rolesArray.Foreach(func(term *ast.Term) {
131+
obj, _ := term.Value.(ast.Object)
132+
// Ignore all names
133+
obj.Insert(ast.StringTerm("name"), ast.StringTerm("ignore"))
134+
obj.Insert(ast.StringTerm("display_name"), ast.StringTerm("ignore"))
135+
})
136+
137+
// Override the names of the scope role
138+
ref = ast.Ref{
139+
ast.StringTerm("subject"),
140+
ast.StringTerm("scope"),
141+
}
142+
scope, err := value.Find(ref)
143+
require.NoError(t, err)
144+
145+
scopeObj, ok := scope.(ast.Object)
146+
require.True(t, ok, "scope is expected to be an object")
50147

148+
scopeObj.Insert(ast.StringTerm("name"), ast.StringTerm("ignore"))
149+
scopeObj.Insert(ast.StringTerm("display_name"), ast.StringTerm("ignore"))
51150
}
52151

53152
func TestRoleByName(t *testing.T) {

‎coderd/rbac/input.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
}
2525
],
2626
"org": {},
27-
"user": [],
27+
"user": []
2828
}
2929
],
3030
"groups": ["b617a647-b5d0-4cbe-9e40-26f89710bf18"],

0 commit comments

Comments
 (0)