From 46eaffdcf448ee2fbc6bc682e7980f0e9257fe49 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 19 May 2026 16:36:24 -0500 Subject: [PATCH 01/33] feat: work on scim 2.0 compliance --- enterprise/coderd/coderd.go | 39 ++++++++------ enterprise/coderd/scim/scim.go | 60 +++++++++++++++++++++ enterprise/coderd/scim/scimusers.go | 82 +++++++++++++++++++++++++++++ go.mod | 4 ++ go.sum | 8 +++ 5 files changed, 177 insertions(+), 16 deletions(-) create mode 100644 enterprise/coderd/scim/scim.go create mode 100644 enterprise/coderd/scim/scimusers.go diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 82fdd4d27f405..d6050256d2752 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -55,6 +55,7 @@ import ( "github.com/coder/coder/v2/enterprise/coderd/prebuilds" "github.com/coder/coder/v2/enterprise/coderd/proxyhealth" "github.com/coder/coder/v2/enterprise/coderd/schedule" + "github.com/coder/coder/v2/enterprise/coderd/scim" "github.com/coder/coder/v2/enterprise/coderd/usage" entchatd "github.com/coder/coder/v2/enterprise/coderd/x/chatd" "github.com/coder/coder/v2/enterprise/dbcrypt" @@ -622,27 +623,33 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { }) }) + // TODO: Move this up and on the api struct maybe? + scimSrv := scim.New(&scim.Options{ + DB: options.Database, + }) if len(options.SCIMAPIKey) != 0 { api.AGPL.RootHandler.Route("/scim/v2", func(r chi.Router) { + // TODO: Should auth be handled here or in the scimSrv? r.Use( api.RequireFeatureMW(codersdk.FeatureSCIM), ) - r.Get("/ServiceProviderConfig", api.scimServiceProviderConfig) - r.Post("/Users", api.scimPostUser) - r.Route("/Users", func(r chi.Router) { - r.Get("/", api.scimGetUsers) - 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() - httpapi.Write(r.Context(), w, http.StatusNotFound, codersdk.Response{ - Message: fmt.Sprintf("SCIM endpoint %s not found", u), - Detail: "This endpoint is not implemented. If it is correct and required, please contact support.", - }) - }) + r.Mount("/", scimSrv) + //r.Get("/ServiceProviderConfig", api.scimServiceProviderConfig) + //r.Post("/Users", api.scimPostUser) + //r.Route("/Users", func(r chi.Router) { + // r.Get("/", api.scimGetUsers) + // 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() + // httpapi.Write(r.Context(), w, http.StatusNotFound, codersdk.Response{ + // Message: fmt.Sprintf("SCIM endpoint %s not found", u), + // Detail: "This endpoint is not implemented. If it is correct and required, please contact support.", + // }) + //}) }) } else { // Show a helpful 404 error. Because this is not under the /api/v2 routes, diff --git a/enterprise/coderd/scim/scim.go b/enterprise/coderd/scim/scim.go new file mode 100644 index 0000000000000..a0e43cd5d76f4 --- /dev/null +++ b/enterprise/coderd/scim/scim.go @@ -0,0 +1,60 @@ +package scim + +import ( + "github.com/elimity-com/scim" + "github.com/elimity-com/scim/optional" + "github.com/elimity-com/scim/schema" + + "github.com/coder/coder/v2/coderd/database" +) + +type Server struct { + opts *Options + *scim.Server +} + +type Options struct { + DB database.Store +} + +func New(opts *Options) *Server { + args := &scim.ServerArgs{ + ServiceProviderConfig: &scim.ServiceProviderConfig{ + DocumentationURI: optional.NewString("https://coder.com/docs/admin/users/oidc-auth#scim"), + AuthenticationSchemes: []scim.AuthenticationScheme{ + { + Type: scim.AuthenticationTypeOauthBearerToken, + Name: "HTTP Header Authentication", + Description: "Authentication scheme using the Authorization header with the shared token", + // TODO: Add docs for this and link here + SpecURI: optional.String{}, + DocumentationURI: optional.String{}, + Primary: true, + }, + }, + MaxResults: 0, + SupportFiltering: false, + SupportPatch: false, + }, + ResourceTypes: []scim.ResourceType{ + { + ID: optional.NewString("User"), + Name: "User", + Description: optional.String{}, + Endpoint: "/User", + Schema: schema.CoreUserSchema(), + SchemaExtensions: nil, + Handler: nil, + }, + }, + } + + srv, err := scim.NewServer(args) + if err != nil { + return nil + } + + return &Server{ + Server: &srv, + } +} diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go new file mode 100644 index 0000000000000..9e2ac0ae489ee --- /dev/null +++ b/enterprise/coderd/scim/scimusers.go @@ -0,0 +1,82 @@ +package scim + +import ( + "fmt" + "net/http" + + "github.com/elimity-com/scim" + "github.com/elimity-com/scim/optional" + "github.com/google/uuid" + + "github.com/coder/coder/v2/coderd/database" +) + +var _ scim.ResourceHandler = (*ResourceUser)(nil) + +type ResourceUser struct { + store database.Store +} + +func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttributes) (scim.Resource, error) { + //TODO implement me + panic("implement me") +} + +func (ru *ResourceUser) Get(r *http.Request, idStr string) (scim.Resource, error) { + ctx := r.Context() + id, err := uuid.Parse(idStr) + if err != nil { + return scim.Resource{}, fmt.Errorf("invalid user ID %q: %w", idStr, err) + } + + usr, err := ru.store.GetUserByID(ctx, id) + if err != nil { + return scim.Resource{}, err + } + + return userResource(usr), nil +} + +func (ru *ResourceUser) GetAll(r *http.Request, params scim.ListRequestParams) (scim.Page, error) { + //TODO implement me + panic("implement me") +} + +func (ru *ResourceUser) Replace(r *http.Request, id string, attributes scim.ResourceAttributes) (scim.Resource, error) { + //TODO implement me + panic("implement me") +} + +func (ru *ResourceUser) Delete(r *http.Request, id string) error { + //TODO implement me + panic("implement me") +} + +func (ru *ResourceUser) Patch(r *http.Request, id string, operations []scim.PatchOperation) (scim.Resource, error) { + //TODO implement me + panic("implement me") +} + +func userResource(u database.User) scim.Resource { + return scim.Resource{ + ID: u.ID.String(), + ExternalID: optional.String{}, + Attributes: scim.ResourceAttributes{ + "name": map[string]interface{}{ + "givenName": u.Username, + }, + "emails": []map[string]interface{}{ + { + "primary": true, + "value": u.Email, + }, + }, + "active": u.Status == database.UserStatusActive, + // TODO: Groups + }, + Meta: scim.Meta{ + Created: &u.CreatedAt, + LastModified: &u.UpdatedAt, + }, + } +} diff --git a/go.mod b/go.mod index e0c11bb5fb7a5..8d8207d0622f2 100644 --- a/go.mod +++ b/go.mod @@ -512,6 +512,7 @@ require ( github.com/danieljoos/wincred v1.2.3 github.com/dgraph-io/ristretto/v2 v2.4.0 github.com/elazarl/goproxy v1.8.0 + github.com/elimity-com/scim v0.0.0-20260506142751-830e1caafcc3 github.com/fsnotify/fsnotify v1.10.1 github.com/go-git/go-git/v5 v5.19.1 github.com/invopop/jsonschema v0.14.0 @@ -571,6 +572,8 @@ require ( github.com/cpuguy83/go-md2man/v2 v2.0.7 // indirect github.com/daixiang0/gci v0.13.7 // indirect github.com/decred/dcrd/dcrec/secp256k1/v4 v4.4.0 // indirect + github.com/di-wu/parser v0.2.2 // indirect + github.com/di-wu/xsd-datetime v1.0.0 // indirect github.com/distribution/reference v0.6.0 // indirect github.com/envoyproxy/go-control-plane/envoy v1.37.0 // indirect github.com/envoyproxy/protoc-gen-validate v1.3.3 // indirect @@ -622,6 +625,7 @@ require ( github.com/rhysd/actionlint v1.7.10 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect github.com/samber/lo v1.52.0 // indirect + github.com/scim2/filter-parser/v2 v2.2.0 // indirect github.com/segmentio/asm v1.2.1 // indirect github.com/sergeymakinen/go-bmp v1.0.0 // indirect github.com/sergeymakinen/go-ico v1.0.0-beta.0 // indirect diff --git a/go.sum b/go.sum index 644911e5ec82b..8f3d128fb098a 100644 --- a/go.sum +++ b/go.sum @@ -418,6 +418,10 @@ github.com/dgryski/trifles v0.0.0-20230903005119-f50d829f2e54 h1:SG7nF6SRlWhcT7c github.com/dgryski/trifles v0.0.0-20230903005119-f50d829f2e54/go.mod h1:if7Fbed8SFyPtHLHbg49SI7NAdJiC5WIA09pe59rfAA= github.com/dhui/dktest v0.4.6 h1:+DPKyScKSEp3VLtbMDHcUq6V5Lm5zfZZVb0Sk7Ahom4= github.com/dhui/dktest v0.4.6/go.mod h1:JHTSYDtKkvFNFHJKqCzVzqXecyv+tKt8EzceOmQOgbU= +github.com/di-wu/parser v0.2.2 h1:I9oHJ8spBXOeL7Wps0ffkFFFiXJf/pk7NX9lcAMqRMU= +github.com/di-wu/parser v0.2.2/go.mod h1:SLp58pW6WamdmznrVRrw2NTyn4wAvT9rrEFynKX7nYo= +github.com/di-wu/xsd-datetime v1.0.0 h1:vZoGNkbzpBNoc+JyfVLEbutNDNydYV8XwHeV7eUJoxI= +github.com/di-wu/xsd-datetime v1.0.0/go.mod h1:i3iEhrP3WchwseOBeIdW/zxeoleXTOzx1WyDXgdmOww= github.com/distribution/reference v0.6.0 h1:0IXCQ5g4/QMHHkarYzh5l+u8T3t73zM5QvfrDyIgxBk= github.com/distribution/reference v0.6.0/go.mod h1:BbU0aIcezP1/5jX/8MP0YiH4SdvB5Y4f/wlDRiLyi3E= github.com/dlclark/regexp2 v1.4.0/go.mod h1:2pZnwuY/m+8K6iRw6wQdMtk+rH5tNGR1i55kozfMjCc= @@ -448,6 +452,8 @@ github.com/elastic/go-windows v1.0.0 h1:qLURgZFkkrYyTTkvYpsZIgf83AUsdIHfvlJaqaZ7 github.com/elastic/go-windows v1.0.0/go.mod h1:TsU0Nrp7/y3+VwE82FoZF8gC/XFg/Elz6CcloAxnPgU= github.com/elazarl/goproxy v1.8.0 h1:dt561rX7UAYMeFRLtzFx6uQGl2TpL1dr6uCG23nFQSY= github.com/elazarl/goproxy v1.8.0/go.mod h1:b5xm6W48AUHNpRTCvlnd0YVh+JafCCtsLsJZvvNTz+E= +github.com/elimity-com/scim v0.0.0-20260506142751-830e1caafcc3 h1:P+JJLBS2QNe5aWBpNoDWqmGwNv/DKP+WZpU/mPIS+28= +github.com/elimity-com/scim v0.0.0-20260506142751-830e1caafcc3/go.mod h1:JkjcmqbLW+khwt2fmBPJFBhx2zGZ8XobRZ+O0VhlwWo= github.com/emersion/go-sasl v0.0.0-20200509203442-7bfe0ed36a21 h1:OJyUGMJTzHTd1XQp98QTaHernxMYzRaOasRir9hUlFQ= github.com/emersion/go-sasl v0.0.0-20200509203442-7bfe0ed36a21/go.mod h1:iL2twTeMvZnrg54ZoPDNfJaJaqy0xIQFuBdrLsmspwQ= github.com/emersion/go-smtp v0.21.2 h1:OLDgvZKuofk4em9fT5tFG5j4jE1/hXnX75UMvcrL4AA= @@ -1063,6 +1069,8 @@ github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 h1:KRzFb2m7YtdldCEkzs6KqmJw4nqEV github.com/santhosh-tekuri/jsonschema/v6 v6.0.2/go.mod h1:JXeL+ps8p7/KNMjDQk3TCwPpBy0wYklyWTfbkIzdIFU= github.com/satori/go.uuid v1.2.1-0.20181028125025-b2ce2384e17b h1:gQZ0qzfKHQIybLANtM3mBXNUtOfsCFXeTsnBqCsx1KM= github.com/satori/go.uuid v1.2.1-0.20181028125025-b2ce2384e17b/go.mod h1:dA0hQrYB0VpLJoorglMZABFdXlWrHn1NEOzdhQKdks0= +github.com/scim2/filter-parser/v2 v2.2.0 h1:QGadEcsmypxg8gYChRSM2j1edLyE/2j72j+hdmI4BJM= +github.com/scim2/filter-parser/v2 v2.2.0/go.mod h1:jWnkDToqX/Y0ugz0P5VvpVEUKcWcyHHj+X+je9ce5JA= github.com/secure-systems-lab/go-securesystemslib v0.10.0 h1:l+H5ErcW0PAehBNrBxoGv1jjNpGYdZ9RcheFkB2WI14= github.com/secure-systems-lab/go-securesystemslib v0.10.0/go.mod h1:MRKONWmRoFzPNQ9USRF9i1mc7MvAVvF1LlW8X5VWDvk= github.com/segmentio/asm v1.2.1 h1:DTNbBqs57ioxAD4PrArqftgypG4/qNpXoJx8TVXxPR0= From a2d0e0d5ba030aae40ecc06a75409f2ea214b5a6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 19 May 2026 16:59:03 -0500 Subject: [PATCH 02/33] scim auth --- coderd/database/dbauthz/dbauthz.go | 25 ++++++++++++++++++++++ coderd/rbac/authz.go | 1 + enterprise/coderd/coderd.go | 33 +++++++++++------------------- 3 files changed, 38 insertions(+), 21 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index e0d5617c1a8dc..1fb97c3353e5d 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -741,6 +741,25 @@ var ( }), Scope: rbac.ScopeAll, }.WithCachedASTValue() + + subjectSCIM = rbac.Subject{ + Type: rbac.SubjectSCIMProvisioner, + FriendlyName: "SCIM Provisioner", + ID: uuid.Nil.String(), + Roles: rbac.Roles([]rbac.Role{ + { + Identifier: rbac.RoleIdentifier{Name: "scim"}, + DisplayName: "SCIM", + Site: rbac.Permissions(map[string][]policy.Action{ + rbac.ResourceUser.Type: {policy.ActionCreate, policy.ActionUpdate, policy.ActionRead}, + rbac.ResourceOrganization.Type: {policy.ActionRead}, + }), + User: []rbac.Permission{}, + ByOrgID: map[string]rbac.OrgPermissions{}, + }, + }), + Scope: rbac.ScopeAll, + }.WithCachedASTValue() ) // AsProvisionerd returns a context with an actor that has permissions required @@ -871,6 +890,12 @@ func AsAIProviderMetadataReader(ctx context.Context) context.Context { return As(ctx, subjectAIProviderMetadataReader) } +// AsSubAgentAPI returns a context with an actor that has permissions required for +// handling the /scim/v2 routes and provisioning users via SCIM. +func AsSCIMProvisioner(ctx context.Context) context.Context { + return As(ctx, subjectSCIM) +} + var AsRemoveActor = rbac.Subject{ ID: "remove-actor", } diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 5d40e59cc6fac..155b1907a498e 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -85,6 +85,7 @@ const ( SubjectTypeWorkspaceBuilder SubjectType = "workspace_builder" SubjectTypeChatd SubjectType = "chatd" SubjectTypeAIProviderMetadataReader SubjectType = "ai_provider_metadata_reader" + SubjectSCIMProvisioner SubjectType = "scim_provisioner" ) const ( diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index d6050256d2752..688b4fb7c19fd 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -623,33 +623,24 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { }) }) - // TODO: Move this up and on the api struct maybe? - scimSrv := scim.New(&scim.Options{ - DB: options.Database, + // TODO: Possibly lift this scimSrv? + scimSrv, err := scim.New(&scim.Options{ + DB: options.Database, + Auditor: &api.AGPL.Auditor, + IDPSync: options.IDPSync, + Logger: options.Logger, + AGPL: api.AGPL, + SCIMAPIKey: options.SCIMAPIKey, }) + if err != nil { + return nil, xerrors.Errorf("create scim server: %w", err) + } if len(options.SCIMAPIKey) != 0 { api.AGPL.RootHandler.Route("/scim/v2", func(r chi.Router) { - // TODO: Should auth be handled here or in the scimSrv? r.Use( api.RequireFeatureMW(codersdk.FeatureSCIM), ) - r.Mount("/", scimSrv) - //r.Get("/ServiceProviderConfig", api.scimServiceProviderConfig) - //r.Post("/Users", api.scimPostUser) - //r.Route("/Users", func(r chi.Router) { - // r.Get("/", api.scimGetUsers) - // 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() - // httpapi.Write(r.Context(), w, http.StatusNotFound, codersdk.Response{ - // Message: fmt.Sprintf("SCIM endpoint %s not found", u), - // Detail: "This endpoint is not implemented. If it is correct and required, please contact support.", - // }) - //}) + r.Mount("/", scimSrv.Handler()) }) } else { // Show a helpful 404 error. Because this is not under the /api/v2 routes, From a277a6eda7cbfaa43ae20e7e491a8bb8718c493f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 19 May 2026 17:07:20 -0500 Subject: [PATCH 03/33] working on primary auth mw --- enterprise/coderd/scim/scim.go | 110 +++++++++++++++++++++++++++------ 1 file changed, 90 insertions(+), 20 deletions(-) diff --git a/enterprise/coderd/scim/scim.go b/enterprise/coderd/scim/scim.go index a0e43cd5d76f4..79d32054dc954 100644 --- a/enterprise/coderd/scim/scim.go +++ b/enterprise/coderd/scim/scim.go @@ -1,60 +1,130 @@ package scim import ( + "bytes" + "crypto/subtle" + "encoding/json" + "net/http" + "sync/atomic" + "github.com/elimity-com/scim" + scimErrors "github.com/elimity-com/scim/errors" "github.com/elimity-com/scim/optional" "github.com/elimity-com/scim/schema" + "cdr.dev/slog/v3" + + agpl "github.com/coder/coder/v2/coderd" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/idpsync" ) +// Server wraps the elimity-com/scim library's Server to implement +// SCIM 2.0 endpoints. The library auto-serves /Schemas, /ResourceTypes, +// and /ServiceProviderConfig from schema definitions. type Server struct { opts *Options - *scim.Server + srv *scim.Server } +// Options holds all the dependencies needed by SCIM resource handlers. type Options struct { - DB database.Store + DB database.Store + Auditor *atomic.Pointer[audit.Auditor] + IDPSync idpsync.IDPSync + Logger slog.Logger + + // AGPL is needed for CreateUser. + AGPL *agpl.API + + // SCIMAPIKey is the bearer token used to authenticate SCIM requests. + SCIMAPIKey []byte } -func New(opts *Options) *Server { +func New(opts *Options) (*Server, error) { + userHandler := &ResourceUser{ + store: opts.DB, + opts: opts, + } + args := &scim.ServerArgs{ ServiceProviderConfig: &scim.ServiceProviderConfig{ DocumentationURI: optional.NewString("https://coder.com/docs/admin/users/oidc-auth#scim"), AuthenticationSchemes: []scim.AuthenticationScheme{ { - Type: scim.AuthenticationTypeOauthBearerToken, - Name: "HTTP Header Authentication", - Description: "Authentication scheme using the Authorization header with the shared token", - // TODO: Add docs for this and link here + Type: scim.AuthenticationTypeOauthBearerToken, + Name: "HTTP Header Authentication", + Description: "Authentication scheme using the Authorization header with the shared token", SpecURI: optional.String{}, - DocumentationURI: optional.String{}, + DocumentationURI: optional.NewString("https://coder.com/docs/admin/users/oidc-auth#scim"), Primary: true, }, }, - MaxResults: 0, - SupportFiltering: false, - SupportPatch: false, + SupportPatch: true, }, ResourceTypes: []scim.ResourceType{ { - ID: optional.NewString("User"), - Name: "User", - Description: optional.String{}, - Endpoint: "/User", - Schema: schema.CoreUserSchema(), - SchemaExtensions: nil, - Handler: nil, + ID: optional.NewString("User"), + Name: "User", + Endpoint: "/Users", + Schema: schema.CoreUserSchema(), + Handler: userHandler, }, }, } srv, err := scim.NewServer(args) if err != nil { - return nil + return nil, err } return &Server{ - Server: &srv, + opts: opts, + srv: &srv, + }, nil +} + +func (s *Server) AuthMiddleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + if !s.verifyAuthHeader(r) { + scimUnauthorized(rw) + return + } + + // All authenticated requests are treated as coming from the SCIM provisioner + ctx := dbauthz.AsSCIMProvisioner(r.Context()) + r = r.WithContext(ctx) + + next.ServeHTTP(rw, r) + }) +} + +func (s *Server) Handler() http.Handler { + return s.AuthMiddleware(s.srv) +} + +func (s *Server) verifyAuthHeader(r *http.Request) bool { + bearer := []byte("bearer ") + hdr := []byte(r.Header.Get("Authorization")) + + // Case-insensitive comparison of the "Bearer " prefix. + if len(hdr) >= len(bearer) && subtle.ConstantTimeCompare(bytes.ToLower(hdr[:len(bearer)]), bearer) == 1 { + hdr = hdr[len(bearer):] } + + return len(s.opts.SCIMAPIKey) != 0 && subtle.ConstantTimeCompare(hdr, s.opts.SCIMAPIKey) == 1 +} + +func scimUnauthorized(rw http.ResponseWriter) { + rw.Header().Set("Content-Type", "application/scim+json") + rw.WriteHeader(http.StatusUnauthorized) + // scim error spec: + // https://datatracker.ietf.org/doc/html/rfc7644#section-3.12 + _ = json.NewEncoder(rw).Encode(scimErrors.ScimError{ + ScimType: "", // No scimType exists for unauthorized errors. + Detail: "invalid authorization", + Status: http.StatusUnauthorized, + }) } From 3bd1b5ad8f122c56209628c59470fbd29cf21fe2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 19 May 2026 17:32:15 -0500 Subject: [PATCH 04/33] work on patch/put --- enterprise/coderd/scim/scim.go | 25 ++- enterprise/coderd/scim/scimusers.go | 333 ++++++++++++++++++++++++++-- 2 files changed, 330 insertions(+), 28 deletions(-) diff --git a/enterprise/coderd/scim/scim.go b/enterprise/coderd/scim/scim.go index 79d32054dc954..c6a665145cabd 100644 --- a/enterprise/coderd/scim/scim.go +++ b/enterprise/coderd/scim/scim.go @@ -54,23 +54,28 @@ func New(opts *Options) (*Server, error) { DocumentationURI: optional.NewString("https://coder.com/docs/admin/users/oidc-auth#scim"), AuthenticationSchemes: []scim.AuthenticationScheme{ { - Type: scim.AuthenticationTypeOauthBearerToken, - Name: "HTTP Header Authentication", - Description: "Authentication scheme using the Authorization header with the shared token", + Type: scim.AuthenticationTypeOauthBearerToken, + Name: "HTTP Header Authentication", + Description: "Authentication scheme using the Authorization header with the shared token", + // TODO: Add documentation links for these specific docs once they exist. SpecURI: optional.String{}, - DocumentationURI: optional.NewString("https://coder.com/docs/admin/users/oidc-auth#scim"), + DocumentationURI: optional.String{}, Primary: true, }, }, - SupportPatch: true, + MaxResults: 0, + SupportFiltering: false, + SupportPatch: true, }, ResourceTypes: []scim.ResourceType{ { - ID: optional.NewString("User"), - Name: "User", - Endpoint: "/Users", - Schema: schema.CoreUserSchema(), - Handler: userHandler, + ID: optional.NewString("User"), + Name: "User", + Description: optional.NewString("User Account"), + Endpoint: "/Users", + Schema: schema.CoreUserSchema(), + Handler: userHandler, + SchemaExtensions: nil, }, }, } diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index 9e2ac0ae489ee..263b525aa035e 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -1,69 +1,337 @@ package scim import ( + "database/sql" "fmt" "net/http" + "strconv" "github.com/elimity-com/scim" + scimErrors "github.com/elimity-com/scim/errors" "github.com/elimity-com/scim/optional" "github.com/google/uuid" + "golang.org/x/xerrors" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/database/dbtime" ) var _ scim.ResourceHandler = (*ResourceUser)(nil) +var scimAuditAdditionalFields = map[string]string{ + "automatic_actor": "coder", + "automatic_subsystem": "scim", +} + type ResourceUser struct { store database.Store + opts *Options } -func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttributes) (scim.Resource, error) { - //TODO implement me - panic("implement me") +func (ru *ResourceUser) Create(_ *http.Request, _ scim.ResourceAttributes) (scim.Resource, error) { + // Creating a new user from SCIM is not currently supported. + return scim.Resource{}, scimErrors.ScimError{ + Status: http.StatusNotImplemented, + Detail: "User creation via SCIM is not supported", + } } +// Get implements scim.ResourceHandler. Returns a single user by ID. func (ru *ResourceUser) Get(r *http.Request, idStr string) (scim.Resource, error) { ctx := r.Context() id, err := uuid.Parse(idStr) if err != nil { - return scim.Resource{}, fmt.Errorf("invalid user ID %q: %w", idStr, err) + return scim.Resource{}, BadUUID(idStr, err) } usr, err := ru.store.GetUserByID(ctx, id) if err != nil { + if xerrors.Is(err, sql.ErrNoRows) { + return scim.Resource{}, scimErrors.ScimErrorResourceNotFound(idStr) + } return scim.Resource{}, err } return userResource(usr), nil } +// GetAll implements scim.ResourceHandler. Returns a paginated list of users. func (ru *ResourceUser) GetAll(r *http.Request, params scim.ListRequestParams) (scim.Page, error) { - //TODO implement me - panic("implement me") + ctx := r.Context() + + users, err := ru.store.GetUsers(ctx, database.GetUsersParams{ + OffsetOpt: int32(params.StartIndex - 1), //nolint:gosec + LimitOpt: int32(params.Count), //nolint:gosec + }) + if err != nil { + return scim.Page{}, err + } + + totalCount, err := ru.store.GetUserCount(ctx, false) + if err != nil { + return scim.Page{}, err + } + + resources := make([]scim.Resource, 0, len(users)) + for _, u := range users { + resources = append(resources, userResourceFromGetUsersRow(u)) + } + + return scim.Page{ + TotalResults: int(totalCount), + Resources: resources, + }, nil } -func (ru *ResourceUser) Replace(r *http.Request, id string, attributes scim.ResourceAttributes) (scim.Resource, error) { - //TODO implement me - panic("implement me") +// Replace implements scim.ResourceHandler (PUT). Replaces user attributes. +// Currently only supports changing the active status per existing behavior. +func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.ResourceAttributes) (scim.Resource, error) { + ctx := r.Context() + + auditor := *ru.opts.Auditor.Load() + aReq, commitAudit := audit.InitRequestWithCancel[database.User](nil, &audit.RequestParams{ + Audit: auditor, + Log: ru.opts.Logger, + Request: r, + Action: database.AuditActionWrite, + AdditionalFields: scimAuditAdditionalFields, + }) + defer commitAudit(true) + + uid, err := uuid.Parse(idStr) + if err != nil { + return scim.Resource{}, BadUUID(idStr, err) + } + + dbUser, err := ru.store.GetUserByID(ctx, uid) + if err != nil { + if xerrors.Is(err, sql.ErrNoRows) { + return scim.Resource{}, scimErrors.ScimErrorResourceNotFound(idStr) + } + return scim.Resource{}, err + } + aReq.Old = dbUser + aReq.UserID = dbUser.ID + + // All of our fields except for active are immutable. + if !AttributeEqual(dbUser.Username, attributes, "userName") { + return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("changing the 'userName' field is not supported (current value: %q)", dbUser.Username)) + } + + // TODO: Check primary email + + active := true + if activeStr, ok := attributes["active"]; ok { + active, err = BooleanValue(activeStr) + if err != nil { + return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid boolean value for 'active' field: %v", activeStr)) + } + } + + newStatus := scimUserStatus(dbUser, &active) + if dbUser.Status != newStatus { + //nolint:gocritic // SCIM operations are a system user + dbUser, err = ru.store.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ + ID: dbUser.ID, + Status: newStatus, + UpdatedAt: dbtime.Now(), + UserIsSeen: false, + }) + if err != nil { + return scim.Resource{}, err + } + } else { + // No change, skip audit log. + commitAudit(false) + } + + aReq.New = dbUser + return userResource(dbUser), nil } -func (ru *ResourceUser) Delete(r *http.Request, id string) error { - //TODO implement me - panic("implement me") +// Delete implements scim.ResourceHandler. Suspends the user (Coder does +// not hard-delete users). +func (ru *ResourceUser) Delete(r *http.Request, idStr string) error { + ctx := r.Context() + + uid, err := uuid.Parse(idStr) + if err != nil { + return scimErrors.ScimErrorResourceNotFound(idStr) + } + + dbUser, err := ru.store.GetUserByID(ctx, uid) + if err != nil { + if xerrors.Is(err, sql.ErrNoRows) { + return scimErrors.ScimErrorResourceNotFound(idStr) + } + return err + } + + if dbUser.Status != database.UserStatusSuspended { + // Audit log the change to suspended status + auditor := *ru.opts.Auditor.Load() + aReq, commitAudit := audit.InitRequestWithCancel[database.User](nil, &audit.RequestParams{ + Audit: auditor, + Log: ru.opts.Logger, + Request: r, + Action: database.AuditActionWrite, + AdditionalFields: scimAuditAdditionalFields, + }) + defer commitAudit(true) + + newUser, err := ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ + ID: dbUser.ID, + Status: database.UserStatusSuspended, + UpdatedAt: dbtime.Now(), + UserIsSeen: false, + }) + if err != nil { + return err + } + aReq.Old = dbUser + aReq.New = newUser + aReq.UserID = dbUser.ID + } + + return nil } -func (ru *ResourceUser) Patch(r *http.Request, id string, operations []scim.PatchOperation) (scim.Resource, error) { - //TODO implement me - panic("implement me") +// Patch implements scim.ResourceHandler. Updates user attributes based on +// SCIM PatchOp operations. Currently, supports changing the active status. +func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.PatchOperation) (scim.Resource, error) { + ctx := r.Context() + + auditor := *ru.opts.Auditor.Load() + aReq, commitAudit := audit.InitRequestWithCancel[database.User](nil, &audit.RequestParams{ + Audit: auditor, + Log: ru.opts.Logger, + Request: r, + Action: database.AuditActionWrite, + AdditionalFields: scimAuditAdditionalFields, + }) + defer commitAudit(true) + + uid, err := uuid.Parse(idStr) + if err != nil { + return scim.Resource{}, BadUUID(idStr, err) + } + + dbUser, err := ru.store.GetUserByID(ctx, uid) + if err != nil { + if xerrors.Is(err, sql.ErrNoRows) { + return scim.Resource{}, scimErrors.ScimErrorResourceNotFound(idStr) + } + return scim.Resource{}, err + } + aReq.Old = dbUser + aReq.UserID = dbUser.ID + + // Process operations. Currently, we only handle the "active" attribute. + var activeSet *bool + for _, op := range operations { + path := "" + if op.Path != nil { + path = op.Path.String() + } + if path == "active" { + v, err := BooleanValue(op.Value) + if err != nil { + return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid boolean value for 'active' field: %v", op.Value)) + } + activeSet = &v + } + // TODO: Should we log other unsupported operations or silently ignore them? For now, we ignore them. + } + + newStatus := scimUserStatus(dbUser, activeSet) + if dbUser.Status != newStatus { + //nolint:gocritic // SCIM operations are a system user + dbUser, err = ru.store.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ + ID: dbUser.ID, + Status: newStatus, + UpdatedAt: dbtime.Now(), + UserIsSeen: false, + }) + if err != nil { + return scim.Resource{}, err + } + } else { + // No meaningful change, skip audit log. + commitAudit(false) + } + + aReq.New = dbUser + return userResource(dbUser), nil } +// scimUserStatus maps the SCIM "active" boolean to Coder's internal user status. +// It preserves the active/dormant distinction: active users stay active, +// dormant or suspended users become dormant when re-activated (they become +// active after their next login). +// +//nolint:revive // active is not a control flag +func scimUserStatus(user database.User, active *bool) database.UserStatus { + if active == nil { + return user.Status + } + + if !(*active) { + // SCIM "active: false" means the user should be suspended + return database.UserStatusSuspended + } + + switch user.Status { + case database.UserStatusActive: + // Active users stay active + return database.UserStatusActive + case database.UserStatusDormant, database.UserStatusSuspended: + // Dormant or suspended users become dormant when re-activated + // The user can then become active by doing something in the product. + return database.UserStatusDormant + default: + return database.UserStatusDormant + } +} + +// userResource converts a database.User into a SCIM Resource. func userResource(u database.User) scim.Resource { return scim.Resource{ ID: u.ID.String(), ExternalID: optional.String{}, Attributes: scim.ResourceAttributes{ + "userName": u.Username, + "name": map[string]interface{}{ + "givenName": u.Name, + "familyName": "", + }, + "emails": []map[string]interface{}{ + { + "primary": true, + "value": u.Email, + }, + }, + "active": u.Status == database.UserStatusActive || + u.Status == database.UserStatusDormant, + }, + Meta: scim.Meta{ + Created: &u.CreatedAt, + LastModified: &u.UpdatedAt, + }, + } +} + +// userResourceFromGetUsersRow converts a database.GetUsersRow into a SCIM Resource. +func userResourceFromGetUsersRow(u database.GetUsersRow) scim.Resource { + return scim.Resource{ + ID: u.ID.String(), + ExternalID: optional.String{}, + Attributes: scim.ResourceAttributes{ + "userName": u.Username, "name": map[string]interface{}{ - "givenName": u.Username, + "givenName": u.Name, + "familyName": "", }, "emails": []map[string]interface{}{ { @@ -71,8 +339,8 @@ func userResource(u database.User) scim.Resource { "value": u.Email, }, }, - "active": u.Status == database.UserStatusActive, - // TODO: Groups + "active": u.Status == database.UserStatusActive || + u.Status == database.UserStatusDormant, }, Meta: scim.Meta{ Created: &u.CreatedAt, @@ -80,3 +348,32 @@ func userResource(u database.User) scim.Resource { }, } } + +func BadUUID(idStr string, err error) scimErrors.ScimError { + return scimErrors.ScimErrorBadRequest(fmt.Sprintf("expected a UUID but got %q: %w", idStr, err)) +} + +func BooleanValue(v interface{}) (bool, error) { + switch b := v.(type) { + case bool: + return b, nil + case string: + return strconv.ParseBool(b) + default: + return false, fmt.Errorf("expected boolean or string value, got %T", v) + } +} + +func AttributeEqual[T comparable](existing T, attrs scim.ResourceAttributes, key string) bool { + found, ok := attrs[key] + if !ok { + return true // No change if the attribute is not present in the request + } + + sameType, ok := found.(T) + if !ok { + return false // Type mismatch, consider it a change + } + + return existing == sameType +} From d35277500a23cf965ffbeb3a2408b6a45955658c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 19 May 2026 17:32:56 -0500 Subject: [PATCH 05/33] no system ctx --- enterprise/coderd/scim/scimusers.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index 263b525aa035e..175717023ff3d 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -14,7 +14,6 @@ import ( "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" ) @@ -132,8 +131,7 @@ func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.R newStatus := scimUserStatus(dbUser, &active) if dbUser.Status != newStatus { - //nolint:gocritic // SCIM operations are a system user - dbUser, err = ru.store.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ + dbUser, err = ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ ID: dbUser.ID, Status: newStatus, UpdatedAt: dbtime.Now(), @@ -247,8 +245,7 @@ func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.P newStatus := scimUserStatus(dbUser, activeSet) if dbUser.Status != newStatus { - //nolint:gocritic // SCIM operations are a system user - dbUser, err = ru.store.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ + dbUser, err = ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ ID: dbUser.ID, Status: newStatus, UpdatedAt: dbtime.Now(), From db681d9dc814b02e0c1583ddb7dbbe0f7dca2c38 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 19 May 2026 18:00:18 -0500 Subject: [PATCH 06/33] work on scim create --- .gitignore | 4 + enterprise/coderd/scim/scimusers.go | 170 +++++++++++++++++++++++++++- 2 files changed, 168 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index 65dd97caf70e5..4693c4d0c27c3 100644 --- a/.gitignore +++ b/.gitignore @@ -93,6 +93,10 @@ __debug_bin* **/.claude/settings.local.json +# SCIM verify output (HAR files from compliance testing) +scripts/scimverify/output/ + + # Local agent configuration AGENTS.local.md diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index 175717023ff3d..50b08fc758836 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -12,9 +12,11 @@ import ( "github.com/google/uuid" "golang.org/x/xerrors" + agpl "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/codersdk" ) var _ scim.ResourceHandler = (*ResourceUser)(nil) @@ -29,12 +31,132 @@ type ResourceUser struct { opts *Options } -func (ru *ResourceUser) Create(_ *http.Request, _ scim.ResourceAttributes) (scim.Resource, error) { - // Creating a new user from SCIM is not currently supported. - return scim.Resource{}, scimErrors.ScimError{ - Status: http.StatusNotImplemented, - Detail: "User creation via SCIM is not supported", +// Create implements scim.ResourceHandler. Creates a new Coder user from +// SCIM attributes, or returns the existing user if a duplicate is found. +func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttributes) (scim.Resource, error) { + ctx := r.Context() + + auditor := *ru.opts.Auditor.Load() + aReq, commitAudit := audit.InitRequestWithCancel[database.User](nil, &audit.RequestParams{ + Audit: auditor, + Log: ru.opts.Logger, + Request: r, + Action: database.AuditActionCreate, + AdditionalFields: scimAuditAdditionalFields, + }) + defer commitAudit(true) + + // Extract fields from the SCIM attributes. + // Do our best to match what the OIDC signup flow also does. + username, _ := attributes["userName"].(string) + email := primaryEmail(attributes) + if email == "" { + // email is required + return scim.Resource{}, scimErrors.ScimErrorBadRequest("no primary email provided") + } + + // This comes from userOIDC + // TODO: Ideally this code would be shared between the two places. + usernameValidErr := codersdk.NameValid(username) + if usernameValidErr != nil { + if username == "" { + username = email + } + username = codersdk.UsernameFrom(username) + } + + // TODO: OIDC has optional configuration like `EmailDomain` to reject emails outside a specific domain. + // We should consider whether we want to support that for SCIM as well, and if so, apply that validation here. + + active := true + if a, ok := attributes["active"]; ok { + v, err := BooleanValue(a) + if err != nil { + return scim.Resource{}, scimErrors.ScimErrorBadRequest( + fmt.Sprintf("invalid boolean value for 'active' field: %v", a)) + } + active = v + } + + // Check for existing user by email or username. + dbUser, err := ru.store.GetUserByEmailOrUsername(ctx, database.GetUserByEmailOrUsernameParams{ + Email: email, + Username: username, + }) + if err == nil { + aReq.Old = dbUser + // User already exists. Update their status if needed + status := scimUserStatus(dbUser, &active) + if active && dbUser.Status != status { + newUser, err := ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ + ID: dbUser.ID, + Status: status, + UpdatedAt: dbtime.Now(), + UserIsSeen: false, + }) + if err != nil { + return scim.Resource{}, err + } + aReq.New = newUser + } else { + // No change has occured, skip audit log. + commitAudit(false) + } + + return userResource(dbUser), nil } + + if !xerrors.Is(err, sql.ErrNoRows) { + // Internal DB errors should be returned. + // ErrNoRows is expected if the user does not exist. + return scim.Resource{}, err + } + + // OIDC login runs org, group, and role sync. SCIM does not have (or not yet) these + // claims. We only need to sync the default organization if that is enabled. + // + // When the user eventually logs in via OIDC, the regular sync will run. + // However, since org sync can be disabled. We need to assign the default org if + // that is how we are configured. + organizations := []uuid.UUID{} + orgSync, err := ru.opts.IDPSync.OrganizationSyncSettings(ctx, ru.store) + if err != nil { + return scim.Resource{}, xerrors.Errorf("get organization sync settings: %w", err) + } + if orgSync.AssignDefault { + // Technically, we could just always assign this. When they eventually log in, + // the org would be removed if necessary. But to avoid confusion of the user + // being in the org before they log in, we apply some intelligence to this guess + // of "Do they belong in the default org". + defaultOrganization, err := ru.store.GetDefaultOrganization(ctx) + if err != nil { + return scim.Resource{}, xerrors.Errorf("get default organization: %w", err) + } + organizations = append(organizations, defaultOrganization.ID) + } + + // CreateUser does InsertOrganizationMember internally, which needs + // broader permissions than the SCIM provisioner role. Use + // AsSystemRestricted for this specific call. + dbUser, err = ru.opts.AGPL.CreateUser(ctx, ru.store, agpl.CreateUserRequest{ + CreateUserRequestWithOrgs: codersdk.CreateUserRequestWithOrgs{ + Username: username, + Email: email, + OrganizationIDs: organizations, + }, + LoginType: database.LoginTypeOIDC, + // Do not send notifications to user admins; SCIM may call this + // sequentially for many users. + // TODO: Maybe we should spam them anyway? + SkipNotifications: true, + }) + if err != nil { + return scim.Resource{}, xerrors.Errorf("create user: %w", err) + } + aReq.New = dbUser + aReq.UserID = dbUser.ID + + return userResource(dbUser), nil } // Get implements scim.ResourceHandler. Returns a single user by ID. @@ -347,7 +469,7 @@ func userResourceFromGetUsersRow(u database.GetUsersRow) scim.Resource { } func BadUUID(idStr string, err error) scimErrors.ScimError { - return scimErrors.ScimErrorBadRequest(fmt.Sprintf("expected a UUID but got %q: %w", idStr, err)) + return scimErrors.ScimErrorBadRequest(fmt.Sprintf("expected a UUID but got %q: %v", idStr, err)) } func BooleanValue(v interface{}) (bool, error) { @@ -374,3 +496,39 @@ func AttributeEqual[T comparable](existing T, attrs scim.ResourceAttributes, key return existing == sameType } + +// primaryEmail extracts the primary email from SCIM resource attributes. +func primaryEmail(attributes scim.ResourceAttributes) string { + emailsRaw, ok := attributes["emails"] + if !ok { + return "" + } + + emails, ok := emailsRaw.([]interface{}) + if !ok { + return "" + } + + for _, e := range emails { + emailMap, ok := e.(map[string]interface{}) + if !ok { + continue + } + if primary, _ := emailMap["primary"].(bool); primary { + if val, ok := emailMap["value"].(string); ok { + return val + } + } + } + + // Fallback: if no email is marked primary, use the first one. + if len(emails) > 0 { + if emailMap, ok := emails[0].(map[string]interface{}); ok { + if val, ok := emailMap["value"].(string); ok { + return val + } + } + } + + return "" +} From 9882e25bdd880f5a6494bf055f0c7ae1ba530fd4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 20 May 2026 10:52:39 -0500 Subject: [PATCH 07/33] add some basic verify script --- coderd/database/dbauthz/dbauthz.go | 9 +- enterprise/coderd/coderd.go | 8 +- enterprise/coderd/scim/scimusers.go | 112 +++++++--------- scripts/scimverify/README.md | 190 ++++++++++++++++++++++++++++ scripts/scimverify/config.yaml | 48 +++++++ scripts/scimverify/run.sh | 186 +++++++++++++++++++++++++++ 6 files changed, 479 insertions(+), 74 deletions(-) create mode 100644 scripts/scimverify/README.md create mode 100644 scripts/scimverify/config.yaml create mode 100755 scripts/scimverify/run.sh diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 1fb97c3353e5d..a46ef74584fc6 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -751,8 +751,10 @@ var ( Identifier: rbac.RoleIdentifier{Name: "scim"}, DisplayName: "SCIM", Site: rbac.Permissions(map[string][]policy.Action{ - rbac.ResourceUser.Type: {policy.ActionCreate, policy.ActionUpdate, policy.ActionRead}, - rbac.ResourceOrganization.Type: {policy.ActionRead}, + rbac.ResourceSystem.Type: {policy.ActionRead}, // Required for idp config reads, this should be fixed + rbac.ResourceUser.Type: {policy.ActionCreate, policy.ActionUpdate, policy.ActionRead}, + rbac.ResourceOrganization.Type: {policy.ActionRead}, + rbac.ResourceOrganizationMember.Type: {policy.ActionRead, policy.ActionCreate, policy.ActionUpdate}, }), User: []rbac.Permission{}, ByOrgID: map[string]rbac.OrgPermissions{}, @@ -4659,7 +4661,8 @@ func (q *querier) GetUserCodeDiffDisplayMode(ctx context.Context, userID uuid.UU } func (q *querier) GetUserCount(ctx context.Context, includeSystem bool) (int64, error) { - if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil { + // If you can read every user, then you can read the count of users. + if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceUser); err != nil { return 0, err } return q.db.GetUserCount(ctx, includeSystem) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 688b4fb7c19fd..d1f0502db4576 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -636,11 +636,15 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { return nil, xerrors.Errorf("create scim server: %w", err) } if len(options.SCIMAPIKey) != 0 { - api.AGPL.RootHandler.Route("/scim/v2", func(r chi.Router) { + // The elimity-com/scim library reads r.URL.Path and strips "/v2" + // internally. Chi's Route/Mount modifies its own routing context + // but not r.URL.Path, so we use http.StripPrefix to ensure the + // library sees paths like "/v2/Users" instead of "/scim/v2/Users". + api.AGPL.RootHandler.Route("/scim", func(r chi.Router) { r.Use( api.RequireFeatureMW(codersdk.FeatureSCIM), ) - r.Mount("/", scimSrv.Handler()) + r.Mount("/", http.StripPrefix("/scim", scimSrv.Handler())) }) } else { // Show a helpful 404 error. Because this is not under the /api/v2 routes, diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index 50b08fc758836..9347aa5291024 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -1,7 +1,9 @@ package scim import ( + "context" "database/sql" + "encoding/json" "fmt" "net/http" "strconv" @@ -21,9 +23,34 @@ import ( var _ scim.ResourceHandler = (*ResourceUser)(nil) -var scimAuditAdditionalFields = map[string]string{ - "automatic_actor": "coder", - "automatic_subsystem": "scim", +// scimAudit emits an audit log for a SCIM operation. This uses +// BackgroundAudit instead of InitRequest because the elimity-com/scim +// library owns the http.ResponseWriter and does not expose it to +// resource handlers. +func (ru *ResourceUser) scimAudit(ctx context.Context, r *http.Request, action database.AuditAction, old, new database.User) { + raw, _ := json.Marshal(map[string]string{ + "automatic_actor": "coder", + "automatic_subsystem": "scim", + }) + auditor := *ru.opts.Auditor.Load() + + ip := r.RemoteAddr + if forwarded := r.Header.Get("X-Forwarded-For"); forwarded != "" { + ip = forwarded + } + + audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ + Audit: auditor, + Log: ru.opts.Logger, + UserID: uuid.Nil, // SCIM provisioner, not a real user + Action: action, + Old: old, + New: new, + IP: ip, + UserAgent: r.UserAgent(), + AdditionalFields: raw, + Status: http.StatusOK, + }) } type ResourceUser struct { @@ -36,16 +63,6 @@ type ResourceUser struct { func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttributes) (scim.Resource, error) { ctx := r.Context() - auditor := *ru.opts.Auditor.Load() - aReq, commitAudit := audit.InitRequestWithCancel[database.User](nil, &audit.RequestParams{ - Audit: auditor, - Log: ru.opts.Logger, - Request: r, - Action: database.AuditActionCreate, - AdditionalFields: scimAuditAdditionalFields, - }) - defer commitAudit(true) - // Extract fields from the SCIM attributes. // Do our best to match what the OIDC signup flow also does. username, _ := attributes["userName"].(string) @@ -84,8 +101,7 @@ func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttribut Username: username, }) if err == nil { - aReq.Old = dbUser - // User already exists. Update their status if needed + // User already exists. Update their status if needed. status := scimUserStatus(dbUser, &active) if active && dbUser.Status != status { newUser, err := ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ @@ -97,10 +113,7 @@ func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttribut if err != nil { return scim.Resource{}, err } - aReq.New = newUser - } else { - // No change has occured, skip audit log. - commitAudit(false) + ru.scimAudit(ctx, r, database.AuditActionWrite, dbUser, newUser) } return userResource(dbUser), nil @@ -153,9 +166,8 @@ func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttribut if err != nil { return scim.Resource{}, xerrors.Errorf("create user: %w", err) } - aReq.New = dbUser - aReq.UserID = dbUser.ID + ru.scimAudit(ctx, r, database.AuditActionCreate, database.User{}, dbUser) return userResource(dbUser), nil } @@ -211,16 +223,6 @@ func (ru *ResourceUser) GetAll(r *http.Request, params scim.ListRequestParams) ( func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.ResourceAttributes) (scim.Resource, error) { ctx := r.Context() - auditor := *ru.opts.Auditor.Load() - aReq, commitAudit := audit.InitRequestWithCancel[database.User](nil, &audit.RequestParams{ - Audit: auditor, - Log: ru.opts.Logger, - Request: r, - Action: database.AuditActionWrite, - AdditionalFields: scimAuditAdditionalFields, - }) - defer commitAudit(true) - uid, err := uuid.Parse(idStr) if err != nil { return scim.Resource{}, BadUUID(idStr, err) @@ -233,8 +235,6 @@ func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.R } return scim.Resource{}, err } - aReq.Old = dbUser - aReq.UserID = dbUser.ID // All of our fields except for active are immutable. if !AttributeEqual(dbUser.Username, attributes, "userName") { @@ -253,6 +253,7 @@ func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.R newStatus := scimUserStatus(dbUser, &active) if dbUser.Status != newStatus { + oldUser := dbUser dbUser, err = ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ ID: dbUser.ID, Status: newStatus, @@ -262,12 +263,9 @@ func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.R if err != nil { return scim.Resource{}, err } - } else { - // No change, skip audit log. - commitAudit(false) + ru.scimAudit(ctx, r, database.AuditActionWrite, oldUser, dbUser) } - aReq.New = dbUser return userResource(dbUser), nil } @@ -290,17 +288,6 @@ func (ru *ResourceUser) Delete(r *http.Request, idStr string) error { } if dbUser.Status != database.UserStatusSuspended { - // Audit log the change to suspended status - auditor := *ru.opts.Auditor.Load() - aReq, commitAudit := audit.InitRequestWithCancel[database.User](nil, &audit.RequestParams{ - Audit: auditor, - Log: ru.opts.Logger, - Request: r, - Action: database.AuditActionWrite, - AdditionalFields: scimAuditAdditionalFields, - }) - defer commitAudit(true) - newUser, err := ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ ID: dbUser.ID, Status: database.UserStatusSuspended, @@ -310,9 +297,7 @@ func (ru *ResourceUser) Delete(r *http.Request, idStr string) error { if err != nil { return err } - aReq.Old = dbUser - aReq.New = newUser - aReq.UserID = dbUser.ID + ru.scimAudit(ctx, r, database.AuditActionWrite, dbUser, newUser) } return nil @@ -323,16 +308,6 @@ func (ru *ResourceUser) Delete(r *http.Request, idStr string) error { func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.PatchOperation) (scim.Resource, error) { ctx := r.Context() - auditor := *ru.opts.Auditor.Load() - aReq, commitAudit := audit.InitRequestWithCancel[database.User](nil, &audit.RequestParams{ - Audit: auditor, - Log: ru.opts.Logger, - Request: r, - Action: database.AuditActionWrite, - AdditionalFields: scimAuditAdditionalFields, - }) - defer commitAudit(true) - uid, err := uuid.Parse(idStr) if err != nil { return scim.Resource{}, BadUUID(idStr, err) @@ -345,8 +320,6 @@ func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.P } return scim.Resource{}, err } - aReq.Old = dbUser - aReq.UserID = dbUser.ID // Process operations. Currently, we only handle the "active" attribute. var activeSet *bool @@ -367,6 +340,7 @@ func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.P newStatus := scimUserStatus(dbUser, activeSet) if dbUser.Status != newStatus { + oldUser := dbUser dbUser, err = ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ ID: dbUser.ID, Status: newStatus, @@ -376,12 +350,9 @@ func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.P if err != nil { return scim.Resource{}, err } - } else { - // No meaningful change, skip audit log. - commitAudit(false) + ru.scimAudit(ctx, r, database.AuditActionWrite, oldUser, dbUser) } - aReq.New = dbUser return userResource(dbUser), nil } @@ -468,8 +439,11 @@ func userResourceFromGetUsersRow(u database.GetUsersRow) scim.Resource { } } -func BadUUID(idStr string, err error) scimErrors.ScimError { - return scimErrors.ScimErrorBadRequest(fmt.Sprintf("expected a UUID but got %q: %v", idStr, err)) +// BadUUID returns a 404 not-found error for non-UUID identifiers. +// SCIM clients may send arbitrary strings as IDs; returning 404 +// (rather than 400) signals that no resource matches. +func BadUUID(idStr string, _ error) scimErrors.ScimError { + return scimErrors.ScimErrorResourceNotFound(idStr) } func BooleanValue(v interface{}) (bool, error) { diff --git a/scripts/scimverify/README.md b/scripts/scimverify/README.md new file mode 100644 index 0000000000000..d4353edbadd82 --- /dev/null +++ b/scripts/scimverify/README.md @@ -0,0 +1,190 @@ +# SCIM Verify + +> This test harness was set up by an AI agent ([Mux](https://mux.coder.com/)) +> during the SCIM 2.0 refactor. It discovered `scimverify`, figured out +> how to run it (the Docker image is a web UI, not the test CLI; use +> `npx scimverify` instead), worked through config quirks (schema detection +> crashes, plural vs singular resource type names, `id: AUTO` targeting the +> admin user), and iterated the config until 14/16 tests passed against a +> live Coder dev server. + +Run SCIM 2.0 compliance tests against a live Coder instance using [scimverify](https://verify.scim.dev/). + +## Prerequisites + +- **Node.js** (npx must be on PATH) +- A running Coder instance with SCIM enabled (`CODER_SCIM_AUTH_HEADER` set) +- An enterprise license (SCIM is an enterprise feature) + +## Quick start + +```bash +# Start a dev server with SCIM enabled +CODER_SCIM_AUTH_HEADER=my-secret-token ./scripts/develop.sh + +# In another terminal, run the tests +./scripts/scimverify/run.sh --token my-secret-token +``` + +## Usage + +``` +./scripts/scimverify/run.sh [--base-url URL] [--token TOKEN] + +Options: + --base-url URL SCIM endpoint base URL (default: http://localhost:3000/scim/v2) + --token TOKEN Bearer token matching CODER_SCIM_AUTH_HEADER (required) + --help Show help + +Environment variables (alternatives to flags): + SCIM_BASE_URL Same as --base-url + SCIM_AUTH_TOKEN Same as --token +``` + +## Reading the output + +The tool outputs [TAP](https://testanything.org/) (Test Anything Protocol) format: + +``` +TAP version 13 +# Subtest: ResourceTypes + ok 1 - Retrieves resource types <-- individual test +ok 1 - ResourceTypes <-- suite result +# Subtest: Schemas + ok 1 - Retrieves schemas +ok 2 - Schemas +# Subtest: Basic tests + ok 1 - Base URL should not contain any query parameters + ok 2 - Base URL should be reachable + ok 3 - Authentication should be required for /Users +ok 3 - Basic tests +# Subtest: Users + ok 1 - userSchema contains attribute userName ... + ok 2 - Retrieves a list of users + not ok 3 - Filters users by userName <-- FAILURE (details follow) + --- + error: |- + User should have matching userName + 'member' !== 'admin' + expected: 'admin' + actual: 'member' + ... + ok 4 - Creates a new user - Alternative 1 +ok 4 - Users +# tests 16 <-- summary +# pass 14 +# fail 1 +# skipped 1 +``` + +Key patterns: +- `ok N - description` means the test passed +- `not ok N - description` means the test failed (error details follow in YAML block) +- `ok N - description # SKIP reason` means the test was skipped +- Suite-level `not ok` means at least one subtest in the suite failed +- The summary at the bottom shows total pass/fail/skip counts + +### Quick filter for results only + +```bash +# Just pass/fail lines +./scripts/scimverify/run.sh --token TOKEN 2>&1 | grep -E " ok| not ok" + +# Summary only +./scripts/scimverify/run.sh --token TOKEN 2>&1 | grep "^# " +``` + +## HAR file + +Each run writes an [HAR file](https://en.wikipedia.org/wiki/HAR_(file_format)) +to `scripts/scimverify/output/output.har` containing every HTTP request and +response. This is useful for debugging failures: + +```bash +# Show all requests with errors +python3 -c " +import json +with open('scripts/scimverify/output/output.har') as f: + har = json.load(f) +for entry in har['log']['entries']: + req = entry['request'] + resp = entry['response'] + if resp['status'] >= 400: + body = resp.get('content', {}).get('text', '') + print(f'{req[\"method\"]} {req[\"url\"]} -> {resp[\"status\"]}') + if body: print(f' {body[:200]}') + print() +" +``` + +The `output/` directory is gitignored. + +## What the tests cover + +The test suite is configured via `config.yaml`. Currently it tests: + +| Category | Tests | What it checks | +|----------|-------|----------------| +| **ResourceTypes** | 1 | `GET /ResourceTypes` returns 200 with valid resource type definitions | +| **Schemas** | 1 | `GET /Schemas` returns 200 with valid schema definitions | +| **Basic** | 3 | URL format, reachability, and authentication enforcement | +| **Users: Read** | 4 | List users, get single user, non-existent user returns 404, attribute filtering | +| **Users: Create** | 2 | POST two test users with valid SCIM payloads, verify 201 response | +| **Users: Filter** | 1 | Filter by userName (expected to fail; `SupportFiltering: false`) | + +### PUT, PATCH, and DELETE + +The `run.sh` script works around scimverify's `id: AUTO` limitation (which +resolves to the first user, typically the admin) by pre-creating a sacrificial +test user via `POST /Users`, capturing its UUID, and generating a temporary +config with the real ID hardcoded for PUT, PATCH, and DELETE tests. The test +user has a random name like `scimverify-a1b2c3d4` and is deleted by the +DELETE test at the end of the run. + +## Configuration + +Edit `config.yaml` to change what gets tested. Key settings: + +```yaml +detectSchema: true # Test /Schemas endpoint +detectResourceTypes: true # Test /ResourceTypes endpoint +verifyPagination: false # Skip pagination tests +verifySorting: false # Skip sorting tests +requireAuthentication: true # Test that unauthenticated requests are rejected + +users: + enabled: true + operations: [GET, POST, PUT, PATCH, DELETE] # Which HTTP methods to test + post_tests: # User creation payloads + - request: { ... } + put_tests: [] # Empty = skip (see note above about id: AUTO) + patch_tests: [] # Empty = skip + delete_tests: [] # Empty = skip + +groups: + enabled: false # Coder does not support SCIM groups +``` + +### Adding test users + +To add more POST test cases, append to `post_tests`. Each entry needs a valid +SCIM User payload. Important Coder-specific notes: + +- `userName` must be a valid Coder username (lowercase, no `@`, no spaces). + If invalid, Coder normalizes it (e.g., `user@example.com` becomes `user`). +- `emails` must include at least one entry with `"primary": true`. +- `active` defaults to `true` if omitted. + +## Expected results + +On a correctly functioning Coder instance with the SCIM 2.0 refactor: + +``` +# tests 16 +# pass 15 +# fail 1 <-- filter test (expected, filtering is not supported) +# skipped 0 +``` + +The single expected failure is the filter-by-userName test, because Coder +declares `SupportFiltering: false` in its ServiceProviderConfig. diff --git a/scripts/scimverify/config.yaml b/scripts/scimverify/config.yaml new file mode 100644 index 0000000000000..a265ace300b94 --- /dev/null +++ b/scripts/scimverify/config.yaml @@ -0,0 +1,48 @@ +# The elimity-com/scim library auto-serves /Schemas and /ResourceTypes +# from the schema definitions, so we can detect them. +detectSchema: true +detectResourceTypes: true +verifyPagination: false +verifySorting: false +requireAuthentication: true + +users: + enabled: true + operations: + - GET + - POST + - PUT + - PATCH + - DELETE + + post_tests: + - request: + { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "scimtest1", + "name": { "givenName": "SCIM", "familyName": "TestOne" }, + "emails": [{ "value": "scimtest1@example.com", "primary": true }], + "active": true, + } + - request: + { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "scimtest2", + "name": { "givenName": "SCIM", "familyName": "TestTwo" }, + "emails": [{ "value": "scimtest2@example.com", "primary": true }], + "active": true, + } + + # PUT/PATCH/DELETE tests use id: AUTO which resolves to the first user + # from GET /Users?count=1. This is typically the admin user, not a test + # user. Suspending or deleting the admin breaks the dev server. + # + # These operations are tested safely in the Go integration tests + # (enterprise/coderd/scim_test.go) where user IDs are controlled. + # The scimverify config only tests operations safe against a live server. + put_tests: [] + patch_tests: [] + delete_tests: [] + +groups: + enabled: false diff --git a/scripts/scimverify/run.sh b/scripts/scimverify/run.sh new file mode 100755 index 0000000000000..b33625aeaf9e4 --- /dev/null +++ b/scripts/scimverify/run.sh @@ -0,0 +1,186 @@ +#!/usr/bin/env bash + +# Run the SCIM Verify compliance test suite against a running Coder instance. +# +# This script creates a temporary test user, generates a config with the real +# user ID (avoiding scimverify's "AUTO" which targets the first user, typically +# the admin), runs the full test suite, and cleans up. +# +# Usage: +# ./scripts/scimverify/run.sh [--base-url URL] [--token TOKEN] +# +# Environment variables: +# SCIM_BASE_URL Base URL of the SCIM endpoint (default: http://localhost:3000/scim/v2) +# SCIM_AUTH_TOKEN Bearer token for SCIM authentication (required) +# +# Prerequisites: +# - Node.js / npx available on PATH +# - A running Coder instance with SCIM enabled (CODER_SCIM_AUTH_HEADER set) +# +# The tool outputs TAP-format test results to stdout and writes a HAR file +# capturing all request/response pairs for debugging. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +BASE_CONFIG="${SCRIPT_DIR}/config.yaml" +OUTPUT_DIR="${SCRIPT_DIR}/output" + +# Defaults +BASE_URL="${SCIM_BASE_URL:-http://localhost:3000/scim/v2}" +AUTH_TOKEN="${SCIM_AUTH_TOKEN:-}" + +# Parse CLI args (override env vars) +while [[ $# -gt 0 ]]; do + case "$1" in + --base-url) + BASE_URL="$2" + shift 2 + ;; + --token) + AUTH_TOKEN="$2" + shift 2 + ;; + --help | -h) + echo "Usage: $0 [--base-url URL] [--token TOKEN]" + echo "" + echo "Environment variables:" + echo " SCIM_BASE_URL Base URL (default: http://localhost:3000/scim/v2)" + echo " SCIM_AUTH_TOKEN Bearer token (required)" + exit 0 + ;; + *) + echo "Unknown option: $1" >&2 + exit 1 + ;; + esac +done + +if [[ -z "${AUTH_TOKEN}" ]]; then + echo "Error: SCIM auth token is required." >&2 + echo "Set SCIM_AUTH_TOKEN or pass --token TOKEN." >&2 + exit 1 +fi + +if ! command -v npx &>/dev/null; then + echo "Error: npx is not installed. Install Node.js to continue." >&2 + exit 1 +fi + +mkdir -p "${OUTPUT_DIR}" + +AUTH_HEADER="Authorization: Bearer ${AUTH_TOKEN}" + +# --- Step 1: Create a sacrificial test user for PUT/PATCH/DELETE tests --- +# scimverify's "id: AUTO" resolves to the first user from GET /Users?count=1, +# which is usually the admin. Instead, we pre-create a test user and hardcode +# its UUID into a temporary config. + +RAND_SUFFIX="$(head -c 4 /dev/urandom | od -An -tx1 | tr -d ' \n')" +TEST_USERNAME="scimverify-${RAND_SUFFIX}" +TEST_EMAIL="${TEST_USERNAME}@scimverify.test" + +echo "=== SCIM Verify ===" +echo "Base URL: ${BASE_URL}" +echo "Test user: ${TEST_USERNAME}" +echo "HAR out: ${OUTPUT_DIR}/output.har" +echo "" + +echo "Creating test user for PUT/PATCH/DELETE..." +CREATE_RESPONSE=$(curl -s -X POST \ + -H "${AUTH_HEADER}" \ + -H "Content-Type: application/scim+json" \ + "${BASE_URL}/Users" \ + -d "{ + \"schemas\": [\"urn:ietf:params:scim:schemas:core:2.0:User\"], + \"userName\": \"${TEST_USERNAME}\", + \"name\": {\"givenName\": \"Verify\", \"familyName\": \"Test\"}, + \"emails\": [{\"value\": \"${TEST_EMAIL}\", \"primary\": true}], + \"active\": true + }") + +TEST_USER_ID=$(echo "${CREATE_RESPONSE}" | python3 -c "import sys,json; print(json.load(sys.stdin).get('id',''))" 2>/dev/null || true) + +if [[ -z "${TEST_USER_ID}" ]]; then + echo "Warning: Failed to create test user. PUT/PATCH/DELETE tests will use AUTO (may target admin)." >&2 + echo "Response: ${CREATE_RESPONSE}" >&2 + echo "" + # Fall back to the base config (empty PUT/PATCH/DELETE) + CONFIG_FILE="${BASE_CONFIG}" +else + echo "Created test user: ${TEST_USERNAME} (${TEST_USER_ID})" + echo "" + + # --- Step 2: Generate a temporary config with the real user ID --- + CONFIG_FILE=$(mktemp "${OUTPUT_DIR}/config-XXXXXX.yaml") + trap 'rm -f "${CONFIG_FILE}"' EXIT + + cat >"${CONFIG_FILE}" < Date: Thu, 21 May 2026 12:03:42 -0500 Subject: [PATCH 08/33] add mocked tests --- coderd/database/dbauthz/dbauthz.go | 4 +- enterprise/coderd/coderd.go | 25 +- enterprise/coderd/scim/scimusers.go | 17 +- enterprise/coderd/scim/scimusers_test.go | 484 +++++++++++++++++++++++ 4 files changed, 509 insertions(+), 21 deletions(-) create mode 100644 enterprise/coderd/scim/scimusers_test.go diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index a46ef74584fc6..c6c73bb85ebcc 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -752,7 +752,9 @@ var ( DisplayName: "SCIM", Site: rbac.Permissions(map[string][]policy.Action{ rbac.ResourceSystem.Type: {policy.ActionRead}, // Required for idp config reads, this should be fixed - rbac.ResourceUser.Type: {policy.ActionCreate, policy.ActionUpdate, policy.ActionRead}, + rbac.ResourceAssignRole.Type: rbac.ResourceAssignRole.AvailableActions(), + rbac.ResourceAssignOrgRole.Type: rbac.ResourceAssignOrgRole.AvailableActions(), + rbac.ResourceUser.Type: {policy.ActionCreate, policy.ActionUpdate, policy.ActionRead, policy.ActionUpdatePersonal}, rbac.ResourceOrganization.Type: {policy.ActionRead}, rbac.ResourceOrganizationMember.Type: {policy.ActionRead, policy.ActionCreate, policy.ActionUpdate}, }), diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index d1f0502db4576..d3f345269a0bd 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -623,19 +623,20 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { }) }) - // TODO: Possibly lift this scimSrv? - scimSrv, err := scim.New(&scim.Options{ - DB: options.Database, - Auditor: &api.AGPL.Auditor, - IDPSync: options.IDPSync, - Logger: options.Logger, - AGPL: api.AGPL, - SCIMAPIKey: options.SCIMAPIKey, - }) - if err != nil { - return nil, xerrors.Errorf("create scim server: %w", err) - } if len(options.SCIMAPIKey) != 0 { + // TODO: Possibly lift this scimSrv? + scimSrv, err := scim.New(&scim.Options{ + DB: options.Database, + Auditor: &api.AGPL.Auditor, + IDPSync: options.IDPSync, + Logger: options.Logger, + AGPL: api.AGPL, + SCIMAPIKey: options.SCIMAPIKey, + }) + if err != nil { + return nil, xerrors.Errorf("create scim server: %w", err) + } + // The elimity-com/scim library reads r.URL.Path and strips "/v2" // internally. Chi's Route/Mount modifies its own routing context // but not r.URL.Path, so we use http.StripPrefix to ensure the diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index 9347aa5291024..ee0ad509f4933 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -27,7 +27,7 @@ var _ scim.ResourceHandler = (*ResourceUser)(nil) // BackgroundAudit instead of InitRequest because the elimity-com/scim // library owns the http.ResponseWriter and does not expose it to // resource handlers. -func (ru *ResourceUser) scimAudit(ctx context.Context, r *http.Request, action database.AuditAction, old, new database.User) { +func (ru *ResourceUser) scimAudit(ctx context.Context, r *http.Request, action database.AuditAction, old, changed database.User) { raw, _ := json.Marshal(map[string]string{ "automatic_actor": "coder", "automatic_subsystem": "scim", @@ -45,7 +45,7 @@ func (ru *ResourceUser) scimAudit(ctx context.Context, r *http.Request, action d UserID: uuid.Nil, // SCIM provisioner, not a real user Action: action, Old: old, - New: new, + New: changed, IP: ip, UserAgent: r.UserAgent(), AdditionalFields: raw, @@ -242,13 +242,14 @@ func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.R } // TODO: Check primary email + activeStr, ok := attributes["active"] + if !ok { + return scim.Resource{}, scimErrors.ScimErrorBadRequest("missing required 'active' field") + } - active := true - if activeStr, ok := attributes["active"]; ok { - active, err = BooleanValue(activeStr) - if err != nil { - return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid boolean value for 'active' field: %v", activeStr)) - } + active, err := BooleanValue(activeStr) + if err != nil { + return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid boolean value for 'active' field: %v", activeStr)) } newStatus := scimUserStatus(dbUser, &active) diff --git a/enterprise/coderd/scim/scimusers_test.go b/enterprise/coderd/scim/scimusers_test.go new file mode 100644 index 0000000000000..fced13d837462 --- /dev/null +++ b/enterprise/coderd/scim/scimusers_test.go @@ -0,0 +1,484 @@ +package scim + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/elimity-com/scim" + scimErrors "github.com/elimity-com/scim/errors" + "github.com/google/uuid" + filter "github.com/scim2/filter-parser/v2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "cdr.dev/slog/v3" + "cdr.dev/slog/v3/sloggers/slogtest" + + "github.com/coder/coder/v2/coderd/audit" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbmock" + "github.com/coder/coder/v2/coderd/database/dbtestutil" +) + +// setupSCIM creates a ResourceUser backed by a real database for testing. +// The returned mock auditor can be inspected for emitted audit logs. +func setupSCIM(t *testing.T) (*ResourceUser, database.Store, *audit.MockAuditor) { + t.Helper() + + db, _ := dbtestutil.NewDB(t) + mockAudit := audit.NewMock() + auditorPtr := atomic.Pointer[audit.Auditor]{} + var a audit.Auditor = mockAudit + auditorPtr.Store(&a) + + ru := &ResourceUser{ + store: db, + opts: &Options{ + DB: db, + Auditor: &auditorPtr, + Logger: slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug), + }, + } + return ru, db, mockAudit +} + +// scimRequest builds an *http.Request with scim provisioner context, +// simulating the auth context that the SCIM middleware normally sets. +func scimRequest(t *testing.T) *http.Request { + t.Helper() + ctx := dbauthz.AsSCIMProvisioner(context.Background()) + return httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx) +} + +// seedUser creates a user in the database for testing. +func seedUser(t *testing.T, db database.Store, opts database.User) database.User { + t.Helper() + return dbgen.User(t, db, opts) +} + +// setupSCIMMock creates a ResourceUser backed by a gomock store for tests +// that only need to verify call patterns (e.g. audit emission) without +// real SQL. +func setupSCIMMock(t *testing.T) (*ResourceUser, *dbmock.MockStore, *audit.MockAuditor) { + t.Helper() + + ctrl := gomock.NewController(t) + mockStore := dbmock.NewMockStore(ctrl) + mockAudit := audit.NewMock() + auditorPtr := atomic.Pointer[audit.Auditor]{} + var a audit.Auditor = mockAudit + auditorPtr.Store(&a) + + ru := &ResourceUser{ + store: mockStore, + opts: &Options{ + DB: mockStore, + Auditor: &auditorPtr, + Logger: slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug), + }, + } + return ru, mockStore, mockAudit +} + +// --- Pure function tests (no DB) --- + +func TestScimUserStatus(t *testing.T) { + t.Parallel() + + boolPtr := func(b bool) *bool { return &b } + + tests := []struct { + name string + status database.UserStatus + active *bool + expected database.UserStatus + }{ + {"active+true=active", database.UserStatusActive, boolPtr(true), database.UserStatusActive}, + {"active+false=suspended", database.UserStatusActive, boolPtr(false), database.UserStatusSuspended}, + {"suspended+true=dormant", database.UserStatusSuspended, boolPtr(true), database.UserStatusDormant}, + {"suspended+false=suspended", database.UserStatusSuspended, boolPtr(false), database.UserStatusSuspended}, + {"dormant+true=dormant", database.UserStatusDormant, boolPtr(true), database.UserStatusDormant}, + {"dormant+false=suspended", database.UserStatusDormant, boolPtr(false), database.UserStatusSuspended}, + {"active+nil=active", database.UserStatusActive, nil, database.UserStatusActive}, + {"suspended+nil=suspended", database.UserStatusSuspended, nil, database.UserStatusSuspended}, + {"dormant+nil=dormant", database.UserStatusDormant, nil, database.UserStatusDormant}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + user := database.User{Status: tt.status} + got := scimUserStatus(user, tt.active) + assert.Equal(t, tt.expected, got) + }) + } +} + +func TestPrimaryEmail(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + attrs scim.ResourceAttributes + expected string + }{ + { + name: "primary email", + attrs: scim.ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{"value": "a@b.com", "primary": true}, + }, + }, + expected: "a@b.com", + }, + { + name: "fallback to first when no primary", + attrs: scim.ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{"value": "first@b.com"}, + }, + }, + expected: "first@b.com", + }, + { + name: "picks primary over first", + attrs: scim.ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{"value": "first@b.com"}, + map[string]interface{}{"value": "primary@b.com", "primary": true}, + }, + }, + expected: "primary@b.com", + }, + { + name: "polluted", + attrs: scim.ResourceAttributes{ + "emails": []interface{}{ + // Try and cause a panic + "not-a-map", + true, + 7, + map[int]interface{}{ + 1: "bad", + }, + map[string]interface{}{ + "value": 123, // value is not a string + }, + map[string]interface{}{}, + map[string]interface{}{"value": "first@b.com"}, + map[string]interface{}{"value": "primary@b.com", "primary": true}, + }, + }, + expected: "primary@b.com", + }, + { + name: "no emails key", + attrs: scim.ResourceAttributes{}, + expected: "", + }, + { + name: "empty emails", + attrs: scim.ResourceAttributes{"emails": []interface{}{}}, + expected: "", + }, + { + name: "wrong type", + attrs: scim.ResourceAttributes{"emails": "not-a-list"}, + expected: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := primaryEmail(tt.attrs) + assert.Equal(t, tt.expected, got) + }) + } +} + +func TestBooleanValue(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input interface{} + want bool + wantErr bool + }{ + {"bool true", true, true, false}, + {"bool false", false, false, false}, + {"string true", "true", true, false}, + {"string false", "false", false, false}, + {"string True", "True", true, false}, + {"int", 42, false, true}, + {"nil", nil, false, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := BooleanValue(tt.input) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + assert.Equal(t, tt.want, got) + } + }) + } +} + +// --- Handler tests (with DB) --- + +func TestResourceUser_Lifecycle(t *testing.T) { + t.Parallel() + + ru, db, _ := setupSCIM(t) + + // Seed an active user. + user := seedUser(t, db, database.User{ + Status: database.UserStatusActive, + LoginType: database.LoginTypeOIDC, + }) + + r := scimRequest(t) + + // Step 1: Get the user. Verify fields match. + res, err := ru.Get(r, user.ID.String()) + require.NoError(t, err) + assert.Equal(t, user.ID.String(), res.ID) + assert.Equal(t, user.Username, res.Attributes["userName"]) + assert.Equal(t, true, res.Attributes["active"]) + + // Step 2: Replace with active=false → suspended. + res, err = ru.Replace(r, user.ID.String(), scim.ResourceAttributes{ + "userName": user.Username, + "active": false, + }) + require.NoError(t, err) + assert.Equal(t, false, res.Attributes["active"]) + + // Step 3: Get → confirm inactive. + res, err = ru.Get(r, user.ID.String()) + require.NoError(t, err) + assert.Equal(t, false, res.Attributes["active"]) + + // Step 4: Patch active=true → dormant (shown as active in SCIM). + res, err = ru.Patch(r, user.ID.String(), []scim.PatchOperation{ + {Op: "replace", Path: mustPath("active"), Value: true}, + }) + require.NoError(t, err) + assert.Equal(t, true, res.Attributes["active"]) + + // Step 5: Get → confirm active again. + res, err = ru.Get(r, user.ID.String()) + require.NoError(t, err) + assert.Equal(t, true, res.Attributes["active"]) + + // Step 6: Delete → suspended. + err = ru.Delete(r, user.ID.String()) + require.NoError(t, err) + + // Step 7: Get → confirm inactive after delete. + res, err = ru.Get(r, user.ID.String()) + require.NoError(t, err) + assert.Equal(t, false, res.Attributes["active"]) +} + +func TestResourceUser_GetAll(t *testing.T) { + t.Parallel() + + ru, db, _ := setupSCIM(t) + + // Seed 3 users. + for i := 0; i < 3; i++ { + seedUser(t, db, database.User{ + LoginType: database.LoginTypeOIDC, + }) + } + + r := scimRequest(t) + + // Get all with large count. + page, err := ru.GetAll(r, scim.ListRequestParams{Count: 100, StartIndex: 1}) + require.NoError(t, err) + assert.GreaterOrEqual(t, page.TotalResults, 3) + assert.GreaterOrEqual(t, len(page.Resources), 3) + + // Paginate: startIndex=2, count=1. + page, err = ru.GetAll(r, scim.ListRequestParams{Count: 1, StartIndex: 2}) + require.NoError(t, err) + assert.Len(t, page.Resources, 1) + assert.GreaterOrEqual(t, page.TotalResults, 3) +} + +func TestResourceUser_Errors(t *testing.T) { + t.Parallel() + + ru, _, _ := setupSCIM(t) + r := scimRequest(t) + missingUUID := uuid.New().String() + + tests := []struct { + name string + run func() error + wantStatus int + }{ + { + name: "Get/non-UUID", + run: func() error { _, err := ru.Get(r, "not-a-uuid"); return err }, + wantStatus: http.StatusNotFound, + }, + { + name: "Get/missing", + run: func() error { _, err := ru.Get(r, missingUUID); return err }, + wantStatus: http.StatusNotFound, + }, + { + name: "Replace/non-UUID", + run: func() error { _, err := ru.Replace(r, "bad", scim.ResourceAttributes{}); return err }, + wantStatus: http.StatusNotFound, + }, + { + name: "Replace/missing", + run: func() error { _, err := ru.Replace(r, missingUUID, scim.ResourceAttributes{}); return err }, + wantStatus: http.StatusNotFound, + }, + { + name: "Replace/immutable-userName", + run: func() error { + // Need a real user for this test. + user := seedUser(t, ru.store, database.User{LoginType: database.LoginTypeOIDC}) + _, err := ru.Replace(r, user.ID.String(), scim.ResourceAttributes{ + "userName": "different-name", + }) + return err + }, + wantStatus: http.StatusBadRequest, + }, + { + name: "Patch/non-UUID", + run: func() error { _, err := ru.Patch(r, "bad", nil); return err }, + wantStatus: http.StatusNotFound, + }, + { + name: "Patch/missing", + run: func() error { _, err := ru.Patch(r, missingUUID, nil); return err }, + wantStatus: http.StatusNotFound, + }, + { + name: "Delete/non-UUID", + run: func() error { return ru.Delete(r, "bad") }, + wantStatus: http.StatusNotFound, + }, + { + name: "Delete/missing", + run: func() error { return ru.Delete(r, missingUUID) }, + wantStatus: http.StatusNotFound, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.run() + require.Error(t, err) + var scimErr scimErrors.ScimError + require.ErrorAs(t, err, &scimErr) + assert.Equal(t, tt.wantStatus, scimErr.Status) + }) + } +} + +func TestResourceUser_AuditLogs(t *testing.T) { + t.Parallel() + + // These tests use dbmock instead of a real database because they only + // verify audit emission logic (does an audit log fire when status + // changes?), not SQL correctness. The handlers call just GetUserByID + // and UpdateUserStatus, both trivially mockable. + + makeUser := func(status database.UserStatus) (database.User, database.User) { + id := uuid.New() + user := database.User{ + ID: id, + Username: "testuser", + Status: status, + LoginType: database.LoginTypeOIDC, + } + suspended := user + suspended.Status = database.UserStatusSuspended + return user, suspended + } + + t.Run("Replace/status-change-emits-audit", func(t *testing.T) { + t.Parallel() + ru, mockStore, mockAudit := setupSCIMMock(t) + activeUser, suspendedUser := makeUser(database.UserStatusActive) + + mockStore.EXPECT().GetUserByID(gomock.Any(), activeUser.ID).Return(activeUser, nil) + mockStore.EXPECT().UpdateUserStatus(gomock.Any(), gomock.Any()).Return(suspendedUser, nil) + + _, err := ru.Replace(scimRequest(t), activeUser.ID.String(), scim.ResourceAttributes{ + "userName": activeUser.Username, + "active": false, + }) + require.NoError(t, err) + assert.Len(t, mockAudit.AuditLogs(), 1) + }) + + t.Run("Replace/no-change-skips-audit", func(t *testing.T) { + t.Parallel() + ru, mockStore, mockAudit := setupSCIMMock(t) + activeUser, _ := makeUser(database.UserStatusActive) + + mockStore.EXPECT().GetUserByID(gomock.Any(), activeUser.ID).Return(activeUser, nil) + // No UpdateUserStatus expected: active=true on an already active user is a no-op. + + _, err := ru.Replace(scimRequest(t), activeUser.ID.String(), scim.ResourceAttributes{ + "userName": activeUser.Username, + "active": true, + }) + require.NoError(t, err) + assert.Empty(t, mockAudit.AuditLogs()) + }) + + t.Run("Delete/active-user-emits-audit", func(t *testing.T) { + t.Parallel() + ru, mockStore, mockAudit := setupSCIMMock(t) + activeUser, suspendedUser := makeUser(database.UserStatusActive) + + mockStore.EXPECT().GetUserByID(gomock.Any(), activeUser.ID).Return(activeUser, nil) + mockStore.EXPECT().UpdateUserStatus(gomock.Any(), gomock.Any()).Return(suspendedUser, nil) + + err := ru.Delete(scimRequest(t), activeUser.ID.String()) + require.NoError(t, err) + assert.Len(t, mockAudit.AuditLogs(), 1) + }) + + t.Run("Delete/suspended-user-skips-audit", func(t *testing.T) { + t.Parallel() + ru, mockStore, mockAudit := setupSCIMMock(t) + _, suspendedUser := makeUser(database.UserStatusSuspended) + + mockStore.EXPECT().GetUserByID(gomock.Any(), suspendedUser.ID).Return(suspendedUser, nil) + // No UpdateUserStatus expected: already suspended. + + err := ru.Delete(scimRequest(t), suspendedUser.ID.String()) + require.NoError(t, err) + assert.Empty(t, mockAudit.AuditLogs()) + }) +} + +// mustPath parses a SCIM attribute path string into a *filter.Path +// for use in PatchOperation test data. +func mustPath(attr string) *filter.Path { + p, err := filter.ParsePath([]byte(attr)) + if err != nil { + panic(fmt.Sprintf("mustPath(%q): %v", attr, err)) + } + return &p +} From eb68c8048cd3e2d013eb259443a4f37f44d7d88c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 21 May 2026 15:33:24 -0500 Subject: [PATCH 09/33] legacy scim --- codersdk/deployment.go | 12 + enterprise/cli/server.go | 1 + enterprise/coderd/coderd.go | 48 +- .../coderd/coderdenttest/coderdenttest.go | 2 + .../{scim.go => legacyscim/legacyscim.go} | 168 ++- .../coderd/{scim => legacyscim}/scimtypes.go | 2 +- enterprise/coderd/scim/scimusers.go | 34 +- enterprise/coderd/scim_test.go | 957 ++++-------------- enterprise/coderd/scimroutes.go | 74 ++ site/src/api/typesGenerated.ts | 1 + 10 files changed, 429 insertions(+), 870 deletions(-) rename enterprise/coderd/{scim.go => legacyscim/legacyscim.go} (67%) rename enterprise/coderd/{scim => legacyscim}/scimtypes.go (99%) create mode 100644 enterprise/coderd/scimroutes.go diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 97dcd6e27d72e..c80fb3d94e7fd 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -638,6 +638,7 @@ type DeploymentValues struct { AgentFallbackTroubleshootingURL serpent.URL `json:"agent_fallback_troubleshooting_url,omitempty" typescript:",notnull"` BrowserOnly serpent.Bool `json:"browser_only,omitempty" typescript:",notnull"` SCIMAPIKey serpent.String `json:"scim_api_key,omitempty" typescript:",notnull"` + UseLegacySCIM serpent.Bool `json:"scim_use_legacy,omitempty" typescript:",notnull"` ExternalTokenEncryptionKeys serpent.StringArray `json:"external_token_encryption_keys,omitempty" typescript:",notnull"` Provisioner ProvisionerConfig `json:"provisioner,omitempty" typescript:",notnull"` RateLimit RateLimitConfig `json:"rate_limit,omitempty" typescript:",notnull"` @@ -3437,6 +3438,17 @@ func (c *DeploymentValues) Options() serpent.OptionSet { Annotations: serpent.Annotations{}.Mark(annotationEnterpriseKey, "true").Mark(annotationSecretKey, "true"), Value: &c.SCIMAPIKey, }, + { + Name: "SCIM Use Legacy", + // The legacy SCIM is a weird mix of SCIM 1.0 and SCIM 2.0 + Description: "Use the legacy SCIM implementation instead of the SCIM 2.0 handler. This is provided for backward compatibility for existing users.", + Flag: "scim-use-legacy", + Env: "CODER_SCIM_USE_LEGACY", + Hidden: true, + Default: "false", + Annotations: serpent.Annotations{}.Mark(annotationEnterpriseKey, "true"), + Value: &c.UseLegacySCIM, + }, { Name: "External Token Encryption Keys", Description: "Encrypt OIDC and Git authentication tokens with AES-256-GCM in the database. The value must be a comma-separated list of base64-encoded keys. Each key, when base64-decoded, must be exactly 32 bytes in length. The first key will be used to encrypt new values. Subsequent keys will be used as a fallback when decrypting. During normal operation it is recommended to only set one key unless you are in the process of rotating keys with the `coder server dbcrypt rotate` command.", diff --git a/enterprise/cli/server.go b/enterprise/cli/server.go index 0ffc730a37a1a..29f05b814e203 100644 --- a/enterprise/cli/server.go +++ b/enterprise/cli/server.go @@ -96,6 +96,7 @@ func (r *RootCmd) Server(_ func()) *serpent.Command { ConnectionLogging: true, BrowserOnly: options.DeploymentValues.BrowserOnly.Value(), SCIMAPIKey: []byte(options.DeploymentValues.SCIMAPIKey.Value()), + UseLegacySCIM: options.DeploymentValues.UseLegacySCIM.Value(), RBAC: true, DERPServerRelayAddress: options.DeploymentValues.DERP.Server.RelayURL.String(), DERPServerRegionID: int(options.DeploymentValues.DERP.Server.RegionID.Value()), diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index d3f345269a0bd..2a55c6218f00f 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -55,7 +55,6 @@ import ( "github.com/coder/coder/v2/enterprise/coderd/prebuilds" "github.com/coder/coder/v2/enterprise/coderd/proxyhealth" "github.com/coder/coder/v2/enterprise/coderd/schedule" - "github.com/coder/coder/v2/enterprise/coderd/scim" "github.com/coder/coder/v2/enterprise/coderd/usage" entchatd "github.com/coder/coder/v2/enterprise/coderd/x/chatd" "github.com/coder/coder/v2/enterprise/dbcrypt" @@ -623,42 +622,12 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { }) }) - if len(options.SCIMAPIKey) != 0 { - // TODO: Possibly lift this scimSrv? - scimSrv, err := scim.New(&scim.Options{ - DB: options.Database, - Auditor: &api.AGPL.Auditor, - IDPSync: options.IDPSync, - Logger: options.Logger, - AGPL: api.AGPL, - SCIMAPIKey: options.SCIMAPIKey, - }) - if err != nil { - return nil, xerrors.Errorf("create scim server: %w", err) - } - - // The elimity-com/scim library reads r.URL.Path and strips "/v2" - // internally. Chi's Route/Mount modifies its own routing context - // but not r.URL.Path, so we use http.StripPrefix to ensure the - // library sees paths like "/v2/Users" instead of "/scim/v2/Users". - api.AGPL.RootHandler.Route("/scim", func(r chi.Router) { - r.Use( - api.RequireFeatureMW(codersdk.FeatureSCIM), - ) - r.Mount("/", http.StripPrefix("/scim", scimSrv.Handler())) - }) - } else { - // Show a helpful 404 error. Because this is not under the /api/v2 routes, - // the frontend is the fallback. A html page is not a helpful error for - // a SCIM provider. This JSON has a call to action that __may__ resolve - // the issue. - // Using Mount to cover all subroute possibilities. - api.AGPL.RootHandler.Mount("/scim/v2", http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - httpapi.Write(r.Context(), w, http.StatusNotFound, codersdk.Response{ - Message: "SCIM is disabled, please contact your administrator if you believe this is an error", - Detail: "SCIM endpoints are disabled if no SCIM is configured. Configure 'CODER_SCIM_AUTH_HEADER' to enable.", - }) - }))) + var mountScimError error + api.AGPL.RootHandler.Route("/scim", func(r chi.Router) { + mountScimError = api.mountScimRoute(options, r) + }) + if mountScimError != nil { + return nil, xerrors.Errorf("mount scim routes: %w", mountScimError) } // We always want to run the replica manager even if we don't have DERP @@ -757,6 +726,11 @@ type Options struct { // Whether to block non-browser connections. BrowserOnly bool SCIMAPIKey []byte + // UseLegacySCIM opts into the legacy SCIM handler implementation + // (imulab/go-scim based). This is provided for backward compatibility + // during the transition to the new elimity-com/scim implementation. + // It will be removed in a future release. + UseLegacySCIM bool ExternalTokenEncryption []dbcrypt.Cipher diff --git a/enterprise/coderd/coderdenttest/coderdenttest.go b/enterprise/coderd/coderdenttest/coderdenttest.go index a7efd1b3023c5..1115ba12118c7 100644 --- a/enterprise/coderd/coderdenttest/coderdenttest.go +++ b/enterprise/coderd/coderdenttest/coderdenttest.go @@ -67,6 +67,7 @@ type Options struct { BrowserOnly bool EntitlementsUpdateInterval time.Duration SCIMAPIKey []byte + UseLegacySCIM bool UserWorkspaceQuota int ProxyHealthInterval time.Duration LicenseOptions *LicenseOptions @@ -108,6 +109,7 @@ func NewWithAPI(t *testing.T, options *Options) ( AuditLogging: options.AuditLogging, BrowserOnly: options.BrowserOnly, SCIMAPIKey: options.SCIMAPIKey, + UseLegacySCIM: options.UseLegacySCIM, DERPServerRelayAddress: serverURL.String(), DERPServerRegionID: int(oop.DeploymentValues.DERP.Server.RegionID.Value()), ReplicaSyncUpdateInterval: options.ReplicaSyncUpdateInterval, diff --git a/enterprise/coderd/scim.go b/enterprise/coderd/legacyscim/legacyscim.go similarity index 67% rename from enterprise/coderd/scim.go rename to enterprise/coderd/legacyscim/legacyscim.go index 5d0b248abdc65..6500d3e8c0b9f 100644 --- a/enterprise/coderd/scim.go +++ b/enterprise/coderd/legacyscim/legacyscim.go @@ -1,4 +1,12 @@ -package coderd +// Package legacyscim contains the legacy SCIM 1.1-era (alleged 2.0) handler +// implementation. For backwards compatibility during the transition, this package +// is preserved in May 2026. Remove this package in it's entirely and rely on the +// new Scim 2.0 implementation in enterprise/coderd/scim after the transition is complete. +// +// It can be enabled via the UseLegacySCIM option. +// +// Deprecated: Use the new enterprise/coderd/scim package instead. +package legacyscim import ( "bytes" @@ -6,6 +14,8 @@ import ( "database/sql" "encoding/json" "net/http" + "net/url" + "sync/atomic" "time" "github.com/go-chi/chi/v5" @@ -16,17 +26,65 @@ import ( "github.com/imulab/go-scim/pkg/v2/spec" "golang.org/x/xerrors" + "cdr.dev/slog/v3" + agpl "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/coder/v2/codersdk" - "github.com/coder/coder/v2/enterprise/coderd/scim" ) -func (api *API) scimVerifyAuthHeader(r *http.Request) bool { +// LegacyServer is the old SCIM handler implementation, kept for backward +// compatibility. It uses the imulab/go-scim library and custom JSON handling. +type LegacyServer struct { + Logger slog.Logger + Database database.Store + IDPSync idpsync.IDPSync + AGPL *agpl.API + AccessURL *url.URL + SCIMAPIKey []byte + Auditor *atomic.Pointer[audit.Auditor] +} + +// Handler returns an http.Handler that serves the legacy SCIM endpoints. +// It should be mounted at /scim/v2. +func (s *LegacyServer) Handler() http.Handler { + r := chi.NewRouter() + r.Get("/ServiceProviderConfig", s.scimServiceProviderConfig) + r.Post("/Users", s.scimPostUser) + r.Route("/Users", func(r chi.Router) { + r.Get("/", s.scimGetUsers) + r.Post("/", s.scimPostUser) + r.Get("/{id}", s.scimGetUser) + r.Patch("/{id}", s.scimPatchUser) + r.Put("/{id}", s.scimPutUser) + }) + r.NotFound(func(w http.ResponseWriter, r *http.Request) { + u := r.URL.String() + httpapi.Write(r.Context(), w, http.StatusNotFound, codersdk.Response{ + Message: "SCIM endpoint not found: " + u, + Detail: "This endpoint is not implemented. If it is correct and required, please contact support.", + }) + }) + return r +} + +// AuthMiddleware verifies the SCIM Bearer token. +func (s *LegacyServer) AuthMiddleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + if !s.scimVerifyAuthHeader(r) { + scimUnauthorized(rw) + return + } + next.ServeHTTP(rw, r) + }) +} + +func (s *LegacyServer) scimVerifyAuthHeader(r *http.Request) bool { bearer := []byte("bearer ") hdr := []byte(r.Header.Get("Authorization")) @@ -35,11 +93,11 @@ func (api *API) scimVerifyAuthHeader(r *http.Request) bool { hdr = hdr[len(bearer):] } - return len(api.SCIMAPIKey) != 0 && subtle.ConstantTimeCompare(hdr, api.SCIMAPIKey) == 1 + return len(s.SCIMAPIKey) != 0 && subtle.ConstantTimeCompare(hdr, s.SCIMAPIKey) == 1 } func scimUnauthorized(rw http.ResponseWriter) { - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusUnauthorized, "invalidAuthorization", xerrors.New("invalid authorization"))) + _ = handlerutil.WriteError(rw, NewHTTPError(http.StatusUnauthorized, "invalidAuthorization", xerrors.New("invalid authorization"))) } // scimServiceProviderConfig returns a static SCIM service provider configuration. @@ -50,7 +108,7 @@ func scimUnauthorized(rw http.ResponseWriter) { // @Tags Enterprise // @Success 200 // @Router /scim/v2/ServiceProviderConfig [get] -func (api *API) scimServiceProviderConfig(rw http.ResponseWriter, _ *http.Request) { +func (s *LegacyServer) scimServiceProviderConfig(rw http.ResponseWriter, _ *http.Request) { // No auth needed to query this endpoint. rw.Header().Set("Content-Type", spec.ApplicationScimJson) @@ -60,35 +118,35 @@ func (api *API) scimServiceProviderConfig(rw http.ResponseWriter, _ *http.Reques // Increment this time if you make any changes to the provider config. providerUpdated := time.Date(2024, 10, 25, 17, 0, 0, 0, time.UTC) var location string - locURL, err := api.AccessURL.Parse("/scim/v2/ServiceProviderConfig") + locURL, err := s.AccessURL.Parse("/scim/v2/ServiceProviderConfig") if err == nil { location = locURL.String() } enc := json.NewEncoder(rw) enc.SetEscapeHTML(true) - _ = enc.Encode(scim.ServiceProviderConfig{ + _ = enc.Encode(ServiceProviderConfig{ Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:ServiceProviderConfig"}, DocURI: "https://coder.com/docs/admin/users/oidc-auth#scim", - Patch: scim.Supported{ + Patch: Supported{ Supported: true, }, - Bulk: scim.BulkSupported{ + Bulk: BulkSupported{ Supported: false, }, - Filter: scim.FilterSupported{ + Filter: FilterSupported{ Supported: false, }, - ChangePassword: scim.Supported{ + ChangePassword: Supported{ Supported: false, }, - Sort: scim.Supported{ + Sort: Supported{ Supported: false, }, - ETag: scim.Supported{ + ETag: Supported{ Supported: false, }, - AuthSchemes: []scim.AuthenticationScheme{ + AuthSchemes: []AuthenticationScheme{ { Type: "oauthbearertoken", Name: "HTTP Header Authentication", @@ -96,7 +154,7 @@ func (api *API) scimServiceProviderConfig(rw http.ResponseWriter, _ *http.Reques DocURI: "https://coder.com/docs/admin/users/oidc-auth#scim", }, }, - Meta: scim.ServiceProviderMeta{ + Meta: ServiceProviderMeta{ Created: providerUpdated, LastModified: providerUpdated, Location: location, @@ -118,8 +176,8 @@ func (api *API) scimServiceProviderConfig(rw http.ResponseWriter, _ *http.Reques // @Router /scim/v2/Users [get] // //nolint:revive -func (api *API) scimGetUsers(rw http.ResponseWriter, r *http.Request) { - if !api.scimVerifyAuthHeader(r) { +func (s *LegacyServer) scimGetUsers(rw http.ResponseWriter, r *http.Request) { + if !s.scimVerifyAuthHeader(r) { scimUnauthorized(rw) return } @@ -146,13 +204,13 @@ func (api *API) scimGetUsers(rw http.ResponseWriter, r *http.Request) { // @Router /scim/v2/Users/{id} [get] // //nolint:revive -func (api *API) scimGetUser(rw http.ResponseWriter, r *http.Request) { - if !api.scimVerifyAuthHeader(r) { +func (s *LegacyServer) scimGetUser(rw http.ResponseWriter, r *http.Request) { + if !s.scimVerifyAuthHeader(r) { scimUnauthorized(rw) return } - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusNotFound, spec.ErrNotFound.Type, xerrors.New("endpoint will always return 404"))) + _ = handlerutil.WriteError(rw, NewHTTPError(http.StatusNotFound, spec.ErrNotFound.Type, xerrors.New("endpoint will always return 404"))) } // We currently use our own struct instead of using the SCIM package. This was @@ -196,17 +254,17 @@ var SCIMAuditAdditionalFields = map[string]string{ // @Param request body coderd.SCIMUser true "New user" // @Success 200 {object} coderd.SCIMUser // @Router /scim/v2/Users [post] -func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { +func (s *LegacyServer) scimPostUser(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - if !api.scimVerifyAuthHeader(r) { + if !s.scimVerifyAuthHeader(r) { scimUnauthorized(rw) return } - auditor := *api.AGPL.Auditor.Load() + auditor := *s.Auditor.Load() aReq, commitAudit := audit.InitRequest[database.User](rw, &audit.RequestParams{ Audit: auditor, - Log: api.Logger, + Log: s.Logger, Request: r, Action: database.AuditActionCreate, AdditionalFields: SCIMAuditAdditionalFields, @@ -216,12 +274,12 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { var sUser SCIMUser err := json.NewDecoder(r.Body).Decode(&sUser) if err != nil { - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "invalidRequest", err)) + _ = handlerutil.WriteError(rw, NewHTTPError(http.StatusBadRequest, "invalidRequest", err)) return } if sUser.Active == nil { - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "invalidRequest", xerrors.New("active field is required"))) + _ = handlerutil.WriteError(rw, NewHTTPError(http.StatusBadRequest, "invalidRequest", xerrors.New("active field is required"))) return } @@ -234,12 +292,12 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { } if email == "" { - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "invalidEmail", xerrors.New("no primary email provided"))) + _ = handlerutil.WriteError(rw, NewHTTPError(http.StatusBadRequest, "invalidEmail", xerrors.New("no primary email provided"))) return } //nolint:gocritic - dbUser, err := api.Database.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ + dbUser, err := s.Database.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ Email: email, Username: sUser.UserName, }) @@ -253,7 +311,7 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { if *sUser.Active && dbUser.Status == database.UserStatusSuspended { //nolint:gocritic - newUser, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{ + newUser, err := s.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{ ID: dbUser.ID, // The user will get transitioned to Active after logging in. Status: database.UserStatusDormant, @@ -295,23 +353,23 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { // This is to preserve single org deployment behavior. organizations := []uuid.UUID{} //nolint:gocritic // SCIM operations are a system user - orgSync, err := api.IDPSync.OrganizationSyncSettings(dbauthz.AsSystemRestricted(ctx), api.Database) + orgSync, err := s.IDPSync.OrganizationSyncSettings(dbauthz.AsSystemRestricted(ctx), s.Database) if err != nil { - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusInternalServerError, "internalError", xerrors.Errorf("failed to get organization sync settings: %w", err))) + _ = handlerutil.WriteError(rw, NewHTTPError(http.StatusInternalServerError, "internalError", xerrors.Errorf("failed to get organization sync settings: %w", err))) return } if orgSync.AssignDefault { //nolint:gocritic // SCIM operations are a system user - defaultOrganization, err := api.Database.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx)) + defaultOrganization, err := s.Database.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx)) if err != nil { - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusInternalServerError, "internalError", xerrors.Errorf("failed to get default organization: %w", err))) + _ = handlerutil.WriteError(rw, NewHTTPError(http.StatusInternalServerError, "internalError", xerrors.Errorf("failed to get default organization: %w", err))) return } organizations = append(organizations, defaultOrganization.ID) } //nolint:gocritic // needed for SCIM - dbUser, err = api.AGPL.CreateUser(dbauthz.AsSystemRestricted(ctx), api.Database, agpl.CreateUserRequest{ + dbUser, err = s.AGPL.CreateUser(dbauthz.AsSystemRestricted(ctx), s.Database, agpl.CreateUserRequest{ CreateUserRequestWithOrgs: codersdk.CreateUserRequestWithOrgs{ Username: sUser.UserName, Email: email, @@ -322,7 +380,7 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { SkipNotifications: true, }) if err != nil { - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusInternalServerError, "internalError", xerrors.Errorf("failed to create user: %w", err))) + _ = handlerutil.WriteError(rw, NewHTTPError(http.StatusInternalServerError, "internalError", xerrors.Errorf("failed to create user: %w", err))) return } aReq.New = dbUser @@ -345,17 +403,17 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { // @Param request body coderd.SCIMUser true "Update user request" // @Success 200 {object} codersdk.User // @Router /scim/v2/Users/{id} [patch] -func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { +func (s *LegacyServer) scimPatchUser(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - if !api.scimVerifyAuthHeader(r) { + if !s.scimVerifyAuthHeader(r) { scimUnauthorized(rw) return } - auditor := *api.AGPL.Auditor.Load() + auditor := *s.Auditor.Load() aReq, commitAudit := audit.InitRequestWithCancel[database.User](rw, &audit.RequestParams{ Audit: auditor, - Log: api.Logger, + Log: s.Logger, Request: r, Action: database.AuditActionWrite, }) @@ -367,19 +425,19 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { var sUser SCIMUser err := json.NewDecoder(r.Body).Decode(&sUser) if err != nil { - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "invalidRequest", err)) + _ = handlerutil.WriteError(rw, NewHTTPError(http.StatusBadRequest, "invalidRequest", err)) return } sUser.ID = id 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))) + _ = handlerutil.WriteError(rw, 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) + dbUser, err := s.Database.GetUserByID(dbauthz.AsSystemRestricted(ctx), uid) if err != nil { _ = handlerutil.WriteError(rw, err) // internal error return @@ -388,14 +446,14 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { aReq.UserID = dbUser.ID if sUser.Active == nil { - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "invalidRequest", xerrors.New("active field is required"))) + _ = handlerutil.WriteError(rw, NewHTTPError(http.StatusBadRequest, "invalidRequest", xerrors.New("active field is required"))) 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{ + userNew, err := s.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{ ID: dbUser.ID, Status: newStatus, UpdatedAt: dbtime.Now(), @@ -429,17 +487,17 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { // @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) { +func (s *LegacyServer) scimPutUser(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - if !api.scimVerifyAuthHeader(r) { + if !s.scimVerifyAuthHeader(r) { scimUnauthorized(rw) return } - auditor := *api.AGPL.Auditor.Load() + auditor := *s.Auditor.Load() aReq, commitAudit := audit.InitRequestWithCancel[database.User](rw, &audit.RequestParams{ Audit: auditor, - Log: api.Logger, + Log: s.Logger, Request: r, Action: database.AuditActionWrite, }) @@ -451,23 +509,23 @@ func (api *API) scimPutUser(rw http.ResponseWriter, r *http.Request) { var sUser SCIMUser err := json.NewDecoder(r.Body).Decode(&sUser) if err != nil { - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusBadRequest, "invalidRequest", err)) + _ = handlerutil.WriteError(rw, 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"))) + _ = handlerutil.WriteError(rw, 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))) + _ = handlerutil.WriteError(rw, 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) + dbUser, err := s.Database.GetUserByID(dbauthz.AsSystemRestricted(ctx), uid) if err != nil { _ = handlerutil.WriteError(rw, err) // internal error return @@ -484,14 +542,14 @@ func (api *API) scimPutUser(rw http.ResponseWriter, r *http.Request) { // 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))) + _ = handlerutil.WriteError(rw, 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{ + userNew, err := s.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{ ID: dbUser.ID, Status: newStatus, UpdatedAt: dbtime.Now(), diff --git a/enterprise/coderd/scim/scimtypes.go b/enterprise/coderd/legacyscim/scimtypes.go similarity index 99% rename from enterprise/coderd/scim/scimtypes.go rename to enterprise/coderd/legacyscim/scimtypes.go index 39e022aa24e05..c96044befbc30 100644 --- a/enterprise/coderd/scim/scimtypes.go +++ b/enterprise/coderd/legacyscim/scimtypes.go @@ -1,4 +1,4 @@ -package scim +package legacyscim import ( "encoding/json" diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index ee0ad509f4933..4e0b94ba8aa39 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -18,6 +18,7 @@ import ( "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" ) @@ -325,18 +326,25 @@ func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.P // Process operations. Currently, we only handle the "active" attribute. var activeSet *bool for _, op := range operations { - path := "" - if op.Path != nil { - path = op.Path.String() - } - if path == "active" { - v, err := BooleanValue(op.Value) - if err != nil { - return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid boolean value for 'active' field: %v", op.Value)) + switch op.Op { + case "add": + case "remove": + if op.Path.String() == "active" { + activeSet = ptr.Ref(false) + } + case "replace": + if m, ok := op.Value.(map[string]interface{}); ok { + // TODO: Should we log other unsupported operations or silently ignore them? For now, we ignore them. + if actV, ok := m["active"]; ok { + v, err := BooleanValue(actV) + if err != nil { + return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid boolean value for 'active' field: %v", actV)) + } + activeSet = &v + } } - activeSet = &v + default: } - // TODO: Should we log other unsupported operations or silently ignore them? For now, we ignore them. } newStatus := scimUserStatus(dbUser, activeSet) @@ -394,8 +402,7 @@ func userResource(u database.User) scim.Resource { Attributes: scim.ResourceAttributes{ "userName": u.Username, "name": map[string]interface{}{ - "givenName": u.Name, - "familyName": "", + "formatted": u.Name, }, "emails": []map[string]interface{}{ { @@ -421,8 +428,7 @@ func userResourceFromGetUsersRow(u database.GetUsersRow) scim.Resource { Attributes: scim.ResourceAttributes{ "userName": u.Username, "name": map[string]interface{}{ - "givenName": u.Name, - "familyName": "", + "formatted": u.Name, }, "emails": []map[string]interface{}{ { diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go index e33c49e2a4834..35e5cb8a07a92 100644 --- a/enterprise/coderd/scim_test.go +++ b/enterprise/coderd/scim_test.go @@ -4,13 +4,10 @@ import ( "context" "encoding/json" "fmt" - "io" "net/http" "net/http/httptest" "testing" - "github.com/golang-jwt/jwt/v4" - "github.com/google/uuid" "github.com/imulab/go-scim/pkg/v2/handlerutil" "github.com/imulab/go-scim/pkg/v2/spec" "github.com/stretchr/testify/assert" @@ -19,25 +16,22 @@ import ( "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/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" "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" + "github.com/coder/coder/v2/enterprise/coderd/legacyscim" "github.com/coder/coder/v2/enterprise/coderd/license" - "github.com/coder/coder/v2/enterprise/coderd/scim" "github.com/coder/coder/v2/testutil" ) //nolint:revive -func makeScimUser(t testing.TB) coderd.SCIMUser { +func makeScimUser(t testing.TB) legacyscim.SCIMUser { rstr, err := cryptorand.String(10) require.NoError(t, err) - return coderd.SCIMUser{ + return legacyscim.SCIMUser{ UserName: rstr, Name: struct { GivenName string `json:"givenName"` @@ -71,796 +65,233 @@ func setScimAuthBearer(key []byte) func(*http.Request) { } } + +// TestLegacyScim tests the legacy SCIM handler (imulab/go-scim based). +// This is a reduced set of integration tests verifying HTTP routing, auth, +// and core CRUD. Detailed handler logic is covered by the unit tests in +// enterprise/coderd/scim/scimusers_test.go. +// //nolint:gocritic // SCIM authenticates via a special header and bypasses internal RBAC. -func TestScim(t *testing.T) { +func TestLegacyScim(t *testing.T) { t.Parallel() - t.Run("postUser", func(t *testing.T) { + t.Run("disabled", 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, "POST", "/scim/v2/Users", struct{}{}) - require.NoError(t, err) - defer 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, "POST", "/scim/v2/Users", struct{}{}) - require.NoError(t, err) - defer res.Body.Close() - assert.Equal(t, http.StatusUnauthorized, res.StatusCode) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + SCIMAPIKey: []byte("hi"), + UseLegacySCIM: true, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{codersdk.FeatureSCIM: 0}, + }, }) - t.Run("OK", func(t *testing.T) { - t.Parallel() - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - - // given - scimAPIKey := []byte("hi") - mockAudit := audit.NewMock() - notifyEnq := ¬ificationstest.FakeEnqueuer{} - client, _ := coderdenttest.New(t, &coderdenttest.Options{ - Options: &coderdtest.Options{ - Auditor: mockAudit, - NotificationsEnqueuer: notifyEnq, - }, - SCIMAPIKey: scimAPIKey, - AuditLogging: true, - LicenseOptions: &coderdenttest.LicenseOptions{ - AccountID: "coolin", - Features: license.Features{ - codersdk.FeatureSCIM: 1, - codersdk.FeatureAuditLog: 1, - }, - }, - }) - mockAudit.ResetLogs() - - // verify scim is enabled - res, err := client.Request(ctx, http.MethodGet, "/scim/v2/ServiceProviderConfig", nil) - require.NoError(t, err) - defer res.Body.Close() - require.Equal(t, http.StatusOK, res.StatusCode) - - // when - sUser := makeScimUser(t) - res, err = client.Request(ctx, http.MethodPost, "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) - require.NoError(t, err) - defer res.Body.Close() - require.Equal(t, http.StatusOK, res.StatusCode) - - // then - // Expect audit logs - aLogs := mockAudit.AuditLogs() - require.Len(t, aLogs, 1) - af := map[string]string{} - err = json.Unmarshal([]byte(aLogs[0].AdditionalFields), &af) - require.NoError(t, err) - assert.Equal(t, coderd.SCIMAuditAdditionalFields, af) - assert.Equal(t, database.AuditActionCreate, aLogs[0].Action) - - // Expect users exposed over API - 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, sUser.Emails[0].Value, userRes.Users[0].Email) - assert.Equal(t, sUser.UserName, userRes.Users[0].Username) - assert.Len(t, userRes.Users[0].OrganizationIDs, 1) - - // Expect zero notifications (SkipNotifications = true) - require.Empty(t, notifyEnq.Sent()) - }) - - t.Run("OK_Bearer", func(t *testing.T) { - t.Parallel() - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - - // given - scimAPIKey := []byte("hi") - mockAudit := audit.NewMock() - notifyEnq := ¬ificationstest.FakeEnqueuer{} - client, _ := coderdenttest.New(t, &coderdenttest.Options{ - Options: &coderdtest.Options{ - Auditor: mockAudit, - NotificationsEnqueuer: notifyEnq, - }, - SCIMAPIKey: scimAPIKey, - AuditLogging: true, - LicenseOptions: &coderdenttest.LicenseOptions{ - AccountID: "coolin", - Features: license.Features{ - codersdk.FeatureSCIM: 1, - codersdk.FeatureAuditLog: 1, - }, - }, - }) - mockAudit.ResetLogs() - - // when - sUser := makeScimUser(t) - res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuthBearer(scimAPIKey)) - require.NoError(t, err) - defer res.Body.Close() - require.Equal(t, http.StatusOK, res.StatusCode) - - // then - // Expect audit logs - aLogs := mockAudit.AuditLogs() - require.Len(t, aLogs, 1) - af := map[string]string{} - err = json.Unmarshal([]byte(aLogs[0].AdditionalFields), &af) - require.NoError(t, err) - assert.Equal(t, coderd.SCIMAuditAdditionalFields, af) - assert.Equal(t, database.AuditActionCreate, aLogs[0].Action) - - // Expect users exposed over API - 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, sUser.Emails[0].Value, userRes.Users[0].Email) - assert.Equal(t, sUser.UserName, userRes.Users[0].Username) - assert.Len(t, userRes.Users[0].OrganizationIDs, 1) - - // Expect zero notifications (SkipNotifications = true) - require.Empty(t, notifyEnq.Sent()) - }) - - t.Run("OKNoDefault", func(t *testing.T) { - t.Parallel() - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - - // given - scimAPIKey := []byte("hi") - mockAudit := audit.NewMock() - notifyEnq := ¬ificationstest.FakeEnqueuer{} - dv := coderdtest.DeploymentValues(t) - dv.OIDC.OrganizationAssignDefault = false - client, _ := coderdenttest.New(t, &coderdenttest.Options{ - Options: &coderdtest.Options{ - Auditor: mockAudit, - NotificationsEnqueuer: notifyEnq, - DeploymentValues: dv, - }, - SCIMAPIKey: scimAPIKey, - AuditLogging: true, - LicenseOptions: &coderdenttest.LicenseOptions{ - AccountID: "coolin", - Features: license.Features{ - codersdk.FeatureSCIM: 1, - codersdk.FeatureAuditLog: 1, - }, - }, - }) - mockAudit.ResetLogs() - - // when - sUser := makeScimUser(t) - res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) - require.NoError(t, err) - defer res.Body.Close() - require.Equal(t, http.StatusOK, res.StatusCode) - - // then - // Expect audit logs - aLogs := mockAudit.AuditLogs() - require.Len(t, aLogs, 1) - af := map[string]string{} - err = json.Unmarshal([]byte(aLogs[0].AdditionalFields), &af) - require.NoError(t, err) - assert.Equal(t, coderd.SCIMAuditAdditionalFields, af) - assert.Equal(t, database.AuditActionCreate, aLogs[0].Action) - - // Expect users exposed over API - 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, sUser.Emails[0].Value, userRes.Users[0].Email) - assert.Equal(t, sUser.UserName, userRes.Users[0].Username) - assert.Len(t, userRes.Users[0].OrganizationIDs, 0) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", struct{}{}) + require.NoError(t, err) + defer res.Body.Close() + assert.Equal(t, http.StatusForbidden, res.StatusCode) + }) - // Expect zero notifications (SkipNotifications = true) - require.Empty(t, notifyEnq.Sent()) + 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"), + UseLegacySCIM: true, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{codersdk.FeatureSCIM: 1}, + }, }) - t.Run("Duplicate", func(t *testing.T) { - t.Parallel() - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() + res, err := client.Request(ctx, "POST", "/scim/v2/Users", struct{}{}) + require.NoError(t, err) + defer res.Body.Close() + assert.Equal(t, http.StatusUnauthorized, res.StatusCode) + }) - scimAPIKey := []byte("hi") - client, _ := coderdenttest.New(t, &coderdenttest.Options{ - SCIMAPIKey: scimAPIKey, - LicenseOptions: &coderdenttest.LicenseOptions{ - AccountID: "coolin", - Features: license.Features{ - codersdk.FeatureSCIM: 1, - }, + t.Run("postUser", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + scimAPIKey := []byte("hi") + mockAudit := audit.NewMock() + notifyEnq := ¬ificationstest.FakeEnqueuer{} + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + Auditor: mockAudit, + NotificationsEnqueuer: notifyEnq, + }, + SCIMAPIKey: scimAPIKey, + UseLegacySCIM: true, + AuditLogging: true, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, + codersdk.FeatureMultipleOrganizations: 1, }, - }) - - sUser := makeScimUser(t) - for i := 0; i < 3; i++ { - res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) - require.NoError(t, err) - _ = res.Body.Close() - assert.Equal(t, http.StatusOK, res.StatusCode) - } - - 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, sUser.Emails[0].Value, userRes.Users[0].Email) - assert.Equal(t, sUser.UserName, userRes.Users[0].Username) + }, }) - t.Run("Unsuspend", func(t *testing.T) { - t.Parallel() - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() + 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) + + var createdUser legacyscim.SCIMUser + err = json.NewDecoder(res.Body).Decode(&createdUser) + require.NoError(t, err) + assert.NotEmpty(t, createdUser.ID) + assert.Equal(t, sUser.UserName, createdUser.UserName) + + // Verify user exists. + userRes, err := client.Users(ctx, codersdk.UsersRequest{Search: createdUser.UserName}) + require.NoError(t, err) + require.Len(t, userRes.Users, 1) + assert.Equal(t, codersdk.LoginTypeOIDC, userRes.Users[0].LoginType) + + // Verify audit log. + require.True(t, len(mockAudit.AuditLogs()) > 0) + + // Verify no user admin notification (SCIM skips notifications). + require.Empty(t, notifyEnq.Sent()) + }) - scimAPIKey := []byte("hi") - client, _ := coderdenttest.New(t, &coderdenttest.Options{ - SCIMAPIKey: scimAPIKey, - LicenseOptions: &coderdenttest.LicenseOptions{ - AccountID: "coolin", - Features: license.Features{ - codersdk.FeatureSCIM: 1, - }, + t.Run("Duplicate", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + scimAPIKey := []byte("hi") + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + SCIMAPIKey: scimAPIKey, + UseLegacySCIM: true, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureMultipleOrganizations: 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) - - 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 = 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) - _ = res.Body.Close() - assert.Equal(t, http.StatusOK, res.StatusCode) - - 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, sUser.Emails[0].Value, userRes.Users[0].Email) - assert.Equal(t, sUser.UserName, userRes.Users[0].Username) - assert.Equal(t, codersdk.UserStatusDormant, userRes.Users[0].Status) + }, }) - t.Run("DomainStrips", func(t *testing.T) { - t.Parallel() + sUser := makeScimUser(t) - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - - scimAPIKey := []byte("hi") - client, _ := coderdenttest.New(t, &coderdenttest.Options{ - SCIMAPIKey: scimAPIKey, - LicenseOptions: &coderdenttest.LicenseOptions{ - AccountID: "coolin", - Features: license.Features{ - codersdk.FeatureSCIM: 1, - }, - }, - }) - - sUser := makeScimUser(t) - sUser.UserName = sUser.UserName + "@coder.com" + // Create same user 3 times. + for i := 0; i < 3; i++ { res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) require.NoError(t, err) - _, _ = io.Copy(io.Discard, res.Body) _ = res.Body.Close() assert.Equal(t, http.StatusOK, res.StatusCode) + } - 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, sUser.Emails[0].Value, userRes.Users[0].Email) - // Username should be the same as the given name. They all use the - // same string before we modified it above. - assert.Equal(t, sUser.Name.GivenName, userRes.Users[0].Username) - }) + // Only 1 user should exist. + userRes, err := client.Users(ctx, codersdk.UsersRequest{Search: sUser.UserName}) + require.NoError(t, err) + require.Len(t, userRes.Users, 1) }) t.Run("patchUser", 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, "PATCH", "/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, - }, + 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, + UseLegacySCIM: true, + AuditLogging: true, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, + codersdk.FeatureMultipleOrganizations: 1, }, - }) - - res, err := client.Request(ctx, "PATCH", "/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("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, "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) - - 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, "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, - "sub": uuid.NewString(), - }) - 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") - }) + // Create user first. + sUser := makeScimUser(t) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + defer res.Body.Close() + require.Equal(t, http.StatusOK, res.StatusCode) + + var createdUser legacyscim.SCIMUser + err = json.NewDecoder(res.Body).Decode(&createdUser) + require.NoError(t, err) + + // Suspend via PATCH. + mockAudit.ResetLogs() + sUser.Active = ptr.Ref(false) + res, err = client.Request(ctx, "PATCH", "/scim/v2/Users/"+createdUser.ID, sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + defer res.Body.Close() + assert.Equal(t, http.StatusOK, res.StatusCode) + + // Verify suspended. + userRes, err := client.User(ctx, createdUser.ID) + require.NoError(t, err) + assert.Equal(t, codersdk.UserStatusSuspended, userRes.Status) }) 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, - }, + 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, + UseLegacySCIM: true, + AuditLogging: true, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, + codersdk.FeatureMultipleOrganizations: 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, - "sub": uuid.NewString(), - }) - 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") - }) + // Create user first. + sUser := makeScimUser(t) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + defer res.Body.Close() + require.Equal(t, http.StatusOK, res.StatusCode) + + var createdUser legacyscim.SCIMUser + err = json.NewDecoder(res.Body).Decode(&createdUser) + require.NoError(t, err) + + // Suspend via PUT. + mockAudit.ResetLogs() + sUser.Active = ptr.Ref(false) + res, err = client.Request(ctx, "PUT", "/scim/v2/Users/"+createdUser.ID, sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + defer res.Body.Close() + assert.Equal(t, http.StatusOK, res.StatusCode) + + // Verify suspended. + userRes, err := client.User(ctx, createdUser.ID) + require.NoError(t, err) + assert.Equal(t, codersdk.UserStatusSuspended, userRes.Status) }) } @@ -876,7 +307,7 @@ func TestScimError(t *testing.T) { // Our error wrapper works rw = httptest.NewRecorder() - _ = handlerutil.WriteError(rw, scim.NewHTTPError(http.StatusNotFound, spec.ErrNotFound.Type, xerrors.New("not found"))) + _ = handlerutil.WriteError(rw, legacyscim.NewHTTPError(http.StatusNotFound, spec.ErrNotFound.Type, xerrors.New("not found"))) resp = rw.Result() defer resp.Body.Close() require.Equal(t, http.StatusNotFound, resp.StatusCode) diff --git a/enterprise/coderd/scimroutes.go b/enterprise/coderd/scimroutes.go new file mode 100644 index 0000000000000..891b760e2f412 --- /dev/null +++ b/enterprise/coderd/scimroutes.go @@ -0,0 +1,74 @@ +package coderd + +import ( + "net/http" + + "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/enterprise/coderd/legacyscim" + "github.com/coder/coder/v2/enterprise/coderd/scim" +) + +func (api *API) mountScimRoute(opt *Options, r chi.Router) error { + if len(opt.SCIMAPIKey) == 0 { + // Show a helpful 404 error. Because this is not under the /api/v2 routes, + // the frontend is the fallback. A html page is not a helpful error for + // a SCIM provider. This JSON has a call to action that __may__ resolve + // the issue. + // + // Using mount to cover all subroute possibilities + r.Mount("/", http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + httpapi.Write(r.Context(), w, http.StatusNotFound, codersdk.Response{ + Message: "SCIM is disabled, please contact your administrator if you believe this is an error", + Detail: "SCIM endpoints are disabled if no SCIM is configured. Configure 'CODER_SCIM_AUTH_HEADER' to enable.", + }) + }))) + return nil + } + + if opt.UseLegacySCIM { + // Legacy SCIM handler (imulab/go-scim based). Opt-in for + // backward compatibility during the transition period. + legacySrv := &legacyscim.LegacyServer{ + Logger: opt.Logger, + Database: opt.Database, + IDPSync: opt.IDPSync, + AGPL: api.AGPL, + AccessURL: api.AccessURL, + SCIMAPIKey: opt.SCIMAPIKey, + Auditor: &api.AGPL.Auditor, + } + r.Mount("/v2", chi.Chain( + api.RequireFeatureMW(codersdk.FeatureSCIM), + legacySrv.AuthMiddleware, + ).Handler(legacySrv.Handler())) + return nil + } + + // SCIM 2.0 handler (elimity-com/scim based). + scimSrv, err := scim.New(&scim.Options{ + DB: opt.Database, + Auditor: &api.AGPL.Auditor, + IDPSync: opt.IDPSync, + Logger: opt.Logger, + AGPL: api.AGPL, + SCIMAPIKey: opt.SCIMAPIKey, + }) + if err != nil { + return xerrors.Errorf("create scim server: %w", err) + } + + // The elimity-com/scim library reads r.URL.Path and strips "/v2" + // internally. Chi's Route/Mount modifies its own routing context + // but not r.URL.Path, so we use http.StripPrefix to ensure the + // library sees paths like "/v2/Users" instead of "/scim/v2/Users". + r.Mount("/", chi.Chain( + api.RequireFeatureMW(codersdk.FeatureSCIM), + middleware.StripPrefix("/scim"), + ).Handler(scimSrv.Handler())) + return nil +} diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index af46758f9fd89..280465a18a41d 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -4083,6 +4083,7 @@ export interface DeploymentValues { readonly agent_fallback_troubleshooting_url?: string; readonly browser_only?: boolean; readonly scim_api_key?: string; + readonly scim_use_legacy?: boolean; readonly external_token_encryption_keys?: string; readonly provisioner?: ProvisionerConfig; readonly rate_limit?: RateLimitConfig; From 3a1817bd82c46e6a89de566a0b37d0514ca3b4a5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 21 May 2026 16:11:52 -0500 Subject: [PATCH 10/33] fixups --- coderd/database/modelqueries.go | 2 + coderd/database/queries.sql.go | 58 +++++++++++++++--------- coderd/database/queries/users.sql | 12 +++++ enterprise/coderd/scim/scimexpression.go | 39 ++++++++++++++++ enterprise/coderd/scim/scimusers.go | 44 ++++++++++++++---- 5 files changed, 125 insertions(+), 30 deletions(-) create mode 100644 enterprise/coderd/scim/scimexpression.go diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go index f33c047c942cc..f7befa7cb1b70 100644 --- a/coderd/database/modelqueries.go +++ b/coderd/database/modelqueries.go @@ -413,6 +413,8 @@ func (q *sqlQuerier) GetAuthorizedUsers(ctx context.Context, arg GetUsersParams, arg.AfterID, arg.Search, arg.Name, + arg.Exactusername, + arg.Exactemail, pq.Array(arg.Status), pq.Array(arg.RbacRole), arg.LastSeenBefore, diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 58f3bd5fc7b3f..3adf2136f540d 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -27765,65 +27765,77 @@ WHERE name ILIKE concat('%', $3, '%') ELSE true END + -- Filter by exact username + AND CASE + WHEN $4 :: text != '' THEN + username = $4 + ELSE true + END + -- Filter by exact email + AND CASE + WHEN $5 :: text != '' THEN + email = $5 + ELSE true + END -- Filter by status AND CASE -- @status needs to be a text because it can be empty, If it was -- user_status enum, it would not. - WHEN cardinality($4 :: user_status[]) > 0 THEN - status = ANY($4 :: user_status[]) + WHEN cardinality($6 :: user_status[]) > 0 THEN + status = ANY($6 :: user_status[]) ELSE true END -- Filter by rbac_roles AND CASE -- @rbac_role allows filtering by rbac roles. If 'member' is included, show everyone, as -- everyone is a member. - WHEN cardinality($5 :: text[]) > 0 AND 'member' != ANY($5 :: text[]) THEN - rbac_roles && $5 :: text[] + WHEN cardinality($7 :: text[]) > 0 AND 'member' != ANY($7 :: text[]) THEN + rbac_roles && $7 :: text[] ELSE true END -- Filter by last_seen AND CASE - WHEN $6 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - last_seen_at <= $6 + WHEN $8 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN + last_seen_at <= $8 ELSE true END AND CASE - WHEN $7 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - last_seen_at >= $7 + WHEN $9 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN + last_seen_at >= $9 ELSE true END -- Filter by created_at AND CASE - WHEN $8 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - created_at <= $8 + WHEN $10 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN + created_at <= $10 ELSE true END AND CASE - WHEN $9 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - created_at >= $9 + WHEN $11 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN + created_at >= $11 ELSE true END -- Filter by system type AND CASE - WHEN $10::bool THEN TRUE + WHEN $12::bool THEN TRUE ELSE is_system = false END -- Filter by github.com user ID AND CASE - WHEN $11 :: bigint != 0 THEN - github_com_user_id = $11 + WHEN $13 :: bigint != 0 THEN + github_com_user_id = $13 ELSE true END -- Filter by login_type AND CASE - WHEN cardinality($12 :: login_type[]) > 0 THEN - login_type = ANY($12 :: login_type[]) + WHEN cardinality($14 :: login_type[]) > 0 THEN + login_type = ANY($14 :: login_type[]) ELSE true END -- Filter by service account. AND CASE - WHEN $13 :: boolean IS NOT NULL THEN - is_service_account = $13 :: boolean + WHEN $15 :: boolean IS NOT NULL THEN + is_service_account = $15 :: boolean ELSE true END -- End of filters @@ -27832,16 +27844,18 @@ WHERE -- @authorize_filter ORDER BY -- Deterministic and consistent ordering of all users. This is to ensure consistent pagination. - LOWER(username) ASC OFFSET $14 + LOWER(username) ASC OFFSET $16 LIMIT -- A null limit means "no limit", so 0 means return all - NULLIF($15 :: int, 0) + NULLIF($17 :: int, 0) ` type GetUsersParams struct { AfterID uuid.UUID `db:"after_id" json:"after_id"` Search string `db:"search" json:"search"` Name string `db:"name" json:"name"` + Exactusername string `db:"exactusername" json:"exactusername"` + Exactemail string `db:"exactemail" json:"exactemail"` Status []UserStatus `db:"status" json:"status"` RbacRole []string `db:"rbac_role" json:"rbac_role"` LastSeenBefore time.Time `db:"last_seen_before" json:"last_seen_before"` @@ -27886,6 +27900,8 @@ func (q *sqlQuerier) GetUsers(ctx context.Context, arg GetUsersParams) ([]GetUse arg.AfterID, arg.Search, arg.Name, + arg.Exactusername, + arg.Exactemail, pq.Array(arg.Status), pq.Array(arg.RbacRole), arg.LastSeenBefore, diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 03f403e145c91..ec8469106ff51 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -486,6 +486,18 @@ WHERE name ILIKE concat('%', @name, '%') ELSE true END + -- Filter by exact username + AND CASE + WHEN @exactUsername :: text != '' THEN + username = @exactUsername + ELSE true + END + -- Filter by exact email + AND CASE + WHEN @exactEmail :: text != '' THEN + email = @exactEmail + ELSE true + END -- Filter by status AND CASE -- @status needs to be a text because it can be empty, If it was diff --git a/enterprise/coderd/scim/scimexpression.go b/enterprise/coderd/scim/scimexpression.go new file mode 100644 index 0000000000000..a0518c9ba10ef --- /dev/null +++ b/enterprise/coderd/scim/scimexpression.go @@ -0,0 +1,39 @@ +package scim + +import ( + "github.com/scim2/filter-parser/v2" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/database" +) + +// userQuery only supports queries of a singular attribute expression. +// Everything else is rejected. Okta just uses username. +// Eg: username eq "alice" +func userQuery(expr filter.Expression) (database.GetUsersParams, error) { + if expr == nil { + return database.GetUsersParams{}, nil + } + + attrExpr, ok := expr.(*filter.AttributeExpression) + if !ok { + return database.GetUsersParams{}, xerrors.Errorf("expected attribute expression") + } + + attrValue, ok := attrExpr.CompareValue.(string) + if !ok { + return database.GetUsersParams{}, xerrors.Errorf("expected string compare value") + } + + var getUsers database.GetUsersParams + switch attrExpr.AttributePath.AttributeName { + case "userName": + getUsers.Exactusername = attrValue + case "email": + getUsers.Exactemail = attrValue + default: + return database.GetUsersParams{}, xerrors.Errorf("unsupported filter attribute: %s", attrExpr.AttributePath.AttributeName) + } + + return getUsers, nil +} diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index 4e0b94ba8aa39..f43d10df23c07 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -195,19 +195,37 @@ func (ru *ResourceUser) Get(r *http.Request, idStr string) (scim.Resource, error func (ru *ResourceUser) GetAll(r *http.Request, params scim.ListRequestParams) (scim.Page, error) { ctx := r.Context() - users, err := ru.store.GetUsers(ctx, database.GetUsersParams{ - OffsetOpt: int32(params.StartIndex - 1), //nolint:gosec - LimitOpt: int32(params.Count), //nolint:gosec - }) - if err != nil { - return scim.Page{}, err + var qry database.GetUsersParams + if params.FilterValidator != nil { + var err error + qry, err = userQuery(params.FilterValidator.GetFilter()) + if err != nil { + return scim.Page{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid filter: %v", err)) + } } - totalCount, err := ru.store.GetUserCount(ctx, false) + qry.LimitOpt = int32(params.Count) //nolint:gosec + qry.OffsetOpt = int32(params.StartIndex - 1) //nolint:gosec + + if qry.LimitOpt <= 0 { + qry.LimitOpt = 100 + } + + users, err := ru.store.GetUsers(ctx, qry) if err != nil { return scim.Page{}, err } + totalCount := int64(len(users)) + if len(users) == int(qry.LimitOpt) { + // If the limit is not reached, that is the count + // TODO: If there is a query and the limit is reached, this is inaccurate. + totalCount, err = ru.store.GetUserCount(ctx, false) + if err != nil { + return scim.Page{}, err + } + } + resources := make([]scim.Resource, 0, len(users)) for _, u := range users { resources = append(resources, userResourceFromGetUsersRow(u)) @@ -333,8 +351,16 @@ func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.P activeSet = ptr.Ref(false) } case "replace": - if m, ok := op.Value.(map[string]interface{}); ok { - // TODO: Should we log other unsupported operations or silently ignore them? For now, we ignore them. + // SCIM PATCH replace can come in two forms: + // 1. Path set: {"op":"replace","path":"active","value":false} + // 2. No path, value is a map: {"op":"replace","value":{"active":false}} + if op.Path != nil && op.Path.String() == "active" { + v, err := BooleanValue(op.Value) + if err != nil { + return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid boolean value for 'active' field: %v", op.Value)) + } + activeSet = &v + } else if m, ok := op.Value.(map[string]interface{}); ok { if actV, ok := m["active"]; ok { v, err := BooleanValue(actV) if err != nil { From 9c387d0a71074a25b28145d5592bd8417ebd6a79 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 09:26:04 -0500 Subject: [PATCH 11/33] pull out verify --- .gitignore | 4 - scripts/scimverify/README.md | 190 --------------------------------- scripts/scimverify/config.yaml | 48 --------- scripts/scimverify/run.sh | 186 -------------------------------- 4 files changed, 428 deletions(-) delete mode 100644 scripts/scimverify/README.md delete mode 100644 scripts/scimverify/config.yaml delete mode 100755 scripts/scimverify/run.sh diff --git a/.gitignore b/.gitignore index 4693c4d0c27c3..65dd97caf70e5 100644 --- a/.gitignore +++ b/.gitignore @@ -93,10 +93,6 @@ __debug_bin* **/.claude/settings.local.json -# SCIM verify output (HAR files from compliance testing) -scripts/scimverify/output/ - - # Local agent configuration AGENTS.local.md diff --git a/scripts/scimverify/README.md b/scripts/scimverify/README.md deleted file mode 100644 index d4353edbadd82..0000000000000 --- a/scripts/scimverify/README.md +++ /dev/null @@ -1,190 +0,0 @@ -# SCIM Verify - -> This test harness was set up by an AI agent ([Mux](https://mux.coder.com/)) -> during the SCIM 2.0 refactor. It discovered `scimverify`, figured out -> how to run it (the Docker image is a web UI, not the test CLI; use -> `npx scimverify` instead), worked through config quirks (schema detection -> crashes, plural vs singular resource type names, `id: AUTO` targeting the -> admin user), and iterated the config until 14/16 tests passed against a -> live Coder dev server. - -Run SCIM 2.0 compliance tests against a live Coder instance using [scimverify](https://verify.scim.dev/). - -## Prerequisites - -- **Node.js** (npx must be on PATH) -- A running Coder instance with SCIM enabled (`CODER_SCIM_AUTH_HEADER` set) -- An enterprise license (SCIM is an enterprise feature) - -## Quick start - -```bash -# Start a dev server with SCIM enabled -CODER_SCIM_AUTH_HEADER=my-secret-token ./scripts/develop.sh - -# In another terminal, run the tests -./scripts/scimverify/run.sh --token my-secret-token -``` - -## Usage - -``` -./scripts/scimverify/run.sh [--base-url URL] [--token TOKEN] - -Options: - --base-url URL SCIM endpoint base URL (default: http://localhost:3000/scim/v2) - --token TOKEN Bearer token matching CODER_SCIM_AUTH_HEADER (required) - --help Show help - -Environment variables (alternatives to flags): - SCIM_BASE_URL Same as --base-url - SCIM_AUTH_TOKEN Same as --token -``` - -## Reading the output - -The tool outputs [TAP](https://testanything.org/) (Test Anything Protocol) format: - -``` -TAP version 13 -# Subtest: ResourceTypes - ok 1 - Retrieves resource types <-- individual test -ok 1 - ResourceTypes <-- suite result -# Subtest: Schemas - ok 1 - Retrieves schemas -ok 2 - Schemas -# Subtest: Basic tests - ok 1 - Base URL should not contain any query parameters - ok 2 - Base URL should be reachable - ok 3 - Authentication should be required for /Users -ok 3 - Basic tests -# Subtest: Users - ok 1 - userSchema contains attribute userName ... - ok 2 - Retrieves a list of users - not ok 3 - Filters users by userName <-- FAILURE (details follow) - --- - error: |- - User should have matching userName - 'member' !== 'admin' - expected: 'admin' - actual: 'member' - ... - ok 4 - Creates a new user - Alternative 1 -ok 4 - Users -# tests 16 <-- summary -# pass 14 -# fail 1 -# skipped 1 -``` - -Key patterns: -- `ok N - description` means the test passed -- `not ok N - description` means the test failed (error details follow in YAML block) -- `ok N - description # SKIP reason` means the test was skipped -- Suite-level `not ok` means at least one subtest in the suite failed -- The summary at the bottom shows total pass/fail/skip counts - -### Quick filter for results only - -```bash -# Just pass/fail lines -./scripts/scimverify/run.sh --token TOKEN 2>&1 | grep -E " ok| not ok" - -# Summary only -./scripts/scimverify/run.sh --token TOKEN 2>&1 | grep "^# " -``` - -## HAR file - -Each run writes an [HAR file](https://en.wikipedia.org/wiki/HAR_(file_format)) -to `scripts/scimverify/output/output.har` containing every HTTP request and -response. This is useful for debugging failures: - -```bash -# Show all requests with errors -python3 -c " -import json -with open('scripts/scimverify/output/output.har') as f: - har = json.load(f) -for entry in har['log']['entries']: - req = entry['request'] - resp = entry['response'] - if resp['status'] >= 400: - body = resp.get('content', {}).get('text', '') - print(f'{req[\"method\"]} {req[\"url\"]} -> {resp[\"status\"]}') - if body: print(f' {body[:200]}') - print() -" -``` - -The `output/` directory is gitignored. - -## What the tests cover - -The test suite is configured via `config.yaml`. Currently it tests: - -| Category | Tests | What it checks | -|----------|-------|----------------| -| **ResourceTypes** | 1 | `GET /ResourceTypes` returns 200 with valid resource type definitions | -| **Schemas** | 1 | `GET /Schemas` returns 200 with valid schema definitions | -| **Basic** | 3 | URL format, reachability, and authentication enforcement | -| **Users: Read** | 4 | List users, get single user, non-existent user returns 404, attribute filtering | -| **Users: Create** | 2 | POST two test users with valid SCIM payloads, verify 201 response | -| **Users: Filter** | 1 | Filter by userName (expected to fail; `SupportFiltering: false`) | - -### PUT, PATCH, and DELETE - -The `run.sh` script works around scimverify's `id: AUTO` limitation (which -resolves to the first user, typically the admin) by pre-creating a sacrificial -test user via `POST /Users`, capturing its UUID, and generating a temporary -config with the real ID hardcoded for PUT, PATCH, and DELETE tests. The test -user has a random name like `scimverify-a1b2c3d4` and is deleted by the -DELETE test at the end of the run. - -## Configuration - -Edit `config.yaml` to change what gets tested. Key settings: - -```yaml -detectSchema: true # Test /Schemas endpoint -detectResourceTypes: true # Test /ResourceTypes endpoint -verifyPagination: false # Skip pagination tests -verifySorting: false # Skip sorting tests -requireAuthentication: true # Test that unauthenticated requests are rejected - -users: - enabled: true - operations: [GET, POST, PUT, PATCH, DELETE] # Which HTTP methods to test - post_tests: # User creation payloads - - request: { ... } - put_tests: [] # Empty = skip (see note above about id: AUTO) - patch_tests: [] # Empty = skip - delete_tests: [] # Empty = skip - -groups: - enabled: false # Coder does not support SCIM groups -``` - -### Adding test users - -To add more POST test cases, append to `post_tests`. Each entry needs a valid -SCIM User payload. Important Coder-specific notes: - -- `userName` must be a valid Coder username (lowercase, no `@`, no spaces). - If invalid, Coder normalizes it (e.g., `user@example.com` becomes `user`). -- `emails` must include at least one entry with `"primary": true`. -- `active` defaults to `true` if omitted. - -## Expected results - -On a correctly functioning Coder instance with the SCIM 2.0 refactor: - -``` -# tests 16 -# pass 15 -# fail 1 <-- filter test (expected, filtering is not supported) -# skipped 0 -``` - -The single expected failure is the filter-by-userName test, because Coder -declares `SupportFiltering: false` in its ServiceProviderConfig. diff --git a/scripts/scimverify/config.yaml b/scripts/scimverify/config.yaml deleted file mode 100644 index a265ace300b94..0000000000000 --- a/scripts/scimverify/config.yaml +++ /dev/null @@ -1,48 +0,0 @@ -# The elimity-com/scim library auto-serves /Schemas and /ResourceTypes -# from the schema definitions, so we can detect them. -detectSchema: true -detectResourceTypes: true -verifyPagination: false -verifySorting: false -requireAuthentication: true - -users: - enabled: true - operations: - - GET - - POST - - PUT - - PATCH - - DELETE - - post_tests: - - request: - { - "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], - "userName": "scimtest1", - "name": { "givenName": "SCIM", "familyName": "TestOne" }, - "emails": [{ "value": "scimtest1@example.com", "primary": true }], - "active": true, - } - - request: - { - "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], - "userName": "scimtest2", - "name": { "givenName": "SCIM", "familyName": "TestTwo" }, - "emails": [{ "value": "scimtest2@example.com", "primary": true }], - "active": true, - } - - # PUT/PATCH/DELETE tests use id: AUTO which resolves to the first user - # from GET /Users?count=1. This is typically the admin user, not a test - # user. Suspending or deleting the admin breaks the dev server. - # - # These operations are tested safely in the Go integration tests - # (enterprise/coderd/scim_test.go) where user IDs are controlled. - # The scimverify config only tests operations safe against a live server. - put_tests: [] - patch_tests: [] - delete_tests: [] - -groups: - enabled: false diff --git a/scripts/scimverify/run.sh b/scripts/scimverify/run.sh deleted file mode 100755 index b33625aeaf9e4..0000000000000 --- a/scripts/scimverify/run.sh +++ /dev/null @@ -1,186 +0,0 @@ -#!/usr/bin/env bash - -# Run the SCIM Verify compliance test suite against a running Coder instance. -# -# This script creates a temporary test user, generates a config with the real -# user ID (avoiding scimverify's "AUTO" which targets the first user, typically -# the admin), runs the full test suite, and cleans up. -# -# Usage: -# ./scripts/scimverify/run.sh [--base-url URL] [--token TOKEN] -# -# Environment variables: -# SCIM_BASE_URL Base URL of the SCIM endpoint (default: http://localhost:3000/scim/v2) -# SCIM_AUTH_TOKEN Bearer token for SCIM authentication (required) -# -# Prerequisites: -# - Node.js / npx available on PATH -# - A running Coder instance with SCIM enabled (CODER_SCIM_AUTH_HEADER set) -# -# The tool outputs TAP-format test results to stdout and writes a HAR file -# capturing all request/response pairs for debugging. - -set -euo pipefail - -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -BASE_CONFIG="${SCRIPT_DIR}/config.yaml" -OUTPUT_DIR="${SCRIPT_DIR}/output" - -# Defaults -BASE_URL="${SCIM_BASE_URL:-http://localhost:3000/scim/v2}" -AUTH_TOKEN="${SCIM_AUTH_TOKEN:-}" - -# Parse CLI args (override env vars) -while [[ $# -gt 0 ]]; do - case "$1" in - --base-url) - BASE_URL="$2" - shift 2 - ;; - --token) - AUTH_TOKEN="$2" - shift 2 - ;; - --help | -h) - echo "Usage: $0 [--base-url URL] [--token TOKEN]" - echo "" - echo "Environment variables:" - echo " SCIM_BASE_URL Base URL (default: http://localhost:3000/scim/v2)" - echo " SCIM_AUTH_TOKEN Bearer token (required)" - exit 0 - ;; - *) - echo "Unknown option: $1" >&2 - exit 1 - ;; - esac -done - -if [[ -z "${AUTH_TOKEN}" ]]; then - echo "Error: SCIM auth token is required." >&2 - echo "Set SCIM_AUTH_TOKEN or pass --token TOKEN." >&2 - exit 1 -fi - -if ! command -v npx &>/dev/null; then - echo "Error: npx is not installed. Install Node.js to continue." >&2 - exit 1 -fi - -mkdir -p "${OUTPUT_DIR}" - -AUTH_HEADER="Authorization: Bearer ${AUTH_TOKEN}" - -# --- Step 1: Create a sacrificial test user for PUT/PATCH/DELETE tests --- -# scimverify's "id: AUTO" resolves to the first user from GET /Users?count=1, -# which is usually the admin. Instead, we pre-create a test user and hardcode -# its UUID into a temporary config. - -RAND_SUFFIX="$(head -c 4 /dev/urandom | od -An -tx1 | tr -d ' \n')" -TEST_USERNAME="scimverify-${RAND_SUFFIX}" -TEST_EMAIL="${TEST_USERNAME}@scimverify.test" - -echo "=== SCIM Verify ===" -echo "Base URL: ${BASE_URL}" -echo "Test user: ${TEST_USERNAME}" -echo "HAR out: ${OUTPUT_DIR}/output.har" -echo "" - -echo "Creating test user for PUT/PATCH/DELETE..." -CREATE_RESPONSE=$(curl -s -X POST \ - -H "${AUTH_HEADER}" \ - -H "Content-Type: application/scim+json" \ - "${BASE_URL}/Users" \ - -d "{ - \"schemas\": [\"urn:ietf:params:scim:schemas:core:2.0:User\"], - \"userName\": \"${TEST_USERNAME}\", - \"name\": {\"givenName\": \"Verify\", \"familyName\": \"Test\"}, - \"emails\": [{\"value\": \"${TEST_EMAIL}\", \"primary\": true}], - \"active\": true - }") - -TEST_USER_ID=$(echo "${CREATE_RESPONSE}" | python3 -c "import sys,json; print(json.load(sys.stdin).get('id',''))" 2>/dev/null || true) - -if [[ -z "${TEST_USER_ID}" ]]; then - echo "Warning: Failed to create test user. PUT/PATCH/DELETE tests will use AUTO (may target admin)." >&2 - echo "Response: ${CREATE_RESPONSE}" >&2 - echo "" - # Fall back to the base config (empty PUT/PATCH/DELETE) - CONFIG_FILE="${BASE_CONFIG}" -else - echo "Created test user: ${TEST_USERNAME} (${TEST_USER_ID})" - echo "" - - # --- Step 2: Generate a temporary config with the real user ID --- - CONFIG_FILE=$(mktemp "${OUTPUT_DIR}/config-XXXXXX.yaml") - trap 'rm -f "${CONFIG_FILE}"' EXIT - - cat >"${CONFIG_FILE}" < Date: Fri, 22 May 2026 09:32:39 -0500 Subject: [PATCH 12/33] case insensitive matching --- coderd/database/queries.sql.go | 4 ++-- coderd/database/queries/users.sql | 4 ++-- enterprise/coderd/scim/scim.go | 5 ++++- enterprise/coderd/scim/scimusers.go | 5 ++--- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 3adf2136f540d..3394539430ba8 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -27768,13 +27768,13 @@ WHERE -- Filter by exact username AND CASE WHEN $4 :: text != '' THEN - username = $4 + lower(username) = lower($4) ELSE true END -- Filter by exact email AND CASE WHEN $5 :: text != '' THEN - email = $5 + lower(email) = lower($5) ELSE true END -- Filter by status diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index ec8469106ff51..fd4a65385227d 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -489,13 +489,13 @@ WHERE -- Filter by exact username AND CASE WHEN @exactUsername :: text != '' THEN - username = @exactUsername + lower(username) = lower(@exactUsername) ELSE true END -- Filter by exact email AND CASE WHEN @exactEmail :: text != '' THEN - email = @exactEmail + lower(email) = lower(@exactEmail) ELSE true END -- Filter by status diff --git a/enterprise/coderd/scim/scim.go b/enterprise/coderd/scim/scim.go index c6a665145cabd..8fa3251da6896 100644 --- a/enterprise/coderd/scim/scim.go +++ b/enterprise/coderd/scim/scim.go @@ -63,7 +63,10 @@ func New(opts *Options) (*Server, error) { Primary: true, }, }, - MaxResults: 0, + MaxResults: 0, + // SupportFiltering is set to false, as all filtering operations are not + // supported. A minimal filtering syntax is supported because Okta seems to + // ignore this field and attempt to filter anyway. SupportFiltering: false, SupportPatch: true, }, diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index f43d10df23c07..6bf26386f51e6 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -35,10 +35,9 @@ func (ru *ResourceUser) scimAudit(ctx context.Context, r *http.Request, action d }) auditor := *ru.opts.Auditor.Load() + // This is a best effort + // TODO: Check X-Forwarded-For and others for proxied requests ip := r.RemoteAddr - if forwarded := r.Header.Get("X-Forwarded-For"); forwarded != "" { - ip = forwarded - } audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ Audit: auditor, From c041338334dd9911d5b822fdcedd534c9474c7ab Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 09:45:26 -0500 Subject: [PATCH 13/33] ad unit test for case insensistive --- enterprise/coderd/scim/scimusers.go | 77 ++++++++-- enterprise/coderd/scim/scimusers_test.go | 184 +++++++++++++++++++++++ 2 files changed, 250 insertions(+), 11 deletions(-) diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index 6bf26386f51e6..d6a0bae03286f 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "strconv" + "strings" "github.com/elimity-com/scim" scimErrors "github.com/elimity-com/scim/errors" @@ -65,7 +66,7 @@ func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttribut // Extract fields from the SCIM attributes. // Do our best to match what the OIDC signup flow also does. - username, _ := attributes["userName"].(string) + username, _ := AttributeAsString(attributes, "userName") email := primaryEmail(attributes) if email == "" { // email is required @@ -86,7 +87,7 @@ func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttribut // We should consider whether we want to support that for SCIM as well, and if so, apply that validation here. active := true - if a, ok := attributes["active"]; ok { + if a, ok := Attribute(attributes, "active"); ok { v, err := BooleanValue(a) if err != nil { return scim.Resource{}, scimErrors.ScimErrorBadRequest( @@ -260,14 +261,14 @@ func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.R } // TODO: Check primary email - activeStr, ok := attributes["active"] + activeInterface, ok := Attribute(attributes, "active") if !ok { return scim.Resource{}, scimErrors.ScimErrorBadRequest("missing required 'active' field") } - active, err := BooleanValue(activeStr) + active, err := BooleanValue(activeInterface) if err != nil { - return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid boolean value for 'active' field: %v", activeStr)) + return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid boolean value for 'active' field: %v", activeInterface)) } newStatus := scimUserStatus(dbUser, &active) @@ -360,7 +361,7 @@ func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.P } activeSet = &v } else if m, ok := op.Value.(map[string]interface{}); ok { - if actV, ok := m["active"]; ok { + if actV, ok := Attribute(m, "active"); ok { v, err := BooleanValue(actV) if err != nil { return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid boolean value for 'active' field: %v", actV)) @@ -471,6 +472,60 @@ func userResourceFromGetUsersRow(u database.GetUsersRow) scim.Resource { } } +func AttributeAsBool(attrs scim.ResourceAttributes, key string) (bool, bool) { + val, ok := Attribute(attrs, key) + if !ok { + return false, false + } + + switch v := val.(type) { + case string: + pv, err := strconv.ParseBool(v) + return pv, err == nil + case bool: + return v, true + default: + return false, false + } +} + +func AttributeAsString(attrs scim.ResourceAttributes, key string) (string, bool) { + val, ok := Attribute(attrs, key) + if !ok { + return "", false + } + + switch v := val.(type) { + case string: + return v, true + case bool: + return strconv.FormatBool(v), true + default: + return "", false + } +} + +func Attribute(attrs scim.ResourceAttributes, key string) (interface{}, bool) { + // attribute names are case-insensitive per SCIM spec + val, ok := attrs[key] + if ok { + return val, true + } + + // This is terrible, but we need to iterate the map to find the key in a case-insensitive way. + // The scim Spec says attribute names are case-insensitive. + for k, v := range attrs { + if k == key { + return v, true + } + if len(k) == len(key) && strings.EqualFold(k, key) { + return v, true + } + } + + return nil, false +} + // BadUUID returns a 404 not-found error for non-UUID identifiers. // SCIM clients may send arbitrary strings as IDs; returning 404 // (rather than 400) signals that no resource matches. @@ -490,7 +545,7 @@ func BooleanValue(v interface{}) (bool, error) { } func AttributeEqual[T comparable](existing T, attrs scim.ResourceAttributes, key string) bool { - found, ok := attrs[key] + found, ok := Attribute(attrs, key) if !ok { return true // No change if the attribute is not present in the request } @@ -505,7 +560,7 @@ func AttributeEqual[T comparable](existing T, attrs scim.ResourceAttributes, key // primaryEmail extracts the primary email from SCIM resource attributes. func primaryEmail(attributes scim.ResourceAttributes) string { - emailsRaw, ok := attributes["emails"] + emailsRaw, ok := Attribute(attributes, "emails") if !ok { return "" } @@ -520,8 +575,8 @@ func primaryEmail(attributes scim.ResourceAttributes) string { if !ok { continue } - if primary, _ := emailMap["primary"].(bool); primary { - if val, ok := emailMap["value"].(string); ok { + if primary, _ := AttributeAsBool(emailMap, "primary"); primary { + if val, ok := AttributeAsString(emailMap, "value"); ok { return val } } @@ -530,7 +585,7 @@ func primaryEmail(attributes scim.ResourceAttributes) string { // Fallback: if no email is marked primary, use the first one. if len(emails) > 0 { if emailMap, ok := emails[0].(map[string]interface{}); ok { - if val, ok := emailMap["value"].(string); ok { + if val, ok := AttributeAsString(emailMap, "value"); ok { return val } } diff --git a/enterprise/coderd/scim/scimusers_test.go b/enterprise/coderd/scim/scimusers_test.go index fced13d837462..2cd54bdf1074d 100644 --- a/enterprise/coderd/scim/scimusers_test.go +++ b/enterprise/coderd/scim/scimusers_test.go @@ -192,6 +192,33 @@ func TestPrimaryEmail(t *testing.T) { attrs: scim.ResourceAttributes{"emails": "not-a-list"}, expected: "", }, + { + name: "case-insensitive top-level key", + attrs: scim.ResourceAttributes{ + "Emails": []interface{}{ + map[string]interface{}{"value": "a@b.com", "primary": true}, + }, + }, + expected: "a@b.com", + }, + { + name: "case-insensitive inner keys", + attrs: scim.ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{"Value": "a@b.com", "Primary": true}, + }, + }, + expected: "a@b.com", + }, + { + name: "all caps keys", + attrs: scim.ResourceAttributes{ + "EMAILS": []interface{}{ + map[string]interface{}{"VALUE": "a@b.com", "PRIMARY": true}, + }, + }, + expected: "a@b.com", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -233,8 +260,165 @@ func TestBooleanValue(t *testing.T) { } } +func TestAttribute(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + attrs scim.ResourceAttributes + key string + wantVal interface{} + wantOK bool + }{ + {"exact match", scim.ResourceAttributes{"active": true}, "active", true, true}, + {"capital first", scim.ResourceAttributes{"active": true}, "Active", true, true}, + {"all caps", scim.ResourceAttributes{"active": true}, "ACTIVE", true, true}, + {"camelCase key", scim.ResourceAttributes{"userName": "alice"}, "username", "alice", true}, + {"camelCase swapped", scim.ResourceAttributes{"username": "alice"}, "userName", "alice", true}, + {"missing key", scim.ResourceAttributes{"active": true}, "missing", nil, false}, + {"empty map", scim.ResourceAttributes{}, "active", nil, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + val, ok := Attribute(tt.attrs, tt.key) + assert.Equal(t, tt.wantOK, ok) + assert.Equal(t, tt.wantVal, val) + }) + } +} + +func TestAttributeAsBool(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + attrs scim.ResourceAttributes + key string + want bool + wantOK bool + }{ + {"exact key bool", scim.ResourceAttributes{"active": true}, "active", true, true}, + {"mixed case bool", scim.ResourceAttributes{"active": false}, "Active", false, true}, + {"all caps bool", scim.ResourceAttributes{"active": true}, "ACTIVE", true, true}, + {"mixed case string true", scim.ResourceAttributes{"active": "true"}, "Active", true, true}, + {"mixed case string false", scim.ResourceAttributes{"active": "false"}, "ACTIVE", false, true}, + {"missing key", scim.ResourceAttributes{}, "active", false, false}, + {"non-convertible", scim.ResourceAttributes{"active": 42}, "active", false, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, ok := AttributeAsBool(tt.attrs, tt.key) + assert.Equal(t, tt.wantOK, ok) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestAttributeAsString(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + attrs scim.ResourceAttributes + key string + want string + wantOK bool + }{ + {"exact key string", scim.ResourceAttributes{"userName": "alice"}, "userName", "alice", true}, + {"mixed case string", scim.ResourceAttributes{"userName": "alice"}, "UserName", "alice", true}, + {"lower case lookup", scim.ResourceAttributes{"userName": "alice"}, "username", "alice", true}, + {"bool to string", scim.ResourceAttributes{"active": true}, "active", "true", true}, + {"mixed case bool to string", scim.ResourceAttributes{"active": false}, "Active", "false", true}, + {"missing key", scim.ResourceAttributes{}, "userName", "", false}, + {"non-convertible", scim.ResourceAttributes{"count": 42}, "count", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, ok := AttributeAsString(tt.attrs, tt.key) + assert.Equal(t, tt.wantOK, ok) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestAttributeEqual(t *testing.T) { + t.Parallel() + + t.Run("exact match same value", func(t *testing.T) { + t.Parallel() + attrs := scim.ResourceAttributes{"userName": "alice"} + assert.True(t, AttributeEqual("alice", attrs, "userName")) + }) + + t.Run("mixed case same value", func(t *testing.T) { + t.Parallel() + attrs := scim.ResourceAttributes{"userName": "alice"} + assert.True(t, AttributeEqual("alice", attrs, "UserName")) + }) + + t.Run("mixed case different value", func(t *testing.T) { + t.Parallel() + attrs := scim.ResourceAttributes{"userName": "bob"} + assert.False(t, AttributeEqual("alice", attrs, "USERNAME")) + }) + + t.Run("missing key means no change", func(t *testing.T) { + t.Parallel() + attrs := scim.ResourceAttributes{} + assert.True(t, AttributeEqual("alice", attrs, "userName")) + }) + + t.Run("type mismatch", func(t *testing.T) { + t.Parallel() + attrs := scim.ResourceAttributes{"userName": 42} + assert.False(t, AttributeEqual("alice", attrs, "userName")) + }) +} + // --- Handler tests (with DB) --- +func TestResourceUser_CaseInsensitive(t *testing.T) { + t.Parallel() + + ru, db, _ := setupSCIM(t) + + // Seed an active user. + user := seedUser(t, db, database.User{ + Status: database.UserStatusActive, + LoginType: database.LoginTypeOIDC, + }) + + r := scimRequest(t) + + // Replace with "Active" (capital A) instead of "active". + res, err := ru.Replace(r, user.ID.String(), scim.ResourceAttributes{ + "userName": user.Username, + "Active": false, + }) + require.NoError(t, err) + assert.Equal(t, false, res.Attributes["active"]) + + // Confirm suspended via Get. + res, err = ru.Get(r, user.ID.String()) + require.NoError(t, err) + assert.Equal(t, false, res.Attributes["active"]) + + // Patch back with map-style replace using "Active" key. + res, err = ru.Patch(r, user.ID.String(), []scim.PatchOperation{ + {Op: "replace", Value: map[string]interface{}{"Active": true}}, + }) + require.NoError(t, err) + assert.Equal(t, true, res.Attributes["active"]) + + // Confirm reactivated via Get. + res, err = ru.Get(r, user.ID.String()) + require.NoError(t, err) + assert.Equal(t, true, res.Attributes["active"]) +} + func TestResourceUser_Lifecycle(t *testing.T) { t.Parallel() From 466d68baf7c3026973f7bc57642fd77e2a6d7d1f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 09:58:17 -0500 Subject: [PATCH 14/33] dry up some update user status --- enterprise/coderd/scim/scimusers.go | 69 +++++++++++------------------ 1 file changed, 26 insertions(+), 43 deletions(-) diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index d6a0bae03286f..bd1903a2aff05 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -104,17 +104,11 @@ func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttribut if err == nil { // User already exists. Update their status if needed. status := scimUserStatus(dbUser, &active) - if active && dbUser.Status != status { - newUser, err := ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ - ID: dbUser.ID, - Status: status, - UpdatedAt: dbtime.Now(), - UserIsSeen: false, - }) + if active { + _, err := ru.updateUserStatus(ctx, r, dbUser, status) if err != nil { return scim.Resource{}, err } - ru.scimAudit(ctx, r, database.AuditActionWrite, dbUser, newUser) } return userResource(dbUser), nil @@ -272,18 +266,9 @@ func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.R } newStatus := scimUserStatus(dbUser, &active) - if dbUser.Status != newStatus { - oldUser := dbUser - dbUser, err = ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ - ID: dbUser.ID, - Status: newStatus, - UpdatedAt: dbtime.Now(), - UserIsSeen: false, - }) - if err != nil { - return scim.Resource{}, err - } - ru.scimAudit(ctx, r, database.AuditActionWrite, oldUser, dbUser) + _, err = ru.updateUserStatus(ctx, r, dbUser, newStatus) + if err != nil { + return scim.Resource{}, err } return userResource(dbUser), nil @@ -307,17 +292,9 @@ func (ru *ResourceUser) Delete(r *http.Request, idStr string) error { return err } - if dbUser.Status != database.UserStatusSuspended { - newUser, err := ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ - ID: dbUser.ID, - Status: database.UserStatusSuspended, - UpdatedAt: dbtime.Now(), - UserIsSeen: false, - }) - if err != nil { - return err - } - ru.scimAudit(ctx, r, database.AuditActionWrite, dbUser, newUser) + _, err = ru.updateUserStatus(ctx, r, dbUser, database.UserStatusSuspended) + if err != nil { + return err } return nil @@ -374,23 +351,29 @@ func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.P } newStatus := scimUserStatus(dbUser, activeSet) - if dbUser.Status != newStatus { - oldUser := dbUser - dbUser, err = ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ - ID: dbUser.ID, - Status: newStatus, - UpdatedAt: dbtime.Now(), - UserIsSeen: false, - }) - if err != nil { - return scim.Resource{}, err - } - ru.scimAudit(ctx, r, database.AuditActionWrite, oldUser, dbUser) + dbUser, err = ru.updateUserStatus(ctx, r, dbUser, newStatus) + if err != nil { + return scim.Resource{}, err } return userResource(dbUser), nil } +// updateUserStatus is a no-op if the status did not change. +func (ru *ResourceUser) updateUserStatus(ctx context.Context, r *http.Request, u database.User, status database.UserStatus) (database.User, error) { + if u.Status == status { + return u, nil + } + newUser, err := ru.store.UpdateUserStatus(ctx, database.UpdateUserStatusParams{ + ID: u.ID, Status: status, UpdatedAt: dbtime.Now(), UserIsSeen: false, + }) + if err != nil { + return database.User{}, err + } + ru.scimAudit(ctx, r, database.AuditActionWrite, u, newUser) + return newUser, nil +} + // scimUserStatus maps the SCIM "active" boolean to Coder's internal user status. // It preserves the active/dormant distinction: active users stay active, // dormant or suspended users become dormant when re-activated (they become From 130db62af88dc43877a0836eba99041a4ad32ae2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 10:00:35 -0500 Subject: [PATCH 15/33] dry up user fetch by uuid --- enterprise/coderd/scim/scimusers.go | 47 ++++++++++++----------------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index bd1903a2aff05..dc9aab68fae07 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -169,16 +169,8 @@ func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttribut // Get implements scim.ResourceHandler. Returns a single user by ID. func (ru *ResourceUser) Get(r *http.Request, idStr string) (scim.Resource, error) { ctx := r.Context() - id, err := uuid.Parse(idStr) - if err != nil { - return scim.Resource{}, BadUUID(idStr, err) - } - - usr, err := ru.store.GetUserByID(ctx, id) + usr, err := ru.user(ctx, idStr) if err != nil { - if xerrors.Is(err, sql.ErrNoRows) { - return scim.Resource{}, scimErrors.ScimErrorResourceNotFound(idStr) - } return scim.Resource{}, err } @@ -236,16 +228,8 @@ func (ru *ResourceUser) GetAll(r *http.Request, params scim.ListRequestParams) ( func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.ResourceAttributes) (scim.Resource, error) { ctx := r.Context() - uid, err := uuid.Parse(idStr) + dbUser, err := ru.user(ctx, idStr) if err != nil { - return scim.Resource{}, BadUUID(idStr, err) - } - - dbUser, err := ru.store.GetUserByID(ctx, uid) - if err != nil { - if xerrors.Is(err, sql.ErrNoRows) { - return scim.Resource{}, scimErrors.ScimErrorResourceNotFound(idStr) - } return scim.Resource{}, err } @@ -279,16 +263,8 @@ func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.R func (ru *ResourceUser) Delete(r *http.Request, idStr string) error { ctx := r.Context() - uid, err := uuid.Parse(idStr) - if err != nil { - return scimErrors.ScimErrorResourceNotFound(idStr) - } - - dbUser, err := ru.store.GetUserByID(ctx, uid) + dbUser, err := ru.user(ctx, idStr) if err != nil { - if xerrors.Is(err, sql.ErrNoRows) { - return scimErrors.ScimErrorResourceNotFound(idStr) - } return err } @@ -359,6 +335,23 @@ func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.P return userResource(dbUser), nil } +func (ru *ResourceUser) user(ctx context.Context, idStr string) (database.User, error) { + id, err := uuid.Parse(idStr) + if err != nil { + return database.User{}, BadUUID(idStr, err) + } + + usr, err := ru.store.GetUserByID(ctx, id) + if err != nil { + if xerrors.Is(err, sql.ErrNoRows) { + return database.User{}, scimErrors.ScimErrorResourceNotFound(idStr) + } + return database.User{}, err + } + + return usr, nil +} + // updateUserStatus is a no-op if the status did not change. func (ru *ResourceUser) updateUserStatus(ctx context.Context, r *http.Request, u database.User, status database.UserStatus) (database.User, error) { if u.Status == status { From 2c9aa7de6e1cf7083965ee46961f3b76bd3c1813 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 10:06:17 -0500 Subject: [PATCH 16/33] 1 pass for primary email --- enterprise/coderd/scim/scimusers.go | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index dc9aab68fae07..1b8b1378ddbbc 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -546,26 +546,21 @@ func primaryEmail(attributes scim.ResourceAttributes) string { return "" } + var fallback string for _, e := range emails { emailMap, ok := e.(map[string]interface{}) if !ok { continue } - if primary, _ := AttributeAsBool(emailMap, "primary"); primary { - if val, ok := AttributeAsString(emailMap, "value"); ok { - return val - } + val, ok := AttributeAsString(emailMap, "value") + if !ok { + continue } - } - - // Fallback: if no email is marked primary, use the first one. - if len(emails) > 0 { - if emailMap, ok := emails[0].(map[string]interface{}); ok { - if val, ok := AttributeAsString(emailMap, "value"); ok { - return val - } + if primary, _ := AttributeAsBool(emailMap, "primary"); primary { + return val } + fallback = val } - return "" + return fallback } From 630f1af3fc93ba08f5d56d74ebd82b2e9438b416 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 10:30:40 -0500 Subject: [PATCH 17/33] linting --- enterprise/coderd/scim/scim.go | 1 + enterprise/coderd/scim/scimusers.go | 2 +- ...ers_test.go => scimusers_internal_test.go} | 2 ++ enterprise/coderd/scim_test.go | 22 ++++++------------- 4 files changed, 11 insertions(+), 16 deletions(-) rename enterprise/coderd/scim/{scimusers_test.go => scimusers_internal_test.go} (99%) diff --git a/enterprise/coderd/scim/scim.go b/enterprise/coderd/scim/scim.go index 8fa3251da6896..944602ee57a4d 100644 --- a/enterprise/coderd/scim/scim.go +++ b/enterprise/coderd/scim/scim.go @@ -102,6 +102,7 @@ func (s *Server) AuthMiddleware(next http.Handler) http.Handler { } // All authenticated requests are treated as coming from the SCIM provisioner + //nolint: auth header authenticates as this identity ctx := dbauthz.AsSCIMProvisioner(r.Context()) r = r.WithContext(ctx) diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index 1b8b1378ddbbc..2a9ee322d0e5a 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -448,7 +448,7 @@ func userResourceFromGetUsersRow(u database.GetUsersRow) scim.Resource { } } -func AttributeAsBool(attrs scim.ResourceAttributes, key string) (bool, bool) { +func AttributeAsBool(attrs scim.ResourceAttributes, key string) (value bool, exists bool) { val, ok := Attribute(attrs, key) if !ok { return false, false diff --git a/enterprise/coderd/scim/scimusers_test.go b/enterprise/coderd/scim/scimusers_internal_test.go similarity index 99% rename from enterprise/coderd/scim/scimusers_test.go rename to enterprise/coderd/scim/scimusers_internal_test.go index 2cd54bdf1074d..a004c3cfe5371 100644 --- a/enterprise/coderd/scim/scimusers_test.go +++ b/enterprise/coderd/scim/scimusers_internal_test.go @@ -568,6 +568,8 @@ func TestResourceUser_Errors(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := tt.run() require.Error(t, err) var scimErr scimErrors.ScimError diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go index 35e5cb8a07a92..ca5b84158c755 100644 --- a/enterprise/coderd/scim_test.go +++ b/enterprise/coderd/scim_test.go @@ -58,14 +58,6 @@ func setScimAuth(key []byte) func(*http.Request) { } } -func setScimAuthBearer(key []byte) func(*http.Request) { - return func(r *http.Request) { - // Do strange casing to ensure it's case-insensitive - r.Header.Set("Authorization", "beAreR "+string(key)) - } -} - - // TestLegacyScim tests the legacy SCIM handler (imulab/go-scim based). // This is a reduced set of integration tests verifying HTTP routing, auth, // and core CRUD. Detailed handler logic is covered by the unit tests in @@ -134,8 +126,8 @@ func TestLegacyScim(t *testing.T) { LicenseOptions: &coderdenttest.LicenseOptions{ AccountID: "coolin", Features: license.Features{ - codersdk.FeatureSCIM: 1, - codersdk.FeatureAuditLog: 1, + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, codersdk.FeatureMultipleOrganizations: 1, }, }, @@ -178,7 +170,7 @@ func TestLegacyScim(t *testing.T) { LicenseOptions: &coderdenttest.LicenseOptions{ AccountID: "coolin", Features: license.Features{ - codersdk.FeatureSCIM: 1, + codersdk.FeatureSCIM: 1, codersdk.FeatureMultipleOrganizations: 1, }, }, @@ -215,8 +207,8 @@ func TestLegacyScim(t *testing.T) { LicenseOptions: &coderdenttest.LicenseOptions{ AccountID: "coolin", Features: license.Features{ - codersdk.FeatureSCIM: 1, - codersdk.FeatureAuditLog: 1, + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, codersdk.FeatureMultipleOrganizations: 1, }, }, @@ -262,8 +254,8 @@ func TestLegacyScim(t *testing.T) { LicenseOptions: &coderdenttest.LicenseOptions{ AccountID: "coolin", Features: license.Features{ - codersdk.FeatureSCIM: 1, - codersdk.FeatureAuditLog: 1, + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, codersdk.FeatureMultipleOrganizations: 1, }, }, From 67b6d6c8388ac65299d8049a1dc682bff3015e39 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 11:06:56 -0500 Subject: [PATCH 18/33] fmyt --- enterprise/coderd/legacyscim/legacyscim.go | 1 - enterprise/coderd/scim/scim.go | 1 - enterprise/coderd/scim/scimusers_internal_test.go | 1 - go.mod | 2 +- 4 files changed, 1 insertion(+), 4 deletions(-) diff --git a/enterprise/coderd/legacyscim/legacyscim.go b/enterprise/coderd/legacyscim/legacyscim.go index 6500d3e8c0b9f..b2cf4182cbb35 100644 --- a/enterprise/coderd/legacyscim/legacyscim.go +++ b/enterprise/coderd/legacyscim/legacyscim.go @@ -27,7 +27,6 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog/v3" - agpl "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" diff --git a/enterprise/coderd/scim/scim.go b/enterprise/coderd/scim/scim.go index 944602ee57a4d..5cd8297a2d629 100644 --- a/enterprise/coderd/scim/scim.go +++ b/enterprise/coderd/scim/scim.go @@ -13,7 +13,6 @@ import ( "github.com/elimity-com/scim/schema" "cdr.dev/slog/v3" - agpl "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" diff --git a/enterprise/coderd/scim/scimusers_internal_test.go b/enterprise/coderd/scim/scimusers_internal_test.go index a004c3cfe5371..974d222725824 100644 --- a/enterprise/coderd/scim/scimusers_internal_test.go +++ b/enterprise/coderd/scim/scimusers_internal_test.go @@ -18,7 +18,6 @@ import ( "cdr.dev/slog/v3" "cdr.dev/slog/v3/sloggers/slogtest" - "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" diff --git a/go.mod b/go.mod index 8d8207d0622f2..7104a57e533a6 100644 --- a/go.mod +++ b/go.mod @@ -518,6 +518,7 @@ require ( github.com/invopop/jsonschema v0.14.0 github.com/mark3labs/mcp-go v0.38.0 github.com/openai/openai-go/v3 v3.28.0 + github.com/scim2/filter-parser/v2 v2.2.0 github.com/shopspring/decimal v1.4.0 github.com/smallstep/pkcs7 v0.2.1 github.com/sony/gobreaker/v2 v2.4.0 @@ -625,7 +626,6 @@ require ( github.com/rhysd/actionlint v1.7.10 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect github.com/samber/lo v1.52.0 // indirect - github.com/scim2/filter-parser/v2 v2.2.0 // indirect github.com/segmentio/asm v1.2.1 // indirect github.com/sergeymakinen/go-bmp v1.0.0 // indirect github.com/sergeymakinen/go-ico v1.0.0-beta.0 // indirect From 874e27b428957f331b2434e7560e541234595c9f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 11:07:26 -0500 Subject: [PATCH 19/33] linting --- enterprise/coderd/scim/scim.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/scim/scim.go b/enterprise/coderd/scim/scim.go index 5cd8297a2d629..8571931e8b9fb 100644 --- a/enterprise/coderd/scim/scim.go +++ b/enterprise/coderd/scim/scim.go @@ -101,7 +101,7 @@ func (s *Server) AuthMiddleware(next http.Handler) http.Handler { } // All authenticated requests are treated as coming from the SCIM provisioner - //nolint: auth header authenticates as this identity + //nolint: gocritic // auth header authenticates as this identity ctx := dbauthz.AsSCIMProvisioner(r.Context()) r = r.WithContext(ctx) From aa3e1f512e2f7cb1044413d952dd1b0dd83f218a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 16:13:10 +0000 Subject: [PATCH 20/33] fix: SCIM Replace returns updated user; dbauthz GetUserCount asserts ResourceUser - Replace: dbUser was discarded after updateUserStatus, so the SCIM response carried the stale pre-update row. Capture the updated row. - GetUserCount dbauthz test: authorization moved from ResourceSystem to ResourceUser; update the assertion to match. Generated by Coder Agents. --- coderd/database/dbauthz/dbauthz_test.go | 2 +- enterprise/coderd/scim/scimusers.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 354453da40000..582af77796dac 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -4546,7 +4546,7 @@ func (s *MethodTestSuite) TestSystemFunctions() { })) s.Run("GetUserCount", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) { dbm.EXPECT().GetUserCount(gomock.Any(), false).Return(int64(0), nil).AnyTimes() - check.Args(false).Asserts(rbac.ResourceSystem, policy.ActionRead).Returns(int64(0)) + check.Args(false).Asserts(rbac.ResourceUser, policy.ActionRead).Returns(int64(0)) })) s.Run("GetTemplates", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) { dbm.EXPECT().GetTemplates(gomock.Any()).Return([]database.Template{}, nil).AnyTimes() diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index 2a9ee322d0e5a..ca2ba4099bcc0 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -250,7 +250,7 @@ func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.R } newStatus := scimUserStatus(dbUser, &active) - _, err = ru.updateUserStatus(ctx, r, dbUser, newStatus) + dbUser, err = ru.updateUserStatus(ctx, r, dbUser, newStatus) if err != nil { return scim.Resource{}, err } From 16faccbfb7a83c42f049f42b03a5f61cef4f6ceb Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 16:28:50 +0000 Subject: [PATCH 21/33] fix: legacy SCIM swag annotations, scim gocritic nolint, regenerate apidocs - legacyscim swag refs: rename coderd.SCIMUser to legacyscim.SCIMUser in @Param/@Success comments. The type moved when the legacy handler was extracted into its own package, but the annotations were not updated, so swag init strict mode failed and make gen could not produce _gen/manifest-staging.json. - scim.go gocritic nolint: drop the stray space after //nolint:. The malformed directive was parsed as an unknown linter name and did not suppress the AsSCIMProvisioner rule, breaking lint/go. - Regenerate coderd/apidoc and docs/reference/api to pick up the legacyscim.SCIMUser rename and the new scim_use_legacy deployment option that was already added to codersdk. Generated by Coder Agents. --- coderd/apidoc/docs.go | 141 +++++++++++---------- coderd/apidoc/swagger.json | 141 +++++++++++---------- docs/reference/api/enterprise.md | 28 ++-- docs/reference/api/general.md | 1 + docs/reference/api/schemas.md | 105 +++++++-------- enterprise/coderd/legacyscim/legacyscim.go | 8 +- enterprise/coderd/scim/scim.go | 2 +- 7 files changed, 218 insertions(+), 208 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 7b85bd3323df2..19063d63b1c05 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -13961,7 +13961,7 @@ const docTemplate = `{ "in": "body", "required": true, "schema": { - "$ref": "#/definitions/coderd.SCIMUser" + "$ref": "#/definitions/legacyscim.SCIMUser" } } ], @@ -13969,7 +13969,7 @@ const docTemplate = `{ "200": { "description": "OK", "schema": { - "$ref": "#/definitions/coderd.SCIMUser" + "$ref": "#/definitions/legacyscim.SCIMUser" } } }, @@ -14035,7 +14035,7 @@ const docTemplate = `{ "in": "body", "required": true, "schema": { - "$ref": "#/definitions/coderd.SCIMUser" + "$ref": "#/definitions/legacyscim.SCIMUser" } } ], @@ -14077,7 +14077,7 @@ const docTemplate = `{ "in": "body", "required": true, "schema": { - "$ref": "#/definitions/coderd.SCIMUser" + "$ref": "#/definitions/legacyscim.SCIMUser" } } ], @@ -14288,71 +14288,6 @@ const docTemplate = `{ "ReinitializeReasonPrebuildClaimed" ] }, - "coderd.SCIMUser": { - "type": "object", - "properties": { - "active": { - "description": "Active is a ptr to prevent the empty value from being interpreted as false.", - "type": "boolean" - }, - "emails": { - "type": "array", - "items": { - "type": "object", - "properties": { - "display": { - "type": "string" - }, - "primary": { - "type": "boolean" - }, - "type": { - "type": "string" - }, - "value": { - "type": "string", - "format": "email" - } - } - } - }, - "groups": { - "type": "array", - "items": {} - }, - "id": { - "type": "string" - }, - "meta": { - "type": "object", - "properties": { - "resourceType": { - "type": "string" - } - } - }, - "name": { - "type": "object", - "properties": { - "familyName": { - "type": "string" - }, - "givenName": { - "type": "string" - } - } - }, - "schemas": { - "type": "array", - "items": { - "type": "string" - } - }, - "userName": { - "type": "string" - } - } - }, "coderd.cspViolation": { "type": "object", "properties": { @@ -18789,6 +18724,9 @@ const docTemplate = `{ "scim_api_key": { "type": "string" }, + "scim_use_legacy": { + "type": "boolean" + }, "session_lifetime": { "$ref": "#/definitions/codersdk.SessionLifetime" }, @@ -27406,6 +27344,71 @@ const docTemplate = `{ "key.NodePublic": { "type": "object" }, + "legacyscim.SCIMUser": { + "type": "object", + "properties": { + "active": { + "description": "Active is a ptr to prevent the empty value from being interpreted as false.", + "type": "boolean" + }, + "emails": { + "type": "array", + "items": { + "type": "object", + "properties": { + "display": { + "type": "string" + }, + "primary": { + "type": "boolean" + }, + "type": { + "type": "string" + }, + "value": { + "type": "string", + "format": "email" + } + } + } + }, + "groups": { + "type": "array", + "items": {} + }, + "id": { + "type": "string" + }, + "meta": { + "type": "object", + "properties": { + "resourceType": { + "type": "string" + } + } + }, + "name": { + "type": "object", + "properties": { + "familyName": { + "type": "string" + }, + "givenName": { + "type": "string" + } + } + }, + "schemas": { + "type": "array", + "items": { + "type": "string" + } + }, + "userName": { + "type": "string" + } + } + }, "netcheck.Report": { "type": "object", "properties": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 0bff17f6dc572..55928186492cb 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -12389,7 +12389,7 @@ "in": "body", "required": true, "schema": { - "$ref": "#/definitions/coderd.SCIMUser" + "$ref": "#/definitions/legacyscim.SCIMUser" } } ], @@ -12397,7 +12397,7 @@ "200": { "description": "OK", "schema": { - "$ref": "#/definitions/coderd.SCIMUser" + "$ref": "#/definitions/legacyscim.SCIMUser" } } }, @@ -12455,7 +12455,7 @@ "in": "body", "required": true, "schema": { - "$ref": "#/definitions/coderd.SCIMUser" + "$ref": "#/definitions/legacyscim.SCIMUser" } } ], @@ -12493,7 +12493,7 @@ "in": "body", "required": true, "schema": { - "$ref": "#/definitions/coderd.SCIMUser" + "$ref": "#/definitions/legacyscim.SCIMUser" } } ], @@ -12692,71 +12692,6 @@ "enum": ["prebuild_claimed"], "x-enum-varnames": ["ReinitializeReasonPrebuildClaimed"] }, - "coderd.SCIMUser": { - "type": "object", - "properties": { - "active": { - "description": "Active is a ptr to prevent the empty value from being interpreted as false.", - "type": "boolean" - }, - "emails": { - "type": "array", - "items": { - "type": "object", - "properties": { - "display": { - "type": "string" - }, - "primary": { - "type": "boolean" - }, - "type": { - "type": "string" - }, - "value": { - "type": "string", - "format": "email" - } - } - } - }, - "groups": { - "type": "array", - "items": {} - }, - "id": { - "type": "string" - }, - "meta": { - "type": "object", - "properties": { - "resourceType": { - "type": "string" - } - } - }, - "name": { - "type": "object", - "properties": { - "familyName": { - "type": "string" - }, - "givenName": { - "type": "string" - } - } - }, - "schemas": { - "type": "array", - "items": { - "type": "string" - } - }, - "userName": { - "type": "string" - } - } - }, "coderd.cspViolation": { "type": "object", "properties": { @@ -17060,6 +16995,9 @@ "scim_api_key": { "type": "string" }, + "scim_use_legacy": { + "type": "boolean" + }, "session_lifetime": { "$ref": "#/definitions/codersdk.SessionLifetime" }, @@ -25271,6 +25209,71 @@ "key.NodePublic": { "type": "object" }, + "legacyscim.SCIMUser": { + "type": "object", + "properties": { + "active": { + "description": "Active is a ptr to prevent the empty value from being interpreted as false.", + "type": "boolean" + }, + "emails": { + "type": "array", + "items": { + "type": "object", + "properties": { + "display": { + "type": "string" + }, + "primary": { + "type": "boolean" + }, + "type": { + "type": "string" + }, + "value": { + "type": "string", + "format": "email" + } + } + } + }, + "groups": { + "type": "array", + "items": {} + }, + "id": { + "type": "string" + }, + "meta": { + "type": "object", + "properties": { + "resourceType": { + "type": "string" + } + } + }, + "name": { + "type": "object", + "properties": { + "familyName": { + "type": "string" + }, + "givenName": { + "type": "string" + } + } + }, + "schemas": { + "type": "array", + "items": { + "type": "string" + } + }, + "userName": { + "type": "string" + } + } + }, "netcheck.Report": { "type": "object", "properties": { diff --git a/docs/reference/api/enterprise.md b/docs/reference/api/enterprise.md index def5c219a2e24..eca72025c0e0d 100644 --- a/docs/reference/api/enterprise.md +++ b/docs/reference/api/enterprise.md @@ -4520,9 +4520,9 @@ curl -X POST http://coder-server:8080/scim/v2/Users \ ### Parameters -| Name | In | Type | Required | Description | -|--------|------|----------------------------------------------|----------|-------------| -| `body` | body | [coderd.SCIMUser](schemas.md#coderdscimuser) | true | New user | +| Name | In | Type | Required | Description | +|--------|------|------------------------------------------------------|----------|-------------| +| `body` | body | [legacyscim.SCIMUser](schemas.md#legacyscimscimuser) | true | New user | ### Example responses @@ -4559,9 +4559,9 @@ curl -X POST http://coder-server:8080/scim/v2/Users \ ### Responses -| Status | Meaning | Description | Schema | -|--------|---------------------------------------------------------|-------------|----------------------------------------------| -| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [coderd.SCIMUser](schemas.md#coderdscimuser) | +| Status | Meaning | Description | Schema | +|--------|---------------------------------------------------------|-------------|------------------------------------------------------| +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [legacyscim.SCIMUser](schemas.md#legacyscimscimuser) | To perform this operation, you must be authenticated. [Learn more](authentication.md). @@ -4638,10 +4638,10 @@ curl -X PUT http://coder-server:8080/scim/v2/Users/{id} \ ### Parameters -| Name | In | Type | Required | Description | -|--------|------|----------------------------------------------|----------|----------------------| -| `id` | path | string(uuid) | true | User ID | -| `body` | body | [coderd.SCIMUser](schemas.md#coderdscimuser) | true | Replace user request | +| Name | In | Type | Required | Description | +|--------|------|------------------------------------------------------|----------|----------------------| +| `id` | path | string(uuid) | true | User ID | +| `body` | body | [legacyscim.SCIMUser](schemas.md#legacyscimscimuser) | true | Replace user request | ### Example responses @@ -4730,10 +4730,10 @@ curl -X PATCH http://coder-server:8080/scim/v2/Users/{id} \ ### Parameters -| Name | In | Type | Required | Description | -|--------|------|----------------------------------------------|----------|---------------------| -| `id` | path | string(uuid) | true | User ID | -| `body` | body | [coderd.SCIMUser](schemas.md#coderdscimuser) | true | Update user request | +| Name | In | Type | Required | Description | +|--------|------|------------------------------------------------------|----------|---------------------| +| `id` | path | string(uuid) | true | User ID | +| `body` | body | [legacyscim.SCIMUser](schemas.md#legacyscimscimuser) | true | Update user request | ### Example responses diff --git a/docs/reference/api/general.md b/docs/reference/api/general.md index 57e564aee31bb..90715c620c84b 100644 --- a/docs/reference/api/general.md +++ b/docs/reference/api/general.md @@ -538,6 +538,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "workspace_agent_logs": 0 }, "scim_api_key": "string", + "scim_use_legacy": true, "session_lifetime": { "default_duration": 0, "default_token_lifetime": 0, diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index c2cffd13d441b..92b379bbfc82a 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -220,57 +220,6 @@ |--------------------| | `prebuild_claimed` | -## coderd.SCIMUser - -```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" -} -``` - -### Properties - -| 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 ```json @@ -6059,6 +6008,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "workspace_agent_logs": 0 }, "scim_api_key": "string", + "scim_use_legacy": true, "session_lifetime": { "default_duration": 0, "default_token_lifetime": 0, @@ -6658,6 +6608,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "workspace_agent_logs": 0 }, "scim_api_key": "string", + "scim_use_legacy": true, "session_lifetime": { "default_duration": 0, "default_token_lifetime": 0, @@ -6818,6 +6769,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | `redirect_to_access_url` | boolean | false | | | | `retention` | [codersdk.RetentionConfig](#codersdkretentionconfig) | false | | | | `scim_api_key` | string | false | | | +| `scim_use_legacy` | boolean | false | | | | `session_lifetime` | [codersdk.SessionLifetime](#codersdksessionlifetime) | false | | | | `ssh_keygen_algorithm` | string | false | | | | `stats_collection` | [codersdk.StatsCollectionConfig](#codersdkstatscollectionconfig) | false | | | @@ -17916,6 +17868,57 @@ Zero means unspecified. There might be a limit, but the client need not try to r None +## legacyscim.SCIMUser + +```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" +} +``` + +### Properties + +| 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 | | | + ## netcheck.Report ```json diff --git a/enterprise/coderd/legacyscim/legacyscim.go b/enterprise/coderd/legacyscim/legacyscim.go index b2cf4182cbb35..d92965f3bd814 100644 --- a/enterprise/coderd/legacyscim/legacyscim.go +++ b/enterprise/coderd/legacyscim/legacyscim.go @@ -250,8 +250,8 @@ var SCIMAuditAdditionalFields = map[string]string{ // @Security Authorization // @Produce json // @Tags Enterprise -// @Param request body coderd.SCIMUser true "New user" -// @Success 200 {object} coderd.SCIMUser +// @Param request body legacyscim.SCIMUser true "New user" +// @Success 200 {object} legacyscim.SCIMUser // @Router /scim/v2/Users [post] func (s *LegacyServer) scimPostUser(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -399,7 +399,7 @@ func (s *LegacyServer) scimPostUser(rw http.ResponseWriter, r *http.Request) { // @Produce application/scim+json // @Tags Enterprise // @Param id path string true "User ID" format(uuid) -// @Param request body coderd.SCIMUser true "Update user request" +// @Param request body legacyscim.SCIMUser true "Update user request" // @Success 200 {object} codersdk.User // @Router /scim/v2/Users/{id} [patch] func (s *LegacyServer) scimPatchUser(rw http.ResponseWriter, r *http.Request) { @@ -483,7 +483,7 @@ func (s *LegacyServer) scimPatchUser(rw http.ResponseWriter, r *http.Request) { // @Produce application/scim+json // @Tags Enterprise // @Param id path string true "User ID" format(uuid) -// @Param request body coderd.SCIMUser true "Replace user request" +// @Param request body legacyscim.SCIMUser true "Replace user request" // @Success 200 {object} codersdk.User // @Router /scim/v2/Users/{id} [put] func (s *LegacyServer) scimPutUser(rw http.ResponseWriter, r *http.Request) { diff --git a/enterprise/coderd/scim/scim.go b/enterprise/coderd/scim/scim.go index 8571931e8b9fb..ee9c18205eea7 100644 --- a/enterprise/coderd/scim/scim.go +++ b/enterprise/coderd/scim/scim.go @@ -101,7 +101,7 @@ func (s *Server) AuthMiddleware(next http.Handler) http.Handler { } // All authenticated requests are treated as coming from the SCIM provisioner - //nolint: gocritic // auth header authenticates as this identity + //nolint:gocritic // auth header authenticates as this identity ctx := dbauthz.AsSCIMProvisioner(r.Context()) r = r.WithContext(ctx) From 329385f6fe64f0f7b4843850e6838acd1d64d40b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 11:42:26 -0500 Subject: [PATCH 22/33] linting --- enterprise/coderd/scim/scimusers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index ca2ba4099bcc0..2d0a5c201d1eb 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -516,7 +516,7 @@ func BooleanValue(v interface{}) (bool, error) { case string: return strconv.ParseBool(b) default: - return false, fmt.Errorf("expected boolean or string value, got %T", v) + return false, xerrors.Errorf("expected boolean or string value, got %T", v) } } From 1a9629ae7a11a5026fa1800b4eb410143e80bce4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 16:59:42 +0000 Subject: [PATCH 23/33] fix(enterprise/coderd/scim): escalate to system context when SCIM creates users ResourceUser.Create called CreateUser with the SCIM provisioner context. CreateUser invokes InsertUser, which always implicitly assigns the member role, and may also call InsertOrganizationMember when org sync assigns the default org. Neither role can be assigned by the SCIM provisioner role, so creating a user through SCIM returned 500 with 'not authorized to assign role "member"'. The legacy SCIM handler wrapped the same call in dbauthz.AsSystemRestricted; the new handler had a comment promising the same wrapper but the wrapper itself was missing. Restore the legacy behavior so SCIM POST /Users succeeds and refresh the comment to describe what the code actually does. Verified with scimverify (https://verify.scim.dev/): 16/16 tests pass, up from 11/13 (2 user-create failures) before this fix. Generated by Coder Agents. --- enterprise/coderd/scim/scimusers.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index 2d0a5c201d1eb..b0cd12b321f6f 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -18,6 +18,7 @@ import ( agpl "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" @@ -143,10 +144,12 @@ func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttribut organizations = append(organizations, defaultOrganization.ID) } - // CreateUser does InsertOrganizationMember internally, which needs - // broader permissions than the SCIM provisioner role. Use - // AsSystemRestricted for this specific call. - dbUser, err = ru.opts.AGPL.CreateUser(ctx, ru.store, agpl.CreateUserRequest{ + // CreateUser does InsertOrganizationMember internally, and InsertUser + // implicitly assigns the member role at site scope. The SCIM provisioner + // role cannot assign either, so escalate to a system context for this + // specific call, matching the legacy SCIM handler. + //nolint:gocritic // SCIM bearer token authenticates as the SCIM provisioner; user creation needs broader rights to assign default roles. + dbUser, err = ru.opts.AGPL.CreateUser(dbauthz.AsSystemRestricted(ctx), ru.store, agpl.CreateUserRequest{ CreateUserRequestWithOrgs: codersdk.CreateUserRequestWithOrgs{ Username: username, Email: email, From e0581b070a87b1130589eae33714bb72482e4ffc Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 May 2026 10:06:01 -0500 Subject: [PATCH 24/33] 409 on create user conflict --- enterprise/coderd/scim/scimusers.go | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/scimusers.go index b0cd12b321f6f..254c023a0a87e 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/scimusers.go @@ -103,16 +103,11 @@ func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttribut Username: username, }) if err == nil { - // User already exists. Update their status if needed. - status := scimUserStatus(dbUser, &active) - if active { - _, err := ru.updateUserStatus(ctx, r, dbUser, status) - if err != nil { - return scim.Resource{}, err - } + return scim.Resource{}, scimErrors.ScimError{ + ScimType: scimErrors.ScimTypeUniqueness, + Detail: fmt.Sprintf("user with email (%q) or username (%q)", email, username), + Status: http.StatusConflict, } - - return userResource(dbUser), nil } if !xerrors.Is(err, sql.ErrNoRows) { @@ -241,7 +236,8 @@ func (ru *ResourceUser) Replace(r *http.Request, idStr string, attributes scim.R return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("changing the 'userName' field is not supported (current value: %q)", dbUser.Username)) } - // TODO: Check primary email + // TODO: Check if the primary email has changed. If it has, should we do something? + activeInterface, ok := Attribute(attributes, "active") if !ok { return scim.Resource{}, scimErrors.ScimErrorBadRequest("missing required 'active' field") @@ -303,14 +299,14 @@ func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.P switch op.Op { case "add": case "remove": - if op.Path.String() == "active" { + if strings.EqualFold(op.Path.String(), "active") { activeSet = ptr.Ref(false) } case "replace": // SCIM PATCH replace can come in two forms: // 1. Path set: {"op":"replace","path":"active","value":false} // 2. No path, value is a map: {"op":"replace","value":{"active":false}} - if op.Path != nil && op.Path.String() == "active" { + if op.Path != nil && strings.EqualFold(op.Path.String(), "active") { v, err := BooleanValue(op.Value) if err != nil { return scim.Resource{}, scimErrors.ScimErrorBadRequest(fmt.Sprintf("invalid boolean value for 'active' field: %v", op.Value)) From 8cae9de56af3dd2807102f5bb663cffc002f641b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 May 2026 10:30:03 -0500 Subject: [PATCH 25/33] PR feedback --- .../coderd/scim/{scimexpression.go => expression.go} | 0 enterprise/coderd/scim/{scimusers.go => users.go} | 7 ++++++- .../{scimusers_internal_test.go => users_internal_test.go} | 0 3 files changed, 6 insertions(+), 1 deletion(-) rename enterprise/coderd/scim/{scimexpression.go => expression.go} (100%) rename enterprise/coderd/scim/{scimusers.go => users.go} (98%) rename enterprise/coderd/scim/{scimusers_internal_test.go => users_internal_test.go} (100%) diff --git a/enterprise/coderd/scim/scimexpression.go b/enterprise/coderd/scim/expression.go similarity index 100% rename from enterprise/coderd/scim/scimexpression.go rename to enterprise/coderd/scim/expression.go diff --git a/enterprise/coderd/scim/scimusers.go b/enterprise/coderd/scim/users.go similarity index 98% rename from enterprise/coderd/scim/scimusers.go rename to enterprise/coderd/scim/users.go index 254c023a0a87e..fb75bd7035076 100644 --- a/enterprise/coderd/scim/scimusers.go +++ b/enterprise/coderd/scim/users.go @@ -191,7 +191,7 @@ func (ru *ResourceUser) GetAll(r *http.Request, params scim.ListRequestParams) ( qry.LimitOpt = int32(params.Count) //nolint:gosec qry.OffsetOpt = int32(params.StartIndex - 1) //nolint:gosec - if qry.LimitOpt <= 0 { + if qry.LimitOpt < 0 { qry.LimitOpt = 100 } @@ -299,10 +299,15 @@ func (ru *ResourceUser) Patch(r *http.Request, idStr string, operations []scim.P switch op.Op { case "add": case "remove": + // TODO: If the path is unspecified, we should fail with the status code 400. + // Today, we only accept the 'active' field and silently drop the rest. if strings.EqualFold(op.Path.String(), "active") { activeSet = ptr.Ref(false) } case "replace": + // TODO: Honor mutability rules of fields like `userName` and `email`. + // Should scim be able to change those fields? + // SCIM PATCH replace can come in two forms: // 1. Path set: {"op":"replace","path":"active","value":false} // 2. No path, value is a map: {"op":"replace","value":{"active":false}} diff --git a/enterprise/coderd/scim/scimusers_internal_test.go b/enterprise/coderd/scim/users_internal_test.go similarity index 100% rename from enterprise/coderd/scim/scimusers_internal_test.go rename to enterprise/coderd/scim/users_internal_test.go From 6ff1e53ec0d957461852f51710549d7bcc7cce74 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 May 2026 11:01:11 -0500 Subject: [PATCH 26/33] default to old scim behavior --- codersdk/deployment.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codersdk/deployment.go b/codersdk/deployment.go index c80fb3d94e7fd..823833c77a9f4 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -3445,7 +3445,8 @@ func (c *DeploymentValues) Options() serpent.OptionSet { Flag: "scim-use-legacy", Env: "CODER_SCIM_USE_LEGACY", Hidden: true, - Default: "false", + // TODO: When SCIM 2.0 has been tested more, flip this to false to default to the new scim + Default: "true", Annotations: serpent.Annotations{}.Mark(annotationEnterpriseKey, "true"), Value: &c.UseLegacySCIM, }, From b3e6e0aa061ed0272ebf6d5474b0ac16119c2310 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 May 2026 11:13:12 -0500 Subject: [PATCH 27/33] handle deleted users on create --- enterprise/coderd/scim/users.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/enterprise/coderd/scim/users.go b/enterprise/coderd/scim/users.go index fb75bd7035076..91deebf66a45d 100644 --- a/enterprise/coderd/scim/users.go +++ b/enterprise/coderd/scim/users.go @@ -103,11 +103,25 @@ func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttribut Username: username, }) if err == nil { - return scim.Resource{}, scimErrors.ScimError{ - ScimType: scimErrors.ScimTypeUniqueness, - Detail: fmt.Sprintf("user with email (%q) or username (%q)", email, username), - Status: http.StatusConflict, + // SCIM spec says to return a StatusConflict if the user already exists. + // However, Coder never deletes a user. So suspended ~= deleted. + // If the user is not suspended, we return a conflict. + if dbUser.Status != database.UserStatusSuspended { + return scim.Resource{}, scimErrors.ScimError{ + ScimType: scimErrors.ScimTypeUniqueness, + Detail: fmt.Sprintf("user with email (%q) or username (%q)", email, username), + Status: http.StatusConflict, + } + } + + // If the user is suspended, then they might be deleted on the SCIM side. + // We can just update their status and return the user as they exist. + status := scimUserStatus(dbUser, &active) + _, err := ru.updateUserStatus(ctx, r, dbUser, status) + if err != nil { + return scim.Resource{}, err } + return userResource(dbUser), nil } if !xerrors.Is(err, sql.ErrNoRows) { From 9d182e30db91434490ba967915506641d24d4a0e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 May 2026 11:17:28 -0500 Subject: [PATCH 28/33] legacy scim test rename --- enterprise/coderd/scim_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go index ca5b84158c755..a1dd674a4e2c0 100644 --- a/enterprise/coderd/scim_test.go +++ b/enterprise/coderd/scim_test.go @@ -287,7 +287,7 @@ func TestLegacyScim(t *testing.T) { }) } -func TestScimError(t *testing.T) { +func TestLegacyScimError(t *testing.T) { t.Parallel() // Demonstrates that we cannot use the standard errors From 77ddef47d8da7318a7a4e67d7bfd0d09e781ffbf Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 May 2026 16:23:49 +0000 Subject: [PATCH 29/33] fix(enterprise/coderd/scim): return reactivated user from Create The suspended-user recreate branch in b3e6e0aa06 discarded the row returned by updateUserStatus, so userResource was built from the pre-update (still suspended) row and the SCIM response reported active=false even though the DB row was now dormant. Capture the updated row, matching the fix applied to Replace in aa3e1f512e. Add TestResourceUser_Create covering the conflict, reactivation, and no-op paths. Generated by Coder Agents. --- enterprise/coderd/scim/users.go | 2 +- enterprise/coderd/scim/users_internal_test.go | 91 +++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/enterprise/coderd/scim/users.go b/enterprise/coderd/scim/users.go index 91deebf66a45d..282c17002d9d2 100644 --- a/enterprise/coderd/scim/users.go +++ b/enterprise/coderd/scim/users.go @@ -117,7 +117,7 @@ func (ru *ResourceUser) Create(r *http.Request, attributes scim.ResourceAttribut // If the user is suspended, then they might be deleted on the SCIM side. // We can just update their status and return the user as they exist. status := scimUserStatus(dbUser, &active) - _, err := ru.updateUserStatus(ctx, r, dbUser, status) + dbUser, err = ru.updateUserStatus(ctx, r, dbUser, status) if err != nil { return scim.Resource{}, err } diff --git a/enterprise/coderd/scim/users_internal_test.go b/enterprise/coderd/scim/users_internal_test.go index 974d222725824..3f3c110a624f4 100644 --- a/enterprise/coderd/scim/users_internal_test.go +++ b/enterprise/coderd/scim/users_internal_test.go @@ -418,6 +418,97 @@ func TestResourceUser_CaseInsensitive(t *testing.T) { assert.Equal(t, true, res.Attributes["active"]) } +func TestResourceUser_Create(t *testing.T) { + t.Parallel() + + // Coder does not hard-delete users. A SCIM Delete suspends the user, so + // when an IdP later re-creates the same user, the handler should match + // them by email/username and reactivate the existing row instead of + // returning 409 Conflict. See commit b3e6e0aa06. + + t.Run("duplicate-active-conflict", func(t *testing.T) { + t.Parallel() + ru, db, _ := setupSCIM(t) + + existing := seedUser(t, db, database.User{ + Status: database.UserStatusActive, + LoginType: database.LoginTypeOIDC, + }) + + _, err := ru.Create(scimRequest(t), scim.ResourceAttributes{ + "userName": existing.Username, + "emails": []interface{}{ + map[string]interface{}{"value": existing.Email, "primary": true}, + }, + "active": true, + }) + require.Error(t, err) + var scimErr scimErrors.ScimError + require.ErrorAs(t, err, &scimErr) + assert.Equal(t, http.StatusConflict, scimErr.Status) + }) + + t.Run("suspended-user-reactivates", func(t *testing.T) { + t.Parallel() + ru, db, mockAudit := setupSCIM(t) + + existing := seedUser(t, db, database.User{ + Status: database.UserStatusSuspended, + LoginType: database.LoginTypeOIDC, + }) + + res, err := ru.Create(scimRequest(t), scim.ResourceAttributes{ + "userName": existing.Username, + "emails": []interface{}{ + map[string]interface{}{"value": existing.Email, "primary": true}, + }, + "active": true, + }) + require.NoError(t, err) + assert.Equal(t, existing.ID.String(), res.ID, "response should reference the existing user, not a new one") + + // The SCIM response must reflect the post-update state so the IdP + // sees active=true after the recreate. + assert.Equal(t, true, res.Attributes["active"], "response should report the reactivated state") + + // Suspended + active=true reactivates to Dormant (not Active) per scimUserStatus. + got, err := db.GetUserByID(dbauthz.AsSCIMProvisioner(context.Background()), existing.ID) + require.NoError(t, err) + assert.Equal(t, database.UserStatusDormant, got.Status, "suspended user should be marked dormant on recreate") + + // Reactivation should emit one audit log for the status change. + assert.Len(t, mockAudit.AuditLogs(), 1) + }) + + t.Run("suspended-user-stays-suspended-when-active-false", func(t *testing.T) { + t.Parallel() + ru, db, mockAudit := setupSCIM(t) + + existing := seedUser(t, db, database.User{ + Status: database.UserStatusSuspended, + LoginType: database.LoginTypeOIDC, + }) + + res, err := ru.Create(scimRequest(t), scim.ResourceAttributes{ + "userName": existing.Username, + "emails": []interface{}{ + map[string]interface{}{"value": existing.Email, "primary": true}, + }, + "active": false, + }) + require.NoError(t, err) + assert.Equal(t, existing.ID.String(), res.ID) + assert.Equal(t, false, res.Attributes["active"]) + + got, err := db.GetUserByID(dbauthz.AsSCIMProvisioner(context.Background()), existing.ID) + require.NoError(t, err) + assert.Equal(t, database.UserStatusSuspended, got.Status) + + // No status change → no audit log. + assert.Empty(t, mockAudit.AuditLogs()) + }) +} + func TestResourceUser_Lifecycle(t *testing.T) { t.Parallel() From 15faf34b3a00561203c0b6427b650027db7ae143 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 May 2026 16:37:57 +0000 Subject: [PATCH 30/33] test(enterprise/coderd/scim_test.go): integration coverage for SCIM 2.0 Adds TestScim, mirroring TestLegacyScim's structure (disabled, noAuth, post, patch, put) and covering behavior unique to the v2 implementation: discovery endpoints, 409 Conflict on duplicate active users, suspended user reactivation on recreate, GET by id, and DELETE. Requests use a SCIM 2.0 compliant body (with the 'schemas' attribute) because the elimity-com/scim library validates against the core User schema URI and rejects bodies that omit it. Generated by Coder Agents. --- enterprise/coderd/scim_test.go | 415 +++++++++++++++++++++++++++++++++ 1 file changed, 415 insertions(+) diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go index a1dd674a4e2c0..0aeb61d8e0221 100644 --- a/enterprise/coderd/scim_test.go +++ b/enterprise/coderd/scim_test.go @@ -287,6 +287,421 @@ func TestLegacyScim(t *testing.T) { }) } +// scim2User is a minimal struct for decoding SCIM 2.0 user responses +// returned by the elimity-com/scim library. +type scim2User struct { + ID string `json:"id"` + UserName string `json:"userName"` + Active bool `json:"active"` +} + +// scim2UserBody is the request body for SCIM 2.0 POST/PUT calls. +// Unlike the legacy handler, the elimity-com/scim library validates the +// "schemas" attribute against the core User schema URI and rejects bodies +// that omit it. +type scim2UserBody struct { + Schemas []string `json:"schemas"` + UserName string `json:"userName"` + Name struct { + GivenName string `json:"givenName"` + FamilyName string `json:"familyName"` + } `json:"name"` + Emails []struct { + Primary bool `json:"primary"` + Value string `json:"value"` + } `json:"emails"` + Active *bool `json:"active,omitempty"` +} + +func makeScim2User(t testing.TB) scim2UserBody { + rstr, err := cryptorand.String(10) + require.NoError(t, err) + + b := scim2UserBody{ + Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"}, + UserName: rstr, + Active: ptr.Ref(true), + } + b.Name.GivenName = rstr + b.Name.FamilyName = rstr + b.Emails = []struct { + Primary bool `json:"primary"` + Value string `json:"value"` + }{{Primary: true, Value: fmt.Sprintf("%s@coder.com", rstr)}} + return b +} + +// TestScim exercises the SCIM 2.0 handler through real HTTP routes. It +// mirrors TestLegacyScim's structure (disabled/noAuth/post/patch/put) and +// adds coverage for behavior unique to the v2 implementation: discovery +// endpoints, 409 Conflict on duplicate active users, suspended-user +// reactivation, GET by id, and DELETE. +// +//nolint:gocritic // SCIM authenticates via a special header and bypasses internal RBAC. +func TestScim(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, "POST", "/scim/v2/Users", struct{}{}) + require.NoError(t, err) + defer 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, "POST", "/scim/v2/Users", struct{}{}) + require.NoError(t, err) + defer res.Body.Close() + assert.Equal(t, http.StatusUnauthorized, res.StatusCode) + }) + + t.Run("discovery", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + scimAPIKey := []byte("hi") + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + SCIMAPIKey: scimAPIKey, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{codersdk.FeatureSCIM: 1}, + }, + }) + + for _, path := range []string{ + "/scim/v2/ServiceProviderConfig", + "/scim/v2/ResourceTypes", + "/scim/v2/Schemas", + } { + res, err := client.Request(ctx, "GET", path, nil, setScimAuth(scimAPIKey)) + require.NoError(t, err) + _ = res.Body.Close() + assert.Equal(t, http.StatusOK, res.StatusCode, "discovery endpoint %s", path) + } + }) + + t.Run("postUser", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + scimAPIKey := []byte("hi") + mockAudit := audit.NewMock() + notifyEnq := ¬ificationstest.FakeEnqueuer{} + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + Auditor: mockAudit, + NotificationsEnqueuer: notifyEnq, + }, + SCIMAPIKey: scimAPIKey, + AuditLogging: true, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + }) + + sUser := makeScim2User(t) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + defer res.Body.Close() + require.Equal(t, http.StatusCreated, res.StatusCode) + + var created scim2User + require.NoError(t, json.NewDecoder(res.Body).Decode(&created)) + assert.NotEmpty(t, created.ID) + assert.Equal(t, sUser.UserName, created.UserName) + assert.True(t, created.Active) + + // Verify user exists. + userRes, err := client.Users(ctx, codersdk.UsersRequest{Search: created.UserName}) + require.NoError(t, err) + require.Len(t, userRes.Users, 1) + assert.Equal(t, codersdk.LoginTypeOIDC, userRes.Users[0].LoginType) + + // Verify audit log. + require.True(t, len(mockAudit.AuditLogs()) > 0) + + // Verify no user admin notification (SCIM skips notifications). + require.Empty(t, notifyEnq.Sent()) + }) + + t.Run("postUserConflict", func(t *testing.T) { + // SCIM 2.0 returns 409 Conflict on duplicate active user, unlike the + // legacy handler which returned 200 with the existing user. + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + scimAPIKey := []byte("hi") + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + SCIMAPIKey: scimAPIKey, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + }) + + sUser := makeScim2User(t) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + _ = res.Body.Close() + require.Equal(t, http.StatusCreated, res.StatusCode) + + res, err = client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + _ = res.Body.Close() + assert.Equal(t, http.StatusConflict, res.StatusCode) + + userRes, err := client.Users(ctx, codersdk.UsersRequest{Search: sUser.UserName}) + require.NoError(t, err) + require.Len(t, userRes.Users, 1) + }) + + t.Run("postUserReactivatesSuspended", func(t *testing.T) { + // When the SCIM client deletes a user (which only suspends in Coder), + // posting the same user again should reactivate the existing row. + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + scimAPIKey := []byte("hi") + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + SCIMAPIKey: scimAPIKey, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + }) + + sUser := makeScim2User(t) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + var created scim2User + require.NoError(t, json.NewDecoder(res.Body).Decode(&created)) + _ = res.Body.Close() + require.Equal(t, http.StatusCreated, res.StatusCode) + require.NotEmpty(t, created.ID) + + // Delete (suspends) the user. + res, err = client.Request(ctx, "DELETE", "/scim/v2/Users/"+created.ID, nil, setScimAuth(scimAPIKey)) + require.NoError(t, err) + _ = res.Body.Close() + assert.Equal(t, http.StatusNoContent, res.StatusCode) + + userRes, err := client.User(ctx, created.ID) + require.NoError(t, err) + assert.Equal(t, codersdk.UserStatusSuspended, userRes.Status) + + // Re-create. The handler should reactivate the existing row. + res, err = client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + var recreated scim2User + require.NoError(t, json.NewDecoder(res.Body).Decode(&recreated)) + _ = res.Body.Close() + require.Equal(t, http.StatusCreated, res.StatusCode) + assert.Equal(t, created.ID, recreated.ID, "recreate should reactivate the existing row, not create a new one") + assert.True(t, recreated.Active, "recreated user should be active in the SCIM response") + + // The DB user moves from suspended → dormant on reactivate; the SCIM + // response reports both Active and Dormant as active=true. + userRes, err = client.User(ctx, created.ID) + require.NoError(t, err) + assert.Equal(t, codersdk.UserStatusDormant, userRes.Status) + }) + + t.Run("getUser", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + scimAPIKey := []byte("hi") + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + SCIMAPIKey: scimAPIKey, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + }) + + sUser := makeScim2User(t) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + var created scim2User + require.NoError(t, json.NewDecoder(res.Body).Decode(&created)) + _ = res.Body.Close() + require.Equal(t, http.StatusCreated, res.StatusCode) + + res, err = client.Request(ctx, "GET", "/scim/v2/Users/"+created.ID, nil, setScimAuth(scimAPIKey)) + require.NoError(t, err) + defer res.Body.Close() + require.Equal(t, http.StatusOK, res.StatusCode) + + var got scim2User + require.NoError(t, json.NewDecoder(res.Body).Decode(&got)) + assert.Equal(t, created.ID, got.ID) + assert.Equal(t, sUser.UserName, got.UserName) + }) + + t.Run("patchUser", 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, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + }) + + sUser := makeScim2User(t) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + var created scim2User + require.NoError(t, json.NewDecoder(res.Body).Decode(&created)) + _ = res.Body.Close() + require.Equal(t, http.StatusCreated, res.StatusCode) + + // PATCH with replace op setting active=false. + mockAudit.ResetLogs() + patchBody := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + {"op": "replace", "path": "active", "value": false}, + }, + } + res, err = client.Request(ctx, "PATCH", "/scim/v2/Users/"+created.ID, patchBody, setScimAuth(scimAPIKey)) + require.NoError(t, err) + _ = res.Body.Close() + assert.Equal(t, http.StatusOK, res.StatusCode) + + userRes, err := client.User(ctx, created.ID) + require.NoError(t, err) + assert.Equal(t, codersdk.UserStatusSuspended, userRes.Status) + }) + + t.Run("putUser", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + scimAPIKey := []byte("hi") + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + SCIMAPIKey: scimAPIKey, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + }) + + sUser := makeScim2User(t) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + var created scim2User + require.NoError(t, json.NewDecoder(res.Body).Decode(&created)) + _ = res.Body.Close() + require.Equal(t, http.StatusCreated, res.StatusCode) + + // PUT with active=false. + sUser.Active = ptr.Ref(false) + res, err = client.Request(ctx, "PUT", "/scim/v2/Users/"+created.ID, sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + _ = res.Body.Close() + assert.Equal(t, http.StatusOK, res.StatusCode) + + userRes, err := client.User(ctx, created.ID) + require.NoError(t, err) + assert.Equal(t, codersdk.UserStatusSuspended, userRes.Status) + }) + + t.Run("deleteUser", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + scimAPIKey := []byte("hi") + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + SCIMAPIKey: scimAPIKey, + LicenseOptions: &coderdenttest.LicenseOptions{ + AccountID: "coolin", + Features: license.Features{ + codersdk.FeatureSCIM: 1, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + }) + + sUser := makeScim2User(t) + res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) + require.NoError(t, err) + var created scim2User + require.NoError(t, json.NewDecoder(res.Body).Decode(&created)) + _ = res.Body.Close() + require.Equal(t, http.StatusCreated, res.StatusCode) + + res, err = client.Request(ctx, "DELETE", "/scim/v2/Users/"+created.ID, nil, setScimAuth(scimAPIKey)) + require.NoError(t, err) + _ = res.Body.Close() + assert.Equal(t, http.StatusNoContent, res.StatusCode) + + // Coder does not hard-delete users. The user should remain but be suspended. + userRes, err := client.User(ctx, created.ID) + require.NoError(t, err) + assert.Equal(t, codersdk.UserStatusSuspended, userRes.Status) + }) +} + func TestLegacyScimError(t *testing.T) { t.Parallel() From 637f59f073015acf615a2fb4bb010cec7900963e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 22 May 2026 09:25:04 -0500 Subject: [PATCH 31/33] test: add script for unit testing scim 2.0 --- scripts/scimverify/.gitignore | 1 + scripts/scimverify/README.md | 190 +++++++++++++++++++++++++++++++++ scripts/scimverify/config.yaml | 48 +++++++++ scripts/scimverify/run.sh | 186 ++++++++++++++++++++++++++++++++ 4 files changed, 425 insertions(+) create mode 100644 scripts/scimverify/.gitignore create mode 100644 scripts/scimverify/README.md create mode 100644 scripts/scimverify/config.yaml create mode 100755 scripts/scimverify/run.sh diff --git a/scripts/scimverify/.gitignore b/scripts/scimverify/.gitignore new file mode 100644 index 0000000000000..53752db253e3b --- /dev/null +++ b/scripts/scimverify/.gitignore @@ -0,0 +1 @@ +output diff --git a/scripts/scimverify/README.md b/scripts/scimverify/README.md new file mode 100644 index 0000000000000..d4353edbadd82 --- /dev/null +++ b/scripts/scimverify/README.md @@ -0,0 +1,190 @@ +# SCIM Verify + +> This test harness was set up by an AI agent ([Mux](https://mux.coder.com/)) +> during the SCIM 2.0 refactor. It discovered `scimverify`, figured out +> how to run it (the Docker image is a web UI, not the test CLI; use +> `npx scimverify` instead), worked through config quirks (schema detection +> crashes, plural vs singular resource type names, `id: AUTO` targeting the +> admin user), and iterated the config until 14/16 tests passed against a +> live Coder dev server. + +Run SCIM 2.0 compliance tests against a live Coder instance using [scimverify](https://verify.scim.dev/). + +## Prerequisites + +- **Node.js** (npx must be on PATH) +- A running Coder instance with SCIM enabled (`CODER_SCIM_AUTH_HEADER` set) +- An enterprise license (SCIM is an enterprise feature) + +## Quick start + +```bash +# Start a dev server with SCIM enabled +CODER_SCIM_AUTH_HEADER=my-secret-token ./scripts/develop.sh + +# In another terminal, run the tests +./scripts/scimverify/run.sh --token my-secret-token +``` + +## Usage + +``` +./scripts/scimverify/run.sh [--base-url URL] [--token TOKEN] + +Options: + --base-url URL SCIM endpoint base URL (default: http://localhost:3000/scim/v2) + --token TOKEN Bearer token matching CODER_SCIM_AUTH_HEADER (required) + --help Show help + +Environment variables (alternatives to flags): + SCIM_BASE_URL Same as --base-url + SCIM_AUTH_TOKEN Same as --token +``` + +## Reading the output + +The tool outputs [TAP](https://testanything.org/) (Test Anything Protocol) format: + +``` +TAP version 13 +# Subtest: ResourceTypes + ok 1 - Retrieves resource types <-- individual test +ok 1 - ResourceTypes <-- suite result +# Subtest: Schemas + ok 1 - Retrieves schemas +ok 2 - Schemas +# Subtest: Basic tests + ok 1 - Base URL should not contain any query parameters + ok 2 - Base URL should be reachable + ok 3 - Authentication should be required for /Users +ok 3 - Basic tests +# Subtest: Users + ok 1 - userSchema contains attribute userName ... + ok 2 - Retrieves a list of users + not ok 3 - Filters users by userName <-- FAILURE (details follow) + --- + error: |- + User should have matching userName + 'member' !== 'admin' + expected: 'admin' + actual: 'member' + ... + ok 4 - Creates a new user - Alternative 1 +ok 4 - Users +# tests 16 <-- summary +# pass 14 +# fail 1 +# skipped 1 +``` + +Key patterns: +- `ok N - description` means the test passed +- `not ok N - description` means the test failed (error details follow in YAML block) +- `ok N - description # SKIP reason` means the test was skipped +- Suite-level `not ok` means at least one subtest in the suite failed +- The summary at the bottom shows total pass/fail/skip counts + +### Quick filter for results only + +```bash +# Just pass/fail lines +./scripts/scimverify/run.sh --token TOKEN 2>&1 | grep -E " ok| not ok" + +# Summary only +./scripts/scimverify/run.sh --token TOKEN 2>&1 | grep "^# " +``` + +## HAR file + +Each run writes an [HAR file](https://en.wikipedia.org/wiki/HAR_(file_format)) +to `scripts/scimverify/output/output.har` containing every HTTP request and +response. This is useful for debugging failures: + +```bash +# Show all requests with errors +python3 -c " +import json +with open('scripts/scimverify/output/output.har') as f: + har = json.load(f) +for entry in har['log']['entries']: + req = entry['request'] + resp = entry['response'] + if resp['status'] >= 400: + body = resp.get('content', {}).get('text', '') + print(f'{req[\"method\"]} {req[\"url\"]} -> {resp[\"status\"]}') + if body: print(f' {body[:200]}') + print() +" +``` + +The `output/` directory is gitignored. + +## What the tests cover + +The test suite is configured via `config.yaml`. Currently it tests: + +| Category | Tests | What it checks | +|----------|-------|----------------| +| **ResourceTypes** | 1 | `GET /ResourceTypes` returns 200 with valid resource type definitions | +| **Schemas** | 1 | `GET /Schemas` returns 200 with valid schema definitions | +| **Basic** | 3 | URL format, reachability, and authentication enforcement | +| **Users: Read** | 4 | List users, get single user, non-existent user returns 404, attribute filtering | +| **Users: Create** | 2 | POST two test users with valid SCIM payloads, verify 201 response | +| **Users: Filter** | 1 | Filter by userName (expected to fail; `SupportFiltering: false`) | + +### PUT, PATCH, and DELETE + +The `run.sh` script works around scimverify's `id: AUTO` limitation (which +resolves to the first user, typically the admin) by pre-creating a sacrificial +test user via `POST /Users`, capturing its UUID, and generating a temporary +config with the real ID hardcoded for PUT, PATCH, and DELETE tests. The test +user has a random name like `scimverify-a1b2c3d4` and is deleted by the +DELETE test at the end of the run. + +## Configuration + +Edit `config.yaml` to change what gets tested. Key settings: + +```yaml +detectSchema: true # Test /Schemas endpoint +detectResourceTypes: true # Test /ResourceTypes endpoint +verifyPagination: false # Skip pagination tests +verifySorting: false # Skip sorting tests +requireAuthentication: true # Test that unauthenticated requests are rejected + +users: + enabled: true + operations: [GET, POST, PUT, PATCH, DELETE] # Which HTTP methods to test + post_tests: # User creation payloads + - request: { ... } + put_tests: [] # Empty = skip (see note above about id: AUTO) + patch_tests: [] # Empty = skip + delete_tests: [] # Empty = skip + +groups: + enabled: false # Coder does not support SCIM groups +``` + +### Adding test users + +To add more POST test cases, append to `post_tests`. Each entry needs a valid +SCIM User payload. Important Coder-specific notes: + +- `userName` must be a valid Coder username (lowercase, no `@`, no spaces). + If invalid, Coder normalizes it (e.g., `user@example.com` becomes `user`). +- `emails` must include at least one entry with `"primary": true`. +- `active` defaults to `true` if omitted. + +## Expected results + +On a correctly functioning Coder instance with the SCIM 2.0 refactor: + +``` +# tests 16 +# pass 15 +# fail 1 <-- filter test (expected, filtering is not supported) +# skipped 0 +``` + +The single expected failure is the filter-by-userName test, because Coder +declares `SupportFiltering: false` in its ServiceProviderConfig. diff --git a/scripts/scimverify/config.yaml b/scripts/scimverify/config.yaml new file mode 100644 index 0000000000000..a265ace300b94 --- /dev/null +++ b/scripts/scimverify/config.yaml @@ -0,0 +1,48 @@ +# The elimity-com/scim library auto-serves /Schemas and /ResourceTypes +# from the schema definitions, so we can detect them. +detectSchema: true +detectResourceTypes: true +verifyPagination: false +verifySorting: false +requireAuthentication: true + +users: + enabled: true + operations: + - GET + - POST + - PUT + - PATCH + - DELETE + + post_tests: + - request: + { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "scimtest1", + "name": { "givenName": "SCIM", "familyName": "TestOne" }, + "emails": [{ "value": "scimtest1@example.com", "primary": true }], + "active": true, + } + - request: + { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "scimtest2", + "name": { "givenName": "SCIM", "familyName": "TestTwo" }, + "emails": [{ "value": "scimtest2@example.com", "primary": true }], + "active": true, + } + + # PUT/PATCH/DELETE tests use id: AUTO which resolves to the first user + # from GET /Users?count=1. This is typically the admin user, not a test + # user. Suspending or deleting the admin breaks the dev server. + # + # These operations are tested safely in the Go integration tests + # (enterprise/coderd/scim_test.go) where user IDs are controlled. + # The scimverify config only tests operations safe against a live server. + put_tests: [] + patch_tests: [] + delete_tests: [] + +groups: + enabled: false diff --git a/scripts/scimverify/run.sh b/scripts/scimverify/run.sh new file mode 100755 index 0000000000000..b33625aeaf9e4 --- /dev/null +++ b/scripts/scimverify/run.sh @@ -0,0 +1,186 @@ +#!/usr/bin/env bash + +# Run the SCIM Verify compliance test suite against a running Coder instance. +# +# This script creates a temporary test user, generates a config with the real +# user ID (avoiding scimverify's "AUTO" which targets the first user, typically +# the admin), runs the full test suite, and cleans up. +# +# Usage: +# ./scripts/scimverify/run.sh [--base-url URL] [--token TOKEN] +# +# Environment variables: +# SCIM_BASE_URL Base URL of the SCIM endpoint (default: http://localhost:3000/scim/v2) +# SCIM_AUTH_TOKEN Bearer token for SCIM authentication (required) +# +# Prerequisites: +# - Node.js / npx available on PATH +# - A running Coder instance with SCIM enabled (CODER_SCIM_AUTH_HEADER set) +# +# The tool outputs TAP-format test results to stdout and writes a HAR file +# capturing all request/response pairs for debugging. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +BASE_CONFIG="${SCRIPT_DIR}/config.yaml" +OUTPUT_DIR="${SCRIPT_DIR}/output" + +# Defaults +BASE_URL="${SCIM_BASE_URL:-http://localhost:3000/scim/v2}" +AUTH_TOKEN="${SCIM_AUTH_TOKEN:-}" + +# Parse CLI args (override env vars) +while [[ $# -gt 0 ]]; do + case "$1" in + --base-url) + BASE_URL="$2" + shift 2 + ;; + --token) + AUTH_TOKEN="$2" + shift 2 + ;; + --help | -h) + echo "Usage: $0 [--base-url URL] [--token TOKEN]" + echo "" + echo "Environment variables:" + echo " SCIM_BASE_URL Base URL (default: http://localhost:3000/scim/v2)" + echo " SCIM_AUTH_TOKEN Bearer token (required)" + exit 0 + ;; + *) + echo "Unknown option: $1" >&2 + exit 1 + ;; + esac +done + +if [[ -z "${AUTH_TOKEN}" ]]; then + echo "Error: SCIM auth token is required." >&2 + echo "Set SCIM_AUTH_TOKEN or pass --token TOKEN." >&2 + exit 1 +fi + +if ! command -v npx &>/dev/null; then + echo "Error: npx is not installed. Install Node.js to continue." >&2 + exit 1 +fi + +mkdir -p "${OUTPUT_DIR}" + +AUTH_HEADER="Authorization: Bearer ${AUTH_TOKEN}" + +# --- Step 1: Create a sacrificial test user for PUT/PATCH/DELETE tests --- +# scimverify's "id: AUTO" resolves to the first user from GET /Users?count=1, +# which is usually the admin. Instead, we pre-create a test user and hardcode +# its UUID into a temporary config. + +RAND_SUFFIX="$(head -c 4 /dev/urandom | od -An -tx1 | tr -d ' \n')" +TEST_USERNAME="scimverify-${RAND_SUFFIX}" +TEST_EMAIL="${TEST_USERNAME}@scimverify.test" + +echo "=== SCIM Verify ===" +echo "Base URL: ${BASE_URL}" +echo "Test user: ${TEST_USERNAME}" +echo "HAR out: ${OUTPUT_DIR}/output.har" +echo "" + +echo "Creating test user for PUT/PATCH/DELETE..." +CREATE_RESPONSE=$(curl -s -X POST \ + -H "${AUTH_HEADER}" \ + -H "Content-Type: application/scim+json" \ + "${BASE_URL}/Users" \ + -d "{ + \"schemas\": [\"urn:ietf:params:scim:schemas:core:2.0:User\"], + \"userName\": \"${TEST_USERNAME}\", + \"name\": {\"givenName\": \"Verify\", \"familyName\": \"Test\"}, + \"emails\": [{\"value\": \"${TEST_EMAIL}\", \"primary\": true}], + \"active\": true + }") + +TEST_USER_ID=$(echo "${CREATE_RESPONSE}" | python3 -c "import sys,json; print(json.load(sys.stdin).get('id',''))" 2>/dev/null || true) + +if [[ -z "${TEST_USER_ID}" ]]; then + echo "Warning: Failed to create test user. PUT/PATCH/DELETE tests will use AUTO (may target admin)." >&2 + echo "Response: ${CREATE_RESPONSE}" >&2 + echo "" + # Fall back to the base config (empty PUT/PATCH/DELETE) + CONFIG_FILE="${BASE_CONFIG}" +else + echo "Created test user: ${TEST_USERNAME} (${TEST_USER_ID})" + echo "" + + # --- Step 2: Generate a temporary config with the real user ID --- + CONFIG_FILE=$(mktemp "${OUTPUT_DIR}/config-XXXXXX.yaml") + trap 'rm -f "${CONFIG_FILE}"' EXIT + + cat >"${CONFIG_FILE}" < Date: Fri, 22 May 2026 15:17:40 +0000 Subject: [PATCH 32/33] docs(scripts/scimverify): fix markdownlint errors - Add 'text' language to three bare fenced code blocks (MD040) - Add blank line before 'Key patterns' list (MD032) Generated by Coder Agents. --- scripts/scimverify/README.md | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/scripts/scimverify/README.md b/scripts/scimverify/README.md index d4353edbadd82..72a373f2e00c2 100644 --- a/scripts/scimverify/README.md +++ b/scripts/scimverify/README.md @@ -28,7 +28,7 @@ CODER_SCIM_AUTH_HEADER=my-secret-token ./scripts/develop.sh ## Usage -``` +```text ./scripts/scimverify/run.sh [--base-url URL] [--token TOKEN] Options: @@ -45,7 +45,7 @@ Environment variables (alternatives to flags): The tool outputs [TAP](https://testanything.org/) (Test Anything Protocol) format: -``` +```text TAP version 13 # Subtest: ResourceTypes ok 1 - Retrieves resource types <-- individual test @@ -78,6 +78,7 @@ ok 4 - Users ``` Key patterns: + - `ok N - description` means the test passed - `not ok N - description` means the test failed (error details follow in YAML block) - `ok N - description # SKIP reason` means the test was skipped @@ -123,14 +124,14 @@ The `output/` directory is gitignored. The test suite is configured via `config.yaml`. Currently it tests: -| Category | Tests | What it checks | -|----------|-------|----------------| -| **ResourceTypes** | 1 | `GET /ResourceTypes` returns 200 with valid resource type definitions | -| **Schemas** | 1 | `GET /Schemas` returns 200 with valid schema definitions | -| **Basic** | 3 | URL format, reachability, and authentication enforcement | -| **Users: Read** | 4 | List users, get single user, non-existent user returns 404, attribute filtering | -| **Users: Create** | 2 | POST two test users with valid SCIM payloads, verify 201 response | -| **Users: Filter** | 1 | Filter by userName (expected to fail; `SupportFiltering: false`) | +| Category | Tests | What it checks | +|-------------------|-------|---------------------------------------------------------------------------------| +| **ResourceTypes** | 1 | `GET /ResourceTypes` returns 200 with valid resource type definitions | +| **Schemas** | 1 | `GET /Schemas` returns 200 with valid schema definitions | +| **Basic** | 3 | URL format, reachability, and authentication enforcement | +| **Users: Read** | 4 | List users, get single user, non-existent user returns 404, attribute filtering | +| **Users: Create** | 2 | POST two test users with valid SCIM payloads, verify 201 response | +| **Users: Filter** | 1 | Filter by userName (expected to fail; `SupportFiltering: false`) | ### PUT, PATCH, and DELETE @@ -179,7 +180,7 @@ SCIM User payload. Important Coder-specific notes: On a correctly functioning Coder instance with the SCIM 2.0 refactor: -``` +```text # tests 16 # pass 15 # fail 1 <-- filter test (expected, filtering is not supported) From 27e439995a16d8370044ee2f25ed8b407d99440d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 May 2026 17:17:21 +0000 Subject: [PATCH 33/33] test(scripts/scimverify): expand test cases config.yaml: add three more POST cases covering a minimal body (no name, no active), initial active=false, and a multi-email payload that exercises primary-email selection. run.sh: add a second PUT case (suspend), two more PATCH cases (path-less reactivate using {value: {active: true}}, then suspend again), and a curl-based recreate-after-delete check that re-POSTs the deleted user and asserts the existing row is reactivated (same ID), not duplicated. The scimverify suite cannot exercise that flow because DELETE is its final step. README: refresh the test count table and expected output (22/22 now, previously 16/16 with one expected filter failure that scimverify no longer emits when SupportFiltering is false). Validated end to end against a coderdenttest server: # tests 22, # pass 22, # fail 0, # skipped 0, plus the recreate-after-delete check passing. Generated by Coder Agents. --- scripts/scimverify/README.md | 23 ++++++--- scripts/scimverify/config.yaml | 30 ++++++++++++ scripts/scimverify/run.sh | 89 ++++++++++++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 8 deletions(-) diff --git a/scripts/scimverify/README.md b/scripts/scimverify/README.md index 72a373f2e00c2..f2644d2024185 100644 --- a/scripts/scimverify/README.md +++ b/scripts/scimverify/README.md @@ -129,9 +129,16 @@ The test suite is configured via `config.yaml`. Currently it tests: | **ResourceTypes** | 1 | `GET /ResourceTypes` returns 200 with valid resource type definitions | | **Schemas** | 1 | `GET /Schemas` returns 200 with valid schema definitions | | **Basic** | 3 | URL format, reachability, and authentication enforcement | -| **Users: Read** | 4 | List users, get single user, non-existent user returns 404, attribute filtering | -| **Users: Create** | 2 | POST two test users with valid SCIM payloads, verify 201 response | -| **Users: Filter** | 1 | Filter by userName (expected to fail; `SupportFiltering: false`) | +| **Users: Read** | 6 | List users, get single user, non-existent user returns 404, attribute filtering | +| **Users: Create** | 5 | Standard, minimal body, `active: false` initial state, and multi-email payloads | +| **Users: PUT** | 2 | Rename via PUT, then suspend via PUT | +| **Users: PATCH** | 3 | Suspend with path, reactivate via path-less op, suspend again | +| **Users: DELETE** | 1 | Delete suspends the user (Coder does not hard-delete) | + +After the scimverify suite finishes, `run.sh` does one extra curl-based check +that the suite itself does not cover: re-`POST`ing the deleted user should +reactivate the existing row (same ID) instead of creating a duplicate or +returning 409 Conflict. This exercises Coder's suspend-equals-delete semantics. ### PUT, PATCH, and DELETE @@ -181,11 +188,11 @@ SCIM User payload. Important Coder-specific notes: On a correctly functioning Coder instance with the SCIM 2.0 refactor: ```text -# tests 16 -# pass 15 -# fail 1 <-- filter test (expected, filtering is not supported) +# tests 22 +# pass 22 +# fail 0 # skipped 0 ``` -The single expected failure is the filter-by-userName test, because Coder -declares `SupportFiltering: false` in its ServiceProviderConfig. +The `run.sh` recreate-after-delete check then prints +`ok - POST after DELETE reactivated existing user ()`. diff --git a/scripts/scimverify/config.yaml b/scripts/scimverify/config.yaml index a265ace300b94..c90d573aaa10a 100644 --- a/scripts/scimverify/config.yaml +++ b/scripts/scimverify/config.yaml @@ -32,6 +32,36 @@ users: "emails": [{ "value": "scimtest2@example.com", "primary": true }], "active": true, } + # Minimal body: only userName + primary email; "active" defaults to true. + - request: + { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "scimtest3", + "emails": [{ "value": "scimtest3@example.com", "primary": true }], + } + # Initial state "active": false. Coder creates the user as dormant and the + # SCIM response reports active=false. + - request: + { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "scimtest4", + "name": { "givenName": "SCIM", "familyName": "TestFour" }, + "emails": [{ "value": "scimtest4@example.com", "primary": true }], + "active": false, + } + # Multiple emails; the primary one (second entry) is what Coder picks. + - request: + { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "scimtest5", + "name": { "givenName": "SCIM", "familyName": "TestFive" }, + "emails": + [ + { "value": "scimtest5-alt@example.com" }, + { "value": "scimtest5@example.com", "primary": true }, + ], + "active": true, + } # PUT/PATCH/DELETE tests use id: AUTO which resolves to the first user # from GET /Users?count=1. This is typically the admin user, not a test diff --git a/scripts/scimverify/run.sh b/scripts/scimverify/run.sh index b33625aeaf9e4..518e53dae2ae5 100755 --- a/scripts/scimverify/run.sh +++ b/scripts/scimverify/run.sh @@ -149,6 +149,36 @@ users: "emails": [{ "value": "scimtest2@example.com", "primary": true }], "active": true, } + # Minimal body: only userName + primary email; "active" defaults to true. + - request: + { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "scimtest3", + "emails": [{ "value": "scimtest3@example.com", "primary": true }], + } + # Initial state "active": false. Coder creates the user as dormant and the + # SCIM response reports active=false. + - request: + { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "scimtest4", + "name": { "givenName": "SCIM", "familyName": "TestFour" }, + "emails": [{ "value": "scimtest4@example.com", "primary": true }], + "active": false, + } + # Multiple emails; the primary one (second entry) is what Coder picks. + - request: + { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "scimtest5", + "name": { "givenName": "SCIM", "familyName": "TestFive" }, + "emails": + [ + { "value": "scimtest5-alt@example.com" }, + { "value": "scimtest5@example.com", "primary": true }, + ], + "active": true, + } put_tests: - id: "${TEST_USER_ID}" @@ -160,6 +190,16 @@ users: "emails": [{ "value": "${TEST_EMAIL}", "primary": true }], "active": true, } + # Suspend via PUT. + - id: "${TEST_USER_ID}" + request: + { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "${TEST_USERNAME}", + "name": { "givenName": "Updated", "familyName": "Test" }, + "emails": [{ "value": "${TEST_EMAIL}", "primary": true }], + "active": false, + } patch_tests: - id: "${TEST_USER_ID}" @@ -169,6 +209,23 @@ users: "Operations": [{ "op": "replace", "path": "active", "value": false }], } + # Reactivate via PATCH using a path-less operation (whole-resource form). + - id: "${TEST_USER_ID}" + request: + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": + [{ "op": "replace", "value": { "active": true } }], + } + # Suspend again via PATCH with an explicit path so we end up suspended + # before the DELETE step. + - id: "${TEST_USER_ID}" + request: + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": + [{ "op": "replace", "path": "active", "value": false }], + } delete_tests: - id: "${TEST_USER_ID}" @@ -184,3 +241,35 @@ npx scimverify \ --auth-header "Bearer ${AUTH_TOKEN}" \ --config "${CONFIG_FILE}" \ --har-file "${OUTPUT_DIR}/output.har" + +# --- Step 4: Verify SCIM recreate-after-delete behavior --- +# Coder does not hard-delete users. POSTing the same SCIM user after a +# DELETE should reactivate the existing row (same ID), not return 409 or +# create a duplicate. The scimverify suite does not test this flow, so we +# verify it directly with curl. +if [[ -n "${TEST_USER_ID:-}" ]]; then + echo "" + echo "=== Recreate-after-delete check ===" + RECREATE_RESPONSE=$(curl -s -w '\n%{http_code}' -X POST \ + -H "${AUTH_HEADER}" \ + -H "Content-Type: application/scim+json" \ + "${BASE_URL}/Users" \ + -d "{ + \"schemas\": [\"urn:ietf:params:scim:schemas:core:2.0:User\"], + \"userName\": \"${TEST_USERNAME}\", + \"name\": {\"givenName\": \"Verify\", \"familyName\": \"Test\"}, + \"emails\": [{\"value\": \"${TEST_EMAIL}\", \"primary\": true}], + \"active\": true + }") + RECREATE_STATUS=$(echo "${RECREATE_RESPONSE}" | tail -n1) + RECREATE_BODY=$(echo "${RECREATE_RESPONSE}" | sed '$d') + RECREATE_ID=$(echo "${RECREATE_BODY}" | python3 -c "import sys,json; print(json.load(sys.stdin).get('id',''))" 2>/dev/null || true) + + if [[ "${RECREATE_STATUS}" == "201" || "${RECREATE_STATUS}" == "200" ]] && [[ "${RECREATE_ID}" == "${TEST_USER_ID}" ]]; then + echo "ok - POST after DELETE reactivated existing user (${TEST_USER_ID})" + else + echo "not ok - POST after DELETE should reactivate, got status=${RECREATE_STATUS} id=${RECREATE_ID} (expected ${TEST_USER_ID})" >&2 + echo " body: ${RECREATE_BODY}" >&2 + exit 1 + fi +fi