Security Hardening: Catalog Registration Scope Enforcement

[Lang-Akshay]

Security harden catalog server registration to enforce caller-owned scope and least-privilege defaults across all registration paths. All changes are backend-only; no UI files are modified.

What changed

Catalog registration scope

• Registrations default to visibility="private" with owner_email derived from the authenticated caller — no forced-public, ownerless gateways
• Token scope validated before gateway creation; foreign teams and public-only token misuse rejected with 403
• ALLOW_PUBLIC_VISIBILITY=false now enforced on the catalog path (_resolve_registration_scope); previously only admin UI handlers checked this flag
• Two serial team-membership DB queries in _resolve_registration_scope replaced with a single JOIN, matching TeamManagementService semantics and eliminating a redundant round-trip
• Bulk registration validates scope once upfront and passes the resolved (visibility, team_id) tuple into each per-server call via _resolved_scope, removing N redundant DB queries from the loop

Gateway ownership transfer

• New POST /admin/gateways/{id}/transfer-ownership endpoint (admin-only) validates target user status and team membership, propagates ownership to linked tools/resources/prompts, and writes an audit trail
• Transferring a private gateway to a team coerces visibility to "team" on the gateway and all linked entities — without this, team members could not access the gateway after transfer

Safe user deletion

• Before permanent deletion, all owned gateways are transferred atomically: deletion is refused if any transfer cannot be completed
• Fallback owner selection now joins EmailUser to exclude deactivated accounts (previously only EmailTeamMember.is_active was checked)
• Orphan-owner ValueError surfaces as HTTP 409 with the actionable message on DELETE /admin/users/{email}; previously swallowed into a generic 500

Tests added

• test_gateway_service_transfer.py — full service-layer transfer coverage (target not found, not a member, success, linked entity propagation, private→team visibility coercion)
• Deny-path regression tests: public-only token, foreign team, ALLOW_PUBLIC_VISIBILITY=false, non-admin on transfer endpoint
• Orphan 409 regression test on the API router path

No schema or migration changes

No new columns or tables; no Alembic migration required.

Internal reference: https://github.ibm.com/contextforge-org/internal_issues/issues/497

[msureshkumar88]

Thanks for tackling this — the shift to private-by-default with explicit ownership tracking is a solid direction, and the ownership-transfer-on-deletion flow with the atomic refuse-if-no-owner fallback is a genuinely good safety net. Found a few gaps that are worth addressing before merge, plus some smaller notes.

Blocking

1. _resolve_registration_scope() doesn't check allow_public_visibility (mcpgateway/services/catalog_service.py:62)
Every other gateway-creation path in admin.py calls _check_public_visibility_allowed() before allowing visibility="public". The new catalog registration path never references allow_public_visibility at all, so with ALLOW_PUBLIC_VISIBILITY=false, registering a catalog server with visibility="public" still succeeds. Since this PR's whole purpose is scope enforcement, this specific control should be wired in here too (CWE-284/CWE-863).

2. transfer_gateway_ownership() updates team_id but not visibility (mcpgateway/services/gateway_service.py:5589)
Transferring a public/private gateway to a team sets gateway.team_id but leaves visibility unchanged. The actual access-scoping query (gateway_service.py:2538-2556) only grants team access when visibility is "team"/"public" — it doesn't derive scope from team_id alone. So the transfer silently doesn't do what it says: the gateway's effective access scope is unchanged, no error is raised, and the existing test (test_gateway_service_transfer.py:1680-1729) only asserts team_id changed, not visibility, so this ships uncaught. (Confirmed this isn't a privilege-escalation path — private gateways stay gated by owner_email regardless — but it does defeat the endpoint's stated purpose.)

3. Deactivated users can inherit gateway ownership (mcpgateway/services/email_auth_service.py:2109)
The alternate-owner lookup in delete_user() filters on EmailTeamMember.is_active only, never EmailUser.is_active. Since deactivating a user (update_user(is_active=False)) doesn't touch their EmailTeamMember rows, a disabled account can still be picked as the new owner of a deleted teammate's gateway — which undercuts the PR's goal of preventing unmanageable/orphaned gateways, since nobody can log into a disabled account. The explicit transfer_gateway_ownership() endpoint already does check is_active for the target — worth aligning the two.

4. Orphan-owner error gets swallowed into a generic 500 (mcpgateway/routers/email_auth.py:913)
The new ValueError("...would become orphaned and no fallback owner is available") is a good, actionable error, but the router's except Exception catches it and returns a generic "Failed to delete user" 500 — the admin never sees the real reason (configure platform_admin_email or reassign manually). The admin-UI equivalent (admin.py ~8669-8671) does surface str(e); worth matching that on the API path.

Suggestions

• _resolve_registration_scope() reimplements team-membership validation instead of reusing TeamManagementService.get_user_role()/verify_team_for_user(), which admin_add_gateway already uses for the same check. Consolidating would prevent the two paths from drifting apart (this PR is itself an example of that drift — see #1).
• bulk_register_servers() validates the batch's scope once up front, then register_catalog_server() re-runs the identical validation (2 queries + a commit) per item in the loop — for a 50-item batch that's 50 redundant round-trips for validation already proven valid.
• transfer_gateway_ownership() runs two near-identical team-membership queries when visibility=="team" and an explicit target_team_id is passed — the second appears to re-check what the first already validated.
• No deny-path test for a non-admin calling POST /admin/gateways/{id}/transfer-ownership — worth adding given this endpoint can reassign any gateway system-wide.
• Visibility/team_id extraction logic in the registration modal is duplicated between modals.js (submitApiKeyForm) and the inline script in mcp_registry_partial.html — could drift if only one gets updated later.

Minor

• The PR references only an internal github.ibm.com tracker issue; no public issue / Closes #NNN link, which makes this harder to trace on the public repo.

No Alembic migration is touched, which looks correct — no schema change is actually needed for this fix. Scope of touched files otherwise looks tight and on-topic.

[Lang-Akshay]

Thanks for the thorough review @msureshkumar88 — all blocking findings and suggestions have been addressed in the latest commit.

Blocking

1. _resolve_registration_scope() missing allow_public_visibility check — Fixed ✅. Added the guard immediately after visibility is resolved: visibility="public" with a team_id now raises CatalogRegistrationPermissionError when ALLOW_PUBLIC_VISIBILITY=false, matching every other creation path in admin.py. Test added: test_resolve_scope_blocks_public_when_flag_disabled.

2. transfer_gateway_ownership() leaves visibility unchanged — Fixed ✅. When target_team_id is supplied and the gateway (or any linked tool/resource/prompt) has visibility="private", it is coerced to "team" so the team can actually see it after transfer. Test added: test_transfer_private_gateway_to_team_coerces_visibility.

3. Deactivated users can inherit gateway ownership — Fixed ✅. The alternate-owner query now JOINs EmailUser and filters EmailUser.is_active == True alongside the existing EmailTeamMember.is_active check. Applied to both the gateway fallback path and the team-ownership transfer path in delete_user().

4. Orphan-owner error swallowed into 500 — Fixed ✅. Added a dedicated except ValueError branch before the generic except Exception in DELETE /admin/users/{email}, returning HTTP 409 with str(e) as the detail. The admin now sees the actionable message ("configure platform_admin_email or reassign manually"). Test added: test_delete_user_orphan_value_error_returns_409.

Suggestions

• Duplicate team-membership validation — Consolidated. The two serial raw queries in _resolve_registration_scope (team existence + membership) are replaced by a single JOIN, mirroring the pattern used by TeamManagementService.verify_team_for_user(). Making the method async to call it directly would cascade through two callers; the JOIN achieves the same consolidation without that change.

• Redundant per-item scope validation in bulk — Fixed. _resolve_registration_scope runs once in bulk_register_servers and the resolved (visibility, team_id) tuple is forwarded via a new _resolved_scope kwarg on register_catalog_server, skipping re-validation for every item in the batch.

• Two near-identical team-membership queries in transfer_gateway_ownership() — The second query (lines 5576–5584) covers the case where visibility != "team" but an explicit target_team_id is provided, which the first query does not evaluate. They guard different conditions so the duplication is load-bearing; left as-is to avoid silently dropping the cross-team membership check.

• No deny-path test for non-admin on the transfer endpoint — Added: test_transfer_gateway_ownership_non_admin_denied asserts HTTP 403 when check_admin_permission returns False.

• submitApiKeyForm / mcp_registry_partial.html duplication — The UI changes in this PR have been reverted; this is now out of scope for this branch.

[gandhipratik203]

I had overlapping comments to Suresh. LGTM once the blocking ones are resolved!