[FIX] Scope user lookups to current env's OIDC connection via plugin-driven filter#1982
Open
jaseemjaskp wants to merge 6 commits into
Open
[FIX] Scope user lookups to current env's OIDC connection via plugin-driven filter#1982jaseemjaskp wants to merge 6 commits into
jaseemjaskp wants to merge 6 commits into
Conversation
…driven filter When one Auth0 organization is wired to multiple OIDC connections (one per environment, e.g. "moodys-qa" + "moodys"), the same email yields distinct user_ids per connection. Unstract creates a separate User row for each, which surfaced as duplicate rows in Manage Users and broke admin access because role promotion landed on a different User row than the one resolved during login. Add an opt-in queryset filter registry in core that plugins can populate at startup. The auth0 plugin registers a filter scoping queries to user_ids prefixed with "oidc|<AUTH0_OIDC_CONNECTION>|" when the setting is defined; unset/empty leaves behavior unchanged. Also harden add_user_role and remove_user_role to raise a clear 409 on multi-match instead of a silent 500. OSS and cloud are unaffected: the plugin/setting only exist in on-prem and the filter no-ops when AUTH0_OIDC_CONNECTION is empty.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a pluggable UserFilterRegistry, AmbiguousUserException (HTTP 409), a uniqueness resolver for filtered user queries that raises on multiple matches, applies registry filters to User and OrganizationMember lookups, and updates AuthenticationController role methods and error handling. ChangesUser Filter Registry System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/account_v2/authentication_controller.py (2)
455-471: 💤 Low valueHoist the late imports to module scope.
UserFilterRegistryandAmbiguousUserExceptionare siblingaccount_v2.*modules with no circular-import risk (the file already imports fromaccount_v2.custom_exceptionsand otheraccount_v2.*modules at the top). Inline imports add per-call overhead and obscure dependencies.♻️ Proposed diff
@@ from account_v2.custom_exceptions import ( + AmbiguousUserException, DuplicateData, Forbidden, MethodNotImplemented, UserNotExistError, ) @@ from account_v2.user import UserService +from account_v2.user_filter_registry import UserFilterRegistry @@ `@staticmethod` def _resolve_unique_member_by_email(email: str) -> OrganizationMember | None: """Resolve a single OrganizationMember by email after registered filters. ... """ - from account_v2.user_filter_registry import UserFilterRegistry - qs = UserFilterRegistry.apply( OrganizationMember.objects.filter(user__email=email), "org_member", ) members = list(qs[:2]) if len(members) > 1: logger.error( "Ambiguous OrganizationMember lookup for email=%s " "(matched %d rows after IdP filter)", email, len(members), ) - from account_v2.custom_exceptions import AmbiguousUserException - raise AmbiguousUserException() return members[0] if members else None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/account_v2/authentication_controller.py` around lines 455 - 471, Move the two inline imports to module scope: import UserFilterRegistry from account_v2.user_filter_registry and AmbiguousUserException from account_v2.custom_exceptions at the top of the file so they are not imported inside the function block where OrganizationMember lookup occurs; update the existing top-of-file imports to include UserFilterRegistry and AmbiguousUserException and remove the in-function import statements around the Ambiguous OrganizationMember lookup that currently reference UserFilterRegistry.apply and raise AmbiguousUserException.
445-472: ⚡ Quick winConsider moving the unique-resolution logic into
OrganizationMemberService.The queryset built here is identical to
OrganizationMemberService.get_user_by_email(sameuser__email=emailfilter, sameUserFilterRegistry.apply(..., "org_member")); only the uniqueness assertion differs. Keeping member-lookup construction in the service keeps the registry plumbing and filter-kind tag in one place and avoids drift if the base filter ever needs to change (e.g., excluding service accounts, soft-deletes, etc.).Suggested shape — a sibling service method that the controller delegates to:
# in OrganizationMemberService `@staticmethod` def get_unique_user_by_email(email: str) -> OrganizationMember | None: qs = UserFilterRegistry.apply( OrganizationMember.objects.filter(user__email=email), "org_member", ) members = list(qs[:2]) if len(members) > 1: raise AmbiguousUserException() return members[0] if members else NoneThen
_resolve_unique_member_by_emailbecomes a thin wrapper that handles logging, oradd_user_role/remove_user_rolecall the service directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/account_v2/authentication_controller.py` around lines 445 - 472, Extract the queryset + uniqueness check into OrganizationMemberService as a new static method (e.g., get_unique_user_by_email) that uses UserFilterRegistry.apply(OrganizationMember.objects.filter(user__email=email), "org_member"), materializes at most 2 rows, and raises AmbiguousUserException when >1 else returns the single member or None; then change _resolve_unique_member_by_email to call OrganizationMemberService.get_unique_user_by_email and keep only the logging/exception-handling (catch AmbiguousUserException to log the ambiguous lookup with the email and re-raise) so the registry/filter plumbing lives in the service and the controller remains a thin wrapper.backend/account_v2/user_filter_registry.py (1)
28-40: 💤 Low valueAnnotate
_filtersasClassVarand consider exposing a reset hook for tests.Ruff flags RUF012 because
_filters: list[FilterFn] = []reads as a mutable default for an instance attribute. The registry is intentionally process-global state, so annotating itClassVarboth signals intent and silences the lint. Also consider a smallclear()(or test-only fixture) since tests/CI that need to isolate registrations currently have to reach into_filtersdirectly.♻️ Proposed diff
from collections.abc import Callable -from typing import Literal +from typing import ClassVar, Literal from django.db.models import QuerySet @@ class UserFilterRegistry: - _filters: list[FilterFn] = [] + _filters: ClassVar[list[FilterFn]] = [] `@classmethod` def register(cls, fn: FilterFn) -> None: if fn not in cls._filters: cls._filters.append(fn) `@classmethod` def apply(cls, qs: QuerySet, kind: FilterKind) -> QuerySet: for fn in cls._filters: qs = fn(qs, kind) return qs + + `@classmethod` + def clear(cls) -> None: + """Test-only helper to reset the registry between cases.""" + cls._filters.clear()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/account_v2/user_filter_registry.py` around lines 28 - 40, Annotate the registry's mutable class attribute and add a test hook: change the type of _filters on UserFilterRegistry to be a ClassVar[list[FilterFn]] (import ClassVar from typing) to indicate process-global state and silence RUF012, and add a classmethod clear() that empties _filters so tests can reset registrations instead of reaching into _filters directly; ensure register and apply continue to operate on cls._filters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/account_v2/authentication_controller.py`:
- Around line 455-471: Move the two inline imports to module scope: import
UserFilterRegistry from account_v2.user_filter_registry and
AmbiguousUserException from account_v2.custom_exceptions at the top of the file
so they are not imported inside the function block where OrganizationMember
lookup occurs; update the existing top-of-file imports to include
UserFilterRegistry and AmbiguousUserException and remove the in-function import
statements around the Ambiguous OrganizationMember lookup that currently
reference UserFilterRegistry.apply and raise AmbiguousUserException.
- Around line 445-472: Extract the queryset + uniqueness check into
OrganizationMemberService as a new static method (e.g.,
get_unique_user_by_email) that uses
UserFilterRegistry.apply(OrganizationMember.objects.filter(user__email=email),
"org_member"), materializes at most 2 rows, and raises AmbiguousUserException
when >1 else returns the single member or None; then change
_resolve_unique_member_by_email to call
OrganizationMemberService.get_unique_user_by_email and keep only the
logging/exception-handling (catch AmbiguousUserException to log the ambiguous
lookup with the email and re-raise) so the registry/filter plumbing lives in the
service and the controller remains a thin wrapper.
In `@backend/account_v2/user_filter_registry.py`:
- Around line 28-40: Annotate the registry's mutable class attribute and add a
test hook: change the type of _filters on UserFilterRegistry to be a
ClassVar[list[FilterFn]] (import ClassVar from typing) to indicate
process-global state and silence RUF012, and add a classmethod clear() that
empties _filters so tests can reset registrations instead of reaching into
_filters directly; ensure register and apply continue to operate on
cls._filters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dea7a709-7adb-4c74-a535-fa2ff2912ef1
📒 Files selected for processing (5)
backend/account_v2/authentication_controller.pybackend/account_v2/custom_exceptions.pybackend/account_v2/user.pybackend/account_v2/user_filter_registry.pybackend/tenant_account_v2/organization_member_service.py
Contributor
|
| Filename | Overview |
|---|---|
| backend/account_v2/user_filter_registry.py | New pluggable registry; clean implementation with deduplication, fail-closed exception propagation, and a test-only clear() escape hatch. |
| backend/account_v2/user.py | Lookup methods route through UserFilterRegistry; PK lookup correctly bypasses filter; _resolve_unique surfaces ambiguity as 409 instead of silent 500. |
| backend/tenant_account_v2/organization_member_service.py | All user lookups scoped via registry; get_user_by_id correctly bypasses filter; get_user_by_email raises on ambiguity; get_user_by_user_id intentionally uses .first() since user_id is unique per connection. |
| backend/account_v2/authentication_controller.py | Fixes pre-existing IndexError from misindented return current_roles[0]; adds except APIException: raise to surface 409s from ambiguous lookups instead of redirecting to /error. |
| backend/account_v2/custom_exceptions.py | New AmbiguousUserException (409) with a descriptive default message; straightforward addition. |
| backend/account_v2/tests/test_user_filter_registry.py | Comprehensive unit tests covering identity, registration order, deduplication, unregister, clear, chaining, and fail-closed error propagation; no DB dependency. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["View / API handler\ncalls lookup method"] --> B{"Which method?"}
B -->|"get_user_by_email\nget_user_by_user_id"| C["Build base QuerySet\n(User.objects.filter / OrgMember.objects.filter)"]
B -->|"get_user_by_id\n(PK lookup)"| G["User.objects.get / OrgMember.objects.get\n(bypasses registry — PK already unique)"]
C --> D["UserFilterRegistry.apply(qs, kind)"]
D --> E{"Filters registered?"}
E -->|"No (OSS / cloud / unset AUTH0_OIDC_CONNECTION)"| F["Identity — qs unchanged"]
E -->|"Yes (auth0 plugin, AUTH0_OIDC_CONNECTION set)"| H["Filter: user_id__startswith=oidc|conn|"]
F --> I["_resolve_unique: list(qs[:2])"]
H --> I
I --> J{"rows count?"}
J -->|"0"| K["return None"]
J -->|"1"| L["return rows[0]"]
J -->|">1"| M["Log PKs (up to 50)\nraise AmbiguousUserException (409)"]
G --> N{"DoesNotExist?"}
N -->|"Yes"| K
N -->|"No"| L
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
backend/account_v2/user.py:89-96
The ambiguity log fires two separate DB queries against the same queryset: `qs[:2]` to detect duplicates, then `qs.values_list("pk", flat=True)[:50]` to gather PKs for the log. The first query already has both row objects in memory (and their PKs via `.pk`), so the second round-trip is redundant for the common two-row case. Seeding the PK list from `rows` and only re-querying for additional PKs avoids this extra hit on every ambiguity event.
```suggestion
rows = list(qs[:2])
if len(rows) > 1:
# Seed PKs from the already-fetched rows; extend from DB only when
# AMBIGUITY_LOG_LIMIT allows more entries than we already have.
known_pks = [r.pk for r in rows]
extra_pks = list(
qs.values_list("pk", flat=True)[len(known_pks) : AMBIGUITY_LOG_LIMIT]
)
pks = known_pks + extra_pks
```
Reviews (5): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jaseemjaskp
commented
May 21, 2026
Contributor
Author
jaseemjaskp
left a comment
jaseemjaskp
left a comment
There was a problem hiding this comment.
PR Review Toolkit — automated multi-agent review
Ran Comment Analyzer, PR Test Analyzer, Silent Failure Hunter, Type Design Analyzer, Code Reviewer, and Code Simplifier on commit 98c95bf8. Existing greptile comments were excluded.
Headline concerns (P1)
- Caller-side regression risk on the admin path —
OrganizationMemberService.get_user_by_id(admin.id)atauthentication_controller.py:412/430can now silently returnNoneif the IdP filter is misconfigured for the admin's own row, and thatNoneflows intoauth_service.add_organization_user_role(...). See inline atauthentication_controller.py:413. - Silent multi-match on UserService lookups —
get_user_by_user_id/get_user_by_emailpreviously raisedMultipleObjectsReturned; now they.first()and silently pick a row. This is the same failure mode the PR is fixing at the controller layer — recommend mirroring_resolve_unique_member_by_emailhere. (Complementary to greptile's existing P1 on the inverse silently-hides case atuser.py:64.) - Latent
IndexErrorinadd_user_role/remove_user_role:return current_roles[0]is unconditional butif current_roles:only guards the save call (pre-existing but adjacent to changed lines).
Tests (no inline anchor)
No tests added. backend/account_v2/tests.py and backend/tenant_account_v2/tests.py are empty placeholders. Highest-value tests to add before merge:
_resolve_unique_member_by_email— 0/1/2+ matches (assertAmbiguousUserExceptionon 2+).UserFilterRegistry.apply— no-op when empty, composition order, dedup viaregister. Important:setUp/tearDownmust clear_filtersto avoid cross-test contamination (this is the same root cause greptile flagged atuser_filter_registry.py:34).UserService.get_user_by_email/user_id/id— confirmNone-on-no-match parity now that.get()was replaced with.first().- End-to-end:
add_user_role/remove_user_roleraise 409 to the view on ambiguity.
P2 findings
See inline — type drift on get_members() return annotation; docstring/exception message both leak the plugin-specific AUTH0_OIDC_CONNECTION env var into core (defeats the registry's plugin-agnostic intent); registry has no exception isolation for buggy plugins; FilterKind is a static-only invariant with no runtime cross-check against qs.model; repeated 4-line apply block across 6 methods.
…stry hardening Bug fixes: - get_user_by_id (both UserService and OrganizationMemberService) bypasses the filter registry. PK lookups are already unique; filtering them risks silently returning None for the executing admin's own row in role-change flows, which would propagate as a downstream crash with no breadcrumb. - UserService.get_user_by_email / get_user_by_user_id no longer silently collapse multi-match results via .first(). They now raise AmbiguousUserException on >1 match, mirroring the OrganizationMemberService behavior. - add_user_role / remove_user_role: return current_roles[0] only when current_roles is non-empty (previously crashed on empty list). Refactors: - Move the ambiguous-resolve helper from authentication_controller into OrganizationMemberService.get_unique_user_by_email, removing inline imports and putting filter plumbing in one place. - Log message reports qs.count() instead of the qs[:2]-capped len, so operators see the real duplicate count. Polish: - UserFilterRegistry: ClassVar annotation, clear()/unregister() helpers for test isolation and plugin shutdown, exception isolation in apply() with plugin attribution logging. - AmbiguousUserException default_detail no longer leaks "IdP filter" to end users; technical diagnostic stays in the logger. - UserFilterRegistry docstring is identity-plugin-generic; no auth0 / env-var names in core. - Fix get_members / get_members_by_role return-type annotations from list[OrganizationMember] to QuerySet[OrganizationMember].
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tenant_account_v2/organization_member_service.py (1)
16-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently collapse duplicate matches with
.first().
get_user_by_email()andget_user_by_user_id()still pick an arbitraryOrganizationMemberafter filters. That means callers likeinvite_user()andremove_user_by_user_id()can still act on whichever row the database returns instead of surfacing the new 409 ambiguity path. These lookups should use the same two-row uniqueness resolution asget_unique_user_by_email().Also applies to: 49-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tenant_account_v2/organization_member_service.py` around lines 16 - 21, Replace the silent .first() behavior in get_user_by_email and get_user_by_user_id with the same two-row uniqueness resolution used by get_unique_user_by_email: query for up to two matching OrganizationMember rows (e.g., qs[:2] or qs.limit(2)), convert to a list, then if len==0 return None, if len==1 return that OrganizationMember, and if len>1 raise or surface the same ambiguity/duplicate error path (the same exception or 409-mapped error used by get_unique_user_by_email) so callers like invite_user() and remove_user_by_user_id() no longer act on an arbitrary row.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/account_v2/user.py`:
- Around line 86-95: The error currently logs raw lookup values (value) in the
Ambiguous User branch; change the Logger.error call in the block that checks if
len(rows) > 1 to redact or hash the lookup value before logging (use the
existing lookup variable: field, value) so the message still includes field and
total match count but not PII; update the code that prepares the log argument
(used by Logger.error) to replace value with a masked string (e.g., redact
partially) or a stable hash (e.g., sha256 hex) and leave the raise
AmbiguousUserException() behavior unchanged.
In `@backend/tenant_account_v2/organization_member_service.py`:
- Around line 37-45: The ambiguity log currently emits the raw email (in the
OrganizationMember lookup branch where rows > 1) which exposes PII; change the
logger.error call in that block (the check that raises AmbiguousUserException)
to avoid printing the plain email—either log only the total count/context or a
non-reversible hash of the email (e.g., SHA-256) if you need a correlate-able
value; update the message and parameters used in logger.error (the code around
rows, qs.count(), and the AmbiguousUserException raise) accordingly so the email
is not written to logs.
---
Outside diff comments:
In `@backend/tenant_account_v2/organization_member_service.py`:
- Around line 16-21: Replace the silent .first() behavior in get_user_by_email
and get_user_by_user_id with the same two-row uniqueness resolution used by
get_unique_user_by_email: query for up to two matching OrganizationMember rows
(e.g., qs[:2] or qs.limit(2)), convert to a list, then if len==0 return None, if
len==1 return that OrganizationMember, and if len>1 raise or surface the same
ambiguity/duplicate error path (the same exception or 409-mapped error used by
get_unique_user_by_email) so callers like invite_user() and
remove_user_by_user_id() no longer act on an arbitrary row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c847fb5a-395a-4815-8262-16ad9116a22b
📒 Files selected for processing (5)
backend/account_v2/authentication_controller.pybackend/account_v2/custom_exceptions.pybackend/account_v2/user.pybackend/account_v2/user_filter_registry.pybackend/tenant_account_v2/organization_member_service.py
SonarCloud python:S8572 — inside an except block, logger.exception attaches
the traceback automatically. The old logger.error(f"...: {ex}") logged only
the str(ex) with no stack, hiding the call site of upstream failures.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/account_v2/authentication_controller.py (1)
412-417:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
admin_userbefore role mutations to avoid hidden-filter 500s.
OrganizationMemberService.get_user_by_id(id=admin.id)can returnNoneunder active scoping, and passing that into role mutation calls can still crash downstream instead of returning a controlled API error.Suggested fix
def add_user_role( self, request: Request, org_id: str, email: str, role: str ) -> str | None: admin: User = request.user admin_user = OrganizationMemberService.get_user_by_id(id=admin.id) + if admin_user is None: + logger.error("Admin organization membership not visible for user id=%s", admin.id) + raise Forbidden() user = OrganizationMemberService.get_unique_user_by_email(email) if user: current_roles = self.auth_service.add_organization_user_role( admin_user, org_id, user.user.user_id, [role], request ) @@ def remove_user_role( self, request: Request, org_id: str, email: str, role: str ) -> str | None: admin: User = request.user admin_user = OrganizationMemberService.get_user_by_id(id=admin.id) + if admin_user is None: + logger.error("Admin organization membership not visible for user id=%s", admin.id) + raise Forbidden() organization_member = OrganizationMemberService.get_unique_user_by_email(email) if organization_member: current_roles = self.auth_service.remove_organization_user_role( admin_user, org_id, organization_member.user.user_id, [role], request )Also applies to: 431-436
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/account_v2/authentication_controller.py` around lines 412 - 417, OrganizationMemberService.get_user_by_id(id=admin.id) can return None under scoping, so guard admin_user before calling role mutation methods (e.g., self.auth_service.add_organization_user_role and the similar calls around lines 431-436); if admin_user is None, return or raise the same controlled API error used elsewhere (e.g., "admin user not found / forbidden") instead of passing None into auth_service methods. Locate the calls to OrganizationMemberService.get_user_by_id and wrap them with a check that returns a proper HTTP error or raises the existing permission/NotFound error when admin_user is falsy, ensuring downstream methods never receive None.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@backend/account_v2/authentication_controller.py`:
- Around line 412-417: OrganizationMemberService.get_user_by_id(id=admin.id) can
return None under scoping, so guard admin_user before calling role mutation
methods (e.g., self.auth_service.add_organization_user_role and the similar
calls around lines 431-436); if admin_user is None, return or raise the same
controlled API error used elsewhere (e.g., "admin user not found / forbidden")
instead of passing None into auth_service methods. Locate the calls to
OrganizationMemberService.get_user_by_id and wrap them with a check that returns
a proper HTTP error or raises the existing permission/NotFound error when
admin_user is falsy, ensuring downstream methods never receive None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd8ae189-4ebe-4ed0-897e-d3b5b32d5d8b
📒 Files selected for processing (1)
backend/account_v2/authentication_controller.py
CodeRabbit follow-up: the ambiguous-lookup error logs in UserService and OrganizationMemberService wrote raw email / user_id values into application logs. Since ambiguity is hit from routine admin flows, this expanded PII retention for no incremental diagnostic gain. Drop the raw lookup value from the message and log the matched row PKs (User.pk and OrganizationMember.member_id) instead — internal database identifiers, not PII — so an operator can still query the offending rows without storing the personal data.
jaseemjaskp
commented
May 21, 2026
Contributor
Author
jaseemjaskp
left a comment
jaseemjaskp
left a comment
There was a problem hiding this comment.
Follow-up review after the ambiguity-log PII fix. Five findings below; one additional silent-failure-pattern outside the diff is noted in the chat summary (auth_controller.py:119 user_organizations swallows the new AmbiguousUserException into an empty 412).
…arrow catch, add tests
P0 — bounded re-query on the ambiguity log path:
- list(qs.values_list("pk", flat=True)) was re-executing the unsliced
queryset, so a misconfigured filter matching thousands of rows turned
the error path into a full table scan plus PK materialization. Cap to
AMBIGUITY_LOG_LIMIT (50) and label the log "≥N rows; first 50 pks=...".
Same fix applied to the OrganizationMember mirror.
P1 — collapse the dual get_user_by_email API:
- Two siblings with different semantics (raise vs silent .first()) is a
contract trap; invite_user at authentication_controller.py:352 was still
hitting the silent variant, so a misconfigured filter could short-circuit
invitations with a misleading "User already in org" response. Make
OrganizationMemberService.get_user_by_email itself raise on multi-match;
delete get_unique_user_by_email; update add_user_role / remove_user_role
call sites.
P1 — narrow except in authorization_callback:
- The broad `except Exception` swallowed AmbiguousUserException (and every
other DRF APIException) into a generic /error redirect, hiding the 409
detail from the admin during OIDC sign-in. Catch APIException first and
re-raise so DRF surfaces the actionable detail.
P1 — regression tests for the registry plumbing:
- Add backend/account_v2/tests/test_user_filter_registry.py covering
apply-as-identity-with-no-filters, register dedupe, unregister/clear,
filter-order, queryset threading, and exception isolation with plugin
attribution. Replaces the empty backend/account_v2/tests.py placeholder
with a proper tests/ package. Resolver tests deferred (need Django-stub
scaffolding similar to usage_v2/tests/test_helper.py).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/tenant_account_v2/organization_member_service.py (1)
49-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
get_user_by_user_idfail closed on duplicate matches.Line 54 still resolves ambiguity with
.first(). That undercuts the ambiguity contract introduced elsewhere in this PR and letsremove_user_by_user_id()act on an arbitrary member if multiple scoped rows survive filtering.Suggested fix
`@staticmethod` def get_user_by_user_id(user_id: str) -> OrganizationMember | None: qs = UserFilterRegistry.apply( OrganizationMember.objects.filter(user__user_id=user_id), # type: ignore "org_member", ) - return qs.first() + rows = list(qs[:2]) + if len(rows) > 1: + member_ids = list( + qs.values_list("member_id", flat=True)[:AMBIGUITY_LOG_LIMIT] + ) + logger.error( + "Ambiguous OrganizationMember lookup by user_id " + "(matched ≥%d rows; first %d member_ids=%s)", + len(member_ids), + len(member_ids), + member_ids, + ) + raise AmbiguousUserException() + return rows[0] if rows else None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tenant_account_v2/organization_member_service.py` around lines 49 - 54, get_user_by_user_id currently uses qs.first() which silently picks an arbitrary match; change it to fail closed by checking the result count after applying UserFilterRegistry to OrganizationMember.objects.filter(user__user_id=user_id) and raise an explicit error (e.g., ValueError or a custom AmbiguousMatchError) if more than one row is returned, return the single object when exactly one match exists, and return None when zero matches exist; update callers such as remove_user_by_user_id to expect this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@backend/tenant_account_v2/organization_member_service.py`:
- Around line 49-54: get_user_by_user_id currently uses qs.first() which
silently picks an arbitrary match; change it to fail closed by checking the
result count after applying UserFilterRegistry to
OrganizationMember.objects.filter(user__user_id=user_id) and raise an explicit
error (e.g., ValueError or a custom AmbiguousMatchError) if more than one row is
returned, return the single object when exactly one match exists, and return
None when zero matches exist; update callers such as remove_user_by_user_id to
expect this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1207b7a7-e66e-4bf2-b206-41a91db1b410
📒 Files selected for processing (6)
backend/account_v2/authentication_controller.pybackend/account_v2/tests.pybackend/account_v2/tests/__init__.pybackend/account_v2/tests/test_user_filter_registry.pybackend/account_v2/user.pybackend/tenant_account_v2/organization_member_service.py
💤 Files with no reviewable changes (1)
- backend/account_v2/tests.py
…coping-multi-idp # Conflicts: # backend/account_v2/authentication_controller.py
|
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
jaseemjaskp
requested review from
chandrasekharan-zipstack,
hari-kuriakose,
kirtimanmishrazipstack and
muhammad-ali-e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
UserFilterRegistryin core that plugins can populate at startup to scopeUser/OrganizationMemberqueries.oidc|<AUTH0_OIDC_CONNECTION>|prefix when the setting is defined — leaving behavior unchanged when it's unset/empty.add_user_role/remove_user_roleto raise a clearAmbiguousUserException(409) instead of a silent 500 when a lookup matches multiple rows.Why
When one Auth0 organization is wired to multiple OIDC connections (one per environment, e.g.
enterprise-qafor QA +enterprisefor prod), Auth0 issues distinctuser_ids for the same email across connections. Unstract keysUserrows on(email, user_id), so each connection creates a separateUserrow in the same DB. Two visible symptoms on the affected QA on-prem env:unstract_userandunstract_admin), because bothUserrows have their ownOrganizationMember.Userrow (oidc|enterprise|…) while their QA login resolved to the other (oidc|enterprise-qa|…), sois_adminreturned false and the route guard blocked them.The fix scopes all user lookups to the connection that belongs to the current environment.
How
backend/account_v2/user_filter_registry.py(new) — a small registry exposingregister(fn)andapply(qs, kind)wherekind ∈ {"user", "org_member"}. With no filters registered,applyis an identity function.backend/account_v2/user.py—UserService.get_user_by_email/get_user_by_user_id/get_user_by_idroute throughUserFilterRegistry.apply(..., "user")..get()becomes.filter().first()so a scoped match returns one row instead of raisingMultipleObjectsReturned.backend/tenant_account_v2/organization_member_service.py— same treatment forget_user_by_email/get_user_by_user_id/get_user_by_id/get_members/get_members_by_role/get_members_by_user_email, all withkind="org_member".backend/account_v2/authentication_controller.py— new_resolve_unique_member_by_emailhelper used byadd_user_roleandremove_user_role. RaisesAmbiguousUserExceptionwhen more than one row matches after filters apply, surfacing configuration issues instead of silently picking one.backend/account_v2/custom_exceptions.py— newAmbiguousUserException(HTTP 409) with a clear error message.The plugin filter itself lives in the private overlay:
backend/plugins/authentication/auth0/apps.pyregisters a filter viaAuth0PluginConfig.ready()ifAUTH0_OIDC_CONNECTIONis set.backend/backend/settings/on-prem.pyadds the setting binding.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
plugins.authentication.auth0plugin is not in OSSINSTALLED_APPS, so the filter is never registered.UserFilterRegistry.apply()with no filters is a pure identity function.cloud.pydoes not defineAUTH0_OIDC_CONNECTION, soAuth0PluginConfig.ready()returns early without registering a filter.AUTH0_OIDC_CONNECTIONis left unset or empty (""short-circuits the same asNone).One semantic shift to be aware of: lookups in
UserService/OrganizationMemberServicepreviously raisedMultipleObjectsReturned(uncaught → 500) on duplicate matches. They now return the first match. For role-change endpoints this is replaced by an explicit 409 (AmbiguousUserException), which is strictly better than the old 500. For other lookup callers the behavior is rare-but-tolerated (DB unique constraints make duplicates unreachable in practice).Database Migrations
Env Config
AUTH0_OIDC_CONNECTION. Opt-in. Set to the Auth0 OIDC connection name for the current environment (e.g.enterprise-qain QA,enterprisein prod). Leave unset/empty for single-IdP deployments — including the documented Azure AD OIDC setup.Userwhoseuser_iddoes not start withoidc|<value>|disappears from queries — including pre-SSO db-auth users (auth0|…), SAML users (samlp|…), and users from other OIDC connections. Only enable when every active user authenticates via the named OIDC connection.Relevant Docs
UserFilterRegistrydescribes the contract for plugin-registered filters.AUTH0_OIDC_CONNECTIONinon-prem.pydocuments the opt-in nature and the consequences of enabling it.Related Issues or PRs
oidc|enterprise-qa|…andoidc|enterprise|…user_ids for the same set of emails (one per environment connection).Dependencies Versions
Notes on Testing
AUTH0_OIDC_CONNECTION=<qa-connection>(e.g.enterprise-qa): Manage Users shows each affected email exactly once, with the QA-connectionuser_id.AUTH0_OIDC_CONNECTION=""(or unset): all users visible, no scoping applied.add_user_rolewith an ambiguous email returns 409 with theAmbiguousUserExceptionmessage.Screenshots
QA Manage Users showing duplicate rows for affected users (admin + user roles) — see ticket attachments.
Auth0 organization Members list showing two
user_ids per affected email, one per connection prefix — see ticket attachments.Checklist
I have read and understood the Contribution Guidelines.