From 872f4a219042ad5c1f6f50f484684aae77f59dd6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 Feb 2024 10:40:13 -0600 Subject: [PATCH 1/5] fix: assign new oauth users to default org This is not a final solution, as we eventually want to be able to map to different orgs. This makes it so multi-org does not break oauth/oidc. --- coderd/apidoc/docs.go | 4 +++ coderd/apidoc/swagger.json | 5 ++- coderd/database/dbauthz/dbauthz.go | 6 ++++ coderd/database/dbauthz/dbauthz_test.go | 4 +++ coderd/database/dbmem/dbmem.go | 13 +++++++ coderd/database/dbmetrics/dbmetrics.go | 7 ++++ coderd/database/dbmock/dbmock.go | 15 ++++++++ coderd/database/dump.sql | 5 ++- coderd/database/models.go | 1 + coderd/database/querier.go | 1 + coderd/database/querier_test.go | 28 +++++++++++++++ coderd/database/queries.sql.go | 43 +++++++++++++++++++---- coderd/database/queries/organizations.sql | 15 ++++++-- coderd/database/unique_constraint.go | 1 + coderd/organizations.go | 1 + coderd/organizations_test.go | 8 +++++ coderd/userauth.go | 15 ++++---- coderd/users.go | 21 ++++++----- coderd/users_test.go | 13 ++++--- codersdk/organizations.go | 1 + docs/api/organizations.md | 2 ++ docs/api/schemas.md | 14 ++++---- docs/api/users.md | 3 ++ site/src/api/typesGenerated.ts | 1 + site/src/testHelpers/entities.ts | 1 + 25 files changed, 191 insertions(+), 37 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 412d11f2f80a7..ba528c74d6b46 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -10342,6 +10342,7 @@ const docTemplate = `{ "required": [ "created_at", "id", + "is_default", "name", "updated_at" ], @@ -10354,6 +10355,9 @@ const docTemplate = `{ "type": "string", "format": "uuid" }, + "is_default": { + "type": "boolean" + }, "name": { "type": "string" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 7b8457ae42042..d09381cf743d1 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -9296,7 +9296,7 @@ }, "codersdk.Organization": { "type": "object", - "required": ["created_at", "id", "name", "updated_at"], + "required": ["created_at", "id", "is_default", "name", "updated_at"], "properties": { "created_at": { "type": "string", @@ -9306,6 +9306,9 @@ "type": "string", "format": "uuid" }, + "is_default": { + "type": "boolean" + }, "name": { "type": "string" }, diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index a0da90eb52f23..a6c6b34f2dafa 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1016,6 +1016,12 @@ func (q *querier) GetDERPMeshKey(ctx context.Context) (string, error) { return q.db.GetDERPMeshKey(ctx) } +func (q *querier) GetDefaultOrganization(ctx context.Context) (database.Organization, error) { + return fetch(q.log, q.auth, func(ctx context.Context, _ any) (database.Organization, error) { + return q.db.GetDefaultOrganization(ctx) + })(ctx, nil) +} + func (q *querier) GetDefaultProxyConfig(ctx context.Context) (database.GetDefaultProxyConfigRow, error) { // No authz checks return q.db.GetDefaultProxyConfig(ctx) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 56b6012ba2193..c55b55a3d164d 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -570,6 +570,10 @@ func (s *MethodTestSuite) TestOrganization() { o := dbgen.Organization(s.T(), db, database.Organization{}) 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{}) + check.Args().Asserts(o, rbac.ActionRead).Returns(o) + })) s.Run("GetOrganizationByName", s.Subtest(func(db database.Store, check *expects) { o := dbgen.Organization(s.T(), db, database.Organization{}) check.Args(o.Name).Asserts(o, rbac.ActionRead).Returns(o) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 8901d13d3d4df..ae0a0d7e48d33 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -1657,6 +1657,18 @@ func (q *FakeQuerier) GetDERPMeshKey(_ context.Context) (string, error) { return q.derpMeshKey, nil } +func (q *FakeQuerier) GetDefaultOrganization(_ context.Context) (database.Organization, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + + for _, org := range q.organizations { + if org.IsDefault { + return org, nil + } + } + return database.Organization{}, sql.ErrNoRows +} + func (q *FakeQuerier) GetDefaultProxyConfig(_ context.Context) (database.GetDefaultProxyConfigRow, error) { return database.GetDefaultProxyConfigRow{ DisplayName: q.defaultProxyDisplayName, @@ -5285,6 +5297,7 @@ func (q *FakeQuerier) InsertOrganization(_ context.Context, arg database.InsertO Name: arg.Name, CreatedAt: arg.CreatedAt, UpdatedAt: arg.UpdatedAt, + IsDefault: len(q.organizations) == 0, } q.organizations = append(q.organizations, organization) return organization, nil diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 948de3c763277..b07b7b0305d9c 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -433,6 +433,13 @@ func (m metricsStore) GetDERPMeshKey(ctx context.Context) (string, error) { return key, err } +func (m metricsStore) GetDefaultOrganization(ctx context.Context) (database.Organization, error) { + start := time.Now() + r0, r1 := m.s.GetDefaultOrganization(ctx) + m.queryLatencies.WithLabelValues("GetDefaultOrganization").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m metricsStore) GetDefaultProxyConfig(ctx context.Context) (database.GetDefaultProxyConfigRow, error) { start := time.Now() resp, err := m.s.GetDefaultProxyConfig(ctx) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index d767fd7cf5bd7..cbe91468c2a6d 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -828,6 +828,21 @@ func (mr *MockStoreMockRecorder) GetDERPMeshKey(arg0 any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDERPMeshKey", reflect.TypeOf((*MockStore)(nil).GetDERPMeshKey), arg0) } +// GetDefaultOrganization mocks base method. +func (m *MockStore) GetDefaultOrganization(arg0 context.Context) (database.Organization, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetDefaultOrganization", arg0) + ret0, _ := ret[0].(database.Organization) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetDefaultOrganization indicates an expected call of GetDefaultOrganization. +func (mr *MockStoreMockRecorder) GetDefaultOrganization(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDefaultOrganization", reflect.TypeOf((*MockStore)(nil).GetDefaultOrganization), arg0) +} + // GetDefaultProxyConfig mocks base method. func (m *MockStore) GetDefaultProxyConfig(arg0 context.Context) (database.GetDefaultProxyConfigRow, error) { m.ctrl.T.Helper() diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index f2b410da5a198..c05a259faabde 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -502,7 +502,8 @@ CREATE TABLE organizations ( name text NOT NULL, description text NOT NULL, created_at timestamp with time zone NOT NULL, - updated_at timestamp with time zone NOT NULL + updated_at timestamp with time zone NOT NULL, + is_default boolean DEFAULT false NOT NULL ); CREATE TABLE parameter_schemas ( @@ -1506,6 +1507,8 @@ CREATE UNIQUE INDEX idx_users_email ON users USING btree (email) WHERE (deleted CREATE UNIQUE INDEX idx_users_username ON users USING btree (username) WHERE (deleted = false); +CREATE UNIQUE INDEX organizations_single_default_org ON organizations USING btree (is_default) WHERE (is_default = true); + CREATE INDEX provisioner_job_logs_id_job_id_idx ON provisioner_job_logs USING btree (job_id, id); CREATE INDEX provisioner_jobs_started_at_idx ON provisioner_jobs USING btree (started_at) WHERE (started_at IS NULL); diff --git a/coderd/database/models.go b/coderd/database/models.go index 50c4d0b5c02d3..7156d772a3c4d 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1823,6 +1823,7 @@ type Organization struct { Description string `db:"description" json:"description"` CreatedAt time.Time `db:"created_at" json:"created_at"` UpdatedAt time.Time `db:"updated_at" json:"updated_at"` + IsDefault bool `db:"is_default" json:"is_default"` } type OrganizationMember struct { diff --git a/coderd/database/querier.go b/coderd/database/querier.go index cbeb5b1caf746..4b459e3141216 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -102,6 +102,7 @@ type sqlcQuerier interface { GetAuthorizationUserRoles(ctx context.Context, userID uuid.UUID) (GetAuthorizationUserRolesRow, error) GetDBCryptKeys(ctx context.Context) ([]DBCryptKey, error) GetDERPMeshKey(ctx context.Context) (string, error) + GetDefaultOrganization(ctx context.Context) (Organization, error) GetDefaultProxyConfig(ctx context.Context) (GetDefaultProxyConfigRow, error) GetDeploymentDAUs(ctx context.Context, tzOffset int32) ([]GetDeploymentDAUsRow, error) GetDeploymentID(ctx context.Context) (string, error) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 4fd0579aff242..0f7b0cd95d7fb 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -494,6 +494,34 @@ func TestUserChangeLoginType(t *testing.T) { require.Equal(t, bobExpPass, bob.HashedPassword, "hashed password should not change") } +func TestDefaultOrg(t *testing.T) { + t.Parallel() + if testing.Short() { + t.SkipNow() + } + + sqlDB := testSQLDB(t) + err := migrations.Up(sqlDB) + require.NoError(t, err) + db := database.New(sqlDB) + ctx := context.Background() + + // Should start with 0 orgs + 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") +} + type tvArgs struct { Status database.ProvisionerJobStatus // CreateWorkspace is true if we should create a workspace for the template version diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 97b33038ea468..002da316cbccf 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3142,9 +3142,34 @@ func (q *sqlQuerier) UpdateMemberRoles(ctx context.Context, arg UpdateMemberRole return i, err } +const getDefaultOrganization = `-- name: GetDefaultOrganization :one +SELECT + id, name, description, created_at, updated_at, is_default +FROM + organizations +WHERE + is_default = true +LIMIT + 1 +` + +func (q *sqlQuerier) GetDefaultOrganization(ctx context.Context) (Organization, error) { + row := q.db.QueryRowContext(ctx, getDefaultOrganization) + var i Organization + err := row.Scan( + &i.ID, + &i.Name, + &i.Description, + &i.CreatedAt, + &i.UpdatedAt, + &i.IsDefault, + ) + return i, err +} + const getOrganizationByID = `-- name: GetOrganizationByID :one SELECT - id, name, description, created_at, updated_at + id, name, description, created_at, updated_at, is_default FROM organizations WHERE @@ -3160,13 +3185,14 @@ func (q *sqlQuerier) GetOrganizationByID(ctx context.Context, id uuid.UUID) (Org &i.Description, &i.CreatedAt, &i.UpdatedAt, + &i.IsDefault, ) return i, err } const getOrganizationByName = `-- name: GetOrganizationByName :one SELECT - id, name, description, created_at, updated_at + id, name, description, created_at, updated_at, is_default FROM organizations WHERE @@ -3184,13 +3210,14 @@ func (q *sqlQuerier) GetOrganizationByName(ctx context.Context, name string) (Or &i.Description, &i.CreatedAt, &i.UpdatedAt, + &i.IsDefault, ) return i, err } const getOrganizations = `-- name: GetOrganizations :many SELECT - id, name, description, created_at, updated_at + id, name, description, created_at, updated_at, is_default FROM organizations ` @@ -3210,6 +3237,7 @@ func (q *sqlQuerier) GetOrganizations(ctx context.Context) ([]Organization, erro &i.Description, &i.CreatedAt, &i.UpdatedAt, + &i.IsDefault, ); err != nil { return nil, err } @@ -3226,7 +3254,7 @@ func (q *sqlQuerier) GetOrganizations(ctx context.Context) ([]Organization, erro const getOrganizationsByUserID = `-- name: GetOrganizationsByUserID :many SELECT - id, name, description, created_at, updated_at + id, name, description, created_at, updated_at, is_default FROM organizations WHERE @@ -3255,6 +3283,7 @@ func (q *sqlQuerier) GetOrganizationsByUserID(ctx context.Context, userID uuid.U &i.Description, &i.CreatedAt, &i.UpdatedAt, + &i.IsDefault, ); err != nil { return nil, err } @@ -3271,9 +3300,10 @@ func (q *sqlQuerier) GetOrganizationsByUserID(ctx context.Context, userID uuid.U const insertOrganization = `-- name: InsertOrganization :one INSERT INTO - organizations (id, "name", description, created_at, updated_at) + organizations (id, "name", description, created_at, updated_at, is_default) VALUES - ($1, $2, $3, $4, $5) RETURNING id, name, description, created_at, updated_at + -- If no organizations exist, and this is the first, make it the default. + ($1, $2, $3, $4, $5, (SELECT TRUE FROM organizations LIMIT 1) IS NULL) RETURNING id, name, description, created_at, updated_at, is_default ` type InsertOrganizationParams struct { @@ -3299,6 +3329,7 @@ func (q *sqlQuerier) InsertOrganization(ctx context.Context, arg InsertOrganizat &i.Description, &i.CreatedAt, &i.UpdatedAt, + &i.IsDefault, ) return i, err } diff --git a/coderd/database/queries/organizations.sql b/coderd/database/queries/organizations.sql index 87c403049efd2..7f901b003bf44 100644 --- a/coderd/database/queries/organizations.sql +++ b/coderd/database/queries/organizations.sql @@ -1,3 +1,13 @@ +-- name: GetDefaultOrganization :one +SELECT + * +FROM + organizations +WHERE + is_default = true +LIMIT + 1; + -- name: GetOrganizations :many SELECT * @@ -39,6 +49,7 @@ WHERE -- name: InsertOrganization :one INSERT INTO - organizations (id, "name", description, created_at, updated_at) + organizations (id, "name", description, created_at, updated_at, is_default) VALUES - ($1, $2, $3, $4, $5) RETURNING *; + -- If no organizations exist, and this is the first, make it the default. + ($1, $2, $3, $4, $5, (SELECT TRUE FROM organizations LIMIT 1) IS NULL) RETURNING *; diff --git a/coderd/database/unique_constraint.go b/coderd/database/unique_constraint.go index 747aa3e07bb2f..fa1efffb8137c 100644 --- a/coderd/database/unique_constraint.go +++ b/coderd/database/unique_constraint.go @@ -74,6 +74,7 @@ const ( UniqueIndexProvisionerDaemonsNameOwnerKey UniqueConstraint = "idx_provisioner_daemons_name_owner_key" // CREATE UNIQUE INDEX idx_provisioner_daemons_name_owner_key ON provisioner_daemons USING btree (name, lower(COALESCE((tags ->> 'owner'::text), ''::text))); UniqueIndexUsersEmail UniqueConstraint = "idx_users_email" // CREATE UNIQUE INDEX idx_users_email ON users USING btree (email) WHERE (deleted = false); UniqueIndexUsersUsername UniqueConstraint = "idx_users_username" // CREATE UNIQUE INDEX idx_users_username ON users USING btree (username) WHERE (deleted = false); + UniqueOrganizationsSingleDefaultOrg UniqueConstraint = "organizations_single_default_org" // CREATE UNIQUE INDEX organizations_single_default_org ON organizations USING btree (is_default) WHERE (is_default = true); UniqueTemplatesOrganizationIDNameIndex UniqueConstraint = "templates_organization_id_name_idx" // CREATE UNIQUE INDEX templates_organization_id_name_idx ON templates USING btree (organization_id, lower((name)::text)) WHERE (deleted = false); UniqueUsersEmailLowerIndex UniqueConstraint = "users_email_lower_idx" // CREATE UNIQUE INDEX users_email_lower_idx ON users USING btree (lower(email)) WHERE (deleted = false); UniqueUsersUsernameLowerIndex UniqueConstraint = "users_username_lower_idx" // CREATE UNIQUE INDEX users_username_lower_idx ON users USING btree (lower(username)) WHERE (deleted = false); diff --git a/coderd/organizations.go b/coderd/organizations.go index ae24edea01597..d50c0a4e250cc 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -118,5 +118,6 @@ func convertOrganization(organization database.Organization) codersdk.Organizati Name: organization.Name, CreatedAt: organization.CreatedAt, UpdatedAt: organization.UpdatedAt, + IsDefault: organization.IsDefault, } } diff --git a/coderd/organizations_test.go b/coderd/organizations_test.go index c8cde696e22a2..6fc98d7c9add9 100644 --- a/coderd/organizations_test.go +++ b/coderd/organizations_test.go @@ -24,6 +24,14 @@ func TestOrganizationsByUser(t *testing.T) { require.NoError(t, err) require.NotNil(t, orgs) require.Len(t, orgs, 1) + require.True(t, orgs[0].IsDefault, "first org is always default") + + // Make an extra org, and it should not be defaulted. + notDefault, err := client.CreateOrganization(ctx, codersdk.CreateOrganizationRequest{ + Name: "another", + }) + require.NoError(t, err) + require.False(t, notDefault.IsDefault, "only 1 default org allowed") } func TestOrganizationByUserAndName(t *testing.T) { diff --git a/coderd/userauth.go b/coderd/userauth.go index dbb01f12e31ad..74bd77f28a54e 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1343,13 +1343,12 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C if user.ID == uuid.Nil { var organizationID uuid.UUID //nolint:gocritic - organizations, _ := tx.GetOrganizations(dbauthz.AsSystemRestricted(ctx)) - if len(organizations) > 0 { - // Add the user to the first organization. Once multi-organization - // support is added, we should enable a configuration map of user - // email to organization. - organizationID = organizations[0].ID - } + organization, _ := tx.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx)) + + // Add the user to the default organization. + // Once multi-organization we should check some configuration to see + // if we should add the user to a different organization. + organizationID = organization.ID //nolint:gocritic _, err := tx.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ @@ -1395,7 +1394,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C // All of the userauth tests depend on this being able to create // the first organization. It shouldn't be possible in normal // operation. - CreateOrganization: len(organizations) == 0, + CreateOrganization: organizationID == uuid.Nil, LoginType: params.LoginType, }) if err != nil { diff --git a/coderd/users.go b/coderd/users.go index ca757ed80436f..ba6587ecac5fc 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -401,10 +401,18 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { return } } else { - // If no organization is provided, add the user to the first - // organization. - organizations, err := api.Database.GetOrganizations(ctx) + // If no organization is provided, add the user to the default + defaultOrg, err := api.Database.GetDefaultOrganization(ctx) if err != nil { + if httpapi.Is404Error(err) { + httpapi.Write(ctx, rw, http.StatusNotFound, + codersdk.Response{ + Message: "Resource not found or you do not have access to this resource", + Detail: "Organization not found", + }, + ) + return + } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching orgs.", Detail: err.Error(), @@ -412,12 +420,7 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { return } - if len(organizations) > 0 { - // Add the user to the first organization. Once multi-organization - // support is added, we should enable a configuration map of user - // email to organization. - req.OrganizationID = organizations[0].ID - } + req.OrganizationID = defaultOrg.ID } var loginType database.LoginType diff --git a/coderd/users_test.go b/coderd/users_test.go index a9b4fa8de72b3..0d7f5a7bb21b9 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -493,21 +493,26 @@ func TestPostUsers(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) - numLogs := len(auditor.AuditLogs()) - firstUser := coderdtest.CreateFirstUser(t, client) - numLogs++ // add an audit log for user create - numLogs++ // add an audit log for login ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() + // Add an extra org to try and confuse user creation + _, err := client.CreateOrganization(ctx, codersdk.CreateOrganizationRequest{ + Name: "foobar", + }) + require.NoError(t, err) + + numLogs := len(auditor.AuditLogs()) + user, err := client.CreateUser(ctx, codersdk.CreateUserRequest{ Email: "another@user.org", Username: "someone-else", Password: "SomeSecurePassword!", }) require.NoError(t, err) + numLogs++ // add an audit log for user create require.Len(t, auditor.AuditLogs(), numLogs) require.Equal(t, database.AuditActionCreate, auditor.AuditLogs()[numLogs-1].Action) diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 6a629160207e8..8bf1ecc96811d 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -30,6 +30,7 @@ type Organization struct { Name string `json:"name" validate:"required"` CreatedAt time.Time `json:"created_at" validate:"required" format:"date-time"` UpdatedAt time.Time `json:"updated_at" validate:"required" format:"date-time"` + IsDefault bool `json:"is_default" validate:"required"` } type OrganizationMember struct { diff --git a/docs/api/organizations.md b/docs/api/organizations.md index 011d3cac5eb2e..478c8aba56648 100644 --- a/docs/api/organizations.md +++ b/docs/api/organizations.md @@ -123,6 +123,7 @@ curl -X POST http://coder-server:8080/api/v2/organizations \ { "created_at": "2019-08-24T14:15:22Z", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", + "is_default": true, "name": "string", "updated_at": "2019-08-24T14:15:22Z" } @@ -163,6 +164,7 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization} \ { "created_at": "2019-08-24T14:15:22Z", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", + "is_default": true, "name": "string", "updated_at": "2019-08-24T14:15:22Z" } diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 2ea43f292b546..be5724e361159 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -3811,6 +3811,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in { "created_at": "2019-08-24T14:15:22Z", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", + "is_default": true, "name": "string", "updated_at": "2019-08-24T14:15:22Z" } @@ -3818,12 +3819,13 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in ### Properties -| Name | Type | Required | Restrictions | Description | -| ------------ | ------ | -------- | ------------ | ----------- | -| `created_at` | string | true | | | -| `id` | string | true | | | -| `name` | string | true | | | -| `updated_at` | string | true | | | +| Name | Type | Required | Restrictions | Description | +| ------------ | ------- | -------- | ------------ | ----------- | +| `created_at` | string | true | | | +| `id` | string | true | | | +| `is_default` | boolean | true | | | +| `name` | string | true | | | +| `updated_at` | string | true | | | ## codersdk.OrganizationMember diff --git a/docs/api/users.md b/docs/api/users.md index e656ed6ac27a7..a057376b38f16 100644 --- a/docs/api/users.md +++ b/docs/api/users.md @@ -993,6 +993,7 @@ curl -X GET http://coder-server:8080/api/v2/users/{user}/organizations \ { "created_at": "2019-08-24T14:15:22Z", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", + "is_default": true, "name": "string", "updated_at": "2019-08-24T14:15:22Z" } @@ -1014,6 +1015,7 @@ Status Code **200** | `[array item]` | array | false | | | | `» created_at` | string(date-time) | true | | | | `» id` | string(uuid) | true | | | +| `» is_default` | boolean | true | | | | `» name` | string | true | | | | `» updated_at` | string(date-time) | true | | | @@ -1047,6 +1049,7 @@ curl -X GET http://coder-server:8080/api/v2/users/{user}/organizations/{organiza { "created_at": "2019-08-24T14:15:22Z", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", + "is_default": true, "name": "string", "updated_at": "2019-08-24T14:15:22Z" } diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index ac605e193f1f3..9c72b8c1c927a 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -768,6 +768,7 @@ export interface Organization { readonly name: string; readonly created_at: string; readonly updated_at: string; + readonly is_default: boolean; } // From codersdk/organizations.go diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index ce4f9d836aa0e..aa91756f725df 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -16,6 +16,7 @@ export const MockOrganization: TypesGen.Organization = { name: "Test Organization", created_at: "", updated_at: "", + is_default: true, }; export const MockTemplateDAUResponse: TypesGen.DAUsResponse = { From acda012a14165daacce89b1d8df0e38ba52d467c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 Feb 2024 13:32:33 -0600 Subject: [PATCH 2/5] Revert "fix: assign new oauth users to default org" This reverts commit 872f4a219042ad5c1f6f50f484684aae77f59dd6. --- coderd/apidoc/docs.go | 4 --- coderd/apidoc/swagger.json | 5 +-- coderd/database/dbauthz/dbauthz.go | 6 ---- coderd/database/dbauthz/dbauthz_test.go | 4 --- coderd/database/dbmem/dbmem.go | 13 ------- coderd/database/dbmetrics/dbmetrics.go | 7 ---- coderd/database/dbmock/dbmock.go | 15 -------- coderd/database/dump.sql | 5 +-- coderd/database/models.go | 1 - coderd/database/querier.go | 1 - coderd/database/querier_test.go | 28 --------------- coderd/database/queries.sql.go | 43 ++++------------------- coderd/database/queries/organizations.sql | 15 ++------ coderd/database/unique_constraint.go | 1 - coderd/organizations.go | 1 - coderd/organizations_test.go | 8 ----- coderd/userauth.go | 15 ++++---- coderd/users.go | 21 +++++------ coderd/users_test.go | 13 +++---- codersdk/organizations.go | 1 - docs/api/organizations.md | 2 -- docs/api/schemas.md | 14 ++++---- docs/api/users.md | 3 -- site/src/api/typesGenerated.ts | 1 - site/src/testHelpers/entities.ts | 1 - 25 files changed, 37 insertions(+), 191 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index ba528c74d6b46..412d11f2f80a7 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -10342,7 +10342,6 @@ const docTemplate = `{ "required": [ "created_at", "id", - "is_default", "name", "updated_at" ], @@ -10355,9 +10354,6 @@ const docTemplate = `{ "type": "string", "format": "uuid" }, - "is_default": { - "type": "boolean" - }, "name": { "type": "string" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index d09381cf743d1..7b8457ae42042 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -9296,7 +9296,7 @@ }, "codersdk.Organization": { "type": "object", - "required": ["created_at", "id", "is_default", "name", "updated_at"], + "required": ["created_at", "id", "name", "updated_at"], "properties": { "created_at": { "type": "string", @@ -9306,9 +9306,6 @@ "type": "string", "format": "uuid" }, - "is_default": { - "type": "boolean" - }, "name": { "type": "string" }, diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index a6c6b34f2dafa..a0da90eb52f23 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1016,12 +1016,6 @@ func (q *querier) GetDERPMeshKey(ctx context.Context) (string, error) { return q.db.GetDERPMeshKey(ctx) } -func (q *querier) GetDefaultOrganization(ctx context.Context) (database.Organization, error) { - return fetch(q.log, q.auth, func(ctx context.Context, _ any) (database.Organization, error) { - return q.db.GetDefaultOrganization(ctx) - })(ctx, nil) -} - func (q *querier) GetDefaultProxyConfig(ctx context.Context) (database.GetDefaultProxyConfigRow, error) { // No authz checks return q.db.GetDefaultProxyConfig(ctx) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index c55b55a3d164d..56b6012ba2193 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -570,10 +570,6 @@ func (s *MethodTestSuite) TestOrganization() { o := dbgen.Organization(s.T(), db, database.Organization{}) 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{}) - check.Args().Asserts(o, rbac.ActionRead).Returns(o) - })) s.Run("GetOrganizationByName", s.Subtest(func(db database.Store, check *expects) { o := dbgen.Organization(s.T(), db, database.Organization{}) check.Args(o.Name).Asserts(o, rbac.ActionRead).Returns(o) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index ae0a0d7e48d33..8901d13d3d4df 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -1657,18 +1657,6 @@ func (q *FakeQuerier) GetDERPMeshKey(_ context.Context) (string, error) { return q.derpMeshKey, nil } -func (q *FakeQuerier) GetDefaultOrganization(_ context.Context) (database.Organization, error) { - q.mutex.RLock() - defer q.mutex.RUnlock() - - for _, org := range q.organizations { - if org.IsDefault { - return org, nil - } - } - return database.Organization{}, sql.ErrNoRows -} - func (q *FakeQuerier) GetDefaultProxyConfig(_ context.Context) (database.GetDefaultProxyConfigRow, error) { return database.GetDefaultProxyConfigRow{ DisplayName: q.defaultProxyDisplayName, @@ -5297,7 +5285,6 @@ func (q *FakeQuerier) InsertOrganization(_ context.Context, arg database.InsertO Name: arg.Name, CreatedAt: arg.CreatedAt, UpdatedAt: arg.UpdatedAt, - IsDefault: len(q.organizations) == 0, } q.organizations = append(q.organizations, organization) return organization, nil diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index b07b7b0305d9c..948de3c763277 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -433,13 +433,6 @@ func (m metricsStore) GetDERPMeshKey(ctx context.Context) (string, error) { return key, err } -func (m metricsStore) GetDefaultOrganization(ctx context.Context) (database.Organization, error) { - start := time.Now() - r0, r1 := m.s.GetDefaultOrganization(ctx) - m.queryLatencies.WithLabelValues("GetDefaultOrganization").Observe(time.Since(start).Seconds()) - return r0, r1 -} - func (m metricsStore) GetDefaultProxyConfig(ctx context.Context) (database.GetDefaultProxyConfigRow, error) { start := time.Now() resp, err := m.s.GetDefaultProxyConfig(ctx) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index cbe91468c2a6d..d767fd7cf5bd7 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -828,21 +828,6 @@ func (mr *MockStoreMockRecorder) GetDERPMeshKey(arg0 any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDERPMeshKey", reflect.TypeOf((*MockStore)(nil).GetDERPMeshKey), arg0) } -// GetDefaultOrganization mocks base method. -func (m *MockStore) GetDefaultOrganization(arg0 context.Context) (database.Organization, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetDefaultOrganization", arg0) - ret0, _ := ret[0].(database.Organization) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetDefaultOrganization indicates an expected call of GetDefaultOrganization. -func (mr *MockStoreMockRecorder) GetDefaultOrganization(arg0 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDefaultOrganization", reflect.TypeOf((*MockStore)(nil).GetDefaultOrganization), arg0) -} - // GetDefaultProxyConfig mocks base method. func (m *MockStore) GetDefaultProxyConfig(arg0 context.Context) (database.GetDefaultProxyConfigRow, error) { m.ctrl.T.Helper() diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index c05a259faabde..f2b410da5a198 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -502,8 +502,7 @@ CREATE TABLE organizations ( name text NOT NULL, description text NOT NULL, created_at timestamp with time zone NOT NULL, - updated_at timestamp with time zone NOT NULL, - is_default boolean DEFAULT false NOT NULL + updated_at timestamp with time zone NOT NULL ); CREATE TABLE parameter_schemas ( @@ -1507,8 +1506,6 @@ CREATE UNIQUE INDEX idx_users_email ON users USING btree (email) WHERE (deleted CREATE UNIQUE INDEX idx_users_username ON users USING btree (username) WHERE (deleted = false); -CREATE UNIQUE INDEX organizations_single_default_org ON organizations USING btree (is_default) WHERE (is_default = true); - CREATE INDEX provisioner_job_logs_id_job_id_idx ON provisioner_job_logs USING btree (job_id, id); CREATE INDEX provisioner_jobs_started_at_idx ON provisioner_jobs USING btree (started_at) WHERE (started_at IS NULL); diff --git a/coderd/database/models.go b/coderd/database/models.go index 7156d772a3c4d..50c4d0b5c02d3 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1823,7 +1823,6 @@ type Organization struct { Description string `db:"description" json:"description"` CreatedAt time.Time `db:"created_at" json:"created_at"` UpdatedAt time.Time `db:"updated_at" json:"updated_at"` - IsDefault bool `db:"is_default" json:"is_default"` } type OrganizationMember struct { diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 4b459e3141216..cbeb5b1caf746 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -102,7 +102,6 @@ type sqlcQuerier interface { GetAuthorizationUserRoles(ctx context.Context, userID uuid.UUID) (GetAuthorizationUserRolesRow, error) GetDBCryptKeys(ctx context.Context) ([]DBCryptKey, error) GetDERPMeshKey(ctx context.Context) (string, error) - GetDefaultOrganization(ctx context.Context) (Organization, error) GetDefaultProxyConfig(ctx context.Context) (GetDefaultProxyConfigRow, error) GetDeploymentDAUs(ctx context.Context, tzOffset int32) ([]GetDeploymentDAUsRow, error) GetDeploymentID(ctx context.Context) (string, error) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 0f7b0cd95d7fb..4fd0579aff242 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -494,34 +494,6 @@ func TestUserChangeLoginType(t *testing.T) { require.Equal(t, bobExpPass, bob.HashedPassword, "hashed password should not change") } -func TestDefaultOrg(t *testing.T) { - t.Parallel() - if testing.Short() { - t.SkipNow() - } - - sqlDB := testSQLDB(t) - err := migrations.Up(sqlDB) - require.NoError(t, err) - db := database.New(sqlDB) - ctx := context.Background() - - // Should start with 0 orgs - 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") -} - type tvArgs struct { Status database.ProvisionerJobStatus // CreateWorkspace is true if we should create a workspace for the template version diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 002da316cbccf..97b33038ea468 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3142,34 +3142,9 @@ func (q *sqlQuerier) UpdateMemberRoles(ctx context.Context, arg UpdateMemberRole return i, err } -const getDefaultOrganization = `-- name: GetDefaultOrganization :one -SELECT - id, name, description, created_at, updated_at, is_default -FROM - organizations -WHERE - is_default = true -LIMIT - 1 -` - -func (q *sqlQuerier) GetDefaultOrganization(ctx context.Context) (Organization, error) { - row := q.db.QueryRowContext(ctx, getDefaultOrganization) - var i Organization - err := row.Scan( - &i.ID, - &i.Name, - &i.Description, - &i.CreatedAt, - &i.UpdatedAt, - &i.IsDefault, - ) - return i, err -} - const getOrganizationByID = `-- name: GetOrganizationByID :one SELECT - id, name, description, created_at, updated_at, is_default + id, name, description, created_at, updated_at FROM organizations WHERE @@ -3185,14 +3160,13 @@ func (q *sqlQuerier) GetOrganizationByID(ctx context.Context, id uuid.UUID) (Org &i.Description, &i.CreatedAt, &i.UpdatedAt, - &i.IsDefault, ) return i, err } const getOrganizationByName = `-- name: GetOrganizationByName :one SELECT - id, name, description, created_at, updated_at, is_default + id, name, description, created_at, updated_at FROM organizations WHERE @@ -3210,14 +3184,13 @@ func (q *sqlQuerier) GetOrganizationByName(ctx context.Context, name string) (Or &i.Description, &i.CreatedAt, &i.UpdatedAt, - &i.IsDefault, ) return i, err } const getOrganizations = `-- name: GetOrganizations :many SELECT - id, name, description, created_at, updated_at, is_default + id, name, description, created_at, updated_at FROM organizations ` @@ -3237,7 +3210,6 @@ func (q *sqlQuerier) GetOrganizations(ctx context.Context) ([]Organization, erro &i.Description, &i.CreatedAt, &i.UpdatedAt, - &i.IsDefault, ); err != nil { return nil, err } @@ -3254,7 +3226,7 @@ func (q *sqlQuerier) GetOrganizations(ctx context.Context) ([]Organization, erro const getOrganizationsByUserID = `-- name: GetOrganizationsByUserID :many SELECT - id, name, description, created_at, updated_at, is_default + id, name, description, created_at, updated_at FROM organizations WHERE @@ -3283,7 +3255,6 @@ func (q *sqlQuerier) GetOrganizationsByUserID(ctx context.Context, userID uuid.U &i.Description, &i.CreatedAt, &i.UpdatedAt, - &i.IsDefault, ); err != nil { return nil, err } @@ -3300,10 +3271,9 @@ func (q *sqlQuerier) GetOrganizationsByUserID(ctx context.Context, userID uuid.U const insertOrganization = `-- name: InsertOrganization :one INSERT INTO - organizations (id, "name", description, created_at, updated_at, is_default) + organizations (id, "name", description, created_at, updated_at) VALUES - -- If no organizations exist, and this is the first, make it the default. - ($1, $2, $3, $4, $5, (SELECT TRUE FROM organizations LIMIT 1) IS NULL) RETURNING id, name, description, created_at, updated_at, is_default + ($1, $2, $3, $4, $5) RETURNING id, name, description, created_at, updated_at ` type InsertOrganizationParams struct { @@ -3329,7 +3299,6 @@ func (q *sqlQuerier) InsertOrganization(ctx context.Context, arg InsertOrganizat &i.Description, &i.CreatedAt, &i.UpdatedAt, - &i.IsDefault, ) return i, err } diff --git a/coderd/database/queries/organizations.sql b/coderd/database/queries/organizations.sql index 7f901b003bf44..87c403049efd2 100644 --- a/coderd/database/queries/organizations.sql +++ b/coderd/database/queries/organizations.sql @@ -1,13 +1,3 @@ --- name: GetDefaultOrganization :one -SELECT - * -FROM - organizations -WHERE - is_default = true -LIMIT - 1; - -- name: GetOrganizations :many SELECT * @@ -49,7 +39,6 @@ WHERE -- name: InsertOrganization :one INSERT INTO - organizations (id, "name", description, created_at, updated_at, is_default) + organizations (id, "name", description, created_at, updated_at) VALUES - -- If no organizations exist, and this is the first, make it the default. - ($1, $2, $3, $4, $5, (SELECT TRUE FROM organizations LIMIT 1) IS NULL) RETURNING *; + ($1, $2, $3, $4, $5) RETURNING *; diff --git a/coderd/database/unique_constraint.go b/coderd/database/unique_constraint.go index fa1efffb8137c..747aa3e07bb2f 100644 --- a/coderd/database/unique_constraint.go +++ b/coderd/database/unique_constraint.go @@ -74,7 +74,6 @@ const ( UniqueIndexProvisionerDaemonsNameOwnerKey UniqueConstraint = "idx_provisioner_daemons_name_owner_key" // CREATE UNIQUE INDEX idx_provisioner_daemons_name_owner_key ON provisioner_daemons USING btree (name, lower(COALESCE((tags ->> 'owner'::text), ''::text))); UniqueIndexUsersEmail UniqueConstraint = "idx_users_email" // CREATE UNIQUE INDEX idx_users_email ON users USING btree (email) WHERE (deleted = false); UniqueIndexUsersUsername UniqueConstraint = "idx_users_username" // CREATE UNIQUE INDEX idx_users_username ON users USING btree (username) WHERE (deleted = false); - UniqueOrganizationsSingleDefaultOrg UniqueConstraint = "organizations_single_default_org" // CREATE UNIQUE INDEX organizations_single_default_org ON organizations USING btree (is_default) WHERE (is_default = true); UniqueTemplatesOrganizationIDNameIndex UniqueConstraint = "templates_organization_id_name_idx" // CREATE UNIQUE INDEX templates_organization_id_name_idx ON templates USING btree (organization_id, lower((name)::text)) WHERE (deleted = false); UniqueUsersEmailLowerIndex UniqueConstraint = "users_email_lower_idx" // CREATE UNIQUE INDEX users_email_lower_idx ON users USING btree (lower(email)) WHERE (deleted = false); UniqueUsersUsernameLowerIndex UniqueConstraint = "users_username_lower_idx" // CREATE UNIQUE INDEX users_username_lower_idx ON users USING btree (lower(username)) WHERE (deleted = false); diff --git a/coderd/organizations.go b/coderd/organizations.go index d50c0a4e250cc..ae24edea01597 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -118,6 +118,5 @@ func convertOrganization(organization database.Organization) codersdk.Organizati Name: organization.Name, CreatedAt: organization.CreatedAt, UpdatedAt: organization.UpdatedAt, - IsDefault: organization.IsDefault, } } diff --git a/coderd/organizations_test.go b/coderd/organizations_test.go index 6fc98d7c9add9..c8cde696e22a2 100644 --- a/coderd/organizations_test.go +++ b/coderd/organizations_test.go @@ -24,14 +24,6 @@ func TestOrganizationsByUser(t *testing.T) { require.NoError(t, err) require.NotNil(t, orgs) require.Len(t, orgs, 1) - require.True(t, orgs[0].IsDefault, "first org is always default") - - // Make an extra org, and it should not be defaulted. - notDefault, err := client.CreateOrganization(ctx, codersdk.CreateOrganizationRequest{ - Name: "another", - }) - require.NoError(t, err) - require.False(t, notDefault.IsDefault, "only 1 default org allowed") } func TestOrganizationByUserAndName(t *testing.T) { diff --git a/coderd/userauth.go b/coderd/userauth.go index 74bd77f28a54e..dbb01f12e31ad 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1343,12 +1343,13 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C if user.ID == uuid.Nil { var organizationID uuid.UUID //nolint:gocritic - organization, _ := tx.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx)) - - // Add the user to the default organization. - // Once multi-organization we should check some configuration to see - // if we should add the user to a different organization. - organizationID = organization.ID + organizations, _ := tx.GetOrganizations(dbauthz.AsSystemRestricted(ctx)) + if len(organizations) > 0 { + // Add the user to the first organization. Once multi-organization + // support is added, we should enable a configuration map of user + // email to organization. + organizationID = organizations[0].ID + } //nolint:gocritic _, err := tx.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ @@ -1394,7 +1395,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C // All of the userauth tests depend on this being able to create // the first organization. It shouldn't be possible in normal // operation. - CreateOrganization: organizationID == uuid.Nil, + CreateOrganization: len(organizations) == 0, LoginType: params.LoginType, }) if err != nil { diff --git a/coderd/users.go b/coderd/users.go index ba6587ecac5fc..ca757ed80436f 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -401,18 +401,10 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { return } } else { - // If no organization is provided, add the user to the default - defaultOrg, err := api.Database.GetDefaultOrganization(ctx) + // If no organization is provided, add the user to the first + // organization. + organizations, err := api.Database.GetOrganizations(ctx) if err != nil { - if httpapi.Is404Error(err) { - httpapi.Write(ctx, rw, http.StatusNotFound, - codersdk.Response{ - Message: "Resource not found or you do not have access to this resource", - Detail: "Organization not found", - }, - ) - return - } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching orgs.", Detail: err.Error(), @@ -420,7 +412,12 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { return } - req.OrganizationID = defaultOrg.ID + if len(organizations) > 0 { + // Add the user to the first organization. Once multi-organization + // support is added, we should enable a configuration map of user + // email to organization. + req.OrganizationID = organizations[0].ID + } } var loginType database.LoginType diff --git a/coderd/users_test.go b/coderd/users_test.go index 0d7f5a7bb21b9..a9b4fa8de72b3 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -493,26 +493,21 @@ func TestPostUsers(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs()) + firstUser := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + numLogs++ // add an audit log for login ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - // Add an extra org to try and confuse user creation - _, err := client.CreateOrganization(ctx, codersdk.CreateOrganizationRequest{ - Name: "foobar", - }) - require.NoError(t, err) - - numLogs := len(auditor.AuditLogs()) - user, err := client.CreateUser(ctx, codersdk.CreateUserRequest{ Email: "another@user.org", Username: "someone-else", Password: "SomeSecurePassword!", }) require.NoError(t, err) - numLogs++ // add an audit log for user create require.Len(t, auditor.AuditLogs(), numLogs) require.Equal(t, database.AuditActionCreate, auditor.AuditLogs()[numLogs-1].Action) diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 8bf1ecc96811d..6a629160207e8 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -30,7 +30,6 @@ type Organization struct { Name string `json:"name" validate:"required"` CreatedAt time.Time `json:"created_at" validate:"required" format:"date-time"` UpdatedAt time.Time `json:"updated_at" validate:"required" format:"date-time"` - IsDefault bool `json:"is_default" validate:"required"` } type OrganizationMember struct { diff --git a/docs/api/organizations.md b/docs/api/organizations.md index 478c8aba56648..011d3cac5eb2e 100644 --- a/docs/api/organizations.md +++ b/docs/api/organizations.md @@ -123,7 +123,6 @@ curl -X POST http://coder-server:8080/api/v2/organizations \ { "created_at": "2019-08-24T14:15:22Z", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "is_default": true, "name": "string", "updated_at": "2019-08-24T14:15:22Z" } @@ -164,7 +163,6 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization} \ { "created_at": "2019-08-24T14:15:22Z", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "is_default": true, "name": "string", "updated_at": "2019-08-24T14:15:22Z" } diff --git a/docs/api/schemas.md b/docs/api/schemas.md index be5724e361159..2ea43f292b546 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -3811,7 +3811,6 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in { "created_at": "2019-08-24T14:15:22Z", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "is_default": true, "name": "string", "updated_at": "2019-08-24T14:15:22Z" } @@ -3819,13 +3818,12 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in ### Properties -| Name | Type | Required | Restrictions | Description | -| ------------ | ------- | -------- | ------------ | ----------- | -| `created_at` | string | true | | | -| `id` | string | true | | | -| `is_default` | boolean | true | | | -| `name` | string | true | | | -| `updated_at` | string | true | | | +| Name | Type | Required | Restrictions | Description | +| ------------ | ------ | -------- | ------------ | ----------- | +| `created_at` | string | true | | | +| `id` | string | true | | | +| `name` | string | true | | | +| `updated_at` | string | true | | | ## codersdk.OrganizationMember diff --git a/docs/api/users.md b/docs/api/users.md index a057376b38f16..e656ed6ac27a7 100644 --- a/docs/api/users.md +++ b/docs/api/users.md @@ -993,7 +993,6 @@ curl -X GET http://coder-server:8080/api/v2/users/{user}/organizations \ { "created_at": "2019-08-24T14:15:22Z", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "is_default": true, "name": "string", "updated_at": "2019-08-24T14:15:22Z" } @@ -1015,7 +1014,6 @@ Status Code **200** | `[array item]` | array | false | | | | `» created_at` | string(date-time) | true | | | | `» id` | string(uuid) | true | | | -| `» is_default` | boolean | true | | | | `» name` | string | true | | | | `» updated_at` | string(date-time) | true | | | @@ -1049,7 +1047,6 @@ curl -X GET http://coder-server:8080/api/v2/users/{user}/organizations/{organiza { "created_at": "2019-08-24T14:15:22Z", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "is_default": true, "name": "string", "updated_at": "2019-08-24T14:15:22Z" } diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 9c72b8c1c927a..ac605e193f1f3 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -768,7 +768,6 @@ export interface Organization { readonly name: string; readonly created_at: string; readonly updated_at: string; - readonly is_default: boolean; } // From codersdk/organizations.go diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index aa91756f725df..ce4f9d836aa0e 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -16,7 +16,6 @@ export const MockOrganization: TypesGen.Organization = { name: "Test Organization", created_at: "", updated_at: "", - is_default: true, }; export const MockTemplateDAUResponse: TypesGen.DAUsResponse = { From bf35196e6b15e9fdb4463bbabcff8bfe3d10d114 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 Feb 2024 13:32:53 -0600 Subject: [PATCH 3/5] Revert then fix diff --- coderd/userauth.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index dbb01f12e31ad..74bd77f28a54e 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1343,13 +1343,12 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C if user.ID == uuid.Nil { var organizationID uuid.UUID //nolint:gocritic - organizations, _ := tx.GetOrganizations(dbauthz.AsSystemRestricted(ctx)) - if len(organizations) > 0 { - // Add the user to the first organization. Once multi-organization - // support is added, we should enable a configuration map of user - // email to organization. - organizationID = organizations[0].ID - } + organization, _ := tx.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx)) + + // Add the user to the default organization. + // Once multi-organization we should check some configuration to see + // if we should add the user to a different organization. + organizationID = organization.ID //nolint:gocritic _, err := tx.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ @@ -1395,7 +1394,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C // All of the userauth tests depend on this being able to create // the first organization. It shouldn't be possible in normal // operation. - CreateOrganization: len(organizations) == 0, + CreateOrganization: organizationID == uuid.Nil, LoginType: params.LoginType, }) if err != nil { From da6c21cb4442e853196f9f1d8798dcebba150b21 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 Feb 2024 10:40:13 -0600 Subject: [PATCH 4/5] feat: set groupsync to use default org --- coderd/coderd.go | 8 +-- coderd/database/dbauthz/dbauthz.go | 18 +++---- coderd/database/dbauthz/dbauthz_test.go | 7 +-- coderd/database/dbmem/dbmem.go | 46 ++++++----------- coderd/database/dbmetrics/dbmetrics.go | 14 +++--- coderd/database/dbmock/dbmock.go | 28 +++++------ coderd/database/querier.go | 2 +- coderd/database/queries.sql.go | 30 +++++------ coderd/database/queries/groupmembers.sql | 5 +- coderd/userauth.go | 40 +++++++++++++-- enterprise/coderd/userauth.go | 64 +++++++++++++----------- 11 files changed, 139 insertions(+), 123 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 6110733edecc3..d6ec155dc42f2 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -134,7 +134,7 @@ type Options struct { BaseDERPMap *tailcfg.DERPMap DERPMapUpdateFrequency time.Duration SwaggerEndpoint bool - SetUserGroups func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, groupNames []string, createMissingGroups bool) error + SetUserGroups func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, orgGroupNames map[uuid.UUID][]string, createMissingGroups bool) error SetUserSiteRoles func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, roles []string) error TemplateScheduleStore *atomic.Pointer[schedule.TemplateScheduleStore] UserQuietHoursScheduleStore *atomic.Pointer[schedule.UserQuietHoursScheduleStore] @@ -301,9 +301,11 @@ func New(options *Options) *API { options.TracerProvider = trace.NewNoopTracerProvider() } if options.SetUserGroups == nil { - options.SetUserGroups = func(ctx context.Context, logger slog.Logger, _ database.Store, userID uuid.UUID, groups []string, createMissingGroups bool) error { + options.SetUserGroups = func(ctx context.Context, logger slog.Logger, _ database.Store, userID uuid.UUID, orgGroupNames map[uuid.UUID][]string, createMissingGroups bool) error { logger.Warn(ctx, "attempted to assign OIDC groups without enterprise license", - slog.F("user_id", userID), slog.F("groups", groups), slog.F("create_missing_groups", createMissingGroups), + slog.F("user_id", userID), + slog.F("groups", orgGroupNames), + slog.F("create_missing_groups", createMissingGroups), ) return nil } diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index a0da90eb52f23..6f29502cf1d11 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -793,16 +793,6 @@ func (q *querier) DeleteGroupMemberFromGroup(ctx context.Context, arg database.D return update(q.log, q.auth, fetch, q.db.DeleteGroupMemberFromGroup)(ctx, arg) } -func (q *querier) DeleteGroupMembersByOrgAndUser(ctx context.Context, arg database.DeleteGroupMembersByOrgAndUserParams) error { - // This will remove the user from all groups in the org. This counts as updating a group. - // NOTE: instead of fetching all groups in the org with arg.UserID as a member, we instead - // check if the caller has permission to update any group in the org. - fetch := func(ctx context.Context, arg database.DeleteGroupMembersByOrgAndUserParams) (rbac.Objecter, error) { - return rbac.ResourceGroup.InOrg(arg.OrganizationID), nil - } - return update(q.log, q.auth, fetch, q.db.DeleteGroupMembersByOrgAndUser)(ctx, arg) -} - func (q *querier) DeleteLicense(ctx context.Context, id int32) (int32, error) { err := deleteQ(q.log, q.auth, q.db.GetLicenseByID, func(ctx context.Context, id int32) error { _, err := q.db.DeleteLicense(ctx, id) @@ -2549,6 +2539,14 @@ func (q *querier) RegisterWorkspaceProxy(ctx context.Context, arg database.Regis return updateWithReturn(q.log, q.auth, fetch, q.db.RegisterWorkspaceProxy)(ctx, arg) } +func (q *querier) RemoveUserFromAllGroups(ctx context.Context, userID uuid.UUID) error { + // This is a system function to clear user groups in group sync. + if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceSystem); err != nil { + return err + } + return q.db.RemoveUserFromAllGroups(ctx, userID) +} + func (q *querier) RevokeDBCryptKey(ctx context.Context, activeKeyDigest string) error { if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceSystem); err != nil { return err diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 56b6012ba2193..8f329f9dfc6ea 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -344,17 +344,14 @@ func (s *MethodTestSuite) TestGroup() { GroupNames: slice.New(g1.Name, g2.Name), }).Asserts(rbac.ResourceGroup.InOrg(o.ID), rbac.ActionUpdate).Returns() })) - s.Run("DeleteGroupMembersByOrgAndUser", s.Subtest(func(db database.Store, check *expects) { + s.Run("RemoveUserFromAllGroups", s.Subtest(func(db database.Store, check *expects) { o := dbgen.Organization(s.T(), db, database.Organization{}) u1 := dbgen.User(s.T(), db, database.User{}) g1 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID}) g2 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID}) _ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g1.ID, UserID: u1.ID}) _ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g2.ID, UserID: u1.ID}) - check.Args(database.DeleteGroupMembersByOrgAndUserParams{ - OrganizationID: o.ID, - UserID: u1.ID, - }).Asserts(rbac.ResourceGroup.InOrg(o.ID), rbac.ActionUpdate).Returns() + check.Args(u1.ID).Asserts(rbac.ResourceSystem, rbac.ActionUpdate).Returns() })) s.Run("UpdateGroupByID", s.Subtest(func(db database.Store, check *expects) { g := dbgen.Group(s.T(), db, database.Group{}) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 8901d13d3d4df..a81ce294fb558 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -1135,36 +1135,6 @@ func (q *FakeQuerier) DeleteGroupMemberFromGroup(_ context.Context, arg database return nil } -func (q *FakeQuerier) DeleteGroupMembersByOrgAndUser(_ context.Context, arg database.DeleteGroupMembersByOrgAndUserParams) error { - q.mutex.Lock() - defer q.mutex.Unlock() - - newMembers := q.groupMembers[:0] - for _, member := range q.groupMembers { - if member.UserID != arg.UserID { - // Do not delete the other members - newMembers = append(newMembers, member) - } else if member.UserID == arg.UserID { - // We only want to delete from groups in the organization in the args. - for _, group := range q.groups { - // Find the group that the member is apartof. - if group.ID == member.GroupID { - // Only add back the member if the organization ID does not match - // the arg organization ID. Since the arg is saying which - // org to delete. - if group.OrganizationID != arg.OrganizationID { - newMembers = append(newMembers, member) - } - break - } - } - } - } - q.groupMembers = newMembers - - return nil -} - func (q *FakeQuerier) DeleteLicense(_ context.Context, id int32) (int32, error) { q.mutex.Lock() defer q.mutex.Unlock() @@ -6083,6 +6053,22 @@ func (q *FakeQuerier) RegisterWorkspaceProxy(_ context.Context, arg database.Reg return database.WorkspaceProxy{}, sql.ErrNoRows } +func (q *FakeQuerier) RemoveUserFromAllGroups(_ context.Context, userID uuid.UUID) error { + q.mutex.Lock() + defer q.mutex.Unlock() + + newMembers := q.groupMembers[:0] + for _, member := range q.groupMembers { + if member.UserID == userID { + continue + } + newMembers = append(newMembers, member) + } + q.groupMembers = newMembers + + return nil +} + func (q *FakeQuerier) RevokeDBCryptKey(_ context.Context, activeKeyDigest string) error { q.mutex.Lock() defer q.mutex.Unlock() diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 948de3c763277..6b876c522f9a7 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -211,13 +211,6 @@ func (m metricsStore) DeleteGroupMemberFromGroup(ctx context.Context, arg databa return err } -func (m metricsStore) DeleteGroupMembersByOrgAndUser(ctx context.Context, arg database.DeleteGroupMembersByOrgAndUserParams) error { - start := time.Now() - err := m.s.DeleteGroupMembersByOrgAndUser(ctx, arg) - m.queryLatencies.WithLabelValues("DeleteGroupMembersByOrgAndUser").Observe(time.Since(start).Seconds()) - return err -} - func (m metricsStore) DeleteLicense(ctx context.Context, id int32) (int32, error) { start := time.Now() licenseID, err := m.s.DeleteLicense(ctx, id) @@ -1635,6 +1628,13 @@ func (m metricsStore) RegisterWorkspaceProxy(ctx context.Context, arg database.R return proxy, err } +func (m metricsStore) RemoveUserFromAllGroups(ctx context.Context, userID uuid.UUID) error { + start := time.Now() + r0 := m.s.RemoveUserFromAllGroups(ctx, userID) + m.queryLatencies.WithLabelValues("RemoveUserFromAllGroups").Observe(time.Since(start).Seconds()) + return r0 +} + func (m metricsStore) RevokeDBCryptKey(ctx context.Context, activeKeyDigest string) error { start := time.Now() r0 := m.s.RevokeDBCryptKey(ctx, activeKeyDigest) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index d767fd7cf5bd7..6f6c420aa07dd 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -313,20 +313,6 @@ func (mr *MockStoreMockRecorder) DeleteGroupMemberFromGroup(arg0, arg1 any) *gom return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteGroupMemberFromGroup", reflect.TypeOf((*MockStore)(nil).DeleteGroupMemberFromGroup), arg0, arg1) } -// DeleteGroupMembersByOrgAndUser mocks base method. -func (m *MockStore) DeleteGroupMembersByOrgAndUser(arg0 context.Context, arg1 database.DeleteGroupMembersByOrgAndUserParams) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteGroupMembersByOrgAndUser", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 -} - -// DeleteGroupMembersByOrgAndUser indicates an expected call of DeleteGroupMembersByOrgAndUser. -func (mr *MockStoreMockRecorder) DeleteGroupMembersByOrgAndUser(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteGroupMembersByOrgAndUser", reflect.TypeOf((*MockStore)(nil).DeleteGroupMembersByOrgAndUser), arg0, arg1) -} - // DeleteLicense mocks base method. func (m *MockStore) DeleteLicense(arg0 context.Context, arg1 int32) (int32, error) { m.ctrl.T.Helper() @@ -3455,6 +3441,20 @@ func (mr *MockStoreMockRecorder) RegisterWorkspaceProxy(arg0, arg1 any) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RegisterWorkspaceProxy", reflect.TypeOf((*MockStore)(nil).RegisterWorkspaceProxy), arg0, arg1) } +// RemoveUserFromAllGroups mocks base method. +func (m *MockStore) RemoveUserFromAllGroups(arg0 context.Context, arg1 uuid.UUID) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RemoveUserFromAllGroups", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// RemoveUserFromAllGroups indicates an expected call of RemoveUserFromAllGroups. +func (mr *MockStoreMockRecorder) RemoveUserFromAllGroups(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveUserFromAllGroups", reflect.TypeOf((*MockStore)(nil).RemoveUserFromAllGroups), arg0, arg1) +} + // RevokeDBCryptKey mocks base method. func (m *MockStore) RevokeDBCryptKey(arg0 context.Context, arg1 string) error { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index cbeb5b1caf746..1416875962b7a 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -58,7 +58,6 @@ type sqlcQuerier interface { DeleteGitSSHKey(ctx context.Context, userID uuid.UUID) error DeleteGroupByID(ctx context.Context, id uuid.UUID) error DeleteGroupMemberFromGroup(ctx context.Context, arg DeleteGroupMemberFromGroupParams) error - DeleteGroupMembersByOrgAndUser(ctx context.Context, arg DeleteGroupMembersByOrgAndUserParams) error DeleteLicense(ctx context.Context, id int32) (int32, error) DeleteOAuth2ProviderAppByID(ctx context.Context, id uuid.UUID) error DeleteOAuth2ProviderAppSecretByID(ctx context.Context, id uuid.UUID) error @@ -321,6 +320,7 @@ type sqlcQuerier interface { InsertWorkspaceResourceMetadata(ctx context.Context, arg InsertWorkspaceResourceMetadataParams) ([]WorkspaceResourceMetadatum, error) ListWorkspaceAgentPortShares(ctx context.Context, workspaceID uuid.UUID) ([]WorkspaceAgentPortShare, error) RegisterWorkspaceProxy(ctx context.Context, arg RegisterWorkspaceProxyParams) (WorkspaceProxy, error) + RemoveUserFromAllGroups(ctx context.Context, userID uuid.UUID) error RevokeDBCryptKey(ctx context.Context, activeKeyDigest string) error // Non blocking lock. Returns true if the lock was acquired, false otherwise. // diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 97b33038ea468..db57ffe0ddeba 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1288,24 +1288,6 @@ func (q *sqlQuerier) DeleteGroupMemberFromGroup(ctx context.Context, arg DeleteG return err } -const deleteGroupMembersByOrgAndUser = `-- name: DeleteGroupMembersByOrgAndUser :exec -DELETE FROM - group_members -WHERE - group_members.user_id = $1 - AND group_id = ANY(SELECT id FROM groups WHERE organization_id = $2) -` - -type DeleteGroupMembersByOrgAndUserParams struct { - UserID uuid.UUID `db:"user_id" json:"user_id"` - OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` -} - -func (q *sqlQuerier) DeleteGroupMembersByOrgAndUser(ctx context.Context, arg DeleteGroupMembersByOrgAndUserParams) error { - _, err := q.db.ExecContext(ctx, deleteGroupMembersByOrgAndUser, arg.UserID, arg.OrganizationID) - return err -} - const getGroupMembers = `-- name: GetGroupMembers :many SELECT users.id, users.email, users.username, users.hashed_password, users.created_at, users.updated_at, users.status, users.rbac_roles, users.login_type, users.avatar_url, users.deleted, users.last_seen_at, users.quiet_hours_schedule, users.theme_preference, users.name @@ -1419,6 +1401,18 @@ func (q *sqlQuerier) InsertUserGroupsByName(ctx context.Context, arg InsertUserG return err } +const removeUserFromAllGroups = `-- name: RemoveUserFromAllGroups :exec +DELETE FROM + group_members +WHERE + user_id = $1 +` + +func (q *sqlQuerier) RemoveUserFromAllGroups(ctx context.Context, userID uuid.UUID) error { + _, err := q.db.ExecContext(ctx, removeUserFromAllGroups, userID) + return err +} + const deleteGroupByID = `-- name: DeleteGroupByID :exec DELETE FROM groups diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql index 4999df7930044..d755212132383 100644 --- a/coderd/database/queries/groupmembers.sql +++ b/coderd/database/queries/groupmembers.sql @@ -42,12 +42,11 @@ SELECT FROM groups; --- name: DeleteGroupMembersByOrgAndUser :exec +-- name: RemoveUserFromAllGroups :exec DELETE FROM group_members WHERE - group_members.user_id = @user_id - AND group_id = ANY(SELECT id FROM groups WHERE organization_id = @organization_id); + user_id = @user_id; -- name: InsertGroupMember :exec INSERT INTO diff --git a/coderd/userauth.go b/coderd/userauth.go index 74bd77f28a54e..cb79db124dc3c 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1217,8 +1217,10 @@ type oauthLoginParams struct { // to the Groups provided. UsingGroups bool CreateMissingGroups bool - Groups []string - GroupFilter *regexp.Regexp + // These are the group names from the IDP. Internally, they will map to + // some organization groups. + Groups []string + GroupFilter *regexp.Regexp // Is UsingRoles is true, then the user will be assigned // the roles provided. UsingRoles bool @@ -1301,7 +1303,6 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C link database.UserLink err error ) - user = params.User link = params.Link @@ -1457,6 +1458,9 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C } // Ensure groups are correct. + // This places all groups into the default organization. + // To go multi-org, we need to add a mapping feature here to know which + // groups go to which orgs. if params.UsingGroups { filtered := params.Groups if params.GroupFilter != nil { @@ -1468,8 +1472,36 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C } } + //nolint:gocritic // No user present in the context. + defaultOrganization, err := tx.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx)) + if err != nil { + // If there is no default org, then we can't assign groups. + // By default, we assume all groups belong to the default org. + return xerrors.Errorf("get default organization: %w", err) + } + + //nolint:gocritic // No user present in the context. + memberships, err := tx.GetOrganizationMembershipsByUserID(dbauthz.AsSystemRestricted(ctx), user.ID) + if err != nil { + return xerrors.Errorf("get organization memberships: %w", err) + } + + inDefault := false + for _, membership := range memberships { + if membership.OrganizationID == defaultOrganization.ID { + inDefault = true + break + } + } + + if !inDefault { + return xerrors.Errorf("user %s is not a member of the default organization, cannot assign to groups in the org", user.ID) + } + //nolint:gocritic - err := api.Options.SetUserGroups(dbauthz.AsSystemRestricted(ctx), logger, tx, user.ID, filtered, params.CreateMissingGroups) + err = api.Options.SetUserGroups(dbauthz.AsSystemRestricted(ctx), logger, tx, user.ID, map[uuid.UUID][]string{ + defaultOrganization.ID: filtered, + }, params.CreateMissingGroups) if err != nil { return xerrors.Errorf("set user groups: %w", err) } diff --git a/enterprise/coderd/userauth.go b/enterprise/coderd/userauth.go index f504a6c0325c4..f35d38ca448d9 100644 --- a/enterprise/coderd/userauth.go +++ b/enterprise/coderd/userauth.go @@ -14,7 +14,7 @@ import ( ) // nolint: revive -func (api *API) setUserGroups(ctx context.Context, logger slog.Logger, db database.Store, userID uuid.UUID, groupNames []string, createMissingGroups bool) error { +func (api *API) setUserGroups(ctx context.Context, logger slog.Logger, db database.Store, userID uuid.UUID, orgGroupNames map[uuid.UUID][]string, createMissingGroups bool) error { api.entitlementsMu.RLock() enabled := api.entitlements.Features[codersdk.FeatureTemplateRBAC].Enabled api.entitlementsMu.RUnlock() @@ -24,6 +24,8 @@ func (api *API) setUserGroups(ctx context.Context, logger slog.Logger, db databa } return db.InTx(func(tx database.Store) error { + // When setting the user's groups, it's easier to just clear their groups and re-add them. + // This ensures that the user's groups are always in sync with the auth provider. orgs, err := tx.GetOrganizationsByUserID(ctx, userID) if err != nil { return xerrors.Errorf("get user orgs: %w", err) @@ -33,43 +35,49 @@ func (api *API) setUserGroups(ctx context.Context, logger slog.Logger, db databa } // Delete all groups the user belongs to. - err = tx.DeleteGroupMembersByOrgAndUser(ctx, database.DeleteGroupMembersByOrgAndUserParams{ - UserID: userID, - OrganizationID: orgs[0].ID, - }) + // nolint:gocritic // Requires system context to remove user from all groups. + err = tx.RemoveUserFromAllGroups(dbauthz.AsSystemRestricted(ctx), userID) if err != nil { return xerrors.Errorf("delete user groups: %w", err) } - if createMissingGroups { - // This is the system creating these additional groups, so we use the system restricted context. - // nolint:gocritic - created, err := tx.InsertMissingGroups(dbauthz.AsSystemRestricted(ctx), database.InsertMissingGroupsParams{ - OrganizationID: orgs[0].ID, + // TODO: This could likely be improved by making these single queries. + // Either by batching or some other means. This for loop could be really + // inefficient if there are a lot of organizations. There was deployments + // on v1 with >100 orgs. + for orgID, groupNames := range orgGroupNames { + // Create the missing groups for each organization. + if createMissingGroups { + // This is the system creating these additional groups, so we use the system restricted context. + // nolint:gocritic + created, err := tx.InsertMissingGroups(dbauthz.AsSystemRestricted(ctx), database.InsertMissingGroupsParams{ + OrganizationID: orgID, + GroupNames: groupNames, + Source: database.GroupSourceOidc, + }) + if err != nil { + return xerrors.Errorf("insert missing groups: %w", err) + } + if len(created) > 0 { + logger.Debug(ctx, "auto created missing groups", + slog.F("org_id", orgID.ID), + slog.F("created", created), + slog.F("num", len(created)), + ) + } + } + + // Re-add the user to all groups returned by the auth provider. + err = tx.InsertUserGroupsByName(ctx, database.InsertUserGroupsByNameParams{ + UserID: userID, + OrganizationID: orgID, GroupNames: groupNames, - Source: database.GroupSourceOidc, }) if err != nil { - return xerrors.Errorf("insert missing groups: %w", err) - } - if len(created) > 0 { - logger.Debug(ctx, "auto created missing groups", - slog.F("org_id", orgs[0].ID), - slog.F("created", created), - ) + return xerrors.Errorf("insert user groups: %w", err) } } - // Re-add the user to all groups returned by the auth provider. - err = tx.InsertUserGroupsByName(ctx, database.InsertUserGroupsByNameParams{ - UserID: userID, - OrganizationID: orgs[0].ID, - GroupNames: groupNames, - }) - if err != nil { - return xerrors.Errorf("insert user groups: %w", err) - } - return nil }, nil) } From 9b63c5a1535108b52c9e11a75bdf576996241a31 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 16 Feb 2024 08:57:46 -0600 Subject: [PATCH 5/5] Use slices.ContainsFunc --- coderd/userauth.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index b74293e1b9284..188a877e51055 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -20,6 +20,7 @@ import ( "github.com/google/go-github/v43/github" "github.com/google/uuid" "github.com/moby/moby/pkg/namesgenerator" + "golang.org/x/exp/slices" "golang.org/x/oauth2" "golang.org/x/xerrors" @@ -1489,15 +1490,11 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C return xerrors.Errorf("get organization memberships: %w", err) } - inDefault := false - for _, membership := range memberships { - if membership.OrganizationID == defaultOrganization.ID { - inDefault = true - break - } - } - - if !inDefault { + // If the user is not in the default organization, then we can't assign groups. + // A user cannot be in groups to an org they are not a member of. + if !slices.ContainsFunc(memberships, func(member database.OrganizationMember) bool { + return member.OrganizationID == defaultOrganization.ID + }) { return xerrors.Errorf("user %s is not a member of the default organization, cannot assign to groups in the org", user.ID) }