From 126a560d06678bb27324afd7d8f84c251d4defc6 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Mon, 16 Sep 2024 19:22:47 +0000 Subject: [PATCH 1/2] fix: remove org assigning from SCIM SCIM will no longer assign orgs, as it's now handled during OIDC login. --- enterprise/coderd/scim.go | 18 +++++------------- enterprise/coderd/scim_test.go | 1 + 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/enterprise/coderd/scim.go b/enterprise/coderd/scim.go index b6733bbd4c052..96706702536df 100644 --- a/enterprise/coderd/scim.go +++ b/enterprise/coderd/scim.go @@ -217,22 +217,14 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { sUser.UserName = codersdk.UsernameFrom(sUser.UserName) } - // TODO: This is a temporary solution that does not support multi-org - // deployments. This assumption places all new SCIM users into the - // default organization. - //nolint:gocritic - defaultOrganization, err := api.Database.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx)) - if err != nil { - _ = handlerutil.WriteError(rw, err) - return - } - //nolint:gocritic // needed for SCIM dbUser, err = api.AGPL.CreateUser(dbauthz.AsSystemRestricted(ctx), api.Database, agpl.CreateUserRequest{ CreateUserRequestWithOrgs: codersdk.CreateUserRequestWithOrgs{ - Username: sUser.UserName, - Email: email, - OrganizationIDs: []uuid.UUID{defaultOrganization.ID}, + Username: sUser.UserName, + Email: email, + // In the multi-org world, SCIM does not assign any orgs. Users will + // be automatically sync'd with the correct organization on login. + OrganizationIDs: []uuid.UUID{}, }, LoginType: database.LoginTypeOIDC, // Do not send notifications to user admins as SCIM endpoint might be called sequentially to all users. diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go index 13b7aa5adca70..792b0e420c024 100644 --- a/enterprise/coderd/scim_test.go +++ b/enterprise/coderd/scim_test.go @@ -157,6 +157,7 @@ func TestScim(t *testing.T) { require.Len(t, userRes.Users, 1) assert.Equal(t, sUser.Emails[0].Value, userRes.Users[0].Email) assert.Equal(t, sUser.UserName, userRes.Users[0].Username) + assert.Len(t, userRes.Users[0].OrganizationIDs, 0) // Expect zero notifications (SkipNotifications = true) require.Empty(t, notifyEnq.Sent) From 1f609423fdf01b80ae0dc9b01ca3f595bd36f10f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 Sep 2024 19:06:42 -0500 Subject: [PATCH 2/2] chore: use legacy "AssignDefault" option for legacy behavior in SCIM (#14696) * chore: reference legacy assign default option for legacy behavior AssignDefault is a boolean flag mainly for single org and legacy deployments. Use this flag to determine SCIM behavior. --- coderd/idpsync/idpsync.go | 1 + coderd/idpsync/organization.go | 4 +++ enterprise/coderd/scim.go | 23 ++++++++++--- enterprise/coderd/scim_test.go | 59 ++++++++++++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 5 deletions(-) diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index 1e8b14956b652..8faa22333bdc6 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -24,6 +24,7 @@ import ( // claims to the internal representation of a user in Coder. // TODO: Move group + role sync into this interface. type IDPSync interface { + AssignDefaultOrganization() bool OrganizationSyncEnabled() bool // ParseOrganizationClaims takes claims from an OIDC provider, and returns the // organization sync params for assigning users into organizations. diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index fa091eba441ad..3e2a0f84d5e5e 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -32,6 +32,10 @@ func (AGPLIDPSync) OrganizationSyncEnabled() bool { return false } +func (s AGPLIDPSync) AssignDefaultOrganization() bool { + return s.OrganizationAssignDefault +} + func (s AGPLIDPSync) ParseOrganizationClaims(_ context.Context, _ jwt.MapClaims) (OrganizationParams, *HTTPError) { // For AGPL we only sync the default organization. return OrganizationParams{ diff --git a/enterprise/coderd/scim.go b/enterprise/coderd/scim.go index 96706702536df..45390b6014a6a 100644 --- a/enterprise/coderd/scim.go +++ b/enterprise/coderd/scim.go @@ -217,14 +217,27 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { sUser.UserName = codersdk.UsernameFrom(sUser.UserName) } + // If organization sync is enabled, the user's organizations will be + // corrected on login. If including the default org, then always assign + // the default org, regardless if sync is enabled or not. + // This is to preserve single org deployment behavior. + organizations := []uuid.UUID{} + if api.IDPSync.AssignDefaultOrganization() { + //nolint:gocritic // SCIM operations are a system user + defaultOrganization, err := api.Database.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx)) + if err != nil { + _ = handlerutil.WriteError(rw, err) + return + } + organizations = append(organizations, defaultOrganization.ID) + } + //nolint:gocritic // needed for SCIM dbUser, err = api.AGPL.CreateUser(dbauthz.AsSystemRestricted(ctx), api.Database, agpl.CreateUserRequest{ CreateUserRequestWithOrgs: codersdk.CreateUserRequestWithOrgs{ - Username: sUser.UserName, - Email: email, - // In the multi-org world, SCIM does not assign any orgs. Users will - // be automatically sync'd with the correct organization on login. - OrganizationIDs: []uuid.UUID{}, + Username: sUser.UserName, + Email: email, + OrganizationIDs: organizations, }, LoginType: database.LoginTypeOIDC, // Do not send notifications to user admins as SCIM endpoint might be called sequentially to all users. diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go index 792b0e420c024..8d65d9bb34531 100644 --- a/enterprise/coderd/scim_test.go +++ b/enterprise/coderd/scim_test.go @@ -151,6 +151,65 @@ func TestScim(t *testing.T) { assert.Equal(t, coderd.SCIMAuditAdditionalFields, af) assert.Equal(t, database.AuditActionCreate, aLogs[0].Action) + // Expect users exposed over API + userRes, err := client.Users(ctx, codersdk.UsersRequest{Search: sUser.Emails[0].Value}) + require.NoError(t, err) + require.Len(t, userRes.Users, 1) + assert.Equal(t, sUser.Emails[0].Value, userRes.Users[0].Email) + assert.Equal(t, sUser.UserName, userRes.Users[0].Username) + assert.Len(t, userRes.Users[0].OrganizationIDs, 1) + + // Expect zero notifications (SkipNotifications = true) + require.Empty(t, notifyEnq.Sent) + }) + + t.Run("OKNoDefault", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // given + scimAPIKey := []byte("hi") + mockAudit := audit.NewMock() + notifyEnq := &testutil.FakeNotificationsEnqueuer{} + dv := coderdtest.DeploymentValues(t) + dv.OIDC.OrganizationAssignDefault = false + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + Auditor: mockAudit, + NotificationsEnqueuer: notifyEnq, + DeploymentValues: dv, + }, + SCIMAPIKey: scimAPIKey, + AuditLogging: true, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, + }, + }, + }) + mockAudit.ResetLogs() + + // when + sUser := makeScimUser(t) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + defer res.Body.Close() + require.Equal(t, http.StatusOK, res.StatusCode) + + // then + // Expect audit logs + aLogs := mockAudit.AuditLogs() + require.Len(t, aLogs, 1) + af := map[string]string{} + err = json.Unmarshal([]byte(aLogs[0].AdditionalFields), &af) + require.NoError(t, err) + assert.Equal(t, coderd.SCIMAuditAdditionalFields, af) + assert.Equal(t, database.AuditActionCreate, aLogs[0].Action) + // Expect users exposed over API userRes, err := client.Users(ctx, codersdk.UsersRequest{Search: sUser.Emails[0].Value}) require.NoError(t, err)