chore: implement SCIM PUT endpoint, protect against missing active (#15829)

Closes https://github.com/coder/coder/issues/15828
This commit is contained in:
Steven Masley
2024-12-12 08:11:13 -06:00
committed by GitHub
parent 36c2cf8a40
commit d31c2f1fe7
7 changed files with 595 additions and 41 deletions

43
coderd/apidoc/docs.go generated
View File

@ -3829,6 +3829,48 @@ const docTemplate = `{
}
}
},
"put": {
"security": [
{
"Authorization": []
}
],
"produces": [
"application/scim+json"
],
"tags": [
"Enterprise"
],
"summary": "SCIM 2.0: Replace user account",
"operationId": "scim-replace-user-status",
"parameters": [
{
"type": "string",
"format": "uuid",
"description": "User ID",
"name": "id",
"in": "path",
"required": true
},
{
"description": "Replace user request",
"name": "request",
"in": "body",
"required": true,
"schema": {
"$ref": "#/definitions/coderd.SCIMUser"
}
}
],
"responses": {
"200": {
"description": "OK",
"schema": {
"$ref": "#/definitions/codersdk.User"
}
}
}
},
"patch": {
"security": [
{
@ -9126,6 +9168,7 @@ const docTemplate = `{
"type": "object",
"properties": {
"active": {
"description": "Active is a ptr to prevent the empty value from being interpreted as false.",
"type": "boolean"
},
"emails": {

View File

@ -3367,6 +3367,44 @@
}
}
},
"put": {
"security": [
{
"Authorization": []
}
],
"produces": ["application/scim+json"],
"tags": ["Enterprise"],
"summary": "SCIM 2.0: Replace user account",
"operationId": "scim-replace-user-status",
"parameters": [
{
"type": "string",
"format": "uuid",
"description": "User ID",
"name": "id",
"in": "path",
"required": true
},
{
"description": "Replace user request",
"name": "request",
"in": "body",
"required": true,
"schema": {
"$ref": "#/definitions/coderd.SCIMUser"
}
}
],
"responses": {
"200": {
"description": "OK",
"schema": {
"$ref": "#/definitions/codersdk.User"
}
}
}
},
"patch": {
"security": [
{
@ -8078,6 +8116,7 @@
"type": "object",
"properties": {
"active": {
"description": "Active is a ptr to prevent the empty value from being interpreted as false.",
"type": "boolean"
},
"emails": {

View File

@ -2270,6 +2270,90 @@ curl -X GET http://coder-server:8080/api/v2/scim/v2/Users/{id} \
To perform this operation, you must be authenticated. [Learn more](authentication.md).
## SCIM 2.0: Replace user account
### Code samples
```shell
# Example request using curl
curl -X PUT http://coder-server:8080/api/v2/scim/v2/Users/{id} \
-H 'Content-Type: application/json' \
-H 'Accept: application/scim+json' \
-H 'Authorizaiton: API_KEY'
```
`PUT /scim/v2/Users/{id}`
> Body parameter
```json
{
"active": true,
"emails": [
{
"display": "string",
"primary": true,
"type": "string",
"value": "user@example.com"
}
],
"groups": [null],
"id": "string",
"meta": {
"resourceType": "string"
},
"name": {
"familyName": "string",
"givenName": "string"
},
"schemas": ["string"],
"userName": "string"
}
```
### Parameters
| Name | In | Type | Required | Description |
| ------ | ---- | -------------------------------------------- | -------- | -------------------- |
| `id` | path | string(uuid) | true | User ID |
| `body` | body | [coderd.SCIMUser](schemas.md#coderdscimuser) | true | Replace user request |
### Example responses
> 200 Response
```json
{
"avatar_url": "http://example.com",
"created_at": "2019-08-24T14:15:22Z",
"email": "user@example.com",
"id": "497f6eca-6276-4993-bfeb-53cbbbba6f08",
"last_seen_at": "2019-08-24T14:15:22Z",
"login_type": "",
"name": "string",
"organization_ids": ["497f6eca-6276-4993-bfeb-53cbbbba6f08"],
"roles": [
{
"display_name": "string",
"name": "string",
"organization_id": "string"
}
],
"status": "active",
"theme_preference": "string",
"updated_at": "2019-08-24T14:15:22Z",
"username": "string"
}
```
### Responses
| Status | Meaning | Description | Schema |
| ------ | ------------------------------------------------------- | ----------- | ---------------------------------------- |
| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [codersdk.User](schemas.md#codersdkuser) |
To perform this operation, you must be authenticated. [Learn more](authentication.md).
## SCIM 2.0: Update user account
### Code samples

View File

@ -187,23 +187,23 @@
### Properties
| Name | Type | Required | Restrictions | Description |
| ---------------- | ------------------ | -------- | ------------ | ----------- |
| `active` | boolean | false | | |
| `emails` | array of object | false | | |
| `» display` | string | false | | |
| `» primary` | boolean | false | | |
| `» type` | string | false | | |
| `» value` | string | false | | |
| `groups` | array of undefined | false | | |
| `id` | string | false | | |
| `meta` | object | false | | |
| `» resourceType` | string | false | | |
| `name` | object | false | | |
| `» familyName` | string | false | | |
| `» givenName` | string | false | | |
| `schemas` | array of string | false | | |
| `userName` | string | false | | |
| Name | Type | Required | Restrictions | Description |
| ---------------- | ------------------ | -------- | ------------ | --------------------------------------------------------------------------- |
| `active` | boolean | false | | Active is a ptr to prevent the empty value from being interpreted as false. |
| `emails` | array of object | false | | |
| `» display` | string | false | | |
| `» primary` | boolean | false | | |
| `» type` | string | false | | |
| `» value` | string | false | | |
| `groups` | array of undefined | false | | |
| `id` | string | false | | |
| `meta` | object | false | | |
| `» resourceType` | string | false | | |
| `name` | object | false | | |
| `» familyName` | string | false | | |
| `» givenName` | string | false | | |
| `schemas` | array of string | false | | |
| `userName` | string | false | | |
## coderd.cspViolation

View File

@ -488,6 +488,7 @@ func New(ctx context.Context, options *Options) (_ *API, err error) {
r.Post("/", api.scimPostUser)
r.Get("/{id}", api.scimGetUser)
r.Patch("/{id}", api.scimPatchUser)
r.Put("/{id}", api.scimPutUser)
})
r.NotFound(func(w http.ResponseWriter, r *http.Request) {
u := r.URL.String()

View File

@ -173,7 +173,8 @@ type SCIMUser struct {
Type string `json:"type"`
Display string `json:"display"`
} `json:"emails"`
Active bool `json:"active"`
// Active is a ptr to prevent the empty value from being interpreted as false.
Active *bool `json:"active"`
Groups []interface{} `json:"groups"`
Meta struct {
ResourceType string `json:"resourceType"`
@ -219,6 +220,11 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) {
return
}
if sUser.Active == nil {
_ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "invalidRequest", xerrors.New("active field is required")))
return
}
email := ""
for _, e := range sUser.Emails {
if e.Primary {
@ -245,7 +251,7 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) {
sUser.ID = dbUser.ID.String()
sUser.UserName = dbUser.Username
if sUser.Active && dbUser.Status == database.UserStatusSuspended {
if *sUser.Active && dbUser.Status == database.UserStatusSuspended {
//nolint:gocritic
newUser, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{
ID: dbUser.ID,
@ -380,29 +386,17 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) {
aReq.Old = dbUser
aReq.UserID = dbUser.ID
var status database.UserStatus
if sUser.Active {
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
if sUser.Active == nil {
_ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "invalidRequest", xerrors.New("active field is required")))
return
}
if dbUser.Status != status {
newStatus := scimUserStatus(dbUser, *sUser.Active)
if dbUser.Status != newStatus {
//nolint:gocritic // needed for SCIM
userNew, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{
ID: dbUser.ID,
Status: status,
Status: newStatus,
UpdatedAt: dbtime.Now(),
})
if err != nil {
@ -418,3 +412,127 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) {
aReq.New = dbUser
httpapi.Write(ctx, rw, http.StatusOK, sUser)
}
// scimPutUser supports suspending and activating users only.
// TODO: SCIM specification requires that the PUT method should replace the entire user object.
// At present, our fields read as 'immutable' except for the 'active' field.
// See: https://datatracker.ietf.org/doc/html/rfc7644#section-3.5.1
//
// @Summary SCIM 2.0: Replace user account
// @ID scim-replace-user-status
// @Security Authorization
// @Produce application/scim+json
// @Tags Enterprise
// @Param id path string true "User ID" format(uuid)
// @Param request body coderd.SCIMUser true "Replace user request"
// @Success 200 {object} codersdk.User
// @Router /scim/v2/Users/{id} [put]
func (api *API) scimPutUser(rw http.ResponseWriter, r *http.Request) {
ctx := r.Context()
if !api.scimVerifyAuthHeader(r) {
scimUnauthorized(rw)
return
}
auditor := *api.AGPL.Auditor.Load()
aReq, commitAudit := audit.InitRequestWithCancel[database.User](rw, &audit.RequestParams{
Audit: auditor,
Log: api.Logger,
Request: r,
Action: database.AuditActionWrite,
})
defer commitAudit(true)
id := chi.URLParam(r, "id")
var sUser SCIMUser
err := json.NewDecoder(r.Body).Decode(&sUser)
if err != nil {
_ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "invalidRequest", err))
return
}
sUser.ID = id
if sUser.Active == nil {
_ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "invalidRequest", xerrors.New("active field is required")))
return
}
uid, err := uuid.Parse(id)
if err != nil {
_ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "invalidId", xerrors.Errorf("id must be a uuid: %w", err)))
return
}
//nolint:gocritic // needed for SCIM
dbUser, err := api.Database.GetUserByID(dbauthz.AsSystemRestricted(ctx), uid)
if err != nil {
_ = handlerutil.WriteError(rw, err) // internal error
return
}
aReq.Old = dbUser
aReq.UserID = dbUser.ID
// Technically our immutability rules dictate that we should not allow
// fields to be changed. According to the SCIM specification, this error should
// be returned.
// This immutability enforcement only exists because we have not implemented it
// yet. If these rules are causing errors, this code should be updated to allow
// the fields to be changed.
// TODO: Currently ignoring a lot of the SCIM fields. Coder's SCIM implementation
// is very basic and only supports active status changes.
if immutabilityViolation(dbUser.Username, sUser.UserName) {
_ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "mutability", xerrors.Errorf("username is currently an immutable field, and cannot be changed. Current: %s, New: %s", dbUser.Username, sUser.UserName)))
return
}
newStatus := scimUserStatus(dbUser, *sUser.Active)
if dbUser.Status != newStatus {
//nolint:gocritic // needed for SCIM
userNew, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{
ID: dbUser.ID,
Status: newStatus,
UpdatedAt: dbtime.Now(),
})
if err != nil {
_ = handlerutil.WriteError(rw, err) // internal error
return
}
dbUser = userNew
} else {
// Do not push an audit log if there is no change.
commitAudit(false)
}
aReq.New = dbUser
httpapi.Write(ctx, rw, http.StatusOK, sUser)
}
func immutabilityViolation[T comparable](old, new T) bool {
var empty T
if new == empty {
// No change
return false
}
return old != new
}
//nolint:revive // active is not a control flag
func scimUserStatus(user database.User, active bool) database.UserStatus {
if !active {
return database.UserStatusSuspended
}
switch user.Status {
case database.UserStatusActive:
// Keep the user active
return database.UserStatusActive
case database.UserStatusDormant, database.UserStatusSuspended:
// Move (or keep) as dormant
return database.UserStatusDormant
default:
// If the status is unknown, just move them to dormant.
// The user will get transitioned to Active after logging in.
return database.UserStatusDormant
}
}

View File

@ -21,6 +21,7 @@ import (
"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/notifications/notificationstest"
"github.com/coder/coder/v2/coderd/util/ptr"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/cryptorand"
"github.com/coder/coder/v2/enterprise/coderd"
@ -52,7 +53,7 @@ func makeScimUser(t testing.TB) coderd.SCIMUser {
}{
{Primary: true, Value: fmt.Sprintf("%s@coder.com", rstr)},
},
Active: true,
Active: ptr.Ref(true),
}
}
@ -355,14 +356,14 @@ func TestScim(t *testing.T) {
err = json.NewDecoder(res.Body).Decode(&sUser)
require.NoError(t, err)
sUser.Active = false
sUser.Active = ptr.Ref(false)
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)
sUser.Active = true
sUser.Active = ptr.Ref(true)
res, err = client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey))
require.NoError(t, err)
_, _ = io.Copy(io.Discard, res.Body)
@ -495,7 +496,7 @@ func TestScim(t *testing.T) {
err = json.NewDecoder(res.Body).Decode(&sUser)
require.NoError(t, err)
sUser.Active = false
sUser.Active = ptr.Ref(false)
res, err = client.Request(ctx, "PATCH", "/scim/v2/Users/"+sUser.ID, sUser, setScimAuth(scimAPIKey))
require.NoError(t, err)
@ -590,6 +591,274 @@ func TestScim(t *testing.T) {
require.Equal(t, codersdk.UserStatusActive, scimUser.Status, "user is still active")
})
})
t.Run("putUser", func(t *testing.T) {
t.Parallel()
t.Run("disabled", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
client, _ := coderdenttest.New(t, &coderdenttest.Options{
SCIMAPIKey: []byte("hi"),
LicenseOptions: &coderdenttest.LicenseOptions{
AccountID: "coolin",
Features: license.Features{
codersdk.FeatureSCIM: 0,
},
},
})
res, err := client.Request(ctx, http.MethodPut, "/scim/v2/Users/bob", struct{}{})
require.NoError(t, err)
_, _ = io.Copy(io.Discard, res.Body)
_ = res.Body.Close()
assert.Equal(t, http.StatusForbidden, res.StatusCode)
})
t.Run("noAuth", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
client, _ := coderdenttest.New(t, &coderdenttest.Options{
SCIMAPIKey: []byte("hi"),
LicenseOptions: &coderdenttest.LicenseOptions{
AccountID: "coolin",
Features: license.Features{
codersdk.FeatureSCIM: 1,
},
},
})
res, err := client.Request(ctx, http.MethodPut, "/scim/v2/Users/bob", struct{}{})
require.NoError(t, err)
_, _ = io.Copy(io.Discard, res.Body)
_ = res.Body.Close()
assert.Equal(t, http.StatusUnauthorized, res.StatusCode)
})
t.Run("MissingActiveField", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
scimAPIKey := []byte("hi")
mockAudit := audit.NewMock()
client, _ := coderdenttest.New(t, &coderdenttest.Options{
Options: &coderdtest.Options{Auditor: mockAudit},
SCIMAPIKey: scimAPIKey,
AuditLogging: true,
LicenseOptions: &coderdenttest.LicenseOptions{
AccountID: "coolin",
Features: license.Features{
codersdk.FeatureSCIM: 1,
codersdk.FeatureAuditLog: 1,
},
},
})
mockAudit.ResetLogs()
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)
mockAudit.ResetLogs()
err = json.NewDecoder(res.Body).Decode(&sUser)
require.NoError(t, err)
sUser.Active = nil
res, err = client.Request(ctx, http.MethodPut, "/scim/v2/Users/"+sUser.ID, sUser, setScimAuth(scimAPIKey))
require.NoError(t, err)
defer res.Body.Close()
assert.Equal(t, http.StatusBadRequest, res.StatusCode)
data, err := io.ReadAll(res.Body)
require.NoError(t, err)
require.Contains(t, string(data), "active field is required")
mockAudit.ResetLogs()
})
t.Run("ImmutabilityViolation", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
scimAPIKey := []byte("hi")
mockAudit := audit.NewMock()
client, _ := coderdenttest.New(t, &coderdenttest.Options{
Options: &coderdtest.Options{Auditor: mockAudit},
SCIMAPIKey: scimAPIKey,
AuditLogging: true,
LicenseOptions: &coderdenttest.LicenseOptions{
AccountID: "coolin",
Features: license.Features{
codersdk.FeatureSCIM: 1,
codersdk.FeatureAuditLog: 1,
},
},
})
mockAudit.ResetLogs()
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)
mockAudit.ResetLogs()
err = json.NewDecoder(res.Body).Decode(&sUser)
require.NoError(t, err)
sUser.UserName += "changed"
res, err = client.Request(ctx, http.MethodPut, "/scim/v2/Users/"+sUser.ID, sUser, setScimAuth(scimAPIKey))
require.NoError(t, err)
defer res.Body.Close()
assert.Equal(t, http.StatusBadRequest, res.StatusCode)
mockAudit.ResetLogs()
data, err := io.ReadAll(res.Body)
require.NoError(t, err)
require.Contains(t, string(data), "mutability")
require.NoError(t, err)
})
t.Run("OK", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
scimAPIKey := []byte("hi")
mockAudit := audit.NewMock()
client, _ := coderdenttest.New(t, &coderdenttest.Options{
Options: &coderdtest.Options{Auditor: mockAudit},
SCIMAPIKey: scimAPIKey,
AuditLogging: true,
LicenseOptions: &coderdenttest.LicenseOptions{
AccountID: "coolin",
Features: license.Features{
codersdk.FeatureSCIM: 1,
codersdk.FeatureAuditLog: 1,
},
},
})
mockAudit.ResetLogs()
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)
mockAudit.ResetLogs()
err = json.NewDecoder(res.Body).Decode(&sUser)
require.NoError(t, err)
sUser.Active = ptr.Ref(false)
res, err = client.Request(ctx, http.MethodPatch, "/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)
aLogs := mockAudit.AuditLogs()
require.Len(t, aLogs, 1)
assert.Equal(t, database.AuditActionWrite, aLogs[0].Action)
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, 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, http.MethodPost, "/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, http.MethodPut, "/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")
})
})
}
func TestScimError(t *testing.T) {