chore: refactor user -> rbac.subject into a function (#13624)

* chore: refactor user subject logic to be in 1 place
* test: implement test to assert deleted custom roles are omitted
* add unit test for deleted role
This commit is contained in:
Steven Masley
2024-06-21 06:30:02 -10:00
committed by GitHub
parent 3ef12ac284
commit 0e933f0537
7 changed files with 328 additions and 94 deletions

View File

@ -406,8 +406,7 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
// 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.
//nolint:gocritic // system needs to update user roles
roles, err := cfg.DB.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), key.UserID)
actor, userStatus, err := UserRBACSubject(ctx, cfg.DB, key.UserID, rbac.ScopeName(key.Scope))
if err != nil {
return write(http.StatusUnauthorized, codersdk.Response{
Message: internalErrorMessage,
@ -415,7 +414,7 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
})
}
if roles.Status == database.UserStatusDormant {
if userStatus == database.UserStatusDormant {
// If coder confirms that the dormant user is valid, it can switch their account to active.
// nolint:gocritic
u, err := cfg.DB.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{
@ -429,42 +428,15 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
Detail: fmt.Sprintf("can't activate a dormant user: %s", err.Error()),
})
}
roles.Status = u.Status
userStatus = u.Status
}
if roles.Status != database.UserStatusActive {
if userStatus != database.UserStatusActive {
return write(http.StatusUnauthorized, codersdk.Response{
Message: fmt.Sprintf("User is not active (status = %q). Contact an admin to reactivate your account.", roles.Status),
Message: fmt.Sprintf("User is not active (status = %q). Contact an admin to reactivate your account.", userStatus),
})
}
roleNames, err := roles.RoleNames()
if err != nil {
return write(http.StatusInternalServerError, codersdk.Response{
Message: "Internal Server Error",
Detail: err.Error(),
})
}
//nolint:gocritic // Permission to lookup custom roles the user has assigned.
rbacRoles, err := rolestore.Expand(dbauthz.AsSystemRestricted(ctx), cfg.DB, roleNames)
if err != nil {
return write(http.StatusInternalServerError, codersdk.Response{
Message: "Failed to expand authenticated user roles",
Detail: err.Error(),
Validations: nil,
})
}
// Actor is the user's authorization context.
actor := rbac.Subject{
FriendlyName: roles.Username,
ID: key.UserID.String(),
Roles: rbacRoles,
Groups: roles.Groups,
Scope: rbac.ScopeName(key.Scope),
}.WithCachedASTValue()
if cfg.PostAuthAdditionalHeadersFunc != nil {
cfg.PostAuthAdditionalHeadersFunc(actor, rw.Header())
}
@ -472,6 +444,36 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
return key, &actor, true
}
// UserRBACSubject fetches a user's rbac.Subject from the database. It pulls all roles from both
// site and organization scopes. It also pulls the groups, and the user's status.
func UserRBACSubject(ctx context.Context, db database.Store, userID uuid.UUID, scope rbac.ExpandableScope) (rbac.Subject, database.UserStatus, error) {
//nolint:gocritic // system needs to update user roles
roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), userID)
if err != nil {
return rbac.Subject{}, "", xerrors.Errorf("get authorization user roles: %w", err)
}
roleNames, err := roles.RoleNames()
if err != nil {
return rbac.Subject{}, "", xerrors.Errorf("expand role names: %w", err)
}
//nolint:gocritic // Permission to lookup custom roles the user has assigned.
rbacRoles, err := rolestore.Expand(dbauthz.AsSystemRestricted(ctx), db, roleNames)
if err != nil {
return rbac.Subject{}, "", xerrors.Errorf("expand role names: %w", err)
}
actor := rbac.Subject{
FriendlyName: roles.Username,
ID: userID.String(),
Roles: rbacRoles,
Groups: roles.Groups,
Scope: scope,
}.WithCachedASTValue()
return actor, roles.Status, nil
}
// APITokenFromRequest returns the api token from the request.
// Find the session token from:
// 1: The cookie

View File

