chore: implement SCIM PUT endpoint, protect against missing active

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