Skip to content

Commit 7297c3c

Browse files
EmyrkEmyrk
committed
fix: Fix acl list rego policy
1 parent 41b79b6 commit 7297c3c

File tree

4 files changed

+52
-70
lines changed

4 files changed

+52
-70
lines changed

‎coderd/rbac/authz.go

Lines changed: 4 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package rbac
33
import (
44
"context"
55
_ "embed"
6-
"fmt"
76
"sync"
87

98
"github.com/open-policy-agent/opa/rego"
@@ -92,12 +91,7 @@ func NewAuthorizer() *RegoAuthorizer {
9291
queryOnce.Do(func() {
9392
var err error
9493
query, err = rego.New(
95-
// Bind the results to 2 variables for easy checking later.
96-
rego.Query(
97-
fmt.Sprintf("%s := data.authz.role_allow "+
98-
"%s := data.authz.scope_allow",
99-
rolesOkCheck, scopeOkCheck),
100-
),
94+
rego.Query("data.authz.allow"),
10195
rego.Module("policy.rego", policy),
10296
).PrepareForEval(context.Background())
10397
if err != nil {
@@ -158,31 +152,10 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [
158152
return ForbiddenWithInternal(xerrors.Errorf("eval rego: %w", err), input, results)
159153
}
160154

161-
// We expect only the 2 bindings for scopes and roles checks.
162-
if len(results) == 1 && len(results[0].Bindings) == 2 {
163-
roleCheck, ok := results[0].Bindings[rolesOkCheck].(bool)
164-
if !ok || !roleCheck {
165-
return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results)
166-
}
167-
168-
scopeCheck, ok := results[0].Bindings[scopeOkCheck].(bool)
169-
if !ok || !scopeCheck {
170-
return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results)
171-
}
172-
173-
// This is purely defensive programming. The two above checks already
174-
// check for 'true' expressions. This is just a sanity check to make
175-
// sure we don't add non-boolean expressions to our query.
176-
// This is super cheap to do, and just adds in some extra safety for
177-
// programmer error.
178-
for _, exp := range results[0].Expressions {
179-
if b, ok := exp.Value.(bool); !ok || !b {
180-
return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results)
181-
}
182-
}
183-
return nil
155+
if !results.Allowed() {
156+
return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results)
184157
}
185-
return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results)
158+
return nil
186159
}
187160

188161
// Prepare will partially execute the rego policy leaving the object fields unknown (except for the type).

‎coderd/rbac/authz_internal_test.go

Lines changed: 31 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -208,37 +208,37 @@ func TestAuthorizeDomain(t *testing.T) {
208208
},
209209
}
210210

211-
//testAuthorize(t, "ACLList", user, []authTestCase{
212-
// {
213-
// resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(unuseID).WithACLUserList(map[string][]Action{
214-
// user.UserID: allActions(),
215-
// }),
216-
// actions: allActions(),
217-
// allow: true,
218-
// },
219-
// {
220-
// resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(unuseID).WithACLUserList(map[string][]Action{
221-
// user.UserID: {WildcardSymbol},
222-
// }),
223-
// actions: allActions(),
224-
// allow: true,
225-
// },
226-
// {
227-
// resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(unuseID).WithACLUserList(map[string][]Action{
228-
// user.UserID: {ActionRead, ActionUpdate},
229-
// }),
230-
// actions: []Action{ActionCreate, ActionDelete},
231-
// allow: false,
232-
// },
233-
// {
234-
// // By default users cannot update templates
235-
// resource: ResourceTemplate.InOrg(defOrg).WithACLUserList(map[string][]Action{
236-
// user.UserID: {ActionUpdate},
237-
// }),
238-
// actions: []Action{ActionRead, ActionUpdate},
239-
// allow: true,
240-
// },
241-
//})
211+
testAuthorize(t, "ACLList", user, []authTestCase{
212+
{
213+
resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(unuseID).WithACLUserList(map[string][]Action{
214+
user.UserID: allActions(),
215+
}),
216+
actions: allActions(),
217+
allow: true,
218+
},
219+
{
220+
resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(unuseID).WithACLUserList(map[string][]Action{
221+
user.UserID: {WildcardSymbol},
222+
}),
223+
actions: allActions(),
224+
allow: true,
225+
},
226+
{
227+
resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(unuseID).WithACLUserList(map[string][]Action{
228+
user.UserID: {ActionRead, ActionUpdate},
229+
}),
230+
actions: []Action{ActionCreate, ActionDelete},
231+
allow: false,
232+
},
233+
{
234+
// By default users cannot update templates
235+
resource: ResourceTemplate.InOrg(defOrg).WithACLUserList(map[string][]Action{
236+
user.UserID: {ActionUpdate},
237+
}),
238+
actions: []Action{ActionRead, ActionUpdate},
239+
allow: true,
240+
},
241+
})
242242

