diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 403bb13ccf3f8..a0ecba163e2b1 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -267,6 +267,26 @@ func requireOrgID[T Auditable](ctx context.Context, id uuid.UUID, log slog.Logge return id } +// InitRequestWithCancel returns a commit function with a boolean arg. +// If the arg is false, future calls to commit() will not create an audit log +// entry. +func InitRequestWithCancel[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request[T], func(commit bool)) { + req, commitF := InitRequest[T](w, p) + cancelled := false + return req, func(commit bool) { + // Once 'commit=false' is called, block + // any future commit attempts. + if !commit { + cancelled = true + return + } + // If it was ever cancelled, block any commits + if !cancelled { + commitF() + } + } +} + // InitRequest initializes an audit log for a request. It returns a function // that should be deferred, causing the audit log to be committed when the // handler returns. diff --git a/enterprise/coderd/scim.go b/enterprise/coderd/scim.go index 2e638e667e9a1..b7f1bc8d106c4 100644 --- a/enterprise/coderd/scim.go +++ b/enterprise/coderd/scim.go @@ -272,13 +272,14 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { } auditor := *api.AGPL.Auditor.Load() - aReq, commitAudit := audit.InitRequest[database.User](rw, &audit.RequestParams{ + aReq, commitAudit := audit.InitRequestWithCancel[database.User](rw, &audit.RequestParams{ Audit: auditor, Log: api.Logger, Request: r, Action: database.AuditActionWrite, }) - defer commitAudit() + + defer commitAudit(true) id := chi.URLParam(r, "id") @@ -307,23 +308,39 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { var status database.UserStatus if sUser.Active { - // The user will get transitioned to Active after logging in. - status = database.UserStatusDormant + switch dbUser.Status { + case database.UserStatusActive: + // Keep the user active + status = database.UserStatusActive + case database.UserStatusDormant, database.UserStatusSuspended: + // Move (or keep) as dormant + status = database.UserStatusDormant + default: + // If the status is unknown, just move them to dormant. + // The user will get transitioned to Active after logging in. + status = database.UserStatusDormant + } } else { status = database.UserStatusSuspended } - //nolint:gocritic // needed for SCIM - userNew, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{ - ID: dbUser.ID, - Status: status, - UpdatedAt: dbtime.Now(), - }) - if err != nil { - _ = handlerutil.WriteError(rw, err) - return + if dbUser.Status != status { + //nolint:gocritic // needed for SCIM + userNew, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{ + ID: dbUser.ID, + Status: status, + UpdatedAt: dbtime.Now(), + }) + if err != nil { + _ = handlerutil.WriteError(rw, err) + return + } + dbUser = userNew + } else { + // Do not push an audit log if there is no change. + commitAudit(false) } - aReq.New = userNew + aReq.New = dbUser httpapi.Write(ctx, rw, http.StatusOK, sUser) } diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go index 237d53983a1a3..9421c6cf5b785 100644 --- a/enterprise/coderd/scim_test.go +++ b/enterprise/coderd/scim_test.go @@ -8,11 +8,13 @@ import ( "net/http" "testing" + "github.com/golang-jwt/jwt/v4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/coderdtest/oidctest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/cryptorand" @@ -364,5 +366,82 @@ func TestScim(t *testing.T) { require.Len(t, userRes.Users, 1) assert.Equal(t, codersdk.UserStatusSuspended, userRes.Users[0].Status) }) + + // Create a user via SCIM, which starts as dormant. + // Log in as the user, making them active. + // Then patch the user again and the user should still be active. + t.Run("ActiveIsActive", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + scimAPIKey := []byte("hi") + + mockAudit := audit.NewMock() + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + Auditor: mockAudit, + OIDCConfig: fake.OIDCConfig(t, []string{}), + }, + SCIMAPIKey: scimAPIKey, + AuditLogging: true, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, + }, + }, + }) + mockAudit.ResetLogs() + + // User is dormant on create + sUser := makeScimUser(t) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + defer res.Body.Close() + assert.Equal(t, http.StatusOK, res.StatusCode) + + err = json.NewDecoder(res.Body).Decode(&sUser) + require.NoError(t, err) + + // Check the audit log + aLogs := mockAudit.AuditLogs() + require.Len(t, aLogs, 1) + assert.Equal(t, database.AuditActionCreate, aLogs[0].Action) + + // Verify the user is dormant + scimUser, err := client.User(ctx, sUser.UserName) + require.NoError(t, err) + require.Equal(t, codersdk.UserStatusDormant, scimUser.Status, "user starts as dormant") + + // Log in as the user, making them active + //nolint:bodyclose + scimUserClient, _ := fake.Login(t, client, jwt.MapClaims{ + "email": sUser.Emails[0].Value, + }) + scimUser, err = scimUserClient.User(ctx, codersdk.Me) + require.NoError(t, err) + require.Equal(t, codersdk.UserStatusActive, scimUser.Status, "user should now be active") + + // Patch the user + mockAudit.ResetLogs() + res, err = client.Request(ctx, "PATCH", "/scim/v2/Users/"+sUser.ID, sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, res.Body) + _ = res.Body.Close() + assert.Equal(t, http.StatusOK, res.StatusCode) + + // Should be no audit logs since there is no diff + aLogs = mockAudit.AuditLogs() + require.Len(t, aLogs, 0) + + // Verify the user is still active. + scimUser, err = client.User(ctx, sUser.UserName) + require.NoError(t, err) + require.Equal(t, codersdk.UserStatusActive, scimUser.Status, "user is still active") + }) }) }