Skip to content

Commit 4848481

Browse files
EmyrkEmyrk
committed
Add sentinel errors for unauth authz errors
1 parent ef1deb5 commit 4848481

File tree

8 files changed

+34
-17
lines changed

8 files changed

+34
-17
lines changed

‎coderd/authzquery/authz.go

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package authzquery
22

33
import (
44
"context"
5+
"database/sql"
56

67
"golang.org/x/xerrors"
78

@@ -12,6 +13,14 @@ import (
1213
// - We need to handle authorizing the CRUD of objects with RBAC being related
1314
// to some other object. Eg: workspace builds, group members, etc.
1415

16+
var (
17+
// NoActorError wraps ErrNoRows for the api to return a 404. This is the correct
18+
// response when the user is not authorized.
19+
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)
22+
)
23+
1524
func authorizedInsert[ArgumentType any,
1625
Insert func(ctx context.Context, arg ArgumentType) error](
1726
// Arguments
@@ -40,13 +49,13 @@ func authorizedInsertWithReturn[ObjectType any, ArgumentType any,
4049
// Fetch the rbac subject
4150
act, ok := ActorFromContext(ctx)
4251
if !ok {
43-
return empty, xerrors.Errorf("no authorization actor in context")
52+
return empty, NoActorError
4453
}
4554

4655
// Authorize the action
4756
err = authorizer.Authorize(ctx, act, action, object.RBACObject())
4857
if err != nil {
49-
return empty, xerrors.Errorf("unauthorized: %w", err)
58+
return empty, NotAuthorizedError
5059
}
5160

5261
// Insert the database object
@@ -125,7 +134,7 @@ func authorizedFetchAndQuery[ObjectType rbac.Objecter, ArgumentType any,
125134
// Fetch the rbac subject
126135
act, ok := ActorFromContext(ctx)
127136
if !ok {
128-
return empty, xerrors.Errorf("no authorization actor in context")
137+
return empty, NoActorError
129138
}
130139

131140
// Fetch the database object
@@ -137,7 +146,7 @@ func authorizedFetchAndQuery[ObjectType rbac.Objecter, ArgumentType any,
137146
// Authorize the action
138147
err = authorizer.Authorize(ctx, act, action, object.RBACObject())
139148
if err != nil {
140-
return empty, xerrors.Errorf("unauthorized: %w", err)
149+
return empty, NotAuthorizedError
141150
}
142151

143152
return queryFunc(ctx, arg)
@@ -174,7 +183,7 @@ func authorizedQuery[ArgumentType any, ObjectType rbac.Objecter,
174183
// Fetch the rbac subject
175184
act, ok := ActorFromContext(ctx)
176185
if !ok {
177-
return empty, xerrors.Errorf("no authorization actor in context")
186+
return empty, NoActorError
178187
}
179188

180189
// Fetch the database object
@@ -186,7 +195,7 @@ func authorizedQuery[ArgumentType any, ObjectType rbac.Objecter,
186195
// Authorize the action
187196
err = authorizer.Authorize(ctx, act, action, object.RBACObject())
188197
if err != nil {
189-
return empty, xerrors.Errorf("unauthorized: %w", err)
198+
return empty, NotAuthorizedError
190199
}
191200

192201
return object, nil

‎coderd/authzquery/authz_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import (
55
"reflect"
66
"testing"
77

8+
"cdr.dev/slog"
9+
810
"github.com/google/uuid"
911

1012
"github.com/coder/coder/coderd/authzquery"
@@ -20,7 +22,7 @@ func TestAuthzQueryRecursive(t *testing.T) {
2022
t.Parallel()
2123
q := authzquery.NewAuthzQuerier(databasefake.New(), &coderdtest.RecordingAuthorizer{
2224
Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: nil},
23-
})
25+
}, slog.Make())
2426
actor := rbac.Subject{
2527
ID: uuid.NewString(),
2628
Roles: rbac.RoleNames{rbac.RoleOwner()},

‎coderd/authzquery/authzquerier.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import (
55
"database/sql"
66
"time"
77

8+
"cdr.dev/slog"
9+
810
"golang.org/x/xerrors"
911

1012
"github.com/coder/coder/coderd/database"
@@ -23,12 +25,14 @@ var _ database.Store = (*AuthzQuerier)(nil)
2325
type AuthzQuerier struct {
2426
database database.Store
2527
authorizer rbac.Authorizer
28+
logger slog.Logger
2629
}
2730

28-
func NewAuthzQuerier(db database.Store, authorizer rbac.Authorizer) *AuthzQuerier {
31+
func NewAuthzQuerier(db database.Store, authorizer rbac.Authorizer, logger slog.Logger) *AuthzQuerier {
2932
return &AuthzQuerier{
3033
database: db,
3134
authorizer: authorizer,
35+
logger: logger,
3236
}
3337
}
3438

@@ -45,7 +49,7 @@ func (q *AuthzQuerier) InTx(function func(querier database.Store) error, txOpts
4549
// TODO: @emyrk verify this works.
4650
return q.database.InTx(func(tx database.Store) error {
4751
// Wrap the transaction store in an AuthzQuerier.
48-
wrapped := NewAuthzQuerier(tx, q.authorizer)
52+
wrapped := NewAuthzQuerier(tx, q.authorizer, slog.Make())
4953
return function(wrapped)
5054
}, txOpts)
5155
}

‎coderd/authzquery/methods_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"strings"
99
"testing"
1010

11+
"cdr.dev/slog"
12+
1113
"github.com/google/uuid"
1214
"github.com/stretchr/testify/require"
1315
"github.com/stretchr/testify/suite"
@@ -91,7 +93,7 @@ func (s *MethodTestSuite) RunMethodTest(testCaseF func(t *testing.T, db database
9193
rec := &coderdtest.RecordingAuthorizer{
9294
Wrapped: &coderdtest.FakeAuthorizer{},
9395
}
94-
az := authzquery.NewAuthzQuerier(db, rec)
96+
az := authzquery.NewAuthzQuerier(db, rec, slog.Make())
9597
actor := rbac.Subject{
9698
ID: uuid.NewString(),
9799
Roles: rbac.RoleNames{},

‎coderd/coderd.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ func New(options *Options) *API {
199199
// TODO: remove this once we promote authz_querier out of experiments.
200200
if experiments.Enabled(codersdk.ExperimentAuthzQuerier) {
201201
if _, ok := (options.Database).(*authzquery.AuthzQuerier); !ok {
202-
options.Database = authzquery.NewAuthzQuerier(options.Database, options.Authorizer)
202+
options.Database = authzquery.NewAuthzQuerier(options.Database, options.Authorizer, options.Logger.Named("authz_querier"))
203203
}
204204
}
205205

‎coderd/coderdtest/coderdtest.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ func NewOptions(t *testing.T, options *Options) (func(http.Handler), context.Can
187187
Wrapped: rbac.NewAuthorizer(prometheus.NewRegistry()),
188188
}
189189
}
190-
options.Database = authzquery.NewAuthzQuerier(options.Database, options.Authorizer)
190+
options.Database = authzquery.NewAuthzQuerier(options.Database, options.Authorizer, slogtest.Make(t, nil).Leveled(slog.LevelDebug))
191191
}
192192
if options.DeploymentConfig == nil {
193193
options.DeploymentConfig = DeploymentConfig(t)

‎coderd/roles.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ func (api *API) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) {
4747
actorRoles := httpmw.UserAuthorization(r)
4848

4949
if !api.Authorize(r, rbac.ActionRead, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) {
50-
httpapi.Forbidden(rw)
50+
httpapi.ResourceNotFound(rw)
5151
return
5252
}
5353

‎coderd/roles_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ func TestListRoles(t *testing.T) {
3030
})
3131
require.NoError(t, err, "create org")
3232

33-
const forbidden = "Forbidden"
33+
const notFound = "Resource not found"
3434
testCases := []struct {
3535
Name string
3636
Client *codersdk.Client
@@ -66,7 +66,7 @@ func TestListRoles(t *testing.T) {
6666
APICall: func(ctx context.Context) ([]codersdk.AssignableRoles, error) {
6767
return member.ListOrganizationRoles(ctx, otherOrg.ID)
6868
},
69-
AuthorizedError: forbidden,
69+
AuthorizedError: notFound,
7070
},
7171
// Org admin
7272
{
@@ -95,7 +95,7 @@ func TestListRoles(t *testing.T) {
9595
APICall: func(ctx context.Context) ([]codersdk.AssignableRoles, error) {
9696
return orgAdmin.ListOrganizationRoles(ctx, otherOrg.ID)
9797
},
98-
AuthorizedError: forbidden,
98+
AuthorizedError: notFound,
9999
},
100100
// Admin
101101
{
@@ -133,7 +133,7 @@ func TestListRoles(t *testing.T) {
133133
if c.AuthorizedError != "" {
134134
var apiErr *codersdk.Error
135135
require.ErrorAs(t, err, &apiErr)
136-
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
136+
require.Equal(t, http.StatusNotFound, apiErr.StatusCode())
137137
require.Contains(t, apiErr.Message, c.AuthorizedError)
138138
} else {
139139
require.NoError(t, err)

0 commit comments

Comments
 (0)