@ -14,16 +14,20 @@ import (
"testing"
"time"
"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/exp/slices"
"golang.org/x/oauth2"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbgen"
"github.com/coder/coder/v2/coderd/database/dbmem"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/coderd/httpapi"
"github.com/coder/coder/v2/coderd/httpmw"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/cryptorand"
"github.com/coder/coder/v2/testutil"
@ -38,6 +42,37 @@ func randomAPIKeyParts() (id string, secret string) {
func TestAPIKey(t *testing.T) {
t.Parallel()
// assertActorOk asserts all the properties of the user auth are ok.
assertActorOk := func(t *testing.T, r *http.Request) {
t.Helper()
actor, ok := dbauthz.ActorFromContext(r.Context())
assert.True(t, ok, "dbauthz actor ok")
if ok {
_, err := actor.Roles.Expand()
assert.NoError(t, err, "actor roles ok")
_, err = actor.Scope.Expand()
assert.NoError(t, err, "actor scope ok")
err = actor.RegoValueOk()
assert.NoError(t, err, "actor rego ok")
}
auth, ok := httpmw.UserAuthorizationOptional(r)
assert.True(t, ok, "httpmw auth ok")
if ok {
_, err := auth.Roles.Expand()
assert.NoError(t, err, "auth roles ok")
_, err = auth.Scope.Expand()
assert.NoError(t, err, "auth scope ok")
err = auth.RegoValueOk()
assert.NoError(t, err, "auth rego ok")
}
}
successHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
// Only called if the API key passes through the handler.
httpapi.Write(context.Background(), rw, http.StatusOK, codersdk.Response{
@ -256,6 +291,7 @@ func TestAPIKey(t *testing.T) {
})(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
// Checks that it exists on the context!
_ = httpmw.APIKey(r)
assertActorOk(t, r)
httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{
Message: "It worked!",
})
@ -296,6 +332,7 @@ func TestAPIKey(t *testing.T) {
// Checks that it exists on the context!
apiKey := httpmw.APIKey(r)
assert.Equal(t, database.APIKeyScopeApplicationConnect, apiKey.Scope)
assertActorOk(t, r)
httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{
Message: "it worked!",
@ -330,6 +367,8 @@ func TestAPIKey(t *testing.T) {
})(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
// Checks that it exists on the context!
_ = httpmw.APIKey(r)
assertActorOk(t, r)
httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{
Message: "It worked!",
})
@ -633,7 +672,7 @@ func TestAPIKey(t *testing.T) {
require.Equal(t, sentAPIKey.LoginType, gotAPIKey.LoginType)
})
t.Run("MissongConfig", func(t *testing.T) {
t.Run("MissingConfig", func(t *testing.T) {
t.Parallel()
var (
db = dbmem.New()
@ -667,4 +706,133 @@ func TestAPIKey(t *testing.T) {
out, _ := io.ReadAll(res.Body)
require.Contains(t, string(out), "Unable to refresh")
})
t.Run("CustomRoles", func(t *testing.T) {
t.Parallel()
var (
db = dbmem.New()
org = dbgen.Organization(t, db, database.Organization{})
customRole = dbgen.CustomRole(t, db, database.CustomRole{
Name: "custom-role",
OrgPermissions: []database.CustomRolePermission{},
OrganizationID: uuid.NullUUID{
UUID: org.ID,
Valid: true,
},
})
user = dbgen.User(t, db, database.User{
RBACRoles: []string{},
})
_ = dbgen.OrganizationMember(t, db, database.OrganizationMember{
UserID: user.ID,
OrganizationID: org.ID,
CreatedAt: time.Time{},
UpdatedAt: time.Time{},
Roles: []string{
rbac.RoleOrgAdmin(),
customRole.Name,
},
})
_, token = dbgen.APIKey(t, db, database.APIKey{
UserID: user.ID,
ExpiresAt: dbtime.Now().AddDate(0, 0, 1),
})
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
)
r.Header.Set(codersdk.SessionTokenHeader, token)
httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
DB: db,
RedirectToLogin: false,
})(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
assertActorOk(t, r)
auth := httpmw.UserAuthorization(r)
roles, err := auth.Roles.Expand()
assert.NoError(t, err, "expand user roles")
// Assert built in org role
assert.True(t, slices.ContainsFunc(roles, func(role rbac.Role) bool {
return role.Identifier.Name == rbac.RoleOrgAdmin() && role.Identifier.OrganizationID == org.ID
}), "org admin role")
// Assert custom role
assert.True(t, slices.ContainsFunc(roles, func(role rbac.Role) bool {
return role.Identifier.Name == customRole.Name && role.Identifier.OrganizationID == org.ID
}), "custom org role")
httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{
Message: "It worked!",
})
})).ServeHTTP(rw, r)
res := rw.Result()
defer res.Body.Close()
require.Equal(t, http.StatusOK, res.StatusCode)
})
// There is no sql foreign key constraint to require all assigned roles
// still exist in the database. We need to handle deleted roles.
t.Run("RoleNotExists", func(t *testing.T) {
t.Parallel()
var (
roleNotExistsName = "role-not-exists"
db = dbmem.New()
org = dbgen.Organization(t, db, database.Organization{})
user = dbgen.User(t, db, database.User{
RBACRoles: []string{
// Also provide an org not exists. In practice this makes no sense
// to store org roles in the user table, but there is no org to
// store it in. So just throw this here for even more unexpected
// behavior handling!
rbac.RoleIdentifier{Name: roleNotExistsName, OrganizationID: uuid.New()}.String(),
},
})
_ = dbgen.OrganizationMember(t, db, database.OrganizationMember{
UserID: user.ID,
OrganizationID: org.ID,
CreatedAt: time.Time{},
UpdatedAt: time.Time{},
Roles: []string{
rbac.RoleOrgAdmin(),
roleNotExistsName,
},
})
_, token = dbgen.APIKey(t, db, database.APIKey{
UserID: user.ID,
ExpiresAt: dbtime.Now().AddDate(0, 0, 1),
})
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
)
r.Header.Set(codersdk.SessionTokenHeader, token)
httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
DB: db,
RedirectToLogin: false,
})(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
assertActorOk(t, r)
auth := httpmw.UserAuthorization(r)
roles, err := auth.Roles.Expand()
assert.NoError(t, err, "expand user roles")
// Assert built in org role
assert.True(t, slices.ContainsFunc(roles, func(role rbac.Role) bool {
return role.Identifier.Name == rbac.RoleOrgAdmin() && role.Identifier.OrganizationID == org.ID
}), "org admin role")
// Assert the role-not-exists is not returned
assert.False(t, slices.ContainsFunc(roles, func(role rbac.Role) bool {
return role.Identifier.Name == roleNotExistsName
}), "role should not exist")
httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{
Message: "It worked!",
})
})).ServeHTTP(rw, r)
res := rw.Result()
defer res.Body.Close()
require.Equal(t, http.StatusOK, res.StatusCode)
})
}

View File

@ -209,27 +209,14 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database
}
// Grab the user roles so we can perform the exchange as the user.
//nolint:gocritic // In the token exchange, there is no user actor.
roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), dbCode.UserID)
actor, _, err := httpmw.UserRBACSubject(ctx, db, dbCode.UserID, rbac.ScopeAll)
if err != nil {
return oauth2.Token{}, err
}
roleNames, err := roles.RoleNames()
if err != nil {
return oauth2.Token{}, xerrors.Errorf("role names: %w", err)
}
userSubj := rbac.Subject{
ID: dbCode.UserID.String(),
Roles: rbac.RoleIdentifiers(roleNames),
Groups: roles.Groups,
Scope: rbac.ScopeAll,
return oauth2.Token{}, xerrors.Errorf("fetch user actor: %w", err)
}
// Do the actual token exchange in the database.
err = db.InTx(func(tx database.Store) error {
ctx := dbauthz.As(ctx, userSubj)
ctx := dbauthz.As(ctx, actor)
err = tx.DeleteOAuth2ProviderAppCodeByID(ctx, dbCode.ID)
if err != nil {
return xerrors.Errorf("delete oauth2 app code: %w", err)
@ -311,22 +298,10 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut
if err != nil {
return oauth2.Token{}, err
}
//nolint:gocritic // There is no user yet so we must use the system.
roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), prevKey.UserID)
if err != nil {
return oauth2.Token{}, err
}
roleNames, err := roles.RoleNames()
actor, _, err := httpmw.UserRBACSubject(ctx, db, prevKey.UserID, rbac.ScopeAll)
if err != nil {
return oauth2.Token{}, xerrors.Errorf("role names: %w", err)
}
userSubj := rbac.Subject{
ID: prevKey.UserID.String(),
Roles: rbac.RoleIdentifiers(roleNames),
Groups: roles.Groups,
Scope: rbac.ScopeAll,
return oauth2.Token{}, xerrors.Errorf("fetch user actor: %w", err)
}
// Generate a new refresh token.
@ -351,7 +326,7 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut
// Replace the token.
err = db.InTx(func(tx database.Store) error {
ctx := dbauthz.As(ctx, userSubj)
ctx := dbauthz.As(ctx, actor)
err = tx.DeleteAPIKeyByID(ctx, prevKey.ID) // This cascades to the token.
if err != nil {
return xerrors.Errorf("delete oauth2 app token: %w", err)

View File

@ -75,6 +75,17 @@ type Subject struct {
cachedASTValue ast.Value
}
// RegoValueOk is only used for unit testing. There is no easy way
// to get the error for the unexported method, and this is intentional.
// Failed rego values can default to the backup json marshal method,
// so errors are not fatal. Unit tests should be aware when the custom
// rego marshaller fails.
func (s Subject) RegoValueOk() error {
tmp := s
_, err := tmp.regoValue()
return err
}
// WithCachedASTValue can be called if the subject is static. This will compute
// the ast value once and cache it for future calls.
func (s Subject) WithCachedASTValue() Subject {

View File

@ -39,6 +39,8 @@ func roleCache(ctx context.Context) *syncmap.Map[string, rbac.Role] {
}
// Expand will expand built in roles, and fetch custom roles from the database.
// If a custom role is defined, but does not exist, the role will be omitted on
// the response. This means deleted roles are silently dropped.
func Expand(ctx context.Context, db database.Store, names []rbac.RoleIdentifier) (rbac.Roles, error) {
if len(names) == 0 {
// That was easy

View File

@ -231,7 +231,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
return
}
user, roles, ok := api.loginRequest(ctx, rw, loginWithPassword)
user, actor, ok := api.loginRequest(ctx, rw, loginWithPassword)
// 'user.ID' will be empty, or will be an actual value. Either is correct
// here.
aReq.UserID = user.ID
@ -240,21 +240,8 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
return
}
roleNames, err := roles.RoleNames()
if err != nil {
httpapi.InternalServerError(rw, err)
return
}
userSubj := rbac.Subject{
ID: user.ID.String(),
Roles: rbac.RoleIdentifiers(roleNames),
Groups: roles.Groups,
Scope: rbac.ScopeAll,
}
//nolint:gocritic // Creating the API key as the user instead of as system.
cookie, key, err := api.createAPIKey(dbauthz.As(ctx, userSubj), apikey.CreateParams{
cookie, key, err := api.createAPIKey(dbauthz.As(ctx, actor), apikey.CreateParams{
UserID: user.ID,
LoginType: database.LoginTypePassword,
RemoteAddr: r.RemoteAddr,
@ -284,7 +271,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
//
// The user struct is always returned, even if authentication failed. This is
// to support knowing what user attempted to login.
func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req codersdk.LoginWithPasswordRequest) (database.User, database.GetAuthorizationUserRolesRow, bool) {
func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req codersdk.LoginWithPasswordRequest) (database.User, rbac.Subject, bool) {
logger := api.Logger.Named(userAuthLoggerName)
//nolint:gocritic // In order to login, we need to get the user first!
@ -296,7 +283,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error.",
})
return user, database.GetAuthorizationUserRolesRow{}, false
return user, rbac.Subject{}, false
}
// If the user doesn't exist, it will be a default struct.
@ -306,7 +293,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error.",
})
return user, database.GetAuthorizationUserRolesRow{}, false
return user, rbac.Subject{}, false
}
if !equal {
@ -315,7 +302,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
Message: "Incorrect email or password.",
})
return user, database.GetAuthorizationUserRolesRow{}, false
return user, rbac.Subject{}, false
}
// If password authentication is disabled and the user does not have the
@ -324,14 +311,14 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
Message: "Password authentication is disabled.",
})
return user, database.GetAuthorizationUserRolesRow{}, false
return user, rbac.Subject{}, false
}
if user.LoginType != database.LoginTypePassword {
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
Message: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", database.LoginTypePassword, user.LoginType),
})
return user, database.GetAuthorizationUserRolesRow{}, false
return user, rbac.Subject{}, false
}
if user.Status == database.UserStatusDormant {
@ -346,29 +333,28 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error occurred. Try again later, or contact an admin for assistance.",
})
return user, database.GetAuthorizationUserRolesRow{}, false
return user, rbac.Subject{}, false
}
}
//nolint:gocritic // System needs to fetch user roles in order to login user.
roles, err := api.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), user.ID)
subject, userStatus, err := httpmw.UserRBACSubject(ctx, api.Database, user.ID, rbac.ScopeAll)
if err != nil {
logger.Error(ctx, "unable to fetch authorization user roles", slog.Error(err))
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error.",
})
return user, database.GetAuthorizationUserRolesRow{}, false
return user, rbac.Subject{}, false
}
// If the user logged into a suspended account, reject the login request.
if roles.Status != database.UserStatusActive {
if userStatus != database.UserStatusActive {
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
Message: fmt.Sprintf("Your account is %s. Contact an admin to reactivate your account.", roles.Status),
Message: fmt.Sprintf("Your account is %s. Contact an admin to reactivate your account.", userStatus),
})
return user, database.GetAuthorizationUserRolesRow{}, false
return user, rbac.Subject{}, false
}
return user, roles, true
return user, subject, true
}
// Clear the user's session cookie.