Skip to content

Commit 32e0271

Browse files
Emyrkstirby
Emyrk
authored and
stirby
committed
chore: keep active users active in scim (#13955)
* chore: scim should keep active users active * chore: add a unit test to excercise dormancy bug * audit log should not be dropped when there is no change * add ability to cancel audit log (cherry picked from commit 03c5d42)
1 parent 5edfccf commit 32e0271

File tree

3 files changed

+130
-14
lines changed

3 files changed

+130
-14
lines changed

‎coderd/audit/request.go

+20
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,26 @@ func requireOrgID[T Auditable](ctx context.Context, id uuid.UUID, log slog.Logge
257257
return id
258258
}
259259

260+
// InitRequestWithCancel returns a commit function with a boolean arg.
261+
// If the arg is false, future calls to commit() will not create an audit log
262+
// entry.
263+
func InitRequestWithCancel[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request[T], func(commit bool)) {
264+
req, commitF := InitRequest[T](w, p)
265+
cancelled := false
266+
return req, func(commit bool) {
267+
// Once 'commit=false' is called, block
268+
// any future commit attempts.
269+
if !commit {
270+
cancelled = true
271+
return
272+
}
273+
// If it was ever cancelled, block any commits
274+
if !cancelled {
275+
commitF()
276+
}
277+
}
278+
}
279+
260280
// InitRequest initializes an audit log for a request. It returns a function
261281
// that should be deferred, causing the audit log to be committed when the
262282
// handler returns.

‎enterprise/coderd/scim.go

+31-14
Original file line numberDiff line numberDiff line change
@@ -272,13 +272,14 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) {
272272
}
273273

274274
auditor := *api.AGPL.Auditor.Load()
275-
aReq, commitAudit := audit.InitRequest[database.User](rw, &audit.RequestParams{
275+
aReq, commitAudit := audit.InitRequestWithCancel[database.User](rw, &audit.RequestParams{
276276
Audit: auditor,
277277
Log: api.Logger,
278278
Request: r,
279279
Action: database.AuditActionWrite,
280280
})
281-
defer commitAudit()
281+
282+
defer commitAudit(true)
282283

283284
id := chi.URLParam(r, "id")
284285

@@ -307,23 +308,39 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) {
307308

308309
var status database.UserStatus
309310
if sUser.Active {
310-
// The user will get transitioned to Active after logging in.
311-
status = database.UserStatusDormant
311+
switch dbUser.Status {
312+
case database.UserStatusActive:
313+
// Keep the user active
314+
status = database.UserStatusActive
315+
case database.UserStatusDormant, database.UserStatusSuspended:
316+
// Move (or keep) as dormant
317+
status = database.UserStatusDormant
318+
default:
319+
// If the status is unknown, just move them to dormant.
320+
// The user will get transitioned to Active after logging in.
321+
status = database.UserStatusDormant
322+
}
312323
} else {
313324
status = database.UserStatusSuspended
314325
}
315326

316-
//nolint:gocritic // needed for SCIM
317-
userNew, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{
318-
ID: dbUser.ID,
319-
Status: status,
320-
UpdatedAt: dbtime.Now(),
321-
})
322-
if err != nil {
323-
_ = handlerutil.WriteError(rw, err)
324-
return
327+
if dbUser.Status != status {
328+
//nolint:gocritic // needed for SCIM
329+
userNew, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{
330+
ID: dbUser.ID,
331+
Status: status,
332+
UpdatedAt: dbtime.Now(),
333+
})
334+
if err != nil {
335+
_ = handlerutil.WriteError(rw, err)
336+
return
337+
}
338+
dbUser = userNew
339+
} else {
340+
// Do not push an audit log if there is no change.
341+
commitAudit(false)
325342
}
326-
aReq.New = userNew
327343

344+
aReq.New = dbUser
328345
httpapi.Write(ctx, rw, http.StatusOK, sUser)
329346
}

‎enterprise/coderd/scim_test.go

+79
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@ import (
88
"net/http"
99
"testing"
1010

11+
"github.com/golang-jwt/jwt/v4"
1112
"github.com/stretchr/testify/assert"
1213
"github.com/stretchr/testify/require"
1314

1415
"github.com/coder/coder/v2/coderd/audit"
1516
"github.com/coder/coder/v2/coderd/coderdtest"
17+
"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
1618
"github.com/coder/coder/v2/coderd/database"
1719
"github.com/coder/coder/v2/codersdk"
1820
"github.com/coder/coder/v2/cryptorand"
@@ -364,5 +366,82 @@ func TestScim(t *testing.T) {
364366
require.Len(t, userRes.Users, 1)
365367
assert.Equal(t, codersdk.UserStatusSuspended, userRes.Users[0].Status)
366368
})
369+
370+
// Create a user via SCIM, which starts as dormant.
371+
// Log in as the user, making them active.
372+
// Then patch the user again and the user should still be active.
373+
t.Run("ActiveIsActive", func(t *testing.T) {
374+
t.Parallel()
375+
376+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
377+
defer cancel()
378+
379+
scimAPIKey := []byte("hi")
380+
381+
mockAudit := audit.NewMock()
382+
fake := oidctest.NewFakeIDP(t, oidctest.WithServing())
383+
client, _ := coderdenttest.New(t, &coderdenttest.Options{
384+
Options: &coderdtest.Options{
385+
Auditor: mockAudit,
386+
OIDCConfig: fake.OIDCConfig(t, []string{}),
387+
},
388+
SCIMAPIKey: scimAPIKey,
389+
AuditLogging: true,
390+
LicenseOptions: &coderdenttest.LicenseOptions{
391+
AccountID: "coolin",
392+
Features: license.Features{
393+
codersdk.FeatureSCIM: 1,
394+
codersdk.FeatureAuditLog: 1,
395+
},
396+
},
397+
})
398+
mockAudit.ResetLogs()
399+
400+
// User is dormant on create
401+
sUser := makeScimUser(t)
402+
res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey))
403+
require.NoError(t, err)
404+
defer res.Body.Close()
405+
assert.Equal(t, http.StatusOK, res.StatusCode)
406+
407+
err = json.NewDecoder(res.Body).Decode(&sUser)
408+
require.NoError(t, err)
409+
410+
// Check the audit log
411+
aLogs := mockAudit.AuditLogs()
412+
require.Len(t, aLogs, 1)
413+
assert.Equal(t, database.AuditActionCreate, aLogs[0].Action)
414+
415+
// Verify the user is dormant
416+
scimUser, err := client.User(ctx, sUser.UserName)
417+
require.NoError(t, err)
418+
require.Equal(t, codersdk.UserStatusDormant, scimUser.Status, "user starts as dormant")
419+
420+
// Log in as the user, making them active
421+
//nolint:bodyclose
422+
scimUserClient, _ := fake.Login(t, client, jwt.MapClaims{
423+
"email": sUser.Emails[0].Value,
424+
})
425+
scimUser, err = scimUserClient.User(ctx, codersdk.Me)
426+
require.NoError(t, err)
427+
require.Equal(t, codersdk.UserStatusActive, scimUser.Status, "user should now be active")
428+
429+
// Patch the user
430+
mockAudit.ResetLogs()
431+
res, err = client.Request(ctx, "PATCH", "/scim/v2/Users/"+sUser.ID, sUser, setScimAuth(scimAPIKey))
432+
require.NoError(t, err)
433+
_, _ = io.Copy(io.Discard, res.Body)
434+
_ = res.Body.Close()
435+
assert.Equal(t, http.StatusOK, res.StatusCode)
436+
437+
// Should be no audit logs since there is no diff
438+
aLogs = mockAudit.AuditLogs()
439+
require.Len(t, aLogs, 0)
440+
441+
// Verify the user is still active.
442+
scimUser, err = client.User(ctx, sUser.UserName)
443+
require.NoError(t, err)
444+
require.Equal(t, codersdk.UserStatusActive, scimUser.Status, "user is still active")
445+
})
367446
})
368447
}

0 commit comments

Comments
 (0)