Skip to content

feat: add API key scopes and application_connect scope #4067

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Sep 19, 2022
Merged

feat: add API key scopes and application_connect scope #4067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f4c5190
feat: add apikey scopes
deansheather Sep 14, 2022
db015ef
devurls -> application_connect
deansheather Sep 15, 2022
ee3b3c9
chore: add rbac scope tests
deansheather Sep 15, 2022
9c06467
feat: add WorkspaceApplicationConnect permission
deansheather Sep 15, 2022
f125172
Merge branch 'main' into dean/token-scopes
deansheather Sep 16, 2022
6bd2398
fixup! Merge branch 'main' into dean/token-scopes
deansheather Sep 16, 2022
a87d6e1
chore: rename ApiKeyScope with sqlc and move migrations code
deansheather Sep 19, 2022
dee284f
Merge branch 'main' into dean/token-scopes
deansheather Sep 19, 2022
f7707a8
chore: migrations.MigrateUp -> migrations.Up
deansheather Sep 19, 2022
49059d1
chore: improve rbac scope tests
deansheather Sep 19, 2022
2b37954
fixup! chore: improve rbac scope tests
deansheather Sep 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cli/resetpassword.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/coder/coder/cli/cliflag"
"github.com/coder/coder/cli/cliui"
"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/database/migrations"
"github.com/coder/coder/coderd/userpassword"
)

Expand All @@ -35,7 +36,7 @@ func resetPassword() *cobra.Command {
return xerrors.Errorf("ping postgres: %w", err)
}

err = database.EnsureClean(sqlDB)
err = migrations.EnsureClean(sqlDB)
if err != nil {
return xerrors.Errorf("database needs migration: %w", err)
}
Expand Down
3 changes: 2 additions & 1 deletion cli/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import (
"github.com/coder/coder/coderd/autobuild/executor"
"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/database/databasefake"
"github.com/coder/coder/coderd/database/migrations"
"github.com/coder/coder/coderd/devtunnel"
"github.com/coder/coder/coderd/gitsshkey"
"github.com/coder/coder/coderd/prometheusmetrics"
Expand Down Expand Up @@ -430,7 +431,7 @@ func Server(newAPI func(*coderd.Options) *coderd.API) *cobra.Command {
if err != nil {
return xerrors.Errorf("ping postgres: %w", err)
}
err = database.MigrateUp(sqlDB)
err = migrations.Up(sqlDB)
if err != nil {
return xerrors.Errorf("migrate up: %w", err)
}
Expand Down
10 changes: 6 additions & 4 deletions coderd/authorize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@ import (
)

func AuthorizeFilter[O rbac.Objecter](h *HTTPAuthorizer, r *http.Request, action rbac.Action, objects []O) ([]O, error) {
roles := httpmw.AuthorizationUserRoles(r)
objects, err := rbac.Filter(r.Context(), h.Authorizer, roles.ID.String(), roles.Roles, action, objects)
roles := httpmw.UserAuthorization(r)
objects, err := rbac.Filter(r.Context(), h.Authorizer, roles.ID.String(), roles.Roles, roles.Scope.ToRBAC(), action, objects)
if err != nil {
// Log the error as Filter should not be erroring.
h.Logger.Error(r.Context(), "filter failed",
slog.Error(err),
slog.F("user_id", roles.ID),
slog.F("username", roles.Username),
slog.F("scope", roles.Scope),
slog.F("route", r.URL.Path),
slog.F("action", action),
)
Expand Down Expand Up @@ -55,8 +56,8 @@ func (api *API) Authorize(r *http.Request, action rbac.Action, object rbac.Objec
// return
// }
func (h *HTTPAuthorizer) Authorize(r *http.Request, action rbac.Action, object rbac.Objecter) bool {
roles := httpmw.AuthorizationUserRoles(r)
err := h.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object.RBACObject())
roles := httpmw.UserAuthorization(r)
err := h.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, roles.Scope.ToRBAC(), action, object.RBACObject())
if err != nil {
// Log the errors for debugging
internalError := new(rbac.UnauthorizedError)
Expand All @@ -70,6 +71,7 @@ func (h *HTTPAuthorizer) Authorize(r *http.Request, action rbac.Action, object r
slog.F("roles", roles.Roles),
slog.F("user_id", roles.ID),
slog.F("username", roles.Username),
slog.F("scope", roles.Scope),
slog.F("route", r.URL.Path),
slog.F("action", action),
slog.F("object", object),
Expand Down
18 changes: 13 additions & 5 deletions coderd/coderdtest/authtest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ func AGPLRoutes(a *AuthTester) (map[string]string, map[string]RouteCheck) {
// Some quick reused objects
workspaceRBACObj := rbac.ResourceWorkspace.InOrg(a.Organization.ID).WithOwner(a.Workspace.OwnerID.String())
workspaceExecObj := rbac.ResourceWorkspaceExecution.InOrg(a.Organization.ID).WithOwner(a.Workspace.OwnerID.String())
applicationConnectObj := rbac.ResourceWorkspaceApplicationConnect.InOrg(a.Organization.ID).WithOwner(a.Workspace.OwnerID.String())

// skipRoutes allows skipping routes from being checked.
skipRoutes := map[string]string{
"POST:/api/v2/users/logout": "Logging out deletes the API Key for other routes",
Expand Down Expand Up @@ -408,11 +410,11 @@ func AGPLRoutes(a *AuthTester) (map[string]string, map[string]RouteCheck) {

assertAllHTTPMethods("/%40{user}/{workspace_and_agent}/apps/{workspaceapp}/*", RouteCheck{
AssertAction: rbac.ActionCreate,
AssertObject: workspaceExecObj,
AssertObject: applicationConnectObj,
})
assertAllHTTPMethods("/@{user}/{workspace_and_agent}/apps/{workspaceapp}/*", RouteCheck{
AssertAction: rbac.ActionCreate,
AssertObject: workspaceExecObj,
AssertObject: applicationConnectObj,
})

return skipRoutes, assertRoute
Expand Down Expand Up @@ -518,6 +520,7 @@ func (a *AuthTester) Test(ctx context.Context, assertRoute map[string]RouteCheck
type authCall struct {
SubjectID string
Roles []string
Scope rbac.Scope
Action rbac.Action
Object rbac.Object
}
Expand All @@ -527,21 +530,25 @@ type recordingAuthorizer struct {
AlwaysReturn error
}

func (r *recordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error {
var _ rbac.Authorizer = (*recordingAuthorizer)(nil)

func (r *recordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, scope rbac.Scope, action rbac.Action, object rbac.Object) error {
r.Called = &authCall{
SubjectID: subjectID,
Roles: roleNames,
Scope: scope,
Action: action,
Object: object,
}
return r.AlwaysReturn
}

func (r *recordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) {
func (r *recordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, scope rbac.Scope, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) {
return &fakePreparedAuthorizer{
Original: r,
SubjectID: subjectID,
Roles: roles,
Scope: scope,
Action: action,
}, nil
}
Expand All @@ -554,9 +561,10 @@ type fakePreparedAuthorizer struct {
Original *recordingAuthorizer
SubjectID string
Roles []string
Scope rbac.Scope
Action rbac.Action
}

func (f *fakePreparedAuthorizer) Authorize(ctx context.Context, object rbac.Object) error {
return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Action, object)
return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Scope, f.Action, object)
}
1 change: 1 addition & 0 deletions coderd/database/databasefake/databasefake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1588,6 +1588,7 @@ func (q *fakeQuerier) InsertAPIKey(_ context.Context, arg database.InsertAPIKeyP
UpdatedAt: arg.UpdatedAt,
LastUsed: arg.LastUsed,
LoginType: arg.LoginType,
Scope: arg.Scope,
}
q.apiKeys = append(q.apiKeys, key)
return key, nil
Expand Down
19 changes: 18 additions & 1 deletion coderd/database/db_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@ package database_test

import (
"context"
"database/sql"
"testing"

"github.com/google/uuid"
"github.com/stretchr/testify/require"

"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/database/migrations"
"github.com/coder/coder/coderd/database/postgres"
)

func TestNestedInTx(t *testing.T) {
Expand All @@ -20,7 +23,7 @@ func TestNestedInTx(t *testing.T) {

uid := uuid.New()
sqlDB := testSQLDB(t)
err := database.MigrateUp(sqlDB)
err := migrations.Up(sqlDB)
require.NoError(t, err, "migrations")

db := database.New(sqlDB)
Expand Down Expand Up @@ -48,3 +51,17 @@ func TestNestedInTx(t *testing.T) {
require.NoError(t, err, "user exists")
require.Equal(t, uid, user.ID, "user id expected")
}

func testSQLDB(t testing.TB) *sql.DB {
t.Helper()

connection, closeFn, err := postgres.Open()
require.NoError(t, err)
t.Cleanup(closeFn)

db, err := sql.Open("postgres", connection)
require.NoError(t, err)
t.Cleanup(func() { _ = db.Close() })

return db
}
8 changes: 7 additions & 1 deletion coderd/database/dump.sql
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions coderd/database/gen/dump/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"path/filepath"
"runtime"

"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/database/migrations"
"github.com/coder/coder/coderd/database/postgres"
)

Expand All @@ -25,7 +25,7 @@ func main() {
panic(err)
}

err = database.MigrateUp(db)
err = migrations.Up(db)
if err != nil {
panic(err)
}
Expand Down
6 changes: 6 additions & 0 deletions coderd/database/migrations/000050_apikey_scope.down.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
-- Avoid "upgrading" devurl keys to fully fledged API keys.
DELETE FROM api_keys WHERE scope != 'all';

ALTER TABLE api_keys DROP COLUMN scope;

DROP TYPE api_key_scope;
6 changes: 6 additions & 0 deletions coderd/database/migrations/000050_apikey_scope.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
CREATE TYPE api_key_scope AS ENUM (
'all',
'application_connect'
);
Comment on lines +1 to +4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add custom scopes later, this cannot be an enum. Roles are not enums.

Your call, but it isn't really necessary to enforce at the db level.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can resolve this problem with migrations when the time comes. Technically, the only correct and future proof option is to use UUIDs and foreign keys to a roles table but we're not at that point yet.


ALTER TABLE api_keys ADD COLUMN scope api_key_scope NOT NULL DEFAULT 'all';
22 changes: 11 additions & 11 deletions coderd/database/migrate.go → coderd/database/migrations/migrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package database
package migrations

import (
"context"
Expand All @@ -14,12 +14,12 @@ import (
"golang.org/x/xerrors"
)

//go:embed migrations/*.sql
//go:embed *.sql
var migrations embed.FS

func migrateSetup(db *sql.DB) (source.Driver, *migrate.Migrate, error) {
func setup(db *sql.DB) (source.Driver, *migrate.Migrate, error) {
ctx := context.Background()
sourceDriver, err := iofs.New(migrations, "migrations")
sourceDriver, err := iofs.New(migrations, ".")
if err != nil {
return nil, nil, xerrors.Errorf("create iofs: %w", err)
}
Expand All @@ -45,9 +45,9 @@ func migrateSetup(db *sql.DB) (source.Driver, *migrate.Migrate, error) {
return sourceDriver, m, nil
}

// MigrateUp runs SQL migrations to ensure the database schema is up-to-date.
func MigrateUp(db *sql.DB) (retErr error) {
_, m, err := migrateSetup(db)
// Up runs SQL migrations to ensure the database schema is up-to-date.
func Up(db *sql.DB) (retErr error) {
_, m, err := setup(db)
if err != nil {
return xerrors.Errorf("migrate setup: %w", err)
}
Expand Down Expand Up @@ -76,9 +76,9 @@ func MigrateUp(db *sql.DB) (retErr error) {
return nil
}

// MigrateDown runs all down SQL migrations.
func MigrateDown(db *sql.DB) error {
_, m, err := migrateSetup(db)
// Down runs all down SQL migrations.
func Down(db *sql.DB) error {
_, m, err := setup(db)
if err != nil {
return xerrors.Errorf("migrate setup: %w", err)
}
Expand All @@ -100,7 +100,7 @@ func MigrateDown(db *sql.DB) error {
// applied, without making any changes to the database. If not, returns a
// non-nil error.
func EnsureClean(db *sql.DB) error {
sourceDriver, m, err := migrateSetup(db)
sourceDriver, m, err := setup(db)
if err != nil {
return xerrors.Errorf("migrate setup: %w", err)
}
Expand Down
18 changes: 9 additions & 9 deletions coderd/database/migrate_test.go → coderd/database/migrations/migrate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//go:build linux

package database_test
package migrations_test

import (
"database/sql"
Expand All @@ -12,7 +12,7 @@ import (
"github.com/stretchr/testify/require"
"go.uber.org/goleak"

"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/database/migrations"
"github.com/coder/coder/coderd/database/postgres"
)

Expand All @@ -33,7 +33,7 @@ func TestMigrate(t *testing.T) {

db := testSQLDB(t)

err := database.MigrateUp(db)
err := migrations.Up(db)
require.NoError(t, err)
})

Expand All @@ -42,10 +42,10 @@ func TestMigrate(t *testing.T) {

db := testSQLDB(t)

err := database.MigrateUp(db)
err := migrations.Up(db)
require.NoError(t, err)

err = database.MigrateUp(db)
err = migrations.Up(db)
require.NoError(t, err)
})

Expand All @@ -54,13 +54,13 @@ func TestMigrate(t *testing.T) {

db := testSQLDB(t)

err := database.MigrateUp(db)
err := migrations.Up(db)
require.NoError(t, err)

err = database.MigrateDown(db)
err = migrations.Down(db)
require.NoError(t, err)

err = database.MigrateUp(db)
err = migrations.Up(db)
require.NoError(t, err)
})
}
Expand Down Expand Up @@ -120,7 +120,7 @@ func TestCheckLatestVersion(t *testing.T) {
})
}

err := database.CheckLatestVersion(driver, tc.currentVersion)
err := migrations.CheckLatestVersion(driver, tc.currentVersion)
var errMessage string
if err != nil {
errMessage = err.Error()
Expand Down
15 changes: 15 additions & 0 deletions coderd/database/modelmethods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,17 @@ import (
"github.com/coder/coder/coderd/rbac"
)

func (s APIKeyScope) ToRBAC() rbac.Scope {
switch s {
case APIKeyScopeAll:
return rbac.ScopeAll
case APIKeyScopeApplicationConnect:
return rbac.ScopeApplicationConnect
default:
panic("developer error: unknown scope type " + string(s))
}
}

func (t Template) RBACObject() rbac.Object {
return rbac.ResourceTemplate.InOrg(t.OrganizationID)
}
Expand All @@ -21,6 +32,10 @@ func (w Workspace) ExecutionRBAC() rbac.Object {
return rbac.ResourceWorkspaceExecution.InOrg(w.OrganizationID).WithOwner(w.OwnerID.String())
}

func (w Workspace) ApplicationConnectRBAC() rbac.Object {
return rbac.ResourceWorkspaceApplicationConnect.InOrg(w.OrganizationID).WithOwner(w.OwnerID.String())
}

func (m OrganizationMember) RBACObject() rbac.Object {
return rbac.ResourceOrganizationMember.InOrg(m.OrganizationID)
}
Expand Down
Loading