243243
testAuthorize(t, "Member", user, []authTestCase{
244244
// Org + me

‎coderd/rbac/partial.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, s
111111
// Run the rego policy with a few unknown fields. This should simplify our
112112
// policy to a set of queries.
113113
partialQueries, err := rego.New(
114-
rego.Query("data.authz.role_allow = true data.authz.scope_allow = true"),
114+
rego.Query("data.authz.allow = true"),
115115
rego.Module("policy.rego", policy),
116116
rego.Unknowns([]string{
117117
"input.object.owner",

‎coderd/rbac/policy.rego

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ package authz
22
import future.keywords
33
# A great playground: https://play.openpolicyagent.org/
44
# Helpful cli commands to debug.
5-
# opa eval --format=pretty 'data.authz.role_allow data.authz.scope_allow' -d policy.rego -i input.json
6-
# opa eval --partial --format=pretty 'data.authz.role_allow = true data.authz.scope_allow = true' -d policy.rego --unknowns input.object.owner --unknowns input.object.org_owner --unknowns input.object.acl_user_list --unknowns input.object.acl_group_list -i input.json
5+
# opa eval --format=pretty 'data.authz.allow' -d policy.rego -i input.json
6+
# opa eval --partial --format=pretty 'data.authz.allow' -d policy.rego --unknowns input.object.owner --unknowns input.object.org_owner --unknowns input.object.acl_user_list --unknowns input.object.acl_group_list -i input.json
77

88
#
99
# This policy is specifically constructed to compress to a set of queries if the
@@ -156,7 +156,6 @@ user_allow(roles) := num {
156156
# Allow query:
157157
# data.authz.role_allow = true data.authz.scope_allow = true
158158

159-
default role_allow = false
160159
role_allow {
161160
site = 1
162161
}
@@ -175,8 +174,6 @@ role_allow {
175174
user = 1
176175
}
177176

178-
179-
default scope_allow = false
180177
scope_allow {
181178
scope_site = 1
182179
}
@@ -196,15 +193,15 @@ scope_allow {
196193
}
197194

198195
# ACL for users
199-
allow {
196+
acl_allow {
200197
# Should you have to be a member of the org too?
201198
perms := input.object.acl_user_list[input.subject.id]
202199
# Either the input action or wildcard
203200
[input.action, "*"][_] in perms
204201
}
205202

206203
# ACL for groups
207-
allow {
204+
acl_allow {
208205
# If there is no organization owner, the object cannot be owned by an
209206
# org_scoped team.
210207
# TODO: This line and 'org_mem' are similiar and should be combined.
@@ -220,3 +217,15 @@ allow {
220217
[input.action, "*"][_] in perms
221218
}
222219

220+
allow {
221+
role_allow
222+
scope_allow
223+
}
224+
225+
# ACL list must also have the scope_allow to pass
226+
allow {
227+
acl_allow
228+
scope_allow
229+
}
230+
231+

0 commit comments

Comments
 (0)