From f618f35e05ed3bc96d1816268ab45bcbd1c43b38 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jul 2024 14:41:49 -0500 Subject: [PATCH 1/7] chore: scim should keep active users active --- enterprise/coderd/scim.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/enterprise/coderd/scim.go b/enterprise/coderd/scim.go index 2e638e667e9a1..393f9b2b2f15b 100644 --- a/enterprise/coderd/scim.go +++ b/enterprise/coderd/scim.go @@ -307,8 +307,18 @@ 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 } From ef4c4d4cd3b1926fdb3d91f59e9f3a709b6ca1f5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jul 2024 14:58:09 -0500 Subject: [PATCH 2/7] chore: add a unit test to excercise dormancy bug --- enterprise/coderd/scim_test.go | 58 ++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go index 237d53983a1a3..1b4943280aee1 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,61 @@ func TestScim(t *testing.T) { require.Len(t, userRes.Users, 1) assert.Equal(t, codersdk.UserStatusSuspended, userRes.Users[0].Status) }) + + t.Run("ActiveIsActive", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + scimAPIKey := []byte("hi") + + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + OIDCConfig: fake.OIDCConfig(t, []string{}), + }, + SCIMAPIKey: scimAPIKey, + AuditLogging: true, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, + }, + }, + }) + + 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) + + scimUser, err := client.User(ctx, sUser.UserName) + require.NoError(t, err) + require.Equal(t, codersdk.UserStatusDormant, scimUser.Status, "user starts as dormant") + + 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 + 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) + + scimUser, err = client.User(ctx, sUser.UserName) + require.NoError(t, err) + require.Equal(t, codersdk.UserStatusActive, scimUser.Status, "user is still active") + }) }) } From 1b6036d3410d48478cb0674a0b2501faedc4183e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jul 2024 15:06:23 -0500 Subject: [PATCH 3/7] lint' --- enterprise/coderd/scim_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go index 1b4943280aee1..7f5f97cb4f930 100644 --- a/enterprise/coderd/scim_test.go +++ b/enterprise/coderd/scim_test.go @@ -404,6 +404,7 @@ func TestScim(t *testing.T) { require.NoError(t, err) require.Equal(t, codersdk.UserStatusDormant, scimUser.Status, "user starts as dormant") + //nolint:bodyclose scimUserClient, _ := fake.Login(t, client, jwt.MapClaims{ "email": sUser.Emails[0].Value, }) From ca1241bbc0b8181f3e4c59a037c4fec15ef223fd Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jul 2024 15:19:02 -0500 Subject: [PATCH 4/7] audit log should not be dropped when there is no change --- enterprise/coderd/scim_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go index 7f5f97cb4f930..9421c6cf5b785 100644 --- a/enterprise/coderd/scim_test.go +++ b/enterprise/coderd/scim_test.go @@ -367,6 +367,9 @@ func TestScim(t *testing.T) { 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() @@ -375,9 +378,11 @@ func TestScim(t *testing.T) { 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, @@ -390,7 +395,9 @@ func TestScim(t *testing.T) { }, }, }) + 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) @@ -400,10 +407,17 @@ func TestScim(t *testing.T) { 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, @@ -413,12 +427,18 @@ func TestScim(t *testing.T) { 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") From 5ffc82970c2e3ba781736032bf8fbf575fc0036d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jul 2024 15:23:34 -0500 Subject: [PATCH 5/7] add ability to cancel audit log --- coderd/audit/request.go | 14 ++++++++++++++ enterprise/coderd/scim.go | 29 ++++++++++++++++++----------- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 403bb13ccf3f8..1824cc6b6237b 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -267,6 +267,20 @@ func requireOrgID[T Auditable](ctx context.Context, id uuid.UUID, log slog.Logge return id } +// InitRequestWithCancel returns 2 functions. The first commits the audit log, +// the second cancels any future calls to commit. +func InitRequestWithCancel[T Auditable](w http.ResponseWriter, p *RequestParams) (aReq *Request[T], commit func(), cancel func()) { + req, commit := InitRequest[T](w, p) + canceled := false + return req, func() { + if !canceled { + commit() + } + }, func() { + canceled = true + } +} + // 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 393f9b2b2f15b..ee4a2efdcb66e 100644 --- a/enterprise/coderd/scim.go +++ b/enterprise/coderd/scim.go @@ -272,12 +272,13 @@ 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, cancelAudit := audit.InitRequestWithCancel[database.User](rw, &audit.RequestParams{ Audit: auditor, Log: api.Logger, Request: r, Action: database.AuditActionWrite, }) + defer commitAudit() id := chi.URLParam(r, "id") @@ -323,17 +324,23 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { 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. + cancelAudit() } - aReq.New = userNew + aReq.New = dbUser httpapi.Write(ctx, rw, http.StatusOK, sUser) } From 806c28413bbcfe045adc597373d5409a68ab6e44 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jul 2024 15:31:17 -0500 Subject: [PATCH 6/7] add cancellable audit log v2 --- coderd/audit/request.go | 25 ++++++++++++++----------- enterprise/coderd/scim.go | 6 +++--- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 1824cc6b6237b..faba47d00316b 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -267,18 +267,21 @@ func requireOrgID[T Auditable](ctx context.Context, id uuid.UUID, log slog.Logge return id } -// InitRequestWithCancel returns 2 functions. The first commits the audit log, -// the second cancels any future calls to commit. -func InitRequestWithCancel[T Auditable](w http.ResponseWriter, p *RequestParams) (aReq *Request[T], commit func(), cancel func()) { - req, commit := InitRequest[T](w, p) - canceled := false - return req, func() { - if !canceled { - commit() - } - }, func() { - canceled = true +// 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) { + if !commit { + cancelled = true + return } + if !cancelled { + commitF() + } + } } // InitRequest initializes an audit log for a request. It returns a function diff --git a/enterprise/coderd/scim.go b/enterprise/coderd/scim.go index ee4a2efdcb66e..b7f1bc8d106c4 100644 --- a/enterprise/coderd/scim.go +++ b/enterprise/coderd/scim.go @@ -272,14 +272,14 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { } auditor := *api.AGPL.Auditor.Load() - aReq, commitAudit, cancelAudit := audit.InitRequestWithCancel[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") @@ -338,7 +338,7 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { dbUser = userNew } else { // Do not push an audit log if there is no change. - cancelAudit() + commitAudit(false) } aReq.New = dbUser From 73c9928c77dee06a6c5f7cf3e9cab3e11f4fbf90 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jul 2024 15:32:48 -0500 Subject: [PATCH 7/7] Add some comments --- coderd/audit/request.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/coderd/audit/request.go b/coderd/audit/request.go index faba47d00316b..a0ecba163e2b1 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -274,10 +274,13 @@ func InitRequestWithCancel[T Auditable](w http.ResponseWriter, p *RequestParams) 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() }