From e4b1bb884cd03bae542ccea9c0ea9f8419cb3be2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 4 Mar 2024 10:30:39 -0600 Subject: [PATCH 01/12] chore: ensure default org always exists First user just joins the org created by the migration --- .../migrations/000198_ensure_default_org.down.sql | 1 + .../migrations/000198_ensure_default_org.up.sql | 15 +++++++++++++++ coderd/users.go | 13 +++++++++++-- 3 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 coderd/database/migrations/000198_ensure_default_org.down.sql create mode 100644 coderd/database/migrations/000198_ensure_default_org.up.sql diff --git a/coderd/database/migrations/000198_ensure_default_org.down.sql b/coderd/database/migrations/000198_ensure_default_org.down.sql new file mode 100644 index 0000000000000..ad2c1fbca0518 --- /dev/null +++ b/coderd/database/migrations/000198_ensure_default_org.down.sql @@ -0,0 +1 @@ +-- There is no down. If the org is created, just let it be. Deleting an org feels dangerous in a migration. diff --git a/coderd/database/migrations/000198_ensure_default_org.up.sql b/coderd/database/migrations/000198_ensure_default_org.up.sql new file mode 100644 index 0000000000000..8131ee8a09627 --- /dev/null +++ b/coderd/database/migrations/000198_ensure_default_org.up.sql @@ -0,0 +1,15 @@ +-- This ensures a default organization always exists. +INSERT INTO + organizations(id, name, description, created_at, updated_at, is_default) +SELECT + -- Avoid calling it "default" as we are reserving that word as a keyword to fetch + -- the default org regardless of the name. + gen_random_uuid(), + 'first-organization', + 'Builtin default organization.', + now(), + now(), + true +WHERE + -- Only insert if no organizations exist. + NOT EXISTS (SELECT * FROM organizations) diff --git a/coderd/users.go b/coderd/users.go index cbc9a75059210..ed1b09299c5e4 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -171,6 +171,15 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { } } + defaultOrg, err := api.Database.GetDefaultOrganization(ctx) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching default organization. If you are encountering this error, you will have to restart the Coder deployment.", + Detail: err.Error(), + }) + return + } + //nolint:gocritic // needed to create first user user, organizationID, err := api.CreateUser(dbauthz.AsSystemRestricted(ctx), api.Database, CreateUserRequest{ CreateUserRequest: codersdk.CreateUserRequest{ @@ -178,9 +187,9 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { Username: createUser.Username, Password: createUser.Password, // Create an org for the first user. - OrganizationID: uuid.Nil, + OrganizationID: defaultOrg.ID, }, - CreateOrganization: true, + CreateOrganization: false, LoginType: database.LoginTypePassword, }) if err != nil { From b4a4dd40d6ebda6e29803569bfae180dca855940 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 4 Mar 2024 10:40:23 -0600 Subject: [PATCH 02/12] fix ctx for inserting first user --- coderd/users.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/users.go b/coderd/users.go index ed1b09299c5e4..8b64ede6ff88e 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -171,7 +171,8 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { } } - defaultOrg, err := api.Database.GetDefaultOrganization(ctx) + //nolint:gocritic // needed to create first user + defaultOrg, err := api.Database.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx)) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching default organization. If you are encountering this error, you will have to restart the Coder deployment.", From 627c9f8e39fae150b637188072e5903eb8a14e7f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 4 Mar 2024 10:46:14 -0600 Subject: [PATCH 03/12] The system user could not read organizations I assumed this was the case before --- coderd/database/dbauthz/dbauthz.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 135703bb0ba71..2373321d46d0d 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -229,7 +229,7 @@ var ( rbac.ResourceGroup.Type: {rbac.ActionCreate, rbac.ActionUpdate}, rbac.ResourceRoleAssignment.Type: {rbac.ActionCreate, rbac.ActionDelete}, rbac.ResourceSystem.Type: {rbac.WildcardSymbol}, - rbac.ResourceOrganization.Type: {rbac.ActionCreate}, + rbac.ResourceOrganization.Type: {rbac.ActionCreate, rbac.ActionRead}, rbac.ResourceOrganizationMember.Type: {rbac.ActionCreate}, rbac.ResourceOrgRoleAssignment.Type: {rbac.ActionCreate}, rbac.ResourceProvisionerDaemon.Type: {rbac.ActionCreate, rbac.ActionUpdate}, From 179828ccc9444baa0bfb518b02d25b0a5f756964 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 4 Mar 2024 10:52:19 -0600 Subject: [PATCH 04/12] manual dbmem migration --- coderd/database/dbmem/dbmem.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 3e6b0e1d15ab4..c7ba3ea4133bf 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -77,6 +77,17 @@ func New() database.Store { locks: map[int64]struct{}{}, }, } + // Always start with a default org. Matching migration 198. + _, err := q.InsertOrganization(context.Background(), database.InsertOrganizationParams{ + ID: uuid.New(), + Name: "first-organization", + Description: "Builtin default organization.", + CreatedAt: dbtime.Now(), + UpdatedAt: dbtime.Now(), + }) + if err != nil { + panic(fmt.Errorf("failed to create default organization: %w", err)) + } q.defaultProxyDisplayName = "Default" q.defaultProxyIconURL = "/emojis/1f3e1.png" return q From db8da5a254b98af5dde770dff2afc02cac4d269d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 4 Mar 2024 11:50:59 -0600 Subject: [PATCH 05/12] Fix first org name check: --- cli/templatelist_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cli/templatelist_test.go b/cli/templatelist_test.go index 98796a3906b06..3ce91da91b75e 100644 --- a/cli/templatelist_test.go +++ b/cli/templatelist_test.go @@ -87,6 +87,10 @@ func TestTemplateList(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{}) owner := coderdtest.CreateFirstUser(t, client) + + org, err := client.Organization(context.Background(), owner.OrganizationID) + require.NoError(t, err) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) inv, root := clitest.New(t, "templates", "list") @@ -107,7 +111,7 @@ func TestTemplateList(t *testing.T) { require.NoError(t, <-errC) pty.ExpectMatch("No templates found in") - pty.ExpectMatch(coderdtest.FirstUserParams.Username) + pty.ExpectMatch(org.Name) pty.ExpectMatch("Create one:") }) } From b83062521021ab6bb4412d38dfb440b02c57c80b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 4 Mar 2024 12:13:33 -0600 Subject: [PATCH 06/12] correctly return implied roles --- coderd/users.go | 43 ++++++++++++++++++++++++++++++++++++------- coderd/users_test.go | 3 ++- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 8b64ede6ff88e..e49407205ae35 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1029,7 +1029,7 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { } resp := codersdk.UserRoles{ - Roles: user.RBACRoles, + Roles: make([]string, 0), OrganizationRoles: make(map[uuid.UUID][]string), } @@ -1042,10 +1042,40 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { return } + // We have to handle rbac permissions manually here. The 'GetAuthorizationUserRoles' does + // not have the ability to check permissions itself. + // The query 'GetAuthorizationUserRoles' handles implied roles, so it is + // the source of truth. + // nolint: gocritic // System is the only actor who can fetch these. + roles, err := api.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), user.ID) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching user's roles.", + Detail: err.Error(), + }) + return + } + + allowedOrgs := make(map[uuid.UUID]struct{}) for _, mem := range memberships { - // If we can read the org member, include the roles. - if err == nil { - resp.OrganizationRoles[mem.OrganizationID] = mem.Roles + allowedOrgs[mem.OrganizationID] = struct{}{} + } + + // Only return roles the user can read. + for _, role := range roles.Roles { + orgID, ok := rbac.IsOrgRole(role) + if !ok { + resp.Roles = append(resp.Roles, role) + continue + } + + uid, err := uuid.Parse(orgID) + if err != nil { + continue // This should never happen + } + if _, ok := allowedOrgs[uid]; ok { + resp.OrganizationRoles[uid] = append(resp.OrganizationRoles[uid], role) + continue } } @@ -1257,9 +1287,8 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create // TODO: When organizations are allowed to be created, we should // come back to determining the default role of the person who // creates the org. Until that happens, all users in an organization - // should be just regular members. - orgRoles = append(orgRoles, rbac.RoleOrgMember(req.OrganizationID)) - + // should be just regular members. Membership role is implied, and + // not required to be explicit. _, err = tx.InsertAllUsersGroup(ctx, organization.ID) if err != nil { return xerrors.Errorf("create %q group: %w", database.EveryoneGroup, err) diff --git a/coderd/users_test.go b/coderd/users_test.go index 1c962a50b945b..9cb8a09c4131a 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1091,11 +1091,12 @@ func TestInitialRoles(t *testing.T) { require.NoError(t, err) require.ElementsMatch(t, roles.Roles, []string{ rbac.RoleOwner(), + rbac.RoleMember(), }, "should be a member and admin") require.ElementsMatch(t, roles.OrganizationRoles[first.OrganizationID], []string{ rbac.RoleOrgMember(first.OrganizationID), - }, "should be a member and admin") + }, "should be a member") } func TestPutUserSuspend(t *testing.T) { From 6b086c6bc00e30a0866c12f1391f68c1c8b79ea2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 4 Mar 2024 12:16:03 -0600 Subject: [PATCH 07/12] fix authz tests --- coderd/database/dbauthz/dbauthz_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index b6fef0deca5fc..b34e170dc681d 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -568,7 +568,7 @@ func (s *MethodTestSuite) TestOrganization() { check.Args(o.ID).Asserts(o, rbac.ActionRead).Returns(o) })) s.Run("GetDefaultOrganization", s.Subtest(func(db database.Store, check *expects) { - o := dbgen.Organization(s.T(), db, database.Organization{}) + o, _ := db.GetDefaultOrganization(context.Background()) check.Args().Asserts(o, rbac.ActionRead).Returns(o) })) s.Run("GetOrganizationByName", s.Subtest(func(db database.Store, check *expects) { @@ -597,9 +597,10 @@ func (s *MethodTestSuite) TestOrganization() { check.Args(u.ID).Asserts(a, rbac.ActionRead, b, rbac.ActionRead).Returns(slice.New(a, b)) })) s.Run("GetOrganizations", s.Subtest(func(db database.Store, check *expects) { + def, _ := db.GetDefaultOrganization(context.Background()) a := dbgen.Organization(s.T(), db, database.Organization{}) b := dbgen.Organization(s.T(), db, database.Organization{}) - check.Args().Asserts(a, rbac.ActionRead, b, rbac.ActionRead).Returns(slice.New(a, b)) + check.Args().Asserts(def, rbac.ActionRead, a, rbac.ActionRead, b, rbac.ActionRead).Returns(slice.New(def, a, b)) })) s.Run("GetOrganizationsByUserID", s.Subtest(func(db database.Store, check *expects) { u := dbgen.User(s.T(), db, database.User{}) From 9997f59e0129beddea723822ccfe3a4ac6a511d8 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 4 Mar 2024 12:24:17 -0600 Subject: [PATCH 08/12] ensure everyone group on first user --- .../migrations/000198_ensure_default_org.up.sql | 3 ++- coderd/users.go | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/coderd/database/migrations/000198_ensure_default_org.up.sql b/coderd/database/migrations/000198_ensure_default_org.up.sql index 8131ee8a09627..4a773753fc0c3 100644 --- a/coderd/database/migrations/000198_ensure_default_org.up.sql +++ b/coderd/database/migrations/000198_ensure_default_org.up.sql @@ -12,4 +12,5 @@ SELECT true WHERE -- Only insert if no organizations exist. - NOT EXISTS (SELECT * FROM organizations) + NOT EXISTS (SELECT * FROM organizations); + diff --git a/coderd/users.go b/coderd/users.go index e49407205ae35..1c3ab1f761a6f 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -181,6 +181,18 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { return } + //nolint:gocritic // ensure everyone group + _, err = api.Database.InsertAllUsersGroup(dbauthz.AsSystemRestricted(ctx), defaultOrg.ID) + // A unique constraint violation just means the group already exists. + // This should not happen, but is ok if it does. + if err != nil && !database.IsUniqueViolation(err) { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error creating all users group.", + Detail: err.Error(), + }) + return + } + //nolint:gocritic // needed to create first user user, organizationID, err := api.CreateUser(dbauthz.AsSystemRestricted(ctx), api.Database, CreateUserRequest{ CreateUserRequest: codersdk.CreateUserRequest{ From 3977c71ff148c2ab671724791bc9b80279ab1a52 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 4 Mar 2024 14:32:30 -0600 Subject: [PATCH 09/12] fix old unit test, changed assumption --- coderd/database/querier_test.go | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 0f7b0cd95d7fb..ae64260db35b4 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -506,20 +506,11 @@ func TestDefaultOrg(t *testing.T) { db := database.New(sqlDB) ctx := context.Background() - // Should start with 0 orgs + // Should start with the default org all, err := db.GetOrganizations(ctx) require.NoError(t, err) - require.Len(t, all, 0) - - org, err := db.InsertOrganization(ctx, database.InsertOrganizationParams{ - ID: uuid.New(), - Name: "default", - Description: "", - CreatedAt: dbtime.Now(), - UpdatedAt: dbtime.Now(), - }) - require.NoError(t, err) - require.True(t, org.IsDefault, "first org should always be default") + require.Len(t, all, 1) + require.True(t, all[0].IsDefault, "first org should always be default") } type tvArgs struct { From d35933d4f07c4a4f94d59fdb41747c43bf2b389e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 5 Mar 2024 08:46:06 -0600 Subject: [PATCH 10/12] log critical on a 'should never happen' parsing org roles --- coderd/users.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 1c3ab1f761a6f..1461ce456be53 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -12,6 +12,7 @@ import ( "github.com/google/uuid" "golang.org/x/xerrors" + "cdr.dev/slog" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" @@ -196,10 +197,9 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { //nolint:gocritic // needed to create first user user, organizationID, err := api.CreateUser(dbauthz.AsSystemRestricted(ctx), api.Database, CreateUserRequest{ CreateUserRequest: codersdk.CreateUserRequest{ - Email: createUser.Email, - Username: createUser.Username, - Password: createUser.Password, - // Create an org for the first user. + Email: createUser.Email, + Username: createUser.Username, + Password: createUser.Password, OrganizationID: defaultOrg.ID, }, CreateOrganization: false, @@ -1083,6 +1083,11 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { uid, err := uuid.Parse(orgID) if err != nil { + api.Logger.Critical(r.Context(), "org role contains invalid uuid", + slog.F("username", user.Username), + slog.F("user_id", user.ID.String()), + slog.F("role", role), + ) continue // This should never happen } if _, ok := allowedOrgs[uid]; ok { From 1b8690f9c6bd6161782275dea703babb12c163c6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 5 Mar 2024 09:15:11 -0600 Subject: [PATCH 11/12] drop code that attempts to correct the implied roles --- coderd/users.go | 41 +---------------------------------------- 1 file changed, 1 insertion(+), 40 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 1461ce456be53..f8e63af5d1c3d 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -12,7 +12,6 @@ import ( "github.com/google/uuid" "golang.org/x/xerrors" - "cdr.dev/slog" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" @@ -1054,46 +1053,8 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { return } - // We have to handle rbac permissions manually here. The 'GetAuthorizationUserRoles' does - // not have the ability to check permissions itself. - // The query 'GetAuthorizationUserRoles' handles implied roles, so it is - // the source of truth. - // nolint: gocritic // System is the only actor who can fetch these. - roles, err := api.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), user.ID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching user's roles.", - Detail: err.Error(), - }) - return - } - - allowedOrgs := make(map[uuid.UUID]struct{}) for _, mem := range memberships { - allowedOrgs[mem.OrganizationID] = struct{}{} - } - - // Only return roles the user can read. - for _, role := range roles.Roles { - orgID, ok := rbac.IsOrgRole(role) - if !ok { - resp.Roles = append(resp.Roles, role) - continue - } - - uid, err := uuid.Parse(orgID) - if err != nil { - api.Logger.Critical(r.Context(), "org role contains invalid uuid", - slog.F("username", user.Username), - slog.F("user_id", user.ID.String()), - slog.F("role", role), - ) - continue // This should never happen - } - if _, ok := allowedOrgs[uid]; ok { - resp.OrganizationRoles[uid] = append(resp.OrganizationRoles[uid], role) - continue - } + resp.OrganizationRoles[mem.OrganizationID] = mem.Roles } httpapi.Write(ctx, rw, http.StatusOK, resp) From c9af8c0ab6c1936cfdcb90d7d333c38f735f5978 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 5 Mar 2024 09:18:01 -0600 Subject: [PATCH 12/12] Test 'TestInitialRoles' to not expect member role --- coderd/users.go | 2 +- coderd/users_test.go | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index f8e63af5d1c3d..efdd351b51fc1 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1040,7 +1040,7 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { } resp := codersdk.UserRoles{ - Roles: make([]string, 0), + Roles: user.RBACRoles, OrganizationRoles: make(map[uuid.UUID][]string), } diff --git a/coderd/users_test.go b/coderd/users_test.go index 9cb8a09c4131a..6ddbf089cd6a0 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1091,12 +1091,9 @@ func TestInitialRoles(t *testing.T) { require.NoError(t, err) require.ElementsMatch(t, roles.Roles, []string{ rbac.RoleOwner(), - rbac.RoleMember(), }, "should be a member and admin") - require.ElementsMatch(t, roles.OrganizationRoles[first.OrganizationID], []string{ - rbac.RoleOrgMember(first.OrganizationID), - }, "should be a member") + require.ElementsMatch(t, roles.OrganizationRoles[first.OrganizationID], []string{}, "should be a member") } func TestPutUserSuspend(t *testing.T) {