Skip to content

Commit 9dbc6bf

Browse files
johnstcnjohnstcn
committed
Merge remote-tracking branch 'origin/authzquerier_layer' into authzquerier_layer
2 parents 4357a3c + 75747f5 commit 9dbc6bf

13 files changed

+160
-88
lines changed

‎coderd/authzquery/apikey.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,11 @@ import (
1010
)
1111

1212
func (q *AuthzQuerier) DeleteAPIKeyByID(ctx context.Context, id string) error {
13-
return authorizedDelete(q.authorizer, q.database.GetAPIKeyByID, q.database.DeleteAPIKeyByID)(ctx, id)
13+
return authorizedDelete(q.logger, q.authorizer, q.database.GetAPIKeyByID, q.database.DeleteAPIKeyByID)(ctx, id)
1414
}
1515

1616
func (q *AuthzQuerier) GetAPIKeyByID(ctx context.Context, id string) (database.APIKey, error) {
17-
return authorizedFetch(q.authorizer, q.database.GetAPIKeyByID)(ctx, id)
17+
return authorizedFetch(q.logger, q.authorizer, q.database.GetAPIKeyByID)(ctx, id)
1818
}
1919

2020
func (q *AuthzQuerier) GetAPIKeysByLoginType(ctx context.Context, loginType database.LoginType) ([]database.APIKey, error) {
@@ -26,7 +26,7 @@ func (q *AuthzQuerier) GetAPIKeysLastUsedAfter(ctx context.Context, lastUsed tim
2626
}
2727

2828
func (q *AuthzQuerier) InsertAPIKey(ctx context.Context, arg database.InsertAPIKeyParams) (database.APIKey, error) {
29-
return authorizedInsertWithReturn(q.authorizer,
29+
return authorizedInsertWithReturn(q.logger, q.authorizer,
3030
rbac.ActionRead,
3131
rbac.ResourceAPIKey.WithOwner(arg.UserID.String()),
3232
q.database.InsertAPIKey)(ctx, arg)
@@ -36,5 +36,5 @@ func (q *AuthzQuerier) UpdateAPIKeyByID(ctx context.Context, arg database.Update
3636
fetch := func(ctx context.Context, arg database.UpdateAPIKeyByIDParams) (database.APIKey, error) {
3737
return q.GetAPIKeyByID(ctx, arg.ID)
3838
}
39-
return authorizedUpdate(q.authorizer, fetch, q.database.UpdateAPIKeyByID)(ctx, arg)
39+
return authorizedUpdate(q.logger, q.authorizer, fetch, q.database.UpdateAPIKeyByID)(ctx, arg)
4040
}

‎coderd/authzquery/audit.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import (
88
)
99

1010
func (q *AuthzQuerier) InsertAuditLog(ctx context.Context, arg database.InsertAuditLogParams) (database.AuditLog, error) {
11-
return authorizedInsertWithReturn(q.authorizer, rbac.ActionCreate, rbac.ResourceAuditLog, q.database.InsertAuditLog)(ctx, arg)
11+
return authorizedInsertWithReturn(q.logger, q.authorizer, rbac.ActionCreate, rbac.ResourceAuditLog, q.database.InsertAuditLog)(ctx, arg)
1212
}
1313

1414
func (q *AuthzQuerier) GetAuditLogsOffset(ctx context.Context, arg database.GetAuditLogsOffsetParams) ([]database.GetAuditLogsOffsetRow, error) {

‎coderd/authzquery/authz.go

Lines changed: 54 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@ package authzquery
33
import (
44
"context"
55
"database/sql"
6+
"fmt"
7+
8+
"cdr.dev/slog"
69

710
"golang.org/x/xerrors"
811

@@ -17,20 +20,51 @@ var (
1720
// NoActorError wraps ErrNoRows for the api to return a 404. This is the correct
1821
// response when the user is not authorized.
1922
NoActorError = xerrors.Errorf("no authorization actor in context: %w", sql.ErrNoRows)
20-
// TODO: Log this error every time it occurs.
21-
NotAuthorizedError = xerrors.Errorf("unauthorized: %w", sql.ErrNoRows)
2223
)
2324

25+
// NotAuthorizedError is a sentinal error that unwraps to sql.ErrNoRows.
26+
// This allows the internal error to be read by the caller if needed. Otherwise
27+
// it will be handled as a 404.
28+
type NotAuthorizedError struct {
29+
Err error
30+
}
31+
32+
func (e NotAuthorizedError) Error() string {
33+
return fmt.Sprintf("unauthorized: %s", e.Err.Error())
34+
}
35+
36+
// Unwrap will always unwrap to a sql.ErrNoRows so the API returns a 404.
37+
// So 'errors.Is(err, sql.ErrNoRows)' will always be true.
38+
func (e NotAuthorizedError) Unwrap() error {
39+
return sql.ErrNoRows
40+
}
41+
42+
func LogNotAuthorizedError(ctx context.Context, logger slog.Logger, err error) error {
43+
// Only log the errors if it is an UnauthorizedError error.
44+
internalError := new(rbac.UnauthorizedError)
45+
if err != nil && xerrors.As(err, internalError) {
46+
logger.Debug(ctx, "unauthorized",
47+
slog.F("internal", internalError.Internal()),
48+
slog.F("input", internalError.Input()),
49+
slog.Error(err),
50+
)
51+
}
52+
return NotAuthorizedError{
53+
Err: err,
54+
}
55+
}
56+
2457
func authorizedInsert[ArgumentType any,
2558
Insert func(ctx context.Context, arg ArgumentType) error](
2659
// Arguments
60+
logger slog.Logger,
2761
authorizer rbac.Authorizer,
2862
action rbac.Action,
2963
object rbac.Objecter,
3064
insertFunc Insert) Insert {
3165

3266
return func(ctx context.Context, arg ArgumentType) error {
33-
_, err := authorizedInsertWithReturn(authorizer, action, object, func(ctx context.Context, arg ArgumentType) (rbac.Objecter, error) {
67+
_, err := authorizedInsertWithReturn(logger, authorizer, action, object, func(ctx context.Context, arg ArgumentType) (rbac.Objecter, error) {
3468
return rbac.Object{}, insertFunc(ctx, arg)
3569
})(ctx, arg)
3670
return err
@@ -40,6 +74,7 @@ func authorizedInsert[ArgumentType any,
4074
func authorizedInsertWithReturn[ObjectType any, ArgumentType any,
4175
Insert func(ctx context.Context, arg ArgumentType) (ObjectType, error)](
4276
// Arguments
77+
logger slog.Logger,
4378
authorizer rbac.Authorizer,
4479
action rbac.Action,
4580
object rbac.Objecter,
@@ -55,7 +90,7 @@ func authorizedInsertWithReturn[ObjectType any, ArgumentType any,
5590
// Authorize the action
5691
err = authorizer.Authorize(ctx, act, action, object.RBACObject())
5792
if err != nil {
58-
return empty, NotAuthorizedError
93+
return empty, LogNotAuthorizedError(ctx, logger, err)
5994
}
6095

6196
// Insert the database object
@@ -67,11 +102,12 @@ func authorizedDelete[ObjectType rbac.Objecter, ArgumentType any,
67102
Fetch func(ctx context.Context, arg ArgumentType) (ObjectType, error),
68103
Delete func(ctx context.Context, arg ArgumentType) error](
69104
// Arguments
105+
logger slog.Logger,
70106
authorizer rbac.Authorizer,
71107
fetchFunc Fetch,
72108
deleteFunc Delete) Delete {
73109

74-
return authorizedFetchAndExec(authorizer,
110+
return authorizedFetchAndExec(logger, authorizer,
75111
rbac.ActionDelete, fetchFunc, deleteFunc)
76112
}
77113

@@ -80,23 +116,25 @@ func authorizedUpdateWithReturn[ObjectType rbac.Objecter,
80116
Fetch func(ctx context.Context, arg ArgumentType) (ObjectType, error),
81117
UpdateQuery func(ctx context.Context, arg ArgumentType) (ObjectType, error)](
82118
// Arguments
119+
logger slog.Logger,
83120
authorizer rbac.Authorizer,
84121
fetchFunc Fetch,
85122
updateQuery UpdateQuery) UpdateQuery {
86123

87-
return authorizedFetchAndQuery(authorizer, rbac.ActionUpdate, fetchFunc, updateQuery)
124+
return authorizedFetchAndQuery(logger, authorizer, rbac.ActionUpdate, fetchFunc, updateQuery)
88125
}
89126

90127
func authorizedUpdate[ObjectType rbac.Objecter,
91128
ArgumentType any,
92129
Fetch func(ctx context.Context, arg ArgumentType) (ObjectType, error),
93130
Exec func(ctx context.Context, arg ArgumentType) error](
94131
// Arguments
132+
logger slog.Logger,
95133
authorizer rbac.Authorizer,
96134
fetchFunc Fetch,
97135
updateExec Exec) Exec {
98136

99-
return authorizedFetchAndExec(authorizer, rbac.ActionUpdate, fetchFunc, updateExec)
137+
return authorizedFetchAndExec(logger, authorizer, rbac.ActionUpdate, fetchFunc, updateExec)
100138
}
101139

102140
// authorizedFetchAndExecWithConverter uses authorizedFetchAndQueryWithConverter but
@@ -107,12 +145,13 @@ func authorizedFetchAndExec[ObjectType rbac.Objecter,
107145
Fetch func(ctx context.Context, arg ArgumentType) (ObjectType, error),
108146
Exec func(ctx context.Context, arg ArgumentType) error](
109147
// Arguments
148+
logger slog.Logger,
110149
authorizer rbac.Authorizer,
111150
action rbac.Action,
112151
fetchFunc Fetch,
113152
execFunc Exec) Exec {
114153

115-
f := authorizedFetchAndQuery(authorizer, action, fetchFunc, func(ctx context.Context, arg ArgumentType) (empty ObjectType, err error) {
154+
f := authorizedFetchAndQuery(logger, authorizer, action, fetchFunc, func(ctx context.Context, arg ArgumentType) (empty ObjectType, err error) {
116155
return empty, execFunc(ctx, arg)
117156
})
118157
return func(ctx context.Context, arg ArgumentType) error {
@@ -125,6 +164,7 @@ func authorizedFetchAndQuery[ObjectType rbac.Objecter, ArgumentType any,
125164
Fetch func(ctx context.Context, arg ArgumentType) (ObjectType, error),
126165
Query func(ctx context.Context, arg ArgumentType) (ObjectType, error)](
127166
// Arguments
167+
logger slog.Logger,
128168
authorizer rbac.Authorizer,
129169
action rbac.Action,
130170
fetchFunc Fetch,
@@ -146,7 +186,7 @@ func authorizedFetchAndQuery[ObjectType rbac.Objecter, ArgumentType any,
146186
// Authorize the action
147187
err = authorizer.Authorize(ctx, act, action, object.RBACObject())
148188
if err != nil {
149-
return empty, NotAuthorizedError
189+
return empty, LogNotAuthorizedError(ctx, logger, err)
150190
}
151191

152192
return queryFunc(ctx, arg)
@@ -156,10 +196,11 @@ func authorizedFetchAndQuery[ObjectType rbac.Objecter, ArgumentType any,
156196
func authorizedFetch[ObjectType rbac.Objecter, ArgumentType any,
157197
Fetch func(ctx context.Context, arg ArgumentType) (ObjectType, error)](
158198
// Arguments
199+
logger slog.Logger,
159200
authorizer rbac.Authorizer,
160201
fetchFunc Fetch) Fetch {
161202

162-
return authorizedQuery(authorizer, rbac.ActionRead, fetchFunc)
203+
return authorizedQuery(logger, authorizer, rbac.ActionRead, fetchFunc)
163204
}
164205

165206
// authorizedQuery is a generic function that wraps a database
@@ -175,6 +216,7 @@ func authorizedFetch[ObjectType rbac.Objecter, ArgumentType any,
175216
func authorizedQuery[ArgumentType any, ObjectType rbac.Objecter,
176217
DatabaseFunc func(ctx context.Context, arg ArgumentType) (ObjectType, error)](
177218
// Arguments
219+
logger slog.Logger,
178220
authorizer rbac.Authorizer,
179221
action rbac.Action,
180222
f DatabaseFunc) DatabaseFunc {
@@ -195,7 +237,7 @@ func authorizedQuery[ArgumentType any, ObjectType rbac.Objecter,
195237
// Authorize the action
196238
err = authorizer.Authorize(ctx, act, action, object.RBACObject())
197239
if err != nil {
198-
return empty, NotAuthorizedError
240+
return empty, LogNotAuthorizedError(ctx, logger, err)
199241
}
200242

201243
return object, nil
@@ -236,6 +278,7 @@ func authorizedFetchSet[ArgumentType any, ObjectType rbac.Objecter,
236278
// are predicated on the RBAC permissions of the related Template object.
237279
func authorizedQueryWithRelated[ObjectType any, ArgumentType any, Related rbac.Objecter](
238280
// Arguments
281+
logger slog.Logger,
239282
authorizer rbac.Authorizer,
240283
action rbac.Action,
241284
relatedFunc func(ObjectType, ArgumentType) (Related, error),

‎coderd/authzquery/authz_test.go

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,42 @@ package authzquery_test
22

33
import (
44
"context"
5+
"database/sql"
56
"reflect"
67
"testing"
78

8-
"cdr.dev/slog"
9+
"github.com/stretchr/testify/require"
10+
11+
"cdr.dev/slog/sloggers/slogtest"
12+
13+
"golang.org/x/xerrors"
914

1015
"github.com/google/uuid"
1116

17+
"cdr.dev/slog"
1218
"github.com/coder/coder/coderd/authzquery"
1319
"github.com/coder/coder/coderd/coderdtest"
1420
"github.com/coder/coder/coderd/database/databasefake"
1521
"github.com/coder/coder/coderd/rbac"
1622
)
1723

24+
func TestNotAuthorizedError(t *testing.T) {
25+
t.Parallel()
26+
27+
t.Run("Is404", func(t *testing.T) {
28+
t.Parallel()
29+
30+
testErr := xerrors.New("custom error")
31+
32+
err := authzquery.LogNotAuthorizedError(context.Background(), slogtest.Make(t, nil), testErr)
33+
require.ErrorIs(t, err, sql.ErrNoRows, "must be a sql.ErrNoRows")
34+
35+
var authErr authzquery.NotAuthorizedError
36+
require.ErrorAs(t, err, &authErr, "must be a NotAuthorizedError")
37+
require.ErrorIs(t, authErr.Err, testErr, "internal error must match")
38+
})
39+
}
40+
1841
// TestAuthzQueryRecursive is a simple test to search for infinite recursion
1942
// bugs. It isn't perfect, and only catches a subset of the possible bugs
2043
// as only the first db call will be made. But it is better than nothing.

‎coderd/authzquery/authzquerier.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@ import (
77

88
"cdr.dev/slog"
99

10-
"golang.org/x/xerrors"
11-
1210
"github.com/coder/coder/coderd/database"
1311
"github.com/coder/coder/coderd/rbac"
1412
)
@@ -58,12 +56,12 @@ func (q *AuthzQuerier) InTx(function func(querier database.Store) error, txOpts
5856
func (q *AuthzQuerier) authorizeContext(ctx context.Context, action rbac.Action, object rbac.Objecter) error {
5957
act, ok := ActorFromContext(ctx)
6058
if !ok {
61-
return xerrors.Errorf("no authorization actor in context")
59+
return NoActorError
6260
}
6361

6462
err := q.authorizer.Authorize(ctx, act, action, object.RBACObject())
6563
if err != nil {
66-
return xerrors.Errorf("unauthorized: %w", err)
64+
return LogNotAuthorizedError(ctx, q.logger, err)
6765
}
6866
return nil
6967
}

‎coderd/authzquery/file.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,13 @@ import (
1111
)
1212

1313
func (q *AuthzQuerier) GetFileByHashAndCreator(ctx context.Context, arg database.GetFileByHashAndCreatorParams) (database.File, error) {
14-
return authorizedFetch(q.authorizer, q.database.GetFileByHashAndCreator)(ctx, arg)
14+
return authorizedFetch(q.logger, q.authorizer, q.database.GetFileByHashAndCreator)(ctx, arg)
1515
}
1616

1717
func (q *AuthzQuerier) GetFileByID(ctx context.Context, id uuid.UUID) (database.File, error) {
18-
return authorizedFetch(q.authorizer, q.database.GetFileByID)(ctx, id)
18+
return authorizedFetch(q.logger, q.authorizer, q.database.GetFileByID)(ctx, id)
1919
}
2020

2121
func (q *AuthzQuerier) InsertFile(ctx context.Context, arg database.InsertFileParams) (database.File, error) {
22-
return authorizedInsertWithReturn(q.authorizer, rbac.ActionCreate, rbac.ResourceFile.WithOwner(arg.CreatedBy.String()), q.database.InsertFile)(ctx, arg)
22+
return authorizedInsertWithReturn(q.logger, q.authorizer, rbac.ActionCreate, rbac.ResourceFile.WithOwner(arg.CreatedBy.String()), q.database.InsertFile)(ctx, arg)
2323
}

‎coderd/authzquery/group.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,20 +11,20 @@ import (
1111
)
1212

1313
func (q *AuthzQuerier) DeleteGroupByID(ctx context.Context, id uuid.UUID) error {
14-
return authorizedDelete(q.authorizer, q.database.GetGroupByID, q.database.DeleteGroupByID)(ctx, id)
14+
return authorizedDelete(q.logger, q.authorizer, q.database.GetGroupByID, q.database.DeleteGroupByID)(ctx, id)
1515
}
1616

1717
func (q *AuthzQuerier) DeleteGroupMember(ctx context.Context, userID uuid.UUID) error {
1818
// Deleting a group member counts as updating a group.
19-
return authorizedUpdate(q.authorizer, q.database.GetGroupByID, q.database.DeleteGroupMember)(ctx, userID)
19+
return authorizedUpdate(q.logger, q.authorizer, q.database.GetGroupByID, q.database.DeleteGroupMember)(ctx, userID)
2020
}
2121

2222
func (q *AuthzQuerier) GetGroupByID(ctx context.Context, id uuid.UUID) (database.Group, error) {
23-
return authorizedFetch(q.authorizer, q.database.GetGroupByID)(ctx, id)
23+
return authorizedFetch(q.logger, q.authorizer, q.database.GetGroupByID)(ctx, id)
2424
}
2525

2626
func (q *AuthzQuerier) GetGroupByOrgAndName(ctx context.Context, arg database.GetGroupByOrgAndNameParams) (database.Group, error) {
27-
return authorizedFetch(q.authorizer, q.database.GetGroupByOrgAndName)(ctx, arg)
27+
return authorizedFetch(q.logger, q.authorizer, q.database.GetGroupByOrgAndName)(ctx, arg)
2828
}
2929

3030
func (q *AuthzQuerier) GetGroupMembers(ctx context.Context, groupID uuid.UUID) ([]database.User, error) {
@@ -41,23 +41,23 @@ func (q *AuthzQuerier) GetGroupMembers(ctx context.Context, groupID uuid.UUID) (
4141

4242
func (q *AuthzQuerier) InsertAllUsersGroup(ctx context.Context, organizationID uuid.UUID) (database.Group, error) {
4343
// This method creates a new group.
44-
return authorizedInsertWithReturn(q.authorizer, rbac.ActionCreate, rbac.ResourceGroup.InOrg(organizationID), q.database.InsertAllUsersGroup)(ctx, organizationID)
44+
return authorizedInsertWithReturn(q.logger, q.authorizer, rbac.ActionCreate, rbac.ResourceGroup.InOrg(organizationID), q.database.InsertAllUsersGroup)(ctx, organizationID)
4545
}
4646

4747
func (q *AuthzQuerier) InsertGroup(ctx context.Context, arg database.InsertGroupParams) (database.Group, error) {
48-
return authorizedInsertWithReturn(q.authorizer, rbac.ActionCreate, rbac.ResourceGroup.InOrg(arg.OrganizationID), q.database.InsertGroup)(ctx, arg)
48+
return authorizedInsertWithReturn(q.logger, q.authorizer, rbac.ActionCreate, rbac.ResourceGroup.InOrg(arg.OrganizationID), q.database.InsertGroup)(ctx, arg)
4949
}
5050

5151
func (q *AuthzQuerier) InsertGroupMember(ctx context.Context, arg database.InsertGroupMemberParams) error {
5252
fetch := func(ctx context.Context, arg database.InsertGroupMemberParams) (database.Group, error) {
5353
return q.database.GetGroupByID(ctx, arg.GroupID)
5454
}
55-
return authorizedUpdate(q.authorizer, fetch, q.database.InsertGroupMember)(ctx, arg)
55+
return authorizedUpdate(q.logger, q.authorizer, fetch, q.database.InsertGroupMember)(ctx, arg)
5656
}
5757

5858
func (q *AuthzQuerier) UpdateGroupByID(ctx context.Context, arg database.UpdateGroupByIDParams) (database.Group, error) {
5959
fetch := func(ctx context.Context, arg database.UpdateGroupByIDParams) (database.Group, error) {
6060
return q.database.GetGroupByID(ctx, arg.ID)
6161
}
62-
return authorizedUpdateWithReturn(q.authorizer, fetch, q.database.UpdateGroupByID)(ctx, arg)
62+
return authorizedUpdateWithReturn(q.logger, q.authorizer, fetch, q.database.UpdateGroupByID)(ctx, arg)
6363
}

‎coderd/authzquery/license.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,23 +16,23 @@ func (q *AuthzQuerier) GetLicenses(ctx context.Context) ([]database.License, err
1616
}
1717

1818
func (q *AuthzQuerier) InsertLicense(ctx context.Context, arg database.InsertLicenseParams) (database.License, error) {
19-
return authorizedInsertWithReturn(q.authorizer, rbac.ActionCreate, rbac.ResourceLicense, q.database.InsertLicense)(ctx, arg)
19+
return authorizedInsertWithReturn(q.logger, q.authorizer, rbac.ActionCreate, rbac.ResourceLicense, q.database.InsertLicense)(ctx, arg)
2020
}
2121

2222
func (q *AuthzQuerier) InsertOrUpdateLogoURL(ctx context.Context, value string) error {
23-
return authorizedInsert(q.authorizer, rbac.ActionUpdate, rbac.ResourceDeploymentConfig, q.database.InsertOrUpdateLogoURL)(ctx, value)
23+
return authorizedInsert(q.logger, q.authorizer, rbac.ActionUpdate, rbac.ResourceDeploymentConfig, q.database.InsertOrUpdateLogoURL)(ctx, value)
2424
}
2525

2626
func (q *AuthzQuerier) InsertOrUpdateServiceBanner(ctx context.Context, value string) error {
27-
return authorizedInsert(q.authorizer, rbac.ActionUpdate, rbac.ResourceDeploymentConfig, q.database.InsertOrUpdateServiceBanner)(ctx, value)
27+
return authorizedInsert(q.logger, q.authorizer, rbac.ActionUpdate, rbac.ResourceDeploymentConfig, q.database.InsertOrUpdateServiceBanner)(ctx, value)
2828
}
2929

3030
func (q *AuthzQuerier) GetLicenseByID(ctx context.Context, id int32) (database.License, error) {
31-
return authorizedFetch(q.authorizer, q.database.GetLicenseByID)(ctx, id)
31+
return authorizedFetch(q.logger, q.authorizer, q.database.GetLicenseByID)(ctx, id)
3232
}
3333

3434
func (q *AuthzQuerier) DeleteLicense(ctx context.Context, id int32) (int32, error) {
35-
err := authorizedDelete(q.authorizer, q.database.GetLicenseByID, func(ctx context.Context, id int32) error {
35+
err := authorizedDelete(q.logger, q.authorizer, q.database.GetLicenseByID, func(ctx context.Context, id int32) error {
3636
_, err := q.database.DeleteLicense(ctx, id)
3737
return err
3838
})(ctx, id)

0 commit comments

Comments
 (0)