Skip to content

fix(scim): harden Phase 7 SCIM after security/pitfall/plan review #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
scarson merged 5 commits into from
Mar 20, 2026
Merged

fix(scim): harden Phase 7 SCIM after security/pitfall/plan review #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c24012b
fix(scim): harden Phase 7 SCIM after security/pitfall/plan review
scarson Mar 20, 2026
68afd8c
refactor(scim): remove redundant manual orgID checks
scarson Mar 20, 2026
d70048a
refactor(store): add orgID to GetGroupIfActive for defense-in-depth
scarson Mar 20, 2026
dc73545
docs: add MSRC adapter CSAF fix + Phase 10 completion plan
scarson Mar 20, 2026
09d9068
fix(store): add org_id constraint to SCIM-managed group member methods
scarson Mar 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
827 changes: 827 additions & 0 deletions dev/plans/2026-03-19-phase10-msrc-csaf-fix-plan.md
View file
Open in desktop

Large diffs are not rendered by default.

16 changes: 6 additions & 10 deletions internal/api/scim_admin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,13 +405,13 @@ func (srv *Server) patchSCIMGroupMappingHandler(w http.ResponseWriter, r *http.R
}

// Get the SCIM group and verify it belongs to this org.
scimGroup, err := srv.store.GetSCIMGroup(r.Context(), groupID)
scimGroup, err := srv.store.GetSCIMGroup(r.Context(), orgID, groupID)
if err != nil {
slog.ErrorContext(r.Context(), "scim group mapping patch: get group", "error", err)
writeProblem(w, http.StatusInternalServerError, "internal error")
return
}
if scimGroup == nil || scimGroup.OrgID != orgID {
if scimGroup == nil {
writeProblem(w, http.StatusNotFound, "SCIM group not found")
return
}
Expand Down Expand Up @@ -442,7 +442,7 @@ func (srv *Server) patchSCIMGroupMappingHandler(w http.ResponseWriter, r *http.R

// Validate mapped_group_id if provided: must be same org and active.
if groupIDSent {
group, err := srv.store.GetGroupIfActive(r.Context(), *req.MappedGroupID)
group, err := srv.store.GetGroupIfActive(r.Context(), orgID, *req.MappedGroupID)
if err != nil {
slog.ErrorContext(r.Context(), "scim group mapping patch: get notification group", "error", err)
writeProblem(w, http.StatusInternalServerError, "internal error")
Expand All @@ -452,10 +452,6 @@ func (srv *Server) patchSCIMGroupMappingHandler(w http.ResponseWriter, r *http.R
writeProblem(w, http.StatusBadRequest, "notification group not found or deleted")
return
}
if group.OrgID != orgID {
writeProblem(w, http.StatusBadRequest, "notification group belongs to a different organization")
return
}
}

// Capture old mapping for comparison.
Expand All @@ -478,7 +474,7 @@ func (srv *Server) patchSCIMGroupMappingHandler(w http.ResponseWriter, r *http.R
mappedGroupIDPtr = &scimGroup.MappedGroupID.UUID
}

if err := srv.store.UpdateSCIMGroupMapping(r.Context(), groupID, mappedRolePtr, mappedGroupIDPtr); err != nil {
if err := srv.store.UpdateSCIMGroupMapping(r.Context(), orgID, groupID, mappedRolePtr, mappedGroupIDPtr); err != nil {
slog.ErrorContext(r.Context(), "scim group mapping patch: update", "error", err)
writeProblem(w, http.StatusInternalServerError, "internal error")
return
Expand All @@ -497,7 +493,7 @@ func (srv *Server) patchSCIMGroupMappingHandler(w http.ResponseWriter, r *http.R
}

// Apply immediate effects to all current members.
members, err := srv.store.ListSCIMGroupMembers(r.Context(), groupID)
members, err := srv.store.ListSCIMGroupMembers(r.Context(), orgID, groupID)
if err != nil {
slog.ErrorContext(r.Context(), "scim group mapping patch: list members", "error", err)
writeProblem(w, http.StatusInternalServerError, "internal error")
Expand Down Expand Up @@ -545,7 +541,7 @@ func (srv *Server) patchSCIMGroupMappingHandler(w http.ResponseWriter, r *http.R
})

// Re-read the group to get updated state.
updated, err := srv.store.GetSCIMGroup(r.Context(), groupID)
updated, err := srv.store.GetSCIMGroup(r.Context(), orgID, groupID)
if err != nil || updated == nil {
slog.ErrorContext(r.Context(), "scim group mapping patch: re-read", "error", err)
writeProblem(w, http.StatusInternalServerError, "internal error")
Expand Down
2 changes: 1 addition & 1 deletion internal/api/scim_admin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,7 @@ func TestGroupMapping_ClearMapping(t *testing.T) {
t.Fatalf("create scim group: %v", err)
}
admin := "admin"
if err := env.db.UpdateSCIMGroupMapping(ctx, scimGroup.ID, &admin, nil); err != nil {
if err := env.db.UpdateSCIMGroupMapping(ctx, env.orgID, scimGroup.ID, &admin, nil); err != nil {
t.Fatalf("set initial mapping: %v", err)
}

Expand Down
2 changes: 1 addition & 1 deletion internal/api/scim_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ func TestSCIME2E_GroupRoleMapping(t *testing.T) {

// Set mapped_role="admin" via store (simulating admin action).
adminRole := "admin"
if err := env.db.UpdateSCIMGroupMapping(ctx, groupUUID, &adminRole, nil); err != nil {
if err := env.db.UpdateSCIMGroupMapping(ctx, env.orgID, groupUUID, &adminRole, nil); err != nil {
t.Fatalf("UpdateSCIMGroupMapping: %v", err)
}

Expand Down
75 changes: 38 additions & 37 deletions internal/api/scim_groups_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ func (srv *Server) scimCreateGroup(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
orgID := ctx.Value(ctxOrgID).(uuid.UUID)

slog.InfoContext(ctx, "scim create group", slog.String("org_id", orgID.String()))

var body scimGroupRequest
if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
writeSCIMError(w, http.StatusBadRequest, "invalidValue", "invalid JSON body")
Expand Down Expand Up @@ -86,7 +88,7 @@ func (srv *Server) scimCreateGroup(w http.ResponseWriter, r *http.Request) {
}

// Load members for response.
memberIDs, err := srv.store.ListSCIMGroupMembers(ctx, group.ID)
memberIDs, err := srv.store.ListSCIMGroupMembers(ctx, orgID, group.ID)
if err != nil {
slog.ErrorContext(ctx, "scim: list members after create", "error", err)
}
Expand All @@ -108,14 +110,18 @@ func (srv *Server) scimCreateGroup(w http.ResponseWriter, r *http.Request) {
// scimGetGroup handles GET /Groups/{id}.
func (srv *Server) scimGetGroup(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
orgID := ctx.Value(ctxOrgID).(uuid.UUID)

slog.InfoContext(ctx, "scim get group", slog.String("org_id", orgID.String()))

groupIDStr := chi.URLParam(r, "id")
groupID, err := uuid.Parse(groupIDStr)
if err != nil {
writeSCIMError(w, http.StatusBadRequest, "invalidValue", "invalid group id")
return
}

group, err := srv.store.GetSCIMGroup(ctx, groupID)
group, err := srv.store.GetSCIMGroup(ctx, orgID, groupID)
if err != nil {
slog.ErrorContext(ctx, "scim: get group", "error", err)
writeSCIMError(w, http.StatusInternalServerError, "", "internal error")
Expand All @@ -126,14 +132,7 @@ func (srv *Server) scimGetGroup(w http.ResponseWriter, r *http.Request) {
return
}

// Verify the group belongs to this org.
orgID := ctx.Value(ctxOrgID).(uuid.UUID)
if group.OrgID != orgID {
writeSCIMError(w, http.StatusNotFound, "", "group not found")
return
}

memberIDs, err := srv.store.ListSCIMGroupMembers(ctx, group.ID)
memberIDs, err := srv.store.ListSCIMGroupMembers(ctx, orgID, group.ID)
if err != nil {
slog.ErrorContext(ctx, "scim: list group members", "error", err)
writeSCIMError(w, http.StatusInternalServerError, "", "internal error")
Expand All @@ -149,6 +148,8 @@ func (srv *Server) scimListGroups(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
orgID := ctx.Value(ctxOrgID).(uuid.UUID)

slog.InfoContext(ctx, "scim list groups", slog.String("org_id", orgID.String()))

filterStr := r.URL.Query().Get("filter")
exprs, err := parseSCIMFilter(filterStr)
if err != nil {
Expand All @@ -167,7 +168,7 @@ func (srv *Server) scimListGroups(w http.ResponseWriter, r *http.Request) {
var filtered []any
for _, g := range groups {
if matchesSCIMGroupFilter(g.ID.String(), g.ExternalID.String, g.DisplayName, exprs) {
memberIDs, mErr := srv.store.ListSCIMGroupMembers(ctx, g.ID)
memberIDs, mErr := srv.store.ListSCIMGroupMembers(ctx, orgID, g.ID)
if mErr != nil {
slog.ErrorContext(ctx, "scim: list group members for list", "group_id", g.ID, "error", mErr)
continue
Expand Down Expand Up @@ -195,20 +196,22 @@ func (srv *Server) scimReplaceGroup(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
orgID := ctx.Value(ctxOrgID).(uuid.UUID)

slog.InfoContext(ctx, "scim replace group", slog.String("org_id", orgID.String()))

groupIDStr := chi.URLParam(r, "id")
groupID, err := uuid.Parse(groupIDStr)
if err != nil {
writeSCIMError(w, http.StatusBadRequest, "invalidValue", "invalid group id")
return
}

group, err := srv.store.GetSCIMGroup(ctx, groupID)
group, err := srv.store.GetSCIMGroup(ctx, orgID, groupID)
if err != nil {
slog.ErrorContext(ctx, "scim: get group for replace", "error", err)
writeSCIMError(w, http.StatusInternalServerError, "", "internal error")
return
}
if group == nil || group.OrgID != orgID {
if group == nil {
writeSCIMError(w, http.StatusNotFound, "", "group not found")
return
}
Expand All @@ -229,7 +232,7 @@ func (srv *Server) scimReplaceGroup(w http.ResponseWriter, r *http.Request) {
externalID = &body.ExternalID
}

if err := srv.store.UpdateSCIMGroup(ctx, groupID, body.DisplayName, externalID); err != nil {
if err := srv.store.UpdateSCIMGroup(ctx, orgID, groupID, body.DisplayName, externalID); err != nil {
if strings.Contains(err.Error(), "duplicate key") || strings.Contains(err.Error(), "unique constraint") {
writeSCIMError(w, http.StatusConflict, "uniqueness", "group displayName already exists in this organization")
return
Expand All @@ -252,7 +255,7 @@ func (srv *Server) scimReplaceGroup(w http.ResponseWriter, r *http.Request) {
}

// Diff members: current vs new.
currentMembers, err := srv.store.ListSCIMGroupMembers(ctx, groupID)
currentMembers, err := srv.store.ListSCIMGroupMembers(ctx, orgID, groupID)
if err != nil {
slog.ErrorContext(ctx, "scim: list members for diff", "error", err)
writeSCIMError(w, http.StatusInternalServerError, "", "internal error")
Expand All @@ -274,7 +277,7 @@ func (srv *Server) scimReplaceGroup(w http.ResponseWriter, r *http.Request) {
}

// Reload group to get the current mapped_role and mapped_group_id.
group, _ = srv.store.GetSCIMGroup(ctx, groupID)
group, _ = srv.store.GetSCIMGroup(ctx, orgID, groupID)

// Add new members.
for uid := range newMemberSet {
Expand All @@ -290,7 +293,7 @@ func (srv *Server) scimReplaceGroup(w http.ResponseWriter, r *http.Request) {
// Remove absent members.
for _, uid := range currentMembers {
if !newMemberSet[uid] {
if rmErr := srv.store.RemoveSCIMGroupMember(ctx, groupID, uid); rmErr != nil {
if rmErr := srv.store.RemoveSCIMGroupMember(ctx, orgID, groupID, uid); rmErr != nil {
slog.ErrorContext(ctx, "scim: remove member in replace", "user_id", uid, "error", rmErr)
continue
}
Expand All @@ -309,8 +312,8 @@ func (srv *Server) scimReplaceGroup(w http.ResponseWriter, r *http.Request) {
})

// Reload for response.
memberIDs, _ := srv.store.ListSCIMGroupMembers(ctx, groupID)
group, _ = srv.store.GetSCIMGroup(ctx, groupID)
memberIDs, _ := srv.store.ListSCIMGroupMembers(ctx, orgID, groupID)
group, _ = srv.store.GetSCIMGroup(ctx, orgID, groupID)
resp := srv.buildSCIMGroupResponse(r, group.ID.String(), group.ExternalID.String, group.DisplayName, group.CreatedAt.Format("2006-01-02T15:04:05Z"), group.UpdatedAt.Format("2006-01-02T15:04:05Z"), memberIDs)
writeSCIMJSON(w, http.StatusOK, resp)
}
Expand All @@ -320,20 +323,22 @@ func (srv *Server) scimPatchGroup(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
orgID := ctx.Value(ctxOrgID).(uuid.UUID)

slog.InfoContext(ctx, "scim patch group", slog.String("org_id", orgID.String()))

groupIDStr := chi.URLParam(r, "id")
groupID, err := uuid.Parse(groupIDStr)
if err != nil {
writeSCIMError(w, http.StatusBadRequest, "invalidValue", "invalid group id")
return
}

group, err := srv.store.GetSCIMGroup(ctx, groupID)
group, err := srv.store.GetSCIMGroup(ctx, orgID, groupID)
if err != nil {
slog.ErrorContext(ctx, "scim: get group for patch", "error", err)
writeSCIMError(w, http.StatusInternalServerError, "", "internal error")
return
}
if group == nil || group.OrgID != orgID {
if group == nil {
writeSCIMError(w, http.StatusNotFound, "", "group not found")
return
}
Expand Down Expand Up @@ -376,7 +381,7 @@ func (srv *Server) scimPatchGroup(w http.ResponseWriter, r *http.Request) {
case "remove":
userIDs := srv.extractRemoveTargets(op)
for _, userID := range userIDs {
if rmErr := srv.store.RemoveSCIMGroupMember(ctx, group.ID, userID); rmErr != nil {
if rmErr := srv.store.RemoveSCIMGroupMember(ctx, orgID, group.ID, userID); rmErr != nil {
slog.ErrorContext(ctx, "scim: patch remove member", "user_id", userID, "error", rmErr)
continue
}
Expand All @@ -394,7 +399,7 @@ func (srv *Server) scimPatchGroup(w http.ResponseWriter, r *http.Request) {
writeSCIMError(w, http.StatusBadRequest, "invalidValue", "displayName cannot be empty")
return
}
if updateErr := srv.store.UpdateSCIMGroup(ctx, group.ID, newName, nil); updateErr != nil {
if updateErr := srv.store.UpdateSCIMGroup(ctx, orgID, group.ID, newName, nil); updateErr != nil {
if strings.Contains(updateErr.Error(), "duplicate key") || strings.Contains(updateErr.Error(), "unique constraint") {
writeSCIMError(w, http.StatusConflict, "uniqueness", "group displayName already exists in this organization")
return
Expand Down Expand Up @@ -425,8 +430,8 @@ func (srv *Server) scimPatchGroup(w http.ResponseWriter, r *http.Request) {
})

// Reload for response.
group, _ = srv.store.GetSCIMGroup(ctx, groupID)
memberIDs, _ := srv.store.ListSCIMGroupMembers(ctx, groupID)
group, _ = srv.store.GetSCIMGroup(ctx, orgID, groupID)
memberIDs, _ := srv.store.ListSCIMGroupMembers(ctx, orgID, groupID)
resp := srv.buildSCIMGroupResponse(r, group.ID.String(), group.ExternalID.String, group.DisplayName, group.CreatedAt.Format("2006-01-02T15:04:05Z"), group.UpdatedAt.Format("2006-01-02T15:04:05Z"), memberIDs)
writeSCIMJSON(w, http.StatusOK, resp)
}
Expand All @@ -436,27 +441,29 @@ func (srv *Server) scimDeleteGroup(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
orgID := ctx.Value(ctxOrgID).(uuid.UUID)

slog.InfoContext(ctx, "scim delete group", slog.String("org_id", orgID.String()))

groupIDStr := chi.URLParam(r, "id")
groupID, err := uuid.Parse(groupIDStr)
if err != nil {
writeSCIMError(w, http.StatusBadRequest, "invalidValue", "invalid group id")
return
}

group, err := srv.store.GetSCIMGroup(ctx, groupID)
group, err := srv.store.GetSCIMGroup(ctx, orgID, groupID)
if err != nil {
slog.ErrorContext(ctx, "scim: get group for delete", "error", err)
writeSCIMError(w, http.StatusInternalServerError, "", "internal error")
return
}
if group == nil || group.OrgID != orgID {
if group == nil {
// Idempotent — already deleted returns 204.
w.WriteHeader(http.StatusNoContent)
return
}

// Collect affected non-exempt users before deletion.
memberIDs, err := srv.store.ListSCIMGroupMembers(ctx, groupID)
memberIDs, err := srv.store.ListSCIMGroupMembers(ctx, orgID, groupID)
if err != nil {
slog.ErrorContext(ctx, "scim: list members for delete", "error", err)
writeSCIMError(w, http.StatusInternalServerError, "", "internal error")
Expand All @@ -476,7 +483,7 @@ func (srv *Server) scimDeleteGroup(w http.ResponseWriter, r *http.Request) {
}

// Delete the group (CASCADE deletes scim_group_members).
if err := srv.store.DeleteSCIMGroup(ctx, groupID); err != nil {
if err := srv.store.DeleteSCIMGroup(ctx, orgID, groupID); err != nil {
slog.ErrorContext(ctx, "scim: delete group", "error", err)
writeSCIMError(w, http.StatusInternalServerError, "", "internal error")
return
Expand Down Expand Up @@ -628,6 +635,7 @@ func matchesSCIMGroupFilter(id, externalID, displayName string, exprs []SCIMFilt
return false
}
default:
slog.Warn("scim list groups: unsupported filter attribute", slog.String("attribute", expr.Attr)) //nolint:gosec // G706: slog structured field, not interpolated into log format string
return false
}
}
Expand Down Expand Up @@ -663,16 +671,9 @@ func (srv *Server) buildSCIMGroupResponse(r *http.Request, id, externalID, displ
// For a request to /api/v1/orgs/{org_id}/scim/v2/Groups/..., returns
// the URL up to and including /scim/v2.
func scimBaseURL(r *http.Request) string {
scheme := "https"
if r.TLS == nil {
scheme = "http"
}
if fwd := r.Header.Get("X-Forwarded-Proto"); fwd != "" {
scheme = fwd
}
path := r.URL.Path
if idx := strings.Index(path, "/scim/v2"); idx >= 0 {
path = path[:idx+len("/scim/v2")]
}
return fmt.Sprintf("%s://%s%s", scheme, r.Host, path)
return fmt.Sprintf("%s://%s%s", scimScheme(r), r.Host, path)
}
4 changes: 2 additions & 2 deletions internal/api/scim_groups_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,7 @@ func TestSCIMDeleteGroup_CascadesMembers(t *testing.T) {
assert.Equal(t, http.StatusNotFound, getResp.StatusCode)

// Verify members are gone.
memberIDs, err := env.db.ListSCIMGroupMembers(ctx, uuid.MustParse(created.ID))
memberIDs, err := env.db.ListSCIMGroupMembers(ctx, env.orgID, uuid.MustParse(created.ID))
require.NoError(t, err)
assert.Empty(t, memberIDs)
}
Expand All @@ -519,7 +519,7 @@ func TestSCIMDeleteGroup_RecomputesRoles(t *testing.T) {
group, err := env.db.CreateSCIMGroup(ctx, env.orgID, nil, "AdminGroup")
require.NoError(t, err)
adminRole := "admin"
require.NoError(t, env.db.UpdateSCIMGroupMapping(ctx, group.ID, &adminRole, nil))
require.NoError(t, env.db.UpdateSCIMGroupMapping(ctx, env.orgID, group.ID, &adminRole, nil))
require.NoError(t, env.db.AddSCIMGroupMember(ctx, group.ID, userID, env.orgID))

// Recompute role to set user to admin.
Expand Down
8 changes: 4 additions & 4 deletions internal/api/scim_notif_sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
// a member (manually or via SCIM), the existing membership is preserved via ON CONFLICT DO NOTHING.
func (srv *Server) syncNotifGroupAdd(ctx context.Context, orgID, userID, mappedGroupID, _ uuid.UUID) error {
// Verify the target notification group exists and is not soft-deleted.
group, err := srv.store.GetGroupIfActive(ctx, mappedGroupID)
group, err := srv.store.GetGroupIfActive(ctx, orgID, mappedGroupID)
if err != nil {
return err
}
Expand All @@ -27,15 +27,15 @@ func (srv *Server) syncNotifGroupAdd(ctx context.Context, orgID, userID, mappedG
// syncNotifGroupRemove removes a user from a notification group, but only if:
// - The membership is scim_managed=true (manual memberships are preserved)
// - No other SCIM group with the same mapped_group_id still includes the user
func (srv *Server) syncNotifGroupRemove(ctx context.Context, _, userID, mappedGroupID, scimGroupID uuid.UUID) error {
func (srv *Server) syncNotifGroupRemove(ctx context.Context, orgID, userID, mappedGroupID, scimGroupID uuid.UUID) error {
// Check if another SCIM group maps to the same notification group and includes this user.
count, err := srv.store.CountOtherSCIMGroupsWithSameMapping(ctx, userID, mappedGroupID, scimGroupID)
count, err := srv.store.CountOtherSCIMGroupsWithSameMapping(ctx, orgID, userID, mappedGroupID, scimGroupID)
if err != nil {
return err
}
if count > 0 {
return nil // another SCIM group still maps here — keep the membership
}

return srv.store.RemoveSCIMManagedGroupMember(ctx, mappedGroupID, userID)
return srv.store.RemoveSCIMManagedGroupMember(ctx, mappedGroupID, userID, orgID)
}
Loading