Skip to content

Commit 2e30d05

Browse files
EmyrkEmyrk
authored
chore: Move scope into the same auth call (coder#4162)
Scopes now are enforced in the same Authorize call as the roles. Vs 2 `Authorize()` calls
1 parent 4183c5e commit 2e30d05

File tree

4 files changed

+305
-237
lines changed

4 files changed

+305
-237
lines changed

‎coderd/rbac/authz.go

Lines changed: 50 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package rbac
33
import (
44
"context"
55
_ "embed"
6+
"fmt"
67

78
"github.com/open-policy-agent/opa/rego"
89
"go.opentelemetry.io/otel/attribute"
@@ -70,12 +71,20 @@ var _ Authorizer = (*RegoAuthorizer)(nil)
7071
//go:embed policy.rego
7172
var policy string
7273

74+
const (
75+
rolesOkCheck = "role_ok"
76+
scopeOkCheck = "scope_ok"
77+
)
78+
7379
func NewAuthorizer() (*RegoAuthorizer, error) {
7480
ctx := context.Background()
7581
query, err := rego.New(
76-
// allowed is the `allow` field from the prepared query. This is the field to check if authorization is
77-
// granted.
78-
rego.Query("data.authz.allow"),
82+
// Bind the results to 2 variables for easy checking later.
83+
rego.Query(
84+
fmt.Sprintf("%s := data.authz.role_allow "+
85+
"%s := data.authz.scope_allow",
86+
rolesOkCheck, scopeOkCheck),
87+
),
7988
rego.Module("policy.rego", policy),
8089
).PrepareForEval(ctx)
8190

@@ -88,6 +97,7 @@ func NewAuthorizer() (*RegoAuthorizer, error) {
8897
type authSubject struct {
8998
ID string `json:"id"`
9099
Roles []Role `json:"roles"`
100+
Scope Role `json:"scope"`
91101
}
92102

93103
// ByRoleName will expand all roleNames into roles before calling Authorize().
@@ -99,37 +109,30 @@ func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNa
99109
return err
100110
}
101111

102-
err = a.Authorize(ctx, subjectID, roles, action, object)
112+
scopeRole, err := ScopeRole(scope)
103113
if err != nil {
104114
return err
105115
}
106116

107-
// If the scope isn't "any", we need to check with the scope's role as well.
108-
if scope != ScopeAll {
109-
scopeRole, err := ScopeRole(scope)
110-
if err != nil {
111-
return err
112-
}
113-
114-
err = a.Authorize(ctx, subjectID, []Role{scopeRole}, action, object)
115-
if err != nil {
116-
return err
117-
}
117+
err = a.Authorize(ctx, subjectID, roles, scopeRole, action, object)
118+
if err != nil {
119+
return err
118120
}
119121

120122
return nil
121123
}
122124

123125
// Authorize allows passing in custom Roles.
124126
// This is really helpful for unit testing, as we can create custom roles to exercise edge cases.
125-
func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles []Role, action Action, object Object) error {
127+
func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles []Role, scope Role, action Action, object Object) error {
126128
ctx, span := tracing.StartSpan(ctx)
127129
defer span.End()
128130

129131
input := map[string]interface{}{
130132
"subject": authSubject{
131133
ID: subjectID,
132134
Roles: roles,
135+
Scope: scope,
133136
},
134137
"object": object,
135138
"action": action,
@@ -140,16 +143,36 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [
140143
return ForbiddenWithInternal(xerrors.Errorf("eval rego: %w", err), input, results)
141144
}
142145

143-
if !results.Allowed() {
144-
return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results)
145-
}
146+
// We expect only the 2 bindings for scopes and roles checks.
147+
if len(results) == 1 && len(results[0].Bindings) == 2 {
148+
roleCheck, ok := results[0].Bindings[rolesOkCheck].(bool)
149+
if !ok || !roleCheck {
150+
return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results)
151+
}
146152

147-
return nil
153+
scopeCheck, ok := results[0].Bindings[scopeOkCheck].(bool)
154+
if !ok || !scopeCheck {
155+
return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results)
156+
}
157+
158+
// This is purely defensive programming. The two above checks already
159+
// check for 'true' expressions. This is just a sanity check to make
160+
// sure we don't add non-boolean expressions to our query.
161+
// This is super cheap to do, and just adds in some extra safety for
162+
// programmer error.
163+
for _, exp := range results[0].Expressions {
164+
if b, ok := exp.Value.(bool); !ok || !b {
165+
return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results)
166+
}
167+
}
168+
return nil
169+
}
170+
return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results)
148171
}
149172

150173
// Prepare will partially execute the rego policy leaving the object fields unknown (except for the type).
151174
// This will vastly speed up performance if batch authorization on the same type of objects is needed.
152-
func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, scope Scope, action Action, objectType string) (*PartialAuthorizer, error) {
175+
func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, scope Role, action Action, objectType string) (*PartialAuthorizer, error) {
153176
ctx, span := tracing.StartSpan(ctx)
154177
defer span.End()
155178

@@ -170,5 +193,10 @@ func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string,
170193
return nil, err
171194
}
172195

173-
return a.Prepare(ctx, subjectID, roles, scope, action, objectType)
196+
scopeRole, err := ScopeRole(scope)
197+
if err != nil {
198+
return nil, err
199+
}
200+
201+
return a.Prepare(ctx, subjectID, roles, scopeRole, action, objectType)
174202
}

0 commit comments

Comments
 (0)