Skip to content

Commit 2f0c2d7

Browse files
EmyrkEmyrk
authored
chore: keep active users active in scim (#13955) (#13974)
* chore: scim should keep active users active * chore: add a unit test to excercise dormancy bug (cherry picked from commit 03c5d42)
1 parent 82ed9e4 commit 2f0c2d7

File tree

3 files changed

+110
-11
lines changed

3 files changed

+110
-11
lines changed

‎coderd/audit/request.go

+20
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,26 @@ func requireOrgID[T Auditable](ctx context.Context, id uuid.UUID, log slog.Logge
233233
return id
234234
}
235235

236+
// InitRequestWithCancel returns a commit function with a boolean arg.
237+
// If the arg is false, future calls to commit() will not create an audit log
238+
// entry.
239+
func InitRequestWithCancel[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request[T], func(commit bool)) {
240+
req, commitF := InitRequest[T](w, p)
241+
cancelled := false
242+
return req, func(commit bool) {
243+
// Once 'commit=false' is called, block
244+
// any future commit attempts.
245+
if !commit {
246+
cancelled = true
247+
return
248+
}
249+
// If it was ever cancelled, block any commits
250+
if !cancelled {
251+
commitF()
252+
}
253+
}
254+
}
255+
236256
// InitRequest initializes an audit log for a request. It returns a function
237257
// that should be deferred, causing the audit log to be committed when the
238258
// handler returns.

‎enterprise/coderd/scim.go

+24-11
Original file line numberDiff line numberDiff line change
@@ -273,21 +273,34 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) {
273273

274274
var status database.UserStatus
275275
if sUser.Active {
276-
// The user will get transitioned to Active after logging in.
277-
status = database.UserStatusDormant
276+
switch dbUser.Status {
277+
case database.UserStatusActive:
278+
// Keep the user active
279+
status = database.UserStatusActive
280+
case database.UserStatusDormant, database.UserStatusSuspended:
281+
// Move (or keep) as dormant
282+
status = database.UserStatusDormant
283+
default:
284+
// If the status is unknown, just move them to dormant.
285+
// The user will get transitioned to Active after logging in.
286+
status = database.UserStatusDormant
287+
}
278288
} else {
279289
status = database.UserStatusSuspended
280290
}
281291

282-
//nolint:gocritic // needed for SCIM
283-
_, err = api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{
284-
ID: dbUser.ID,
285-
Status: status,
286-
UpdatedAt: dbtime.Now(),
287-
})
288-
if err != nil {
289-
_ = handlerutil.WriteError(rw, err)
290-
return
292+
if dbUser.Status != status {
293+
//nolint:gocritic // needed for SCIM
294+
userNew, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{
295+
ID: dbUser.ID,
296+
Status: status,
297+
UpdatedAt: dbtime.Now(),
298+
})
299+
if err != nil {
300+
_ = handlerutil.WriteError(rw, err)
301+
return
302+
}
303+
dbUser = userNew
291304
}
292305

293306
httpapi.Write(ctx, rw, http.StatusOK, sUser)

‎enterprise/coderd/scim_test.go

+66
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,12 @@ 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

15+
"github.com/coder/coder/v2/coderd/coderdtest"
16+
"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
1417
"github.com/coder/coder/v2/codersdk"
1518
"github.com/coder/coder/v2/cryptorand"
1619
"github.com/coder/coder/v2/enterprise/coderd"
@@ -338,5 +341,68 @@ func TestScim(t *testing.T) {
338341
require.Len(t, userRes.Users, 1)
339342
assert.Equal(t, codersdk.UserStatusSuspended, userRes.Users[0].Status)
340343
})
344+
345+
// Create a user via SCIM, which starts as dormant.
346+
// Log in as the user, making them active.
347+
// Then patch the user again and the user should still be active.
348+
t.Run("ActiveIsActive", func(t *testing.T) {
349+
t.Parallel()
350+
351+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
352+
defer cancel()
353+
354+
scimAPIKey := []byte("hi")
355+
356+
fake := oidctest.NewFakeIDP(t, oidctest.WithServing())
357+
client, _ := coderdenttest.New(t, &coderdenttest.Options{
358+
Options: &coderdtest.Options{
359+
OIDCConfig: fake.OIDCConfig(t, []string{}),
360+
},
361+
SCIMAPIKey: scimAPIKey,
362+
AuditLogging: true,
363+
LicenseOptions: &coderdenttest.LicenseOptions{
364+
AccountID: "coolin",
365+
Features: license.Features{
366+
codersdk.FeatureSCIM: 1,
367+
},
368+
},
369+
})
370+
371+
// User is dormant on create
372+
sUser := makeScimUser(t)
373+
res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey))
374+
require.NoError(t, err)
375+
defer res.Body.Close()
376+
assert.Equal(t, http.StatusOK, res.StatusCode)
377+
378+
err = json.NewDecoder(res.Body).Decode(&sUser)
379+
require.NoError(t, err)
380+
381+
// Verify the user is dormant
382+
scimUser, err := client.User(ctx, sUser.UserName)
383+
require.NoError(t, err)
384+
require.Equal(t, codersdk.UserStatusDormant, scimUser.Status, "user starts as dormant")
385+
386+
// Log in as the user, making them active
387+
//nolint:bodyclose
388+
scimUserClient, _ := fake.Login(t, client, jwt.MapClaims{
389+
"email": sUser.Emails[0].Value,
390+
})
391+
scimUser, err = scimUserClient.User(ctx, codersdk.Me)
392+
require.NoError(t, err)
393+
require.Equal(t, codersdk.UserStatusActive, scimUser.Status, "user should now be active")
394+
395+
// Patch the user
396+
res, err = client.Request(ctx, "PATCH", "/scim/v2/Users/"+sUser.ID, sUser, setScimAuth(scimAPIKey))
397+
require.NoError(t, err)
398+
_, _ = io.Copy(io.Discard, res.Body)
399+
_ = res.Body.Close()
400+
assert.Equal(t, http.StatusOK, res.StatusCode)
401+
402+
// Verify the user is still active.
403+
scimUser, err = client.User(ctx, sUser.UserName)
404+
require.NoError(t, err)
405+
require.Equal(t, codersdk.UserStatusActive, scimUser.Status, "user is still active")
406+
})
341407
})
342408
}

0 commit comments

Comments
 (0)