opensource/coder
3
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2026-09-24 15:04:27 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: Suspended users cannot authenticate (#1849)

Browse Source 
* 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'
This commit is contained in:
Steven Masley
2022-05-31 08:06:42 -05:00
committed by GitHub
parent e02ef6f228
commit 26a2a169df
17 changed files with 197 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files
coderd/coderd.go
+2 -13
View File
@@ -82,8 +82,6 @@ func New(options *Options) *API {
apiKeyMiddleware := httpmw.ExtractAPIKey(options.Database, &httpmw.OAuth2Configs{
Github: options.GithubOAuth2Config,
})
// TODO: @emyrk we should just move this into 'ExtractAPIKey'.
authRolesMiddleware := httpmw.ExtractUserRoles(options.Database)
r.Use(
func(next http.Handler) http.Handler {
@@ -125,7 +123,6 @@ func New(options *Options) *API {
r.Route("/files", func(r chi.Router) {
r.Use(
apiKeyMiddleware,
authRolesMiddleware,
// This number is arbitrary, but reading/writing
// file content is expensive so it should be small.
httpmw.RateLimitPerMinute(12),
@@ -136,14 +133,12 @@ func New(options *Options) *API {
r.Route("/provisionerdaemons", func(r chi.Router) {
r.Use(
apiKeyMiddleware,
authRolesMiddleware,
)
r.Get("/", api.provisionerDaemons)
})
r.Route("/organizations", func(r chi.Router) {
r.Use(
apiKeyMiddleware,
authRolesMiddleware,
)
r.Post("/", api.postOrganizations)
r.Route("/{organization}", func(r chi.Router) {
@@ -179,7 +174,7 @@ func New(options *Options) *API {
})
})
r.Route("/parameters/{scope}/{id}", func(r chi.Router) {
r.Use(apiKeyMiddleware, authRolesMiddleware)
r.Use(apiKeyMiddleware)
r.Post("/", api.postParameter)
r.Get("/", api.parameters)
r.Route("/{name}", func(r chi.Router) {
@@ -189,7 +184,6 @@ func New(options *Options) *API {
r.Route("/templates/{template}", func(r chi.Router) {
r.Use(
apiKeyMiddleware,
authRolesMiddleware,
httpmw.ExtractTemplateParam(options.Database),
)
@@ -204,7 +198,6 @@ func New(options *Options) *API {
r.Route("/templateversions/{templateversion}", func(r chi.Router) {
r.Use(
apiKeyMiddleware,
authRolesMiddleware,
httpmw.ExtractTemplateVersionParam(options.Database),
)
@@ -229,7 +222,6 @@ func New(options *Options) *API {
r.Group(func(r chi.Router) {
r.Use(
apiKeyMiddleware,
authRolesMiddleware,
)
r.Post("/", api.postUser)
r.Get("/", api.users)
@@ -244,7 +236,7 @@ func New(options *Options) *API {
r.Put("/profile", api.putUserProfile)
r.Route("/status", func(r chi.Router) {
r.Put("/suspend", api.putUserStatus(database.UserStatusSuspended))
r.Put("/active", api.putUserStatus(database.UserStatusActive))
r.Put("/activate", api.putUserStatus(database.UserStatusActive))
})
r.Route("/password", func(r chi.Router) {
r.Put("/", api.putUserPassword)
@@ -292,7 +284,6 @@ func New(options *Options) *API {
r.Route("/workspaceresources/{workspaceresource}", func(r chi.Router) {
r.Use(
apiKeyMiddleware,
authRolesMiddleware,
httpmw.ExtractWorkspaceResourceParam(options.Database),
httpmw.ExtractWorkspaceParam(options.Database),
)
@@ -301,7 +292,6 @@ func New(options *Options) *API {
r.Route("/workspaces", func(r chi.Router) {
r.Use(
apiKeyMiddleware,
authRolesMiddleware,
)
r.Get("/", api.workspaces)
r.Route("/{workspace}", func(r chi.Router) {
@@ -327,7 +317,6 @@ func New(options *Options) *API {
r.Route("/workspacebuilds/{workspacebuild}", func(r chi.Router) {
r.Use(
apiKeyMiddleware,
authRolesMiddleware,
httpmw.ExtractWorkspaceBuildParam(options.Database),
httpmw.ExtractWorkspaceParam(options.Database),
)
coderd/database/databasefake/databasefake.go
+6 -3
View File
@@ -231,11 +231,13 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams
users = tmp
}
if params.Status != "" {
if len(params.Status) > 0 {
usersFilteredByStatus := make([]database.User, 0, len(users))
for i, user := range users {
if params.Status == string(user.Status) {
usersFilteredByStatus = append(usersFilteredByStatus, users[i])
for _, status := range params.Status {
if user.Status == status {
usersFilteredByStatus = append(usersFilteredByStatus, users[i])
}
}
}
users = usersFilteredByStatus
@@ -302,6 +304,7 @@ func (q *fakeQuerier) GetAllUserRoles(_ context.Context, userID uuid.UUID) (data
return database.GetAllUserRolesRow{
ID: userID,
Username: user.Username,
Status: user.Status,
Roles: roles,
}, nil
}
coderd/database/queries.sql.go
+25 -15
View File
@@ -2091,7 +2091,9 @@ func (q *sqlQuerier) UpdateTemplateVersionDescriptionByJobID(ctx context.Context
const getAllUserRoles = `-- name: GetAllUserRoles :one
SELECT
-- username is returned just to help for logging purposes
id, username, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles
-- status is used to enforce 'suspended' users, as all roles are ignored
-- when suspended.
id, username, status, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles
FROM
users
LEFT JOIN organization_members
@@ -2101,15 +2103,21 @@ WHERE
`
type GetAllUserRolesRow struct {
ID uuid.UUID `db:"id" json:"id"`
Username string `db:"username" json:"username"`
Roles []string `db:"roles" json:"roles"`
ID uuid.UUID `db:"id" json:"id"`
Username string `db:"username" json:"username"`
Status UserStatus `db:"status" json:"status"`
Roles []string `db:"roles" json:"roles"`
}
func (q *sqlQuerier) GetAllUserRoles(ctx context.Context, userID uuid.UUID) (GetAllUserRolesRow, error) {
row := q.db.QueryRowContext(ctx, getAllUserRoles, userID)
var i GetAllUserRolesRow
err := row.Scan(&i.ID, &i.Username, pq.Array(&i.Roles))
err := row.Scan(
&i.ID,
&i.Username,
&i.Status,
pq.Array(&i.Roles),
)
return i, err
}
@@ -2218,17 +2226,19 @@ WHERE
WHEN $2 :: text != '' THEN (
email LIKE concat('%', $2, '%')
OR username LIKE concat('%', $2, '%')
)
)
ELSE true
END
-- Filter by status
AND CASE
-- @status needs to be a text because it can be empty, If it was
-- user_status enum, it would not.
WHEN $3 :: text != '' THEN (
status = $3 :: user_status
WHEN cardinality($3 :: user_status[]) > 0 THEN (
status = ANY($3 :: user_status[])
)
ELSE true
ELSE
-- Only show active by default
status = 'active'
END
-- End of filters
ORDER BY
@@ -2241,18 +2251,18 @@ LIMIT
`
type GetUsersParams struct {
AfterID uuid.UUID `db:"after_id" json:"after_id"`
Search string `db:"search" json:"search"`
Status string `db:"status" json:"status"`
OffsetOpt int32 `db:"offset_opt" json:"offset_opt"`
LimitOpt int32 `db:"limit_opt" json:"limit_opt"`
AfterID uuid.UUID `db:"after_id" json:"after_id"`
Search string `db:"search" json:"search"`
Status []UserStatus `db:"status" json:"status"`
OffsetOpt int32 `db:"offset_opt" json:"offset_opt"`
LimitOpt int32 `db:"limit_opt" json:"limit_opt"`
}
func (q *sqlQuerier) GetUsers(ctx context.Context, arg GetUsersParams) ([]User, error) {
rows, err := q.db.QueryContext(ctx, getUsers,
arg.AfterID,
arg.Search,
arg.Status,
pq.Array(arg.Status),
arg.OffsetOpt,
arg.LimitOpt,
)
coderd/database/queries/users.sql
+9 -5
View File
@@ -101,17 +101,19 @@ WHERE
WHEN @search :: text != '' THEN (
email LIKE concat('%', @search, '%')
OR username LIKE concat('%', @search, '%')
)
)
ELSE true
END
-- Filter by status
AND CASE
-- @status needs to be a text because it can be empty, If it was
-- user_status enum, it would not.
WHEN @status :: text != '' THEN (
status = @status :: user_status
WHEN cardinality(@status :: user_status[]) > 0 THEN (
status = ANY(@status :: user_status[])
)
ELSE true
ELSE
-- Only show active by default
status = 'active'
END
-- End of filters
ORDER BY
@@ -135,7 +137,9 @@ WHERE
-- name: GetAllUserRoles :one
SELECT
-- username is returned just to help for logging purposes
id, username, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles
-- status is used to enforce 'suspended' users, as all roles are ignored
-- when suspended.
id, username, status, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles
FROM
users
LEFT JOIN organization_members
coderd/httpmw/apikey.go
+21 -1
View File
@@ -175,7 +175,27 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
}
}
ctx := context.WithValue(r.Context(), apiKeyContextKey{}, key)
// If the key is valid, we also fetch the user roles and status.
// The roles are used for RBAC authorize checks, and the status
// is to block 'suspended' users from accessing the platform.
roles, err := db.GetAllUserRoles(r.Context(), key.UserID)
if err != nil {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: "roles not found",
})
return
}
if roles.Status != database.UserStatusActive {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: fmt.Sprintf("user is not active (status = %q), contact an admin to reactivate your account", roles.Status),
})
return
}
ctx := r.Context()
ctx = context.WithValue(ctx, apiKeyContextKey{}, key)
ctx = context.WithValue(ctx, userRolesKey{}, roles)
next.ServeHTTP(rw, r.WithContext(ctx))
})
}
coderd/httpmw/apikey_test.go
+31
View File
@@ -9,6 +9,7 @@ import (
"testing"
"time"
"github.com/google/uuid"
"github.com/stretchr/testify/require"
"golang.org/x/oauth2"
@@ -128,6 +129,7 @@ func TestAPIKey(t *testing.T) {
id, secret = randomAPIKeyParts()
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: httpmw.SessionTokenKey,
@@ -139,6 +141,7 @@ func TestAPIKey(t *testing.T) {
_, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
ID: id,
HashedSecret: hashed[:],
UserID: user.ID,
})
require.NoError(t, err)
httpmw.ExtractAPIKey(db, nil)(successHandler).ServeHTTP(rw, r)
@@ -155,6 +158,7 @@ func TestAPIKey(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: httpmw.SessionTokenKey,
@@ -164,6 +168,7 @@ func TestAPIKey(t *testing.T) {
_, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
ID: id,
HashedSecret: hashed[:],
UserID: user.ID,
})
require.NoError(t, err)
httpmw.ExtractAPIKey(db, nil)(successHandler).ServeHTTP(rw, r)
@@ -180,6 +185,7 @@ func TestAPIKey(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: httpmw.SessionTokenKey,
@@ -190,6 +196,7 @@ func TestAPIKey(t *testing.T) {
ID: id,
HashedSecret: hashed[:],
ExpiresAt: database.Now().AddDate(0, 0, 1),
UserID: user.ID,
})
require.NoError(t, err)
httpmw.ExtractAPIKey(db, nil)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
@@ -217,6 +224,7 @@ func TestAPIKey(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
q := r.URL.Query()
q.Add(httpmw.SessionTokenKey, fmt.Sprintf("%s-%s", id, secret))
@@ -226,6 +234,7 @@ func TestAPIKey(t *testing.T) {
ID: id,
HashedSecret: hashed[:],
ExpiresAt: database.Now().AddDate(0, 0, 1),
UserID: user.ID,
})
require.NoError(t, err)
httpmw.ExtractAPIKey(db, nil)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
@@ -248,6 +257,7 @@ func TestAPIKey(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: httpmw.SessionTokenKey,
@@ -259,6 +269,7 @@ func TestAPIKey(t *testing.T) {
HashedSecret: hashed[:],
LastUsed: database.Now().AddDate(0, 0, -1),
ExpiresAt: database.Now().AddDate(0, 0, 1),
UserID: user.ID,
})
require.NoError(t, err)
httpmw.ExtractAPIKey(db, nil)(successHandler).ServeHTTP(rw, r)
@@ -281,6 +292,7 @@ func TestAPIKey(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: httpmw.SessionTokenKey,
@@ -292,6 +304,7 @@ func TestAPIKey(t *testing.T) {
HashedSecret: hashed[:],
LastUsed: database.Now(),
ExpiresAt: database.Now().Add(time.Minute),
UserID: user.ID,
})
require.NoError(t, err)
httpmw.ExtractAPIKey(db, nil)(successHandler).ServeHTTP(rw, r)
@@ -314,6 +327,7 @@ func TestAPIKey(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: httpmw.SessionTokenKey,
@@ -326,6 +340,7 @@ func TestAPIKey(t *testing.T) {
LoginType: database.LoginTypeGithub,
LastUsed: database.Now(),
ExpiresAt: database.Now().AddDate(0, 0, 1),
UserID: user.ID,
})
require.NoError(t, err)
httpmw.ExtractAPIKey(db, nil)(successHandler).ServeHTTP(rw, r)
@@ -348,6 +363,7 @@ func TestAPIKey(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: httpmw.SessionTokenKey,
@@ -360,6 +376,7 @@ func TestAPIKey(t *testing.T) {
LoginType: database.LoginTypeGithub,
LastUsed: database.Now(),
OAuthExpiry: database.Now().AddDate(0, 0, -1),
UserID: user.ID,
})
require.NoError(t, err)
token := &oauth2.Token{
@@ -387,6 +404,20 @@ func TestAPIKey(t *testing.T) {
})
}
func createUser(ctx context.Context, t *testing.T, db database.Store) database.User {
user, err := db.InsertUser(ctx, database.InsertUserParams{
ID: uuid.New(),
Email: "email@coder.com",
Username: "username",
HashedPassword: []byte{},
CreatedAt: time.Now(),
UpdatedAt: time.Now(),
RBACRoles: []string{},
})
require.NoError(t, err, "create user")
return user
}
type oauth2Config struct {
tokenSource oauth2TokenSource
}
coderd/httpmw/authorize_test.go
-1
View File
@@ -84,7 +84,6 @@ func TestExtractUserRoles(t *testing.T) {
)
rtr.Use(
httpmw.ExtractAPIKey(db, &httpmw.OAuth2Configs{}),
httpmw.ExtractUserRoles(db),
)
rtr.Get("/", func(_ http.ResponseWriter, r *http.Request) {
roles := httpmw.UserRoles(r)
coderd/users.go
+30 -4
View File
@@ -7,6 +7,7 @@ import (
"errors"
"fmt"
"net/http"
"strings"
"time"
"github.com/go-chi/chi/v5"
@@ -105,10 +106,27 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) {
func (api *API) users(rw http.ResponseWriter, r *http.Request) {
var (
searchName = r.URL.Query().Get("search")
statusFilter = r.URL.Query().Get("status")
searchName = r.URL.Query().Get("search")
statusFilters = r.URL.Query().Get("status")
)
statuses := make([]database.UserStatus, 0)
if statusFilters != "" {
// Split on commas if present to account for it being a list
for _, filter := range strings.Split(statusFilters, ",") {
switch database.UserStatus(filter) {
case database.UserStatusSuspended, database.UserStatusActive:
statuses = append(statuses, database.UserStatus(filter))
default:
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: fmt.Sprintf("%q is not a valid user status", filter),
})
return
}
}
}
// Reading all users across the site.
if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUser) {
return
@@ -124,7 +142,7 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) {
OffsetOpt: int32(paginationParams.Offset),
LimitOpt: int32(paginationParams.Limit),
Search: searchName,
Status: statusFilter,
Status: statuses,
})
if errors.Is(err, sql.ErrNoRows) {
httpapi.Write(rw, http.StatusOK, []codersdk.User{})
@@ -598,7 +616,15 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
// This message is the same as above to remove ease in detecting whether
// users are registered or not. Attackers still could with a timing attack.
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: "invalid email or password",
Message: "Incorrect email or password.",
})
return
}
// If the user logged into a suspended account, reject the login request.
if user.Status != database.UserStatusActive {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: "You are suspended, contact an admin to reactivate your account",
})
return
}
coderd/users_test.go
+29
View File
@@ -84,6 +84,35 @@ func TestPostLogin(t *testing.T) {
require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
})
t.Run("Suspended", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
first := coderdtest.CreateFirstUser(t, client)
member := coderdtest.CreateAnotherUser(t, client, first.OrganizationID)
memberUser, err := member.User(context.Background(), codersdk.Me)
require.NoError(t, err, "fetch member user")
_, err = client.UpdateUserStatus(context.Background(), memberUser.Username, codersdk.UserStatusSuspended)
require.NoError(t, err, "suspend member")
// Test an existing session
_, err = member.User(context.Background(), codersdk.Me)
var apiErr *codersdk.Error
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
require.Contains(t, apiErr.Message, "contact an admin")
// Test a new session
_, err = client.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{
Email: memberUser.Email,
Password: "testpass",
})
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
require.Contains(t, apiErr.Message, "suspended")
})
t.Run("Success", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)