Skip to content

Commit 26a2a16

Browse files
EmyrkEmyrk
authored
fix: Suspended users cannot authenticate (#1849)
* fix: Suspended users cannot authenticate - Merge roles and apikey extract httpmw - Add member account to make dev - feat: UI Shows suspended error logging into suspended account - change 'active' route to 'activate'
1 parent e02ef6f commit 26a2a16

File tree

17 files changed

+197
-56
lines changed

17 files changed

+197
-56
lines changed

‎coderd/coderd.go

+2-13
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,6 @@ func New(options *Options) *API {
8282
apiKeyMiddleware := httpmw.ExtractAPIKey(options.Database, &httpmw.OAuth2Configs{
8383
Github: options.GithubOAuth2Config,
8484
})
85-
// TODO: @emyrk we should just move this into 'ExtractAPIKey'.
86-
authRolesMiddleware := httpmw.ExtractUserRoles(options.Database)
8785

8886
r.Use(
8987
func(next http.Handler) http.Handler {
@@ -125,7 +123,6 @@ func New(options *Options) *API {
125123
r.Route("/files", func(r chi.Router) {
126124
r.Use(
127125
apiKeyMiddleware,
128-
authRolesMiddleware,
129126
// This number is arbitrary, but reading/writing
130127
// file content is expensive so it should be small.
131128
httpmw.RateLimitPerMinute(12),
@@ -136,14 +133,12 @@ func New(options *Options) *API {
136133
r.Route("/provisionerdaemons", func(r chi.Router) {
137134
r.Use(
138135
apiKeyMiddleware,
139-
authRolesMiddleware,
140136
)
141137
r.Get("/", api.provisionerDaemons)
142138
})
143139
r.Route("/organizations", func(r chi.Router) {
144140
r.Use(
145141
apiKeyMiddleware,
146-
authRolesMiddleware,
147142
)
148143
r.Post("/", api.postOrganizations)
149144
r.Route("/{organization}", func(r chi.Router) {
@@ -179,7 +174,7 @@ func New(options *Options) *API {
179174
})
180175
})
181176
r.Route("/parameters/{scope}/{id}", func(r chi.Router) {
182-
r.Use(apiKeyMiddleware, authRolesMiddleware)
177+
r.Use(apiKeyMiddleware)
183178
r.Post("/", api.postParameter)
184179
r.Get("/", api.parameters)
185180
r.Route("/{name}", func(r chi.Router) {
@@ -189,7 +184,6 @@ func New(options *Options) *API {
189184
r.Route("/templates/{template}", func(r chi.Router) {
190185
r.Use(
191186
apiKeyMiddleware,
192-
authRolesMiddleware,
193187
httpmw.ExtractTemplateParam(options.Database),
194188
)
195189

@@ -204,7 +198,6 @@ func New(options *Options) *API {
204198
r.Route("/templateversions/{templateversion}", func(r chi.Router) {
205199
r.Use(
206200
apiKeyMiddleware,
207-
authRolesMiddleware,
208201
httpmw.ExtractTemplateVersionParam(options.Database),
209202
)
210203

@@ -229,7 +222,6 @@ func New(options *Options) *API {
229222
r.Group(func(r chi.Router) {
230223
r.Use(
231224
apiKeyMiddleware,
232-
authRolesMiddleware,
233225
)
234226
r.Post("/", api.postUser)
235227
r.Get("/", api.users)
@@ -244,7 +236,7 @@ func New(options *Options) *API {
244236
r.Put("/profile", api.putUserProfile)
245237
r.Route("/status", func(r chi.Router) {
246238
r.Put("/suspend", api.putUserStatus(database.UserStatusSuspended))
247-
r.Put("/active", api.putUserStatus(database.UserStatusActive))
239+
r.Put("/activate", api.putUserStatus(database.UserStatusActive))
248240
})
249241
r.Route("/password", func(r chi.Router) {
250242
r.Put("/", api.putUserPassword)
@@ -292,7 +284,6 @@ func New(options *Options) *API {
292284
r.Route("/workspaceresources/{workspaceresource}", func(r chi.Router) {
293285
r.Use(
294286
apiKeyMiddleware,
295-
authRolesMiddleware,
296287
httpmw.ExtractWorkspaceResourceParam(options.Database),
297288
httpmw.ExtractWorkspaceParam(options.Database),
298289
)
@@ -301,7 +292,6 @@ func New(options *Options) *API {
301292
r.Route("/workspaces", func(r chi.Router) {
302293
r.Use(
303294
apiKeyMiddleware,
304-
authRolesMiddleware,
305295
)
306296
r.Get("/", api.workspaces)
307297
r.Route("/{workspace}", func(r chi.Router) {
@@ -327,7 +317,6 @@ func New(options *Options) *API {
327317
r.Route("/workspacebuilds/{workspacebuild}", func(r chi.Router) {
328318
r.Use(
329319
apiKeyMiddleware,
330-
authRolesMiddleware,
331320
httpmw.ExtractWorkspaceBuildParam(options.Database),
332321
httpmw.ExtractWorkspaceParam(options.Database),
333322
)

‎coderd/database/databasefake/databasefake.go

+6-3
Original file line numberDiff line numberDiff line change
@@ -231,11 +231,13 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams
231231
users = tmp
232232
}
233233

234-
if params.Status != "" {
234+
if len(params.Status) > 0 {
235235
usersFilteredByStatus := make([]database.User, 0, len(users))
236236
for i, user := range users {
237-
if params.Status == string(user.Status) {
238-
usersFilteredByStatus = append(usersFilteredByStatus, users[i])
237+
for _, status := range params.Status {
238+
if user.Status == status {
239+
usersFilteredByStatus = append(usersFilteredByStatus, users[i])
240+
}
239241
}
240242
}
241243
users = usersFilteredByStatus
@@ -302,6 +304,7 @@ func (q *fakeQuerier) GetAllUserRoles(_ context.Context, userID uuid.UUID) (data
302304
return database.GetAllUserRolesRow{
303305
ID: userID,
304306
Username: user.Username,
307+
Status: user.Status,
305308
Roles: roles,
306309
}, nil
307310
}

‎coderd/database/queries.sql.go

+25-15
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/users.sql

+9-5
Original file line numberDiff line numberDiff line change
@@ -101,17 +101,19 @@ WHERE
101101
WHEN @search :: text != '' THEN (
102102
email LIKE concat('%', @search, '%')
103103
OR username LIKE concat('%', @search, '%')
104-
)
104+
)
105105
ELSE true
106106
END
107107
-- Filter by status
108108
AND CASE
109109
-- @status needs to be a text because it can be empty, If it was
110110
-- user_status enum, it would not.
111-
WHEN @status :: text != '' THEN (
112-
status = @status :: user_status
111+
WHEN cardinality(@status :: user_status[]) > 0 THEN (
112+
status = ANY(@status :: user_status[])
113113
)
114-
ELSE true
114+
ELSE
115+
-- Only show active by default
116+
status = 'active'
115117
END
116118
-- End of filters
117119
ORDER BY
@@ -135,7 +137,9 @@ WHERE
135137
-- name: GetAllUserRoles :one
136138
SELECT
137139
-- username is returned just to help for logging purposes
138-
id, username, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles
140+
-- status is used to enforce 'suspended' users, as all roles are ignored
141+
-- when suspended.
142+
id, username, status, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles
139143
FROM
140144
users
141145
LEFT JOIN organization_members

‎coderd/httpmw/apikey.go

+21-1
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,27 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
175175
}
176176
}
177177

178-
ctx := context.WithValue(r.Context(), apiKeyContextKey{}, key)
178+
// If the key is valid, we also fetch the user roles and status.
179+
// The roles are used for RBAC authorize checks, and the status
180+
// is to block 'suspended' users from accessing the platform.
181+
roles, err := db.GetAllUserRoles(r.Context(), key.UserID)
182+
if err != nil {
183+
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
184+
Message: "roles not found",
185+
})
186+
return
187+
}
188+
189+
if roles.Status != database.UserStatusActive {
190+
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
191+
Message: fmt.Sprintf("user is not active (status = %q), contact an admin to reactivate your account", roles.Status),
192+
})
193+
return
194+
}
195+
196+
ctx := r.Context()
197+
ctx = context.WithValue(ctx, apiKeyContextKey{}, key)
198+
ctx = context.WithValue(ctx, userRolesKey{}, roles)
179199
next.ServeHTTP(rw, r.WithContext(ctx))
180200
})
181201
}

0 commit comments

Comments
 (0)