Skip to content

Commit a0790b0

Browse files
EmyrkEmyrk
committed
test: Add unit test verifying acl behavior against all normal cases.
- bug: OrgAdmin could not make new groups - Also refactor some function names
1 parent b9b1c4e commit a0790b0

File tree

10 files changed

+283
-47
lines changed

10 files changed

+283
-47
lines changed

‎coderd/coderdtest/authorize.go

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -551,10 +551,10 @@ func (f *fakePreparedAuthorizer) Authorize(ctx context.Context, object rbac.Obje
551551
return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Scope, f.Groups, f.Action, object)
552552
}
553553

554-
// Compile returns a compiled version of the authorizer that will work for
554+
// CompileToSQL returns a compiled version of the authorizer that will work for
555555
// in memory databases. This fake version will not work against a SQL database.
556-
func (f *fakePreparedAuthorizer) Compile(_ regosql.ConvertConfig) (rbac.AuthorizeFilter, error) {
557-
return f, nil
556+
func (f *fakePreparedAuthorizer) CompileToSQL(_ regosql.ConvertConfig) (string, error) {
557+
return "", fmt.Errorf("not implemented")
558558
}
559559

560560
func (f *fakePreparedAuthorizer) Eval(object rbac.Object) bool {
@@ -567,10 +567,3 @@ func (f fakePreparedAuthorizer) RegoString() string {
567567
}
568568
panic("not implemented")
569569
}
570-
571-
func (f fakePreparedAuthorizer) SQLString() string {
572-
if f.HardCodedSQLString != "" {
573-
return f.HardCodedSQLString
574-
}
575-
panic("not implemented")
576-
}

‎coderd/database/databasefake/databasefake.go

Lines changed: 21 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -488,11 +488,20 @@ func (q *fakeQuerier) GetFilteredUserCount(ctx context.Context, arg database.Get
488488
return count, err
489489
}
490490

491-
func (q *fakeQuerier) GetAuthorizedUserCount(_ context.Context, params database.GetFilteredUserCountParams, prepared rbac.PreparedAuthorized) (int64, error) {
491+
func (q *fakeQuerier) GetAuthorizedUserCount(ctx context.Context, params database.GetFilteredUserCountParams, prepared rbac.PreparedAuthorized) (int64, error) {
492492
q.mutex.RLock()
493493
defer q.mutex.RUnlock()
494494

495-
users := append([]database.User{}, q.users...)
495+
users := make([]database.User, 0, len(q.users))
496+
497+
for _, user := range q.users {
498+
// If the filter exists, ensure the object is authorized.
499+
if prepared != nil && prepared.Authorize(ctx, user.RBACObject()) != nil {
500+
continue
501+
}
502+
503+
users = append(users, user)
504+
}
496505

497506
if params.Deleted {
498507
tmp := make([]database.User, 0, len(users))
@@ -539,22 +548,6 @@ func (q *fakeQuerier) GetAuthorizedUserCount(_ context.Context, params database.
539548
users = usersFilteredByRole
540549
}
541550

542-
var authorizedFilter rbac.AuthorizeFilter
543-
var err error
544-
if prepared != nil {
545-
authorizedFilter, err = prepared.Compile(rbac.ConfigWithoutACL())
546-
if err != nil {
547-
return -1, xerrors.Errorf("compile authorized filter: %w", err)
548-
}
549-
}
550-
551-
for _, user := range q.workspaces {
552-
// If the filter exists, ensure the object is authorized.
553-
if authorizedFilter != nil && !authorizedFilter.Eval(user.RBACObject()) {
554-
continue
555-
}
556-
}
557-
558551
return int64(len(users)), nil
559552
}
560553

@@ -763,11 +756,6 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
763756
q.mutex.RLock()
764757
defer q.mutex.RUnlock()
765758

766-
filter, err := prepared.Compile(rbac.ConfigWithoutACL())
767-
if err != nil {
768-
return nil, xerrors.Errorf("compile authorized filter: %w", err)
769-
}
770-
771759
workspaces := make([]database.Workspace, 0)
772760
for _, workspace := range q.workspaces {
773761
if arg.OwnerID != uuid.Nil && workspace.OwnerID != arg.OwnerID {
@@ -899,7 +887,7 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
899887
}
900888

901889
// If the filter exists, ensure the object is authorized.
902-
if filter != nil && !filter.Eval(workspace.RBACObject()) {
890+
if prepared != nil && prepared.Authorize(ctx, workspace.RBACObject()) != nil {
903891
continue
904892
}
905893
workspaces = append(workspaces, workspace)
@@ -1447,12 +1435,20 @@ func (q *fakeQuerier) UpdateTemplateMetaByID(_ context.Context, arg database.Upd
14471435
return database.Template{}, sql.ErrNoRows
14481436
}
14491437

1450-
func (q *fakeQuerier) GetTemplatesWithFilter(_ context.Context, arg database.GetTemplatesWithFilterParams) ([]database.Template, error) {
1438+
func (q *fakeQuerier) GetTemplatesWithFilter(ctx context.Context, arg database.GetTemplatesWithFilterParams) ([]database.Template, error) {
1439+
return q.GetAuthorizedTemplates(ctx, arg, nil)
1440+
}
1441+
1442+
func (q *fakeQuerier) GetAuthorizedTemplates(ctx context.Context, arg database.GetTemplatesWithFilterParams, prepared rbac.PreparedAuthorized) ([]database.Template, error) {
14511443
q.mutex.RLock()
14521444
defer q.mutex.RUnlock()
14531445

14541446
var templates []database.Template
14551447
for _, template := range q.templates {
1448+
if prepared != nil && prepared.Authorize(ctx, template.RBACObject()) != nil {
1449+
continue
1450+
}
1451+
14561452
if template.Deleted != arg.Deleted {
14571453
continue
14581454
}

‎coderd/database/modelqueries.go

Lines changed: 57 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,63 @@ type customQuerier interface {
2323
}
2424

2525
type templateQuerier interface {
26+
GetAuthorizedTemplates(ctx context.Context, arg GetTemplatesWithFilterParams, prepared rbac.PreparedAuthorized) ([]Template, error)
2627
GetTemplateGroupRoles(ctx context.Context, id uuid.UUID) ([]TemplateGroup, error)
2728
GetTemplateUserRoles(ctx context.Context, id uuid.UUID) ([]TemplateUser, error)
2829
}
2930

31+
func (q *sqlQuerier) GetAuthorizedTemplates(ctx context.Context, arg GetTemplatesWithFilterParams, prepared rbac.PreparedAuthorized) ([]Template, error) {
32+
authorizedFilter, err := prepared.CompileToSQL(rbac.ConfigWithACL())
33+
if err != nil {
34+
return nil, xerrors.Errorf("compile authorized filter: %w", err)
35+
}
36+
37+
filter := strings.Replace(getTemplatesWithFilter, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter), 1)
38+
// The name comment is for metric tracking
39+
query := fmt.Sprintf("-- name: GetAuthorizedTemplates :many\n%s", filter)
40+
rows, err := q.db.QueryContext(ctx, query,
41+
arg.Deleted,
42+
arg.OrganizationID,
43+
arg.ExactName,
44+
pq.Array(arg.IDs),
45+
)
46+
if err != nil {
47+
return nil, err
48+
}
49+
defer rows.Close()
50+
var items []Template
51+
for rows.Next() {
52+
var i Template
53+
if err := rows.Scan(
54+
&i.ID,
55+
&i.CreatedAt,
56+
&i.UpdatedAt,
57+
&i.OrganizationID,
58+
&i.Deleted,
59+
&i.Name,
60+
&i.Provisioner,
61+
&i.ActiveVersionID,
62+
&i.Description,
63+
&i.DefaultTTL,
64+
&i.CreatedBy,
65+
&i.Icon,
66+
&i.UserACL,
67+
&i.GroupACL,
68+
&i.DisplayName,
69+
); err != nil {
70+
return nil, err
71+
}
72+
items = append(items, i)
73+
}
74+
if err := rows.Close(); err != nil {
75+
return nil, err
76+
}
77+
if err := rows.Err(); err != nil {
78+
return nil, err
79+
}
80+
return items, nil
81+
}
82+
3083
type TemplateUser struct {
3184
User
3285
Actions Actions `db:"actions"`
@@ -119,14 +172,14 @@ type workspaceQuerier interface {
119172
// This code is copied from `GetWorkspaces` and adds the authorized filter WHERE
120173
// clause.
121174
func (q *sqlQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg GetWorkspacesParams, prepared rbac.PreparedAuthorized) ([]GetWorkspacesRow, error) {
122-
authorizedFilter, err := prepared.Compile(rbac.ConfigWithoutACL())
175+
authorizedFilter, err := prepared.CompileToSQL(rbac.ConfigWithoutACL())
123176
if err != nil {
124177
return nil, xerrors.Errorf("compile authorized filter: %w", err)
125178
}
126179

127180
// In order to properly use ORDER BY, OFFSET, and LIMIT, we need to inject the
128181
// authorizedFilter between the end of the where clause and those statements.
129-
filter := strings.Replace(getWorkspaces, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter.SQLString()), 1)
182+
filter := strings.Replace(getWorkspaces, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter), 1)
130183
// The name comment is for metric tracking
131184
query := fmt.Sprintf("-- name: GetAuthorizedWorkspaces :many\n%s", filter)
132185
rows, err := q.db.QueryContext(ctx, query,
@@ -179,12 +232,12 @@ type userQuerier interface {
179232
}
180233

181234
func (q *sqlQuerier) GetAuthorizedUserCount(ctx context.Context, arg GetFilteredUserCountParams, prepared rbac.PreparedAuthorized) (int64, error) {
182-
authorizedFilter, err := prepared.Compile(rbac.ConfigWithoutACL())
235+
authorizedFilter, err := prepared.CompileToSQL(rbac.ConfigWithoutACL())
183236
if err != nil {
184237
return -1, xerrors.Errorf("compile authorized filter: %w", err)
185238
}
186239

187-
filter := strings.Replace(getFilteredUserCount, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter.SQLString()), 1)
240+
filter := strings.Replace(getFilteredUserCount, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter), 1)
188241
query := fmt.Sprintf("-- name: GetAuthorizedUserCount :one\n%s", filter)
189242
row := q.db.QueryRowContext(ctx, query,
190243
arg.Deleted,

‎coderd/database/queries.sql.go

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/templates.sql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ WHERE
3434
id = ANY(@ids)
3535
ELSE true
3636
END
37+
-- Authorize Filter clause will be injected below in GetAuthorizedTemplates
38+
-- @authorize_filter
3739
ORDER BY (name, id) ASC
3840
;
3941

‎coderd/rbac/authz.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ type Authorizer interface {
2121

2222
type PreparedAuthorized interface {
2323
Authorize(ctx context.Context, object Object) error
24-
Compile(cfg regosql.ConvertConfig) (AuthorizeFilter, error)
24+
CompileToSQL(cfg regosql.ConvertConfig) (string, error)
2525
}
2626

2727
// Filter takes in a list of objects, and will filter the list removing all

‎coderd/rbac/partial.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,12 @@ type PartialAuthorizer struct {
2929

3030
var _ PreparedAuthorized = (*PartialAuthorizer)(nil)
3131

32-
func (pa *PartialAuthorizer) Compile(cfg regosql.ConvertConfig) (AuthorizeFilter, error) {
32+
func (pa *PartialAuthorizer) CompileToSQL(cfg regosql.ConvertConfig) (string, error) {
3333
filter, err := Compile(cfg, pa)
3434
if err != nil {
35-
return nil, xerrors.Errorf("compile: %w", err)
35+
return "", xerrors.Errorf("compile: %w", err)
3636
}
37-
return filter, nil
37+
return filter.SQLString(), nil
3838
}
3939

4040
func (pa *PartialAuthorizer) Authorize(ctx context.Context, object Object) error {

‎coderd/rbac/query.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,6 @@ import (
1414

1515
type AuthorizeFilter interface {
1616
SQLString() string
17-
// Eval is required for the fake in memory database to work. The in memory
18-
// database can use this function to filter the results.
19-
Eval(object Object) bool
2017
}
2118

2219
type authorizedSQLFilter struct {

‎enterprise/coderd/groups.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ func (api *API) postGroupByOrganization(rw http.ResponseWriter, r *http.Request)
3131
)
3232
defer commitAudit()
3333

34-
if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceGroup) {
34+
if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceGroup.InOrg(org.ID)) {
3535
http.NotFound(rw, r)
3636
return
3737
}

0 commit comments

Comments
 (0)