[feature] Made RegisteredUser model support multi-tenancy #692#698
[feature] Made RegisteredUser model support multi-tenancy #692#698
Conversation
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
|
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:
📝 WalkthroughWalkthroughThis PR converts RegisteredUser from a one-to-one model to a multitenant one-to-many model: UUID primary key, ForeignKey user with related_name="registered_users", and nullable organization FK. It adds uniqueness constraints (user, organization) and a unique global-record constraint, migration logic (CT/CTCR and multitenant data/constraints), and updates model APIs, identity-verification helpers, serializers, views, admin inlines/filters, monitoring/accounting, management commands, SAML/social hooks, and many tests to use organization-scoped RegisteredUser records and adjusted query/prefetch logic. Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API
participant OrgResolver as Org
participant DB as "RegisteredUser(s)"
rect rgba(150,200,100,0.5)
Note over Client,API: Org-scoped lookup and authorization flow
Client->>API: Request (with org context)
API->>OrgResolver: Resolve current organization (view.organization)
API->>DB: Query registered_users filtered by organization OR organization IS NULL (prefer org)
DB-->>API: Return matching RegisteredUser (org-specific or global) or none
API->>API: Compute is_verified/method and authorization decision
API->>Client: Respond (allow/deny, token, serialized user data)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
There was a problem hiding this comment.
SQLite cannot modify primary keys in place, so Django works around this by rebuilding the entire table when a PK or certain fields are altered. During your migration, the table rebuild temporarily ends up with both the old implicit PK and the new id column, which leads to the duplicate column name: id error. This typically happens when adding a new PK column and removing the old PK within the same schema transition. Splitting the operations into separate migrations avoids the conflict because each table rebuild sees a consistent schema.
Multiple Flake8 Violations and Test FailuresHello @pandafy,
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_radius/saml/backends.py (1)
15-33: 🧹 Nitpick | 🔵 TrivialDead exception handler:
ObjectDoesNotExistwill never be raised.The
try/except ObjectDoesNotExistblock was designed for the old singularuser.registered_useraccess pattern. With the newuser.registered_users.exclude(method="saml").exists()query, an empty queryset simply returnsFalserather than raisingObjectDoesNotExist.The exception handler on line 32 is now unreachable dead code. Consider removing it for clarity:
Proposed fix
if ( not app_settings.SAML_UPDATES_PRE_EXISTING_USERNAME and not user._state.adding ): # Skip updating user's username if the user didn't signed up # with SAML registration method. - try: - attribute_mapping = attribute_mapping.copy() - # Check if any of the user's registered_users records - # were NOT created via SAML - has_non_saml = user.registered_users.exclude(method="saml").exists() - if has_non_saml: - for key, value in attribute_mapping.items(): - if "username" in value: - break - if len(value) == 1: - attribute_mapping.pop(key, None) - else: - attribute_mapping[key] = [] - for attr in value: - if attr != "username": - attribute_mapping[key].append(attr) - - except ObjectDoesNotExist: - pass + attribute_mapping = attribute_mapping.copy() + # Check if any of the user's registered_users records + # were NOT created via SAML + has_non_saml = user.registered_users.exclude(method="saml").exists() + if has_non_saml: + for key, value in attribute_mapping.items(): + if "username" in value: + break + if len(value) == 1: + attribute_mapping.pop(key, None) + else: + attribute_mapping[key] = [] + for attr in value: + if attr != "username": + attribute_mapping[key].append(attr) return super()._update_user(user, attributes, attribute_mapping, force_save)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/saml/backends.py` around lines 15 - 33, The try/except around the attribute mapping logic is using ObjectDoesNotExist which can never be raised now because you call user.registered_users.exclude(method="saml").exists(); remove the dead exception handler and the surrounding try/except so the code simply copies attribute_mapping, computes has_non_saml via user.registered_users.exclude(method="saml").exists(), and then performs the username-removal logic (using variables attribute_mapping, has_non_saml, key, value) without catching ObjectDoesNotExist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 77: CI installs openwisp-users from the GitHub branch
"issues/497-export-users" but setup.py's install_requires still references
"1.3", causing environment mismatch; update setup.py's install_requires entry
for "openwisp-users" (the install_requires list) to the same GitHub
tarball/branch URL
"https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" (or
otherwise ensure the "1.3" release truly contains the export changes) so all
environments use the identical version that CI uses.
In `@openwisp_radius/base/models.py`:
- Around line 1061-1068: The code currently instantiates RegisteredUser(...) and
calls registered_user.save() unconditionally which can violate the unique (user,
organization) constraint on repeated imports; replace this with a safe
retrieval-or-creation using RegisteredUser.get_or_create_for_user_and_org(user,
self.organization) (or RegisteredUser.objects.get_or_create(user=user,
organization=self.organization, defaults={...} if preferred), and use the
returned instance rather than blindly calling save(); ensure this change is
applied in the same block that later checks for OrganizationUser to avoid
IntegrityError during batch re-imports.
- Around line 1613-1623: The organization ForeignKey on the RegistrationInfo
model is missing related_name and should include related_name="registered_users"
to match migration 0043; update the organization field definition (the
models.ForeignKey created with swapper.get_model_name("openwisp_users",
"Organization")) to add related_name="registered_users" so the runtime model
matches the migration and enables the reverse accessor
organization.registered_users.
- Around line 1577-1582: Scope the verified-user check to the target
organization: update _validate_already_verified() to accept an organization
parameter (or derive org from PhoneToken if you add an organization field to
PhoneToken) and use RegisteredUser.get_global_or_org_specific(user=self.user,
organization=org) instead of RegisteredUser.objects.filter(...).exists();
alternatively, add an organization field to PhoneToken and pass that into
_validate_already_verified() so RegisteredUser lookup is org-scoped with global
fallback per the RegisteredUser design.
In `@openwisp_radius/integrations/monitoring/tasks.py`:
- Around line 185-199: The query for RegisteredUser may return a global record
before an org-specific one because it uses .first(); change the lookup to prefer
org-specific records by first attempting
RegisteredUser.objects.only("method").filter(user__username=username,
organization_id=organization_id).first() and if that returns None then fall back
to RegisteredUser.objects.only("method").filter(user__username=username,
organization__isnull=True).first(), then extract .method (or set "unspecified")
and pass it to clean_registration_method(registration_method); update the logic
around the registration_method variable and keep references to username,
organization_id, RegisteredUser, and clean_registration_method to locate the
code to change.
In `@openwisp_radius/management/commands/base/delete_unverified_users.py`:
- Around line 36-41: The current queryset qs on User (using
registered_users__is_verified=False) still matches users who are verified in
some organizations; change it to only select users with zero verified
registrations by annotating a Count of registered_users filtered by
is_verified=True (e.g., annotate(num_verified=Count('registered_users',
filter=Q(registered_users__is_verified=True)))) and then filter(num_verified=0)
together with the existing date_joined and is_staff conditions; add the
necessary imports (django.db.models.Count and Q) and replace the qs definition
accordingly so users verified in any org are excluded from deletion.
In `@openwisp_radius/migrations/__init__.py`:
- Around line 11-15: The functions copy_registered_users_ctcr_forward,
copy_registered_users_ctcr_reverse,
migrate_registered_users_multitenant_forward, and
migrate_registered_users_multitenant_reverse (and any historical transformation
logic currently defined next to
BATCH_SIZE/REGISTERED_USER_ORGANIZATION_HELP_TEXT) must be removed from the live
helper module and made immutable for historical migrations: either inline the
exact implementation into each migration that currently imports them (migrations
0002, 0003, 0004, 0012, 0014, 0018, 0043, 0044, and sample app 0032) or move
them into a new versioned module (e.g., migrations.history_v1) that is created
once and never edited, and update those migrations to import from that versioned
module; ensure all references to the original helper functions/classes are
updated so shipped migrations retain their original behavior.
- Around line 91-93: The current ordering uses the lexical "method" field which
can prefer weaker verification methods; update both querysets that build from
RegisteredUserNew to annotate each row with an explicit verification-priority
(e.g. using Django's Case/When to map method names to integer priorities where
stronger methods get higher priority) and then order_by that annotated priority
instead of "method" (preserve the other order_by keys like "user_id",
"-is_verified", "pk"); apply this change for the two occurrences that create the
queryset from RegisteredUserNew so rollbacks pick the strongest verification
method deterministically.
In `@openwisp_radius/saml/views.py`:
- Around line 75-82: The current try/except around
user.registered_users.get(...) followed by creating RegisteredUser(...) is
race-prone and can raise IntegrityError under concurrent logins; replace this
read-then-create with a race-safe approach such as
RegisteredUser.objects.get_or_create(...) (or
user.registered_users.get_or_create(...)) passing defaults={'method': 'saml',
'is_verified': app_settings.SAML_IS_VERIFIED} and use the returned (instance,
created) tuple, or alternately wrap creation in a transaction.atomic and catch
IntegrityError then re-fetch the existing RegisteredUser; update the code paths
that reference the created variable accordingly (search for RegisteredUser,
user.registered_users.get, RegisteredUser(...)).
In `@openwisp_radius/social/views.py`:
- Around line 50-59: Replace the non-atomic try/except and manual creation with
an atomic get-or-create call to avoid the race on the unique (user,
organization) constraint: use
user.registered_users.get_or_create(organization=org, defaults={'method':
'social_login', 'is_verified': False}) (referencing RegisteredUser and
user.registered_users) and remove the separate RegisteredUser(...) /
full_clean() / save() flow; if you still need model validation, perform it
inside a transaction and handle IntegrityError by re-fetching the existing
RegisteredUser.
In `@openwisp_radius/tests/test_api/test_api.py`:
- Around line 356-358: The test uses user.registered_users.first() which can
return an arbitrary RegisteredUser and make the test flaky; change that access
to explicitly fetch the org-scoped record (use
user.registered_users.get(organization=self.default_org)) when setting expected
fields like is_verified and method so the assertions match the serializer's
organization-scoped behavior (replace both occurrences of
user.registered_users.first() in this assertion block).
In `@openwisp_radius/tests/test_batch_add_users.py`:
- Around line 146-148: The assertion is using self.default_org instead of the
test-local organization variable created in setup; update the lookup so reg_user
= user.registered_users.get(organization=<test org>) uses the test's
organization object (replace self.default_org with the test-local organization
variable used in setup, e.g. self.organization) so the get call and subsequent
assertions (reg_user.is_verified, reg_user.method) reference the correct
organization.
In `@openwisp_radius/tests/test_commands.py`:
- Around line 279-282: The test uses user.registered_users.first() which can
pick the wrong registration when multiple exist; change the lookup to scope by
organization (use the RegisteredUser queryset on user.registered_users filtered
with organization=self.default_org, e.g.,
filter(organization=self.default_org).first() or
.get(organization=self.default_org)) and then set is_verified/method and
save(update_fields=["is_verified", "method"]) so the test deterministically
targets the registration for self.default_org.
---
Outside diff comments:
In `@openwisp_radius/saml/backends.py`:
- Around line 15-33: The try/except around the attribute mapping logic is using
ObjectDoesNotExist which can never be raised now because you call
user.registered_users.exclude(method="saml").exists(); remove the dead exception
handler and the surrounding try/except so the code simply copies
attribute_mapping, computes has_non_saml via
user.registered_users.exclude(method="saml").exists(), and then performs the
username-removal logic (using variables attribute_mapping, has_non_saml, key,
value) without catching ObjectDoesNotExist.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0b13918d-1065-4f91-948f-2411f3eb07fe
📒 Files selected for processing (34)
.github/workflows/ci.ymlopenwisp_radius/admin.pyopenwisp_radius/api/freeradius_views.pyopenwisp_radius/api/serializers.pyopenwisp_radius/api/utils.pyopenwisp_radius/api/views.pyopenwisp_radius/base/admin_filters.pyopenwisp_radius/base/models.pyopenwisp_radius/integrations/monitoring/tasks.pyopenwisp_radius/integrations/monitoring/tests/test_metrics.pyopenwisp_radius/management/commands/base/delete_unverified_users.pyopenwisp_radius/migrations/0043_registereduser_add_uuid.pyopenwisp_radius/migrations/0044_registered_user_multitenant_data.pyopenwisp_radius/migrations/0045_registered_user_multitenant_constraints.pyopenwisp_radius/migrations/__init__.pyopenwisp_radius/saml/backends.pyopenwisp_radius/saml/views.pyopenwisp_radius/settings.pyopenwisp_radius/social/views.pyopenwisp_radius/tests/mixins.pyopenwisp_radius/tests/test_admin.pyopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/tests/test_api/test_phone_verification.pyopenwisp_radius/tests/test_api/test_rest_token.pyopenwisp_radius/tests/test_batch_add_users.pyopenwisp_radius/tests/test_commands.pyopenwisp_radius/tests/test_saml/test_views.pyopenwisp_radius/tests/test_selenium.pyopenwisp_radius/tests/test_social.pyopenwisp_radius/tests/test_tasks.pyopenwisp_radius/tests/test_token.pyopenwisp_radius/tests/test_users_integration.pyrunteststests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-06T08:47:54.428Z
Learnt from: stktyagi
Repo: openwisp/openwisp-radius PR: 689
File: .github/workflows/bot-ci-failure.yml:60-60
Timestamp: 2026-03-06T08:47:54.428Z
Learning: In openwisp/openwisp-radius and similar OpenWISP repos, reusable workflow caller files intentionally reference upstream reusable workflows with master (e.g., openwisp/openwisp-utils/.github/workflows/reusable-bot-ci-failure.ymlmaster). This is by design to ensure callers always use the latest changes from the master branch of the upstream project. Do not flag such references as security or pinning issues during code reviews. If applying this pattern broadly, treat any workflow call that uses a remote reusable workflow from a different repository with master as an intentional design choice rather than a vulnerability.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-03-24T21:08:04.331Z
Learnt from: CR
Repo: openwisp/openwisp-radius PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:08:04.331Z
Learning: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior
Applied to files:
runtests
🔇 Additional comments (18)
openwisp_radius/tests/test_users_integration.py (1)
99-106: No changes needed. The test setup is correct and does not violate the unique constraint.The code creates a single
RegisteredUserrecord per user/organization pair, which satisfies theUniqueConstraint(fields=["user", "organization"])defined in the model._create_org_user()creates anOrganizationUser(membership), while the explicitRegisteredUser.objects.create()creates a separate registration record. These are distinct entities: a user can be an organization member without being a registered user, and vice versa. The test intentionally sets up both relationships.No signal handler automatically creates
RegisteredUseronOrganizationUsercreation, so the explicit call is necessary and expected for this test scenario.> Likely an incorrect or invalid review comment.openwisp_radius/tests/mixins.py (1)
100-103: Inline formset prefix update is correct.The
registered_users-*management form keys align with the updated reverse relation naming and admin inline behavior.openwisp_radius/tests/test_saml/test_views.py (1)
155-157: Org-scoped SAML fixture setup looks correct.Creating
RegisteredUserwith an explicitorganizationis consistent with the new multitenant model behavior.openwisp_radius/tests/test_token.py (1)
68-71: Token verification fixture is correctly organization-scoped.Adding
organization=self.default_orgmakes this test consistent with the new registration model constraints.runtests (1)
6-6: Good improvement to test-run visibility.Preserving stdout while still combining stderr into the same stream makes CI/debug output more actionable.
openwisp_radius/integrations/monitoring/tests/test_metrics.py (1)
25-29: Helper defaults are aligned with multitenancy.Defaulting
organizationin_create_registered_useris correct and still safely overridable throughkwargs.openwisp_radius/tests/test_admin.py (2)
1362-1362: Inline management-form assertion is correctly updated.
id_registered_users-TOTAL_FORMSmatches the pluralized inline relation.
1419-1423: Admin verification fixtures are correctly organization-aware.These
RegisteredUserrecords now match the org-scoped model behavior used by the changelist and filter logic.Also applies to: 1430-1434, 1458-1462, 1469-1473
openwisp_radius/tests/test_selenium.py (1)
24-24: Good isolation change for Selenium stability.Adding
no_parallelhere is a solid guard against cross-test interference in browser/websocket flows.Also applies to: 210-210
openwisp_radius/api/freeradius_views.py (2)
293-293: LGTM!The
.distinct()is necessary here because the join to theregistered_usersrelation can produce duplicate user rows. This correctly prevents the same user from appearing multiple times in the result set.
412-429: LGTM!The organization-scoped query conditions correctly implement the org-specific-or-global fallback pattern:
org_or_globalensures both org-specific and global (null organization)RegisteredUserrecords are considered- The verification and authorization method checks are properly scoped to this combined predicate
This aligns with the multi-tenancy model requirements where org-specific records take precedence but global records serve as a fallback.
openwisp_radius/api/views.py (2)
387-395: LGTM!The switch from
select_related("user__registered_user")toprefetch_related("user__registered_users")correctly handles the new one-to-many relationship. The separateselect_related("user")ensures the user object is still efficiently loaded in a single query.
763-778: LGTM!The phone verification logic correctly uses
RegisteredUser.get_or_create_for_user_and_orgto ensure org-specific verification records are created. The flow properly:
- Gets or creates an org-specific
RegisteredUserrecord- Updates verification status and method
- Saves both user and registered user records
openwisp_radius/api/utils.py (1)
33-45: LGTM!The implementation correctly follows the org-specific-then-global fallback pattern that's consistent with the serializer's
_get_registered_usermethod. The iteration through prefetchedregistered_usersavoids N+1 queries when the relation is already loaded.openwisp_radius/tests/test_tasks.py (2)
142-145: LGTM!The test correctly adapts to the new multi-row
registered_userspattern. Using.first()is appropriate here sincebatch_add_userscreates a singleRegisteredUserper user.
324-393: LGTM!The test fixture setup and assertions are correctly updated:
- All
RegisteredUser.objects.create()calls now includeorganization=self.default_org- Assertions use explicit org-scoped lookup
.get(organization=self.default_org)to verify the correct record- The test logic properly verifies that only
mobile_phoneregistered users are unverified after the task runsopenwisp_radius/admin.py (2)
537-545: LGTM!Good changes for multi-tenancy support:
TabularInlineis more appropriate for displaying multiple org-scoped records- Making
organizationreadonly prevents accidental modification of the association- The explicit
fieldstuple provides clear column ordering
559-570: Behavior note: verification status shows "yes" if verified in ANY organization.The current logic returns "yes" if the user has any verified
RegisteredUserrecord across all organizations. This is a reasonable default for the global user list, but admins should be aware that "yes" doesn't mean verified in a specific org—just verified somewhere.This aligns with the admin's cross-org view perspective, so no change needed.
| pip install -U pip wheel setuptools | ||
| pip install -U -r requirements-test.txt | ||
| pip install -e .[saml,openvpn_status] | ||
| pip install --upgrade --no-deps --no-cache-dir "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'openwisp[-_]users|openwisp_users' \
--iglob 'pyproject.toml' \
--iglob 'setup.py' \
--iglob 'setup.cfg' \
--iglob 'requirements*.txt' \
--iglob '*.in'Repository: openwisp/openwisp-radius
Length of output: 560
Unify the openwisp-users dependency across all environments.
CI installs openwisp-users from the issues/497-export-users branch (line 77), but setup.py:44-45 declares it from the 1.3 branch instead. Since openwisp_radius/settings.py depends on export functionality, non-CI environments installing from setup.py will use a mismatched version and potentially break.
Update setup.py to reference the same issues/497-export-users branch that CI uses, or confirm that both branches contain the required export support and are equivalent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 77, CI installs openwisp-users from the
GitHub branch "issues/497-export-users" but setup.py's install_requires still
references "1.3", causing environment mismatch; update setup.py's
install_requires entry for "openwisp-users" (the install_requires list) to the
same GitHub tarball/branch URL
"https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" (or
otherwise ensure the "1.3" release truly contains the export changes) so all
environments use the identical version that CI uses.
There was a problem hiding this comment.
@coderabbitai we will remove this from the CI file once the related PR in openwisp-users is merged. You can ignore this.
There was a problem hiding this comment.
@pandafy, understood! Thanks for the context — I'll ignore this temporary CI dependency on the issues/497-export-users branch of openwisp-users. It makes sense to keep it until the upstream PR is merged.
🧠 Learnings used
Learnt from: stktyagi
Repo: openwisp/openwisp-radius PR: 689
File: .github/workflows/bot-ci-failure.yml:60-60
Timestamp: 2026-03-06T08:47:54.428Z
Learning: In openwisp/openwisp-radius and similar OpenWISP repos, reusable workflow caller files intentionally reference upstream reusable workflows with master (e.g., openwisp/openwisp-utils/.github/workflows/reusable-bot-ci-failure.ymlmaster). This is by design to ensure callers always use the latest changes from the master branch of the upstream project. Do not flag such references as security or pinning issues during code reviews. If applying this pattern broadly, treat any workflow call that uses a remote reusable workflow from a different repository with master as an intentional design choice rather than a vulnerability.
| user.registered_users.get(organization=org) | ||
| except RegisteredUser.DoesNotExist: | ||
| registered_user = RegisteredUser( | ||
| user=user, method="saml", is_verified=app_settings.SAML_IS_VERIFIED | ||
| user=user, | ||
| organization=org, | ||
| method="saml", | ||
| is_verified=app_settings.SAML_IS_VERIFIED, | ||
| ) |
There was a problem hiding this comment.
Handle concurrent create race for org-scoped RegisteredUser.
This read-then-create flow is race-prone under concurrent logins and can surface an IntegrityError on unique (user, organization).
🛠️ Race-safe pattern
+from django.db import IntegrityError
@@
- try:
- user.registered_users.get(organization=org)
- except RegisteredUser.DoesNotExist:
- registered_user = RegisteredUser(
- user=user,
- organization=org,
- method="saml",
- is_verified=app_settings.SAML_IS_VERIFIED,
- )
- registered_user.full_clean()
- registered_user.save()
+ try:
+ user.registered_users.get(organization=org)
+ except RegisteredUser.DoesNotExist:
+ registered_user = RegisteredUser(
+ user=user,
+ organization=org,
+ method="saml",
+ is_verified=app_settings.SAML_IS_VERIFIED,
+ )
+ registered_user.full_clean()
+ try:
+ registered_user.save()
+ except IntegrityError:
+ # Created by a concurrent request
+ user.registered_users.get(organization=org)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/saml/views.py` around lines 75 - 82, The current try/except
around user.registered_users.get(...) followed by creating RegisteredUser(...)
is race-prone and can raise IntegrityError under concurrent logins; replace this
read-then-create with a race-safe approach such as
RegisteredUser.objects.get_or_create(...) (or
user.registered_users.get_or_create(...)) passing defaults={'method': 'saml',
'is_verified': app_settings.SAML_IS_VERIFIED} and use the returned (instance,
created) tuple, or alternately wrap creation in a transaction.atomic and catch
IntegrityError then re-fetch the existing RegisteredUser; update the code paths
that reference the created variable accordingly (search for RegisteredUser,
user.registered_users.get, RegisteredUser(...)).
| user.registered_users.get(organization=org) | ||
| except RegisteredUser.DoesNotExist: | ||
| registered_user = RegisteredUser( | ||
| user=user, method="social_login", is_verified=False | ||
| user=user, | ||
| organization=org, | ||
| method="social_login", | ||
| is_verified=False, | ||
| ) | ||
| registered_user.full_clean() | ||
| registered_user.save() |
There was a problem hiding this comment.
Make org-scoped RegisteredUser creation atomic.
Line 50 currently does a non-atomic check-then-create flow. Concurrent requests can both miss and then race on insert, causing a server error on the unique (user, organization) constraint.
Suggested fix
- try:
- user.registered_users.get(organization=org)
- except RegisteredUser.DoesNotExist:
- registered_user = RegisteredUser(
- user=user,
- organization=org,
- method="social_login",
- is_verified=False,
- )
- registered_user.full_clean()
- registered_user.save()
+ RegisteredUser.objects.get_or_create(
+ user=user,
+ organization=org,
+ defaults={"method": "social_login", "is_verified": False},
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/social/views.py` around lines 50 - 59, Replace the non-atomic
try/except and manual creation with an atomic get-or-create call to avoid the
race on the unique (user, organization) constraint: use
user.registered_users.get_or_create(organization=org, defaults={'method':
'social_login', 'is_verified': False}) (referencing RegisteredUser and
user.registered_users) and remove the separate RegisteredUser(...) /
full_clean() / save() flow; if you still need model validation, perform it
inside a transaction and handle IntegrityError by re-fetching the existing
RegisteredUser.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 77: The CI step is installing
"https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"
with the --no-deps flag which skips dependency resolution and can produce broken
test environments; remove the --no-deps flag from the pip install command so pip
can resolve and install required dependencies, or if --no-deps is intentionally
required, add a clear comment in the workflow and ensure earlier steps
explicitly install and pin all openwisp-users dependencies (e.g., Django and
related packages) before this pip install to guarantee a consistent environment.
In `@openwisp_radius/migrations/0043_registereduser_add_uuid.py`:
- Around line 11-14: The migration currently imports REGISTRATION_METHOD_CHOICES
and get_registration_choices from registration.py which creates a runtime
dependency; remove those imports from this migration and update the temporary
model RegisteredUserNew to not reference them—either remove the choices=...
argument from the CharField named method or replace it with a frozen local tuple
literal if you need a value snapshot; ensure no calls to
get_registration_choices or references to REGISTRATION_METHOD_CHOICES remain in
the migration so the migration is self-contained.
- Around line 32-35: The migration declares foreign keys to
openwisp_users.Organization (in migration 0043_registereduser_add_uuid.py at the
FK definitions around lines referencing Organization) but does not depend on
that model; add a migration dependency for the Organization model by including
migrations.swappable_dependency("openwisp_users.Organization") (or the app/model
swap-aware dependency) in the dependencies list alongside the existing
settings.AUTH_USER_MODEL and the 0042 dependency so the migration graph ensures
the Organization table exists before running this migration.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 623b5948-4fb3-4254-bbdc-217b77e67bc7
📒 Files selected for processing (2)
.github/workflows/ci.ymlopenwisp_radius/migrations/0043_registereduser_add_uuid.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Analyze (python)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-06T08:47:54.428Z
Learnt from: stktyagi
Repo: openwisp/openwisp-radius PR: 689
File: .github/workflows/bot-ci-failure.yml:60-60
Timestamp: 2026-03-06T08:47:54.428Z
Learning: In openwisp/openwisp-radius and similar OpenWISP repos, reusable workflow caller files intentionally reference upstream reusable workflows with master (e.g., openwisp/openwisp-utils/.github/workflows/reusable-bot-ci-failure.ymlmaster). This is by design to ensure callers always use the latest changes from the master branch of the upstream project. Do not flag such references as security or pinning issues during code reviews. If applying this pattern broadly, treat any workflow call that uses a remote reusable workflow from a different repository with master as an intentional design choice rather than a vulnerability.
Applied to files:
.github/workflows/ci.yml
🔇 Additional comments (2)
.github/workflows/ci.yml (1)
77-77: 🧹 Nitpick | 🔵 TrivialPin to a specific commit SHA for reproducible CI builds.
Referencing the branch name
issues/497-export-usersdirectly makes CI builds non-reproducible, since the branch can be updated, force-pushed, or rebased between runs. This complicates debugging when CI results differ across runs of the same code.📌 Pin to a commit SHA
Replace the branch name with a specific commit SHA:
- pip install --upgrade --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" + pip install --upgrade --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/<COMMIT_SHA>"Replace
<COMMIT_SHA>with the actual commit hash from theissues/497-export-usersbranch that contains the required functionality. Update this SHA when the upstream branch is updated and you need the latest changes.⛔ Skipped due to learnings
Learnt from: stktyagi Repo: openwisp/openwisp-radius PR: 689 File: .github/workflows/bot-ci-failure.yml:60-60 Timestamp: 2026-03-06T08:48:01.200Z Learning: In the openwisp/openwisp-radius repository (and other openwisp repositories), reusable workflow caller files (e.g., `.github/workflows/bot-ci-failure.yml`) intentionally reference upstream reusable workflows with `master` (e.g., `openwisp/openwisp-utils/.github/workflows/reusable-bot-ci-failure.ymlmaster`). This is by design so callers always use the latest changes from the master branch of openwisp-utils. Do not flag this as a security or pinning issue in code reviews.openwisp_radius/migrations/0043_registereduser_add_uuid.py (1)
152-160: Good call using copy/delete/rename instead of an in-place PK rewrite.This temp-table path is much safer across backends than trying to mutate the primary key in place.
| pip install -U pip wheel setuptools | ||
| pip install -U -r requirements-test.txt | ||
| pip install -e .[saml,openvpn_status] | ||
| pip install --upgrade --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" |
There was a problem hiding this comment.
The --no-deps flag bypasses dependency resolution and risks broken environments.
Installing openwisp-users with --no-deps prevents pip from checking or installing its dependencies. If the issues/497-export-users branch requires different versions of shared dependencies (Django, other OpenWISP packages, etc.), those won't be resolved, potentially causing runtime failures that only surface during test execution rather than at install time.
🔧 Safer installation approach
- pip install --upgrade --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"
+ pip install --upgrade --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"If --no-deps is intentionally used to avoid dependency conflicts, document why and verify that all required dependencies are already correctly installed by the previous steps.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pip install --upgrade --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" | |
| pip install --upgrade --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 77, The CI step is installing
"https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"
with the --no-deps flag which skips dependency resolution and can produce broken
test environments; remove the --no-deps flag from the pip install command so pip
can resolve and install required dependencies, or if --no-deps is intentionally
required, add a clear comment in the workflow and ensure earlier steps
explicitly install and pin all openwisp-users dependencies (e.g., Django and
related packages) before this pip install to guarantee a consistent environment.
There was a problem hiding this comment.
@pandafy, understood! I'll disregard this comment. The temporary pip install --no-deps line is a workaround pending the merge of the upstream openwisp-users PR #498.
✏️ Learnings added
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 698
File: .github/workflows/ci.yml:77-77
Timestamp: 2026-04-17T13:53:45.430Z
Learning: In openwisp/openwisp-radius PR `#698`, the CI step `pip install --upgrade --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"` in `.github/workflows/ci.yml` is intentionally temporary. It installs a pre-release branch of openwisp-users as a blocker dependency and will be removed once the upstream openwisp-users PR `#498` is merged. Do not flag this install command as an issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: stktyagi
Repo: openwisp/openwisp-radius PR: 689
File: .github/workflows/bot-ci-failure.yml:60-60
Timestamp: 2026-03-06T08:47:54.428Z
Learning: In openwisp/openwisp-radius and similar OpenWISP repos, reusable workflow caller files intentionally reference upstream reusable workflows with master (e.g., openwisp/openwisp-utils/.github/workflows/reusable-bot-ci-failure.ymlmaster). This is by design to ensure callers always use the latest changes from the master branch of the upstream project. Do not flag such references as security or pinning issues during code reviews. If applying this pattern broadly, treat any workflow call that uses a remote reusable workflow from a different repository with master as an intentional design choice rather than a vulnerability.
| from openwisp_radius.registration import ( | ||
| REGISTRATION_METHOD_CHOICES, | ||
| get_registration_choices, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the migration file mentioned in the review
find . -path "*openwisp_radius/migrations/0043_registereduser_add_uuid.py" -type fRepository: openwisp/openwisp-radius
Length of output: 128
🏁 Script executed:
#!/bin/bash
# List the openwisp_radius directory structure to understand the project layout
find ./openwisp_radius -type f -name "*.py" | head -20Repository: openwisp/openwisp-radius
Length of output: 722
🏁 Script executed:
#!/bin/bash
# Check if the registration module exists and examine its structure
find . -path "*openwisp_radius/registration.py" -o -path "*openwisp_radius/registration/*" -type fRepository: openwisp/openwisp-radius
Length of output: 101
🏁 Script executed:
cat -n ./openwisp_radius/migrations/0043_registereduser_add_uuid.pyRepository: openwisp/openwisp-radius
Length of output: 7715
🏁 Script executed:
cat -n ./openwisp_radius/registration.pyRepository: openwisp/openwisp-radius
Length of output: 2361
🏁 Script executed:
#!/bin/bash
# Search for other migrations that import from application code
grep -r "from openwisp_radius\." ./openwisp_radius/migrations/ --include="*.py" | head -20Repository: openwisp/openwisp-radius
Length of output: 522
🏁 Script executed:
#!/bin/bash
# Check if there are other migrations that import runtime choices
grep -r "REGISTRATION_METHOD_CHOICES\|get_registration_choices" ./openwisp_radius/migrations/Repository: openwisp/openwisp-radius
Length of output: 893
🏁 Script executed:
cat -n ./openwisp_radius/migrations/0023_registered_user.pyRepository: openwisp/openwisp-radius
Length of output: 3691
Remove runtime registration choice imports from this temporary migration.
REGISTRATION_METHOD_CHOICES and get_registration_choices imported from application code create a dependency that can break fresh installs if registration.py is refactored. Since RegisteredUserNew is a temporary table created only for data migration within SeparateDatabaseAndState, the choices parameter on the method field serves no purpose—CharField choices don't affect database schema.
Remove the import and either freeze the choices value locally or omit it entirely from the temporary table definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/migrations/0043_registereduser_add_uuid.py` around lines 11 -
14, The migration currently imports REGISTRATION_METHOD_CHOICES and
get_registration_choices from registration.py which creates a runtime
dependency; remove those imports from this migration and update the temporary
model RegisteredUserNew to not reference them—either remove the choices=...
argument from the CharField named method or replace it with a frozen local tuple
literal if you need a value snapshot; ensure no calls to
get_registration_choices or references to REGISTRATION_METHOD_CHOICES remain in
the migration so the migration is self-contained.
Code Review SummaryStatus: No Critical Issues Found | Recommendation: Merge after addressing CI dependency Major Changes in This UpdateThe PR now implements the complete multi-tenancy feature with a new "pending_verification" registration method that enables cross-organization authentication flows. Key Features Added
Migration & Data Integrity
CI DependencyThe temporary tarball dependency on Testing
Review these changes in Kilo Cloud Reviewed by kimi-k2.5-0127 · 511,132 tokens |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py`:
- Around line 62-65: This migration declares a FK to
swapper.get_model_name("openwisp_users", "Organization") and calls a helper that
queries OrganizationUser, but the dependencies list is missing the swapper
dependency; add swapper.dependency("openwisp_users", "Organization") to the
dependencies list in 0032_registered_user_multitenant.py (same place as
migrations.swappable_dependency(settings.AUTH_USER_MODEL)) so Django ensures the
openwisp_users Organization model/migration runs first (follow the pattern used
in 0002_initial_openwisp_app.py).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 186af29f-74b8-4be5-8fae-2eb398733995
📒 Files selected for processing (1)
tests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py
|
coverage: 97.064% (-0.4%) from 97.46% — issues/692-different-identity-verification into master |
| pip install -U pip wheel setuptools | ||
| pip install -U -r requirements-test.txt | ||
| pip install -e .[saml,openvpn_status] | ||
| pip install --upgrade --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" |
There was a problem hiding this comment.
TODO: Remove this before merging!
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_radius/api/views.py (1)
339-348:⚠️ Potential issue | 🟠 MajorCreate the org-scoped registration when admitting a new member.
This branch adds the
OrganizationUser, but it still leaves the user without aRegisteredUserforself.organization. Subsequent checks keep falling back to the global row, so org-specific verification is never initiated for the newly joined org.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/api/views.py` around lines 339 - 348, After creating and saving the OrganizationUser in the branch that admits a new member (the OrganizationUser instance built in this block), also ensure you create or get a RegisteredUser record scoped to the same organization so org-specific verification applies; specifically call a get_or_create for RegisteredUser with user=user and organization=self.organization (after org_user.save()) so the newly joined user has an org-scoped RegisteredUser row instead of falling back to the global one.
♻️ Duplicate comments (1)
openwisp_radius/integrations/monitoring/tasks.py (1)
185-190:⚠️ Potential issue | 🟠 Major
-organization_idstill does not guarantee org-specific rows win.This is the same root issue as the earlier review:
.first()is still driven by database NULL ordering, andorder_by("-organization_id")on a nullable field is not portable enough to rely on for “org row before global row”. That can still select the global registration first and tag accounting with the wrong method. Use an explicit org-first lookup, or an ordering that forcesNULLS LAST.In Django/PostgreSQL, when ordering a nullable column descending (for example `ORDER BY organization_id DESC` / `order_by("-organization_id")`), do NULL values sort first or last by default?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/integrations/monitoring/tasks.py` around lines 185 - 190, The current lookup using RegisteredUser.objects...order_by("-organization_id").first() can still pick a NULL (global) row first due to DB NULL ordering; change to an explicit org-first lookup: first query RegisteredUser.objects.only("method").filter(user__username=username, organization_id=organization_id).first() and if that returns None, fall back to RegisteredUser.objects.only("method").filter(user__username=username, organization__isnull=True).first(); alternatively, on Postgres you can replace the order_by call with an explicit NULLS LAST ordering using F('organization_id').desc(nulls_last=True) to guarantee org-specific rows win. Ensure you update the registration_method assignment accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_radius/admin.py`:
- Around line 559-566: get_is_verified currently triggers up to two DB queries
per row by calling obj.registered_users.exists() and
obj.registered_users.filter(...).exists(); instead, annotate or prefetch the
verification state in UserAdmin.get_queryset() (e.g., add an Exists subquery or
a Prefetch that computes has_registered and is_verified flags) and then read
those annotated/prefetched attributes inside get_is_verified (referencing
get_is_verified and UserAdmin.get_queryset and the registered_users relation) so
get_is_verified only reads in-memory attributes and performs no extra queries.
In `@openwisp_radius/api/serializers.py`:
- Around line 795-815: The serializer method _get_registered_user relies on
self.context["view"].organization but callers in openwisp_radius/api/views.py
instantiate RadiusUserSerializer(user) without context, causing org-specific
RegisteredUser rows to be missed; fix by changing those instantiations (the
token endpoints that create RadiusUserSerializer) to pass the current view in
the context (e.g. context={"view": self}) so _get_registered_user can read
view.organization and correctly prefer org-specific RegisteredUser over the
global fallback.
In `@openwisp_radius/api/views.py`:
- Around line 765-782: The create path is passing an invalid field and the
update path fails to mark existing records as mobile_phone-verified: remove
is_active from the defaults passed to
RegisteredUser.get_or_create_for_user_and_org and include
defaults={"is_verified": True, "method": "mobile_phone"} so new records are
created correctly; after creation/update ensure you set reg_user.is_verified =
True and reg_user.method = "mobile_phone" (in addition to saving
user.phone_number and updating user.username if it matched user.phone_number)
and then call reg_user.save() to persist the updated verification method and
status.
In `@openwisp_radius/base/models.py`:
- Around line 1061-1074: When reusing an existing RegisteredUser row from
RegisteredUser.get_or_create_for_user_and_org you only set
registered_user.is_verified=True which leaves its method as "" or "email" and
still failing is_identity_verified_strong; update the reuse branch to also set
registered_user.method="manual" (or upgrade it when it's weaker than "manual")
before saving so the row becomes strongly verified for orgs that require strong
verification (modify the block that checks not created and
self.organization.radius_settings.needs_identity_verification to update
registered_user.method and registered_user.is_verified then call
registered_user.save()).
In `@openwisp_radius/integrations/monitoring/tasks.py`:
- Around line 133-135: The current users_without_registereduser_query on
OrganizationUser uses user__registered_users__isnull=True which ignores
organization scope; change it to only select OrganizationUser rows where there
is no RegisteredUser for that same organization (e.g. replace the isnull filter
with an Exists/Subquery check: use
RegisteredUser.objects.filter(user=OuterRef('user'),
organization=OuterRef('organization')) and wrap with ~Exists or annotate a
boolean and filter False). Update the users_without_registereduser_query
variable (and imports of Exists/OuterRef/Subquery if needed) so the fallback
"unspecified" metric counts users missing a registration in that specific
OrganizationUser.organization, not globally.
In
`@tests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py`:
- Around line 18-21: The migration imports REGISTRATION_METHOD_CHOICES and
get_registration_choices at runtime which can break later; update the migration
so RegisteredUserNew.method does not depend on application imports by either
removing the choices argument from the CharField entirely or replacing it with
an inline frozen tuple (e.g., _FROZEN_METHOD_CHOICES) defined inside the
migration file and then use that frozen symbol in the method field; ensure you
remove the imports of REGISTRATION_METHOD_CHOICES and get_registration_choices
so the migration is self-contained.
---
Outside diff comments:
In `@openwisp_radius/api/views.py`:
- Around line 339-348: After creating and saving the OrganizationUser in the
branch that admits a new member (the OrganizationUser instance built in this
block), also ensure you create or get a RegisteredUser record scoped to the same
organization so org-specific verification applies; specifically call a
get_or_create for RegisteredUser with user=user and
organization=self.organization (after org_user.save()) so the newly joined user
has an org-scoped RegisteredUser row instead of falling back to the global one.
---
Duplicate comments:
In `@openwisp_radius/integrations/monitoring/tasks.py`:
- Around line 185-190: The current lookup using
RegisteredUser.objects...order_by("-organization_id").first() can still pick a
NULL (global) row first due to DB NULL ordering; change to an explicit org-first
lookup: first query
RegisteredUser.objects.only("method").filter(user__username=username,
organization_id=organization_id).first() and if that returns None, fall back to
RegisteredUser.objects.only("method").filter(user__username=username,
organization__isnull=True).first(); alternatively, on Postgres you can replace
the order_by call with an explicit NULLS LAST ordering using
F('organization_id').desc(nulls_last=True) to guarantee org-specific rows win.
Ensure you update the registration_method assignment accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1e2d19d0-36f0-46b3-9a04-3f29ce6a99c6
📒 Files selected for processing (15)
openwisp_radius/admin.pyopenwisp_radius/api/serializers.pyopenwisp_radius/api/utils.pyopenwisp_radius/api/views.pyopenwisp_radius/base/models.pyopenwisp_radius/integrations/monitoring/tasks.pyopenwisp_radius/management/commands/base/delete_unverified_users.pyopenwisp_radius/migrations/0043_registereduser_add_uuid.pyopenwisp_radius/migrations/__init__.pyopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/tests/test_api/test_rest_token.pyopenwisp_radius/tests/test_batch_add_users.pyopenwisp_radius/tests/test_commands.pyopenwisp_radius/tests/test_tasks.pytests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Kilo Code Review
🔇 Additional comments (16)
openwisp_radius/migrations/__init__.py (2)
12-16: Historical migration logic still lives in a mutable module.The functions
copy_registered_users_ctcr_forward,copy_registered_users_ctcr_reverse,migrate_registered_users_multitenant_forward, andmigrate_registered_users_multitenant_reverseare imported by migrations 0043, 0044, and the sample app's 0032. Any future edit to this file will retroactively alter the behavior of already-shipped migrations. Consider freezing this logic inline in each migration or in a versioned, never-to-be-edited module.
95-103: Method priority logic correctly addresses verification strength ordering.The
Case/Whenannotation now explicitly prioritizes stronger verification methods (mobile_phoneand others get priority 2) over weaker ones (openwisp_radius/migrations/0043_registereduser_add_uuid.py (2)
11-14: Runtime registration choice imports create fragile migration.This migration imports
REGISTRATION_METHOD_CHOICESandget_registration_choicesfrom application code. Ifregistration.pyis refactored after this migration ships, fresh installs may fail. SinceRegisteredUserNewis temporary and CharField choices don't affect the database schema, these imports serve no purpose here.
32-36: Organization dependency correctly added.The migration now properly declares
swapper.dependency("openwisp_users", "Organization")in dependencies, ensuring correct migration ordering.tests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py (1)
62-66: Organization dependency correctly added.The
swapper.dependency("openwisp_users", "Organization")is now included in the dependencies, ensuring the Organization table exists before this migration runs.openwisp_radius/tests/test_batch_add_users.py (1)
146-148: Correct org-scoped verification assertion.The test now properly retrieves the
RegisteredUserfor the specific organization created in the test setup, ensuring assertions validate the correct record.openwisp_radius/tests/test_api/test_rest_token.py (2)
223-225: Correct org-scoped RegisteredUser creation for test.The test now creates
RegisteredUserwith explicitorganization=org2, properly testing the multi-tenant verification flow where users can have different verification states per organization.
310-311: Query count increase is expected.The bump from 16 to 17 queries aligns with the change from
select_related("registered_user")toprefetch_related("registered_users")in the token validation view.openwisp_radius/management/commands/base/delete_unverified_users.py (1)
37-51: Correctly prevents deletion of users verified in any organization.The annotation-based approach (
num_verified=Count(..., filter=Q(is_verified=True))followed by.filter(num_verified=0)) correctly identifies only users with zero verified registrations across all organizations. This ensures a user verified in one org but unverified in another won't be deleted.openwisp_radius/tests/test_api/test_api.py (4)
162-165: Correct org-scoped verification check.The assertion now explicitly retrieves the
RegisteredUserforself.default_org, ensuring the test validates the correct organization-specific verification state.
325-334: Appropriate query optimization for multi-tenant model.Switching from
select_related("registered_user")toprefetch_related("registered_users")correctly handles the new ForeignKey relationship where users can have multiple registration records.
339-345: Serializer context correctly provides organization.The mocked view with
organizationattribute ensuresRadiusUserSerializercan perform org-scoped lookups. The subsequentregistered_users.get(organization=self.default_org)is consistent with this context.
358-359: Consistent org-scoped access pattern.Using
registered_user.is_verifiedandregistered_user.methodfrom the explicitly fetched org-specific record matches the serializer's behavior being tested.openwisp_radius/tests/test_tasks.py (3)
142-142: Correct bulk update for multi-tenant model.Using
user.registered_users.update(...)properly updates all registration records for each user, which is appropriate for the test scenario.
321-350: RegisteredUser fixtures properly scoped to organization.Each
RegisteredUser.objects.createcall now explicitly includesorganization=self.default_organd the appropriatemethodvalue, correctly setting up the multi-tenant test scenario.
359-390: Assertions correctly verify org-scoped registration state.The assertions now use
registered_users.get(organization=self.default_org).is_verifiedto check the verification status for the specific organization, properly testing the multi-tenant behavior where onlymobile_registered_usershould be unverified.
| def get_is_verified(self, obj): | ||
| try: | ||
| value = "yes" if obj.registered_user.is_verified else "no" | ||
| if not obj.registered_users.exists(): | ||
| value = "unknown" | ||
| elif obj.registered_users.filter(is_verified=True).exists(): | ||
| value = "yes" | ||
| else: | ||
| value = "no" |
There was a problem hiding this comment.
Avoid N+1 queries in get_is_verified.
This now does up to two registered_users queries per user row, and the old list_select_related optimization is gone. On the changelist that becomes O(n) extra queries and will noticeably slow large user tables. Prefetch or annotate the verification state in UserAdmin.get_queryset() and read it here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/admin.py` around lines 559 - 566, get_is_verified currently
triggers up to two DB queries per row by calling obj.registered_users.exists()
and obj.registered_users.filter(...).exists(); instead, annotate or prefetch the
verification state in UserAdmin.get_queryset() (e.g., add an Exists subquery or
a Prefetch that computes has_registered and is_verified flags) and then read
those annotated/prefetched attributes inside get_is_verified (referencing
get_is_verified and UserAdmin.get_queryset and the registered_users relation) so
get_is_verified only reads in-memory attributes and performs no extra queries.
| reg_user, __ = RegisteredUser.get_or_create_for_user_and_org( | ||
| user=user, | ||
| organization=self.organization, | ||
| defaults={ | ||
| "is_verified": True, | ||
| "method": "mobile_phone", | ||
| "is_active": True, | ||
| }, | ||
| ) | ||
| reg_user.is_verified = True | ||
| # Update username if phone_number is used as username | ||
| if user.username == user.phone_number: | ||
| user.username = phone_token.phone_number | ||
| # now that the phone number is verified | ||
| # we can write it to the user field | ||
| user.phone_number = phone_token.phone_number | ||
| user.save() | ||
| user.registered_user.save() | ||
| reg_user.save() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '\nRegisteredUser field list:\n'
sed -n '1622,1670p' openwisp_radius/base/models.py
printf '\nCall site using invalid defaults:\n'
sed -n '760,778p' openwisp_radius/api/views.pyRepository: openwisp/openwisp-radius
Length of output: 2781
🏁 Script executed:
#!/bin/bash
set -e
# Find the get_or_create_for_user_and_org method definition
printf '\nSearching for get_or_create_for_user_and_org method:\n'
rg -n "def get_or_create_for_user_and_org" openwisp_radius/
# Find where this method is used to understand the pattern
printf '\nChecking RegisteredUser model for custom manager:\n'
sed -n '1610,1630p' openwisp_radius/base/models.py
# Check if there's a custom manager or queryset implementation
printf '\nSearching for RegisteredUserQuerySet or RegisteredUserManager:\n'
rg -n "class.*RegisteredUser.*Manager\|class.*RegisteredUser.*QuerySet" openwisp_radius/Repository: openwisp/openwisp-radius
Length of output: 1062
🏁 Script executed:
#!/bin/bash
set -e
# View the get_or_create_for_user_and_org method implementation
printf '\nget_or_create_for_user_and_org method implementation:\n'
sed -n '1704,1730p' openwisp_radius/base/models.py
# Check if there's more implementation beyond line 1730
printf '\nExpanding view:\n'
sed -n '1700,1750p' openwisp_radius/base/models.pyRepository: openwisp/openwisp-radius
Length of output: 3190
🏁 Script executed:
#!/bin/bash
set -e
# Verify the exact code around the problematic call site one more time
printf '\nFull context of the phone verification code:\n'
sed -n '755,785p' openwisp_radius/api/views.py
# Check if there are any other calls to get_or_create_for_user_and_org in the codebase
printf '\nAll calls to get_or_create_for_user_and_org:\n'
rg -n "get_or_create_for_user_and_org" openwisp_radius/ -A 3Repository: openwisp/openwisp-radius
Length of output: 2770
Fix the RegisteredUser write-path after SMS verification.
Two critical bugs: defaults={"is_active": True} will crash during creation because RegisteredUser has no is_active field, and the update path never sets method="mobile_phone", leaving weak verification records unchanged after successful phone verification.
🐛 Proposed fix
reg_user, __ = RegisteredUser.get_or_create_for_user_and_org(
user=user,
organization=self.organization,
defaults={
"is_verified": True,
"method": "mobile_phone",
- "is_active": True,
},
)
reg_user.is_verified = True
+ reg_user.method = "mobile_phone"
# Update username if phone_number is used as username
if user.username == user.phone_number:
user.username = phone_token.phone_number
@@
user.phone_number = phone_token.phone_number
user.save()
- reg_user.save()
+ reg_user.save(update_fields=["is_verified", "method"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| reg_user, __ = RegisteredUser.get_or_create_for_user_and_org( | |
| user=user, | |
| organization=self.organization, | |
| defaults={ | |
| "is_verified": True, | |
| "method": "mobile_phone", | |
| "is_active": True, | |
| }, | |
| ) | |
| reg_user.is_verified = True | |
| # Update username if phone_number is used as username | |
| if user.username == user.phone_number: | |
| user.username = phone_token.phone_number | |
| # now that the phone number is verified | |
| # we can write it to the user field | |
| user.phone_number = phone_token.phone_number | |
| user.save() | |
| user.registered_user.save() | |
| reg_user.save() | |
| reg_user, __ = RegisteredUser.get_or_create_for_user_and_org( | |
| user=user, | |
| organization=self.organization, | |
| defaults={ | |
| "is_verified": True, | |
| "method": "mobile_phone", | |
| }, | |
| ) | |
| reg_user.is_verified = True | |
| reg_user.method = "mobile_phone" | |
| # Update username if phone_number is used as username | |
| if user.username == user.phone_number: | |
| user.username = phone_token.phone_number | |
| # now that the phone number is verified | |
| # we can write it to the user field | |
| user.phone_number = phone_token.phone_number | |
| user.save() | |
| reg_user.save(update_fields=["is_verified", "method"]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/api/views.py` around lines 765 - 782, The create path is
passing an invalid field and the update path fails to mark existing records as
mobile_phone-verified: remove is_active from the defaults passed to
RegisteredUser.get_or_create_for_user_and_org and include
defaults={"is_verified": True, "method": "mobile_phone"} so new records are
created correctly; after creation/update ensure you set reg_user.is_verified =
True and reg_user.method = "mobile_phone" (in addition to saving
user.phone_number and updating user.username if it matched user.phone_number)
and then call reg_user.save() to persist the updated verification method and
status.
There was a problem hiding this comment.
@coderabbitai can you give me a test scenario for these bugs?
There was a problem hiding this comment.
🧩 Analysis chain
Let me look at the existing test patterns to craft realistic test scenarios.
🏁 Script executed:
#!/bin/bash
set -e
# Look at existing phone token validation tests
printf '\n=== Existing phone token validate tests ===\n'
rg -n "validate_phone_token\|ValidatePhoneToken\|test_validate_phone_token\|test_phone_token" openwisp_radius/tests/test_api/test_phone_verification.py -A 5 | head -120
# Check what RegisteredUser fields are available
printf '\n=== RegisteredUser model fields ===\n'
rg -n "is_active\|method\|is_verified" openwisp_radius/base/models.py | grep -i "registereduser\|class Abstract" | head -30
# Check the AbstractRegisteredUser class directly
printf '\n=== AbstractRegisteredUser class ===\n'
sed -n '1600,1680p' openwisp_radius/base/models.py
# Check get_or_create_for_user_and_org implementation
printf '\n=== get_or_create_for_user_and_org ===\n'
rg -n "def get_or_create_for_user_and_org" openwisp_radius/ -A 20
Length of output: 4737
🏁 Script executed:
#!/bin/bash
set -e
# Look at the validate phone token test method
printf '\n=== Phone verification test file structure ===\n'
grep -n "def test_" openwisp_radius/tests/test_api/test_phone_verification.py | head -40
# Get the validate phone token tests
printf '\n=== Validate phone token tests ===\n'
grep -n "def test_validate\|def _test_validate\|def _create_token\|def setUp" openwisp_radius/tests/test_api/test_phone_verification.py | head -20
# Get a sample test for context
printf '\n=== Sample validate phone token test ===\n'
sed -n '1,80p' openwisp_radius/tests/test_api/test_phone_verification.py
# Find the validate phone token URL
printf '\n=== ValidatePhoneToken URL pattern ===\n'
rg -n "validate.phone.token\|validate_phone_token" openwisp_radius/api/urls.py 2>/dev/null || rg -rn "validate.phone.token" openwisp_radius/ --include="urls.py"
Length of output: 6433
🏁 Script executed:
#!/bin/bash
set -e
# Get the test_validate_phone_token_200 test
printf '\n=== test_validate_phone_token_200 ===\n'
sed -n '354,393p' openwisp_radius/tests/test_api/test_phone_verification.py
# Get the URL for validate phone token
printf '\n=== URL patterns ===\n'
rg -n "validate.phone.token\|validate_phone_token" openwisp_radius/api/urls.py
# Get the ApiTokenMixin._create_phone_token helper if any
printf '\n=== _create_phone_token or similar helper ===\n'
grep -n "_create_phone_token\|_validate_phone_token\|validate_phone_token" openwisp_radius/tests/mixins.py | head -20
sed -n '1,60p' openwisp_radius/tests/mixins.py
Length of output: 1793
🏁 Script executed:
#!/bin/bash
set -e
# Get test_create_phone_token_201 for context
printf '\n=== test_create_phone_token_201 ===\n'
sed -n '187,220p' openwisp_radius/tests/test_api/test_phone_verification.py
# Get the phone_token_validate URL name
printf '\n=== phone_token_validate URL ===\n'
rg -n "phone_token_validate\|validate_phone_token" openwisp_radius/
# Get the test_phone_change_unverifies_only_specific_org test (recent, org-aware)
printf '\n=== test_phone_change_unverifies_only_specific_org ===\n'
sed -n '962,1016p' openwisp_radius/tests/test_api/test_phone_verification.py
Length of output: 1686
@pandafy Here are concrete test scenarios for both bugs, written to match the patterns already in openwisp_radius/tests/test_api/test_phone_verification.py.
Bug 1 — is_active=True in defaults crashes when no RegisteredUser exists
get_or_create only applies the defaults dict when it must create a new row. In the normal registration flow RegisteredUser is already created, so the crash is hidden. This test exposes it by deleting the record first:
`@mock.patch`("openwisp_radius.utils.SmsMessage.send")
def test_validate_phone_token_creates_registered_user_no_prior_record(
self, send_messages_mock
):
"""
Regression: defaults={"is_active": True} crashes with TypeError when
get_or_create_for_user_and_org needs to INSERT a new RegisteredUser row,
because RegisteredUser has no is_active field.
"""
self._register_user()
user = User.objects.get(email=self._test_email)
# Delete the RegisteredUser created during registration so the
# validate path is forced to CREATE (and thus apply defaults).
user.registered_users.filter(organization=self.default_org).delete()
user_token = Token.objects.filter(user=user).last()
phone_token = PhoneToken.objects.create(
user=user,
ip="127.0.0.1",
phone_number=user.phone_number,
)
url = reverse("radius:phone_token_validate", args=[self.default_org.slug])
r = self.client.post(
url,
json.dumps({"code": phone_token.token}),
content_type="application/json",
HTTP_AUTHORIZATION=f"Bearer {user_token.key}",
)
# Without the fix this raises TypeError and returns 500.
self.assertEqual(r.status_code, 200)
reg_user = user.registered_users.get(organization=self.default_org)
self.assertTrue(reg_user.is_verified)
self.assertEqual(reg_user.method, "mobile_phone")Bug 2 — method is never updated on the existing record, leaving is_identity_verified_strong broken
When get_or_create returns an existing RegisteredUser (the common path), defaults are ignored, and since reg_user.method = "mobile_phone" is absent from the update path, the method stays at whatever it was before (e.g. "pending_verification" — which is in _weak_verification_methods). That means is_identity_verified_strong returns False even after a successful SMS verification.
`@mock.patch`("openwisp_radius.utils.SmsMessage.send")
def test_validate_phone_token_updates_method_on_existing_registered_user(
self, send_messages_mock
):
"""
Regression: after a successful phone validation the update path sets
reg_user.is_verified=True but never sets reg_user.method="mobile_phone".
If the existing RegisteredUser had method="pending_verification" the record
stays weakly verified and is_identity_verified_strong remains False.
"""
self._register_user()
user = User.objects.get(email=self._test_email)
# Simulate the user having joined via ObtainAuthTokenView which creates
# RegisteredUser(method="pending_verification", is_verified=False).
user.registered_users.filter(organization=self.default_org).update(
method="pending_verification",
is_verified=False,
)
user_token = Token.objects.filter(user=user).last()
phone_token = PhoneToken.objects.create(
user=user,
ip="127.0.0.1",
phone_number=user.phone_number,
)
url = reverse("radius:phone_token_validate", args=[self.default_org.slug])
r = self.client.post(
url,
json.dumps({"code": phone_token.token}),
content_type="application/json",
HTTP_AUTHORIZATION=f"Bearer {user_token.key}",
)
self.assertEqual(r.status_code, 200)
user.refresh_from_db()
reg_user = user.registered_users.get(organization=self.default_org)
# Without the fix: is_verified=True but method is still "pending_verification"
self.assertTrue(reg_user.is_verified)
self.assertEqual(reg_user.method, "mobile_phone")
# The critical downstream effect: strong verification must hold
self.assertTrue(reg_user.is_identity_verified_strong)Why existing tests don't catch these:
| Scenario | What the happy path does | Why bug is hidden |
|---|---|---|
Bug 1 (is_active) |
_register_user() creates RegisteredUser first; get_or_create finds it → defaults never applied |
No crash |
Bug 2 (method not set) |
Registration sets method="mobile_phone" already; so even without the assignment, the assertion passes |
Test passes accidentally |
Both tests fit directly into TestPhoneVerification in test_phone_verification.py alongside the existing test_validate_phone_token_200.
🧠 Learnings used
Learnt from: CR
Repo: openwisp/openwisp-radius PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:08:04.331Z
Learning: Bug fixes must include a regression test that reproduces the bug scenario
There was a problem hiding this comment.
Bug 2 — method is never updated on the existing record, leaving is_identity_verified_strong broken
@coderabbitai when the code reaches at this point, then the user has already verified the that they have entered the correct PhoneToken. They will only get the phone token if they have registered using their mobile number, which means the RegisteredUser.method already has "mobile_phone" in the datbase.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
# Check CreatePhoneTokenView for any method-based restriction
printf '\n=== CreatePhoneTokenView.create ===\n'
sed -n '470,530p' openwisp_radius/api/views.py
# Check ObtainAuthTokenView.validate_membership - the new code that creates pending_verification
printf '\n=== ObtainAuthTokenView.validate_membership ===\n'
rg -n "pending_verification\|validate_membership" openwisp_radius/api/views.py -A 5
# Check is_identity_verified_strong to see if pending_verification is treated as weak
printf '\n=== is_identity_verified_strong or _weak_verification_methods ===\n'
rg -n "_weak_verification_methods\|is_identity_verified_strong\|pending_verification" openwisp_radius/base/models.py -A 3
Length of output: 2033
🏁 Script executed:
#!/bin/bash
set -e
# Find CreatePhoneTokenView in the file
printf '\n=== CreatePhoneTokenView line numbers ===\n'
rg -n "class CreatePhoneTokenView\|def create" openwisp_radius/api/views.py | head -20
# Find ObtainAuthTokenView.validate_membership
printf '\n=== ObtainAuthTokenView.validate_membership ===\n'
rg -n "pending_verification" openwisp_radius/api/views.py -B 5 -A 5
# Check _weak_verification_methods in models
printf '\n=== _weak_verification_methods ===\n'
rg -n "_weak_verification_methods\|is_identity_verified_strong" openwisp_radius/base/models.py -A 5 | head -60
Length of output: 1519
🏁 Script executed:
#!/bin/bash
set -e
# Broaden search for weak verification methods
printf '\n=== weak verification methods ===\n'
rg -n "weak\|pending_verification\|is_identity_verified_strong" openwisp_radius/base/models.py | head -40
# Check what CreatePhoneTokenView.create actually does - find the class first
printf '\n=== CreatePhoneTokenView location ===\n'
rg -n "class CreatePhoneTokenView" openwisp_radius/api/views.py
# Check IsSmsVerificationEnabled permission - does it gate by method?
printf '\n=== IsSmsVerificationEnabled ===\n'
rg -n "class IsSmsVerificationEnabled\|IsSmsVerificationEnabled" openwisp_radius/api/permissions.py -A 10
Length of output: 139
🏁 Script executed:
#!/bin/bash
set -e
# Read CreatePhoneTokenView.create directly
printf '\n=== CreatePhoneTokenView.create (lines 615-680) ===\n'
sed -n '615,680p' openwisp_radius/api/views.py
# Read is_identity_verified_strong from models
printf '\n=== is_identity_verified_strong in models ===\n'
grep -n "is_identity_verified_strong\|_weak_verification\|pending_verification" openwisp_radius/base/models.py | head -30
Length of output: 2906
@pandafy You're correct for the traditional registration path — a user who registers via mobile phone will already have method="mobile_phone" in the database, so the update path not explicitly setting it doesn't matter in that case.
However, there's a new code path introduced by this very PR that creates a RegisteredUser(method="pending_verification"), and that's where the bug surfaces:
- A user already exists in Org A with
method="mobile_phone". - They log in to Org B via
ObtainAuthTokenView.validate_membership(Line 347–351) creates a newRegisteredUserwithmethod="pending_verification"for Org B. - Org B requires SMS verification.
CreatePhoneTokenView.createhas no method-based restriction — it only gates on_validate_already_verified(which checksis_verified, notmethod), so the user can get a phone token. - User validates the phone token.
get_or_create_for_user_and_orgfinds the existingRegisteredUser(method="pending_verification")for Org B — it's aget, sodefaultsare ignored. - Without
reg_user.method = "mobile_phone"in the update path,methodstays as"pending_verification". - This matters because (Line 1671 in
base/models.py):So_weak_verification_methods = {"", "email", "pending_verification"}
is_identity_verified_strongreturnsFalseeven after a successful SMS verification, leaving the user unable to authenticate.
The original registration path is fine as you say. The bug only occurs in the cross-org join path via ObtainAuthTokenView.validate_membership, which is new in this PR. The proposed fix — adding reg_user.method = "mobile_phone" to the update path — handles both paths safely.
| registered_user, created = RegisteredUser.get_or_create_for_user_and_org( | ||
| user=user, | ||
| organization=self.organization, | ||
| defaults={ | ||
| "method": "manual", | ||
| "is_verified": self.organization.radius_settings.needs_identity_verification, | ||
| }, | ||
| ) | ||
| if ( | ||
| not created | ||
| and self.organization.radius_settings.needs_identity_verification | ||
| ): | ||
| registered_user.is_verified = True | ||
| registered_user.save() | ||
| registered_user.save() |
There was a problem hiding this comment.
Update the method when reusing an existing registration row.
If this path reuses an existing org-scoped RegisteredUser with method="" or "email", it only flips is_verified=True. The row still fails is_identity_verified_strong, so a batch-imported/manual-verified user can remain blocked in orgs that require strong verification.
🔧 Proposed fix
if (
not created
and self.organization.radius_settings.needs_identity_verification
):
registered_user.is_verified = True
- registered_user.save()
+ registered_user.method = "manual"
+ registered_user.save(update_fields=["is_verified", "method"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/base/models.py` around lines 1061 - 1074, When reusing an
existing RegisteredUser row from RegisteredUser.get_or_create_for_user_and_org
you only set registered_user.is_verified=True which leaves its method as "" or
"email" and still failing is_identity_verified_strong; update the reuse branch
to also set registered_user.method="manual" (or upgrade it when it's weaker than
"manual") before saving so the row becomes strongly verified for orgs that
require strong verification (modify the block that checks not created and
self.organization.radius_settings.needs_identity_verification to update
registered_user.method and registered_user.is_verified then call
registered_user.save()).
There was a problem hiding this comment.
@coderabbitai can you give me a test scenario?
There was a problem hiding this comment.
@pandafy Here is a concrete test scenario that demonstrates the failure:
def test_save_user_strong_verification_with_existing_weak_registered_user(self):
"""
Regression: save_user() must update `method` to "manual" when reusing
an existing RegisteredUser that has a weak method (e.g. "" or "email"),
so that is_identity_verified_strong returns True for orgs that require
strong identity verification.
"""
from openwisp_radius.utils import load_model
RegisteredUser = load_model("RegisteredUser")
# 1. An org that requires identity verification
org = self._get_org()
radius_settings = org.radius_settings
radius_settings.needs_identity_verification = True
radius_settings.save()
# 2. A user who already has a RegisteredUser row for that org
# with a *weak* method (e.g. "email" — common after self-registration)
user = self._create_user()
RegisteredUser.objects.create(
user=user,
organization=org,
method="email", # <-- weak method; also try method="" for the other case
is_verified=False,
)
# 3. An operator runs a batch import for that same org/user
batch = self._create_radius_batch(
strategy="prefix",
prefix="testbatch",
organization=org,
)
batch.save_user(user) # triggers get_or_create_for_user_and_org (created=False)
# 4. Reload from DB and assert strong verification
reg_user = RegisteredUser.objects.get(user=user, organization=org)
self.assertTrue(reg_user.is_verified)
# ↓ FAILS with current code — method is still "email", so
# is_identity_verified_strong returns False
self.assertEqual(reg_user.method, "manual")
self.assertTrue(reg_user.is_identity_verified_strong)What the test exposes:
| State | is_verified |
method |
is_identity_verified_strong |
|---|---|---|---|
Before save_user |
False |
"email" |
False |
After save_user (current code) |
True |
"email" ← not updated |
False ← still blocked |
After save_user (with fix) |
True |
"manual" |
True ✅ |
The same scenario applies when method="" (e.g. a user whose registration was never completed). The proposed fix adding registered_user.method = "manual" before the save resolves both cases.
There was a problem hiding this comment.
@coderabbitai in your test case, an existing user was adding to creation of new RadiuBatch. Is this possible in real wold scenario?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| users_without_registereduser_query = OrganizationUser.objects.filter( | ||
| user__registered_user__isnull=True | ||
| user__registered_users__isnull=True | ||
| ) |
There was a problem hiding this comment.
Scope the “unspecified” org metric to the same organization.
user__registered_users__isnull=True only catches users with no RegisteredUser anywhere. After this PR, a user can have a registration in org A and none in org B; this query will exclude that user from org B’s fallback bucket, so per-org signup metrics become inaccurate in multi-org setups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/integrations/monitoring/tasks.py` around lines 133 - 135, The
current users_without_registereduser_query on OrganizationUser uses
user__registered_users__isnull=True which ignores organization scope; change it
to only select OrganizationUser rows where there is no RegisteredUser for that
same organization (e.g. replace the isnull filter with an Exists/Subquery check:
use RegisteredUser.objects.filter(user=OuterRef('user'),
organization=OuterRef('organization')) and wrap with ~Exists or annotate a
boolean and filter False). Update the users_without_registereduser_query
variable (and imports of Exists/OuterRef/Subquery if needed) so the fallback
"unspecified" metric counts users missing a registration in that specific
OrganizationUser.organization, not globally.
| from openwisp_radius.registration import ( | ||
| REGISTRATION_METHOD_CHOICES, | ||
| get_registration_choices, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Runtime imports from application code in migration.
Importing REGISTRATION_METHOD_CHOICES and get_registration_choices from openwisp_radius.registration creates a runtime dependency. If registration.py is refactored, this migration may break. Since RegisteredUserNew is a temporary table and CharField choices don't affect DB schema, consider removing or freezing these choices.
♻️ Suggested change
Either remove the choices parameter entirely from the temporary model's method field, or inline a frozen tuple:
-from openwisp_radius.registration import (
- REGISTRATION_METHOD_CHOICES,
- get_registration_choices,
-)
+# Frozen choices for migration stability
+_FROZEN_METHOD_CHOICES = [
+ ("", "Unspecified"),
+ ("manual", "Manually created"),
+ ("email", "Email"),
+ ("mobile_phone", "Mobile phone"),
+]Then use _FROZEN_METHOD_CHOICES in the field definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py`
around lines 18 - 21, The migration imports REGISTRATION_METHOD_CHOICES and
get_registration_choices at runtime which can break later; update the migration
so RegisteredUserNew.method does not depend on application imports by either
removing the choices argument from the CharField entirely or replacing it with
an inline frozen tuple (e.g., _FROZEN_METHOD_CHOICES) defined inside the
migration file and then use that frozen symbol in the method field; ensure you
remove the imports of REGISTRATION_METHOD_CHOICES and get_registration_choices
so the migration is self-contained.
Flake8 E501 Line Too LongHello @pandafy, The CI build failed due to a Fix: |
- Updated the RegisteredUser model to support organization-specific records. - Changed the primary key to UUID and added organization as a nullable ForeignKey. - Modified related code across the application to handle multiple registered users per organization. - Updated tests to reflect changes in the RegisteredUser model and ensure proper functionality. - Added migration scripts to handle the transition from the old model to the new schema. Closes #692
pandafy
force-pushed
the
issues/692-different-identity-verification
branch
from
f9ac085 to
dd7228a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_radius/saml/backends.py (1)
15-33: 🛠️ Refactor suggestion | 🟠 MajorRemove unreachable exception handler.
The
except ObjectDoesNotExistblock (line 32) is now unreachable. The previous code accesseduser.registered_user.methodwhich could raiseObjectDoesNotExist, but the new code usesuser.registered_users.exclude(method="saml").exists()which returnsFalseinstead of raising an exception when no records exist.♻️ Suggested fix
if ( not app_settings.SAML_UPDATES_PRE_EXISTING_USERNAME and not user._state.adding ): - # Skip updating user's username if the user didn't signed up - # with SAML registration method. - try: - attribute_mapping = attribute_mapping.copy() - # Check if any of the user's registered_users records - # were NOT created via SAML - has_non_saml = user.registered_users.exclude(method="saml").exists() - if has_non_saml: - for key, value in attribute_mapping.items(): - if "username" in value: - break - if len(value) == 1: - attribute_mapping.pop(key, None) - else: - attribute_mapping[key] = [] - for attr in value: - if attr != "username": - attribute_mapping[key].append(attr) - - except ObjectDoesNotExist: - pass + # Skip updating user's username if the user didn't sign up + # with SAML registration method. + attribute_mapping = attribute_mapping.copy() + # Check if any of the user's registered_users records + # were NOT created via SAML + has_non_saml = user.registered_users.exclude(method="saml").exists() + if has_non_saml: + for key, value in attribute_mapping.items(): + if "username" in value: + break + if len(value) == 1: + attribute_mapping.pop(key, None) + else: + attribute_mapping[key] = [] + for attr in value: + if attr != "username": + attribute_mapping[key].append(attr) return super()._update_user(user, attributes, attribute_mapping, force_save)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/saml/backends.py` around lines 15 - 33, The catch for ObjectDoesNotExist is now unreachable because the code uses user.registered_users.exclude(method="saml").exists(), so remove the try/except wrapper around the attribute_mapping manipulation in saml/backends.py; simply run the attribute_mapping.copy() and the has_non_saml check (user.registered_users.exclude(method="saml").exists()) and the subsequent loop without catching ObjectDoesNotExist, leaving normal exceptions to surface or add a more specific guard if needed.
♻️ Duplicate comments (8)
tests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py (1)
18-21:⚠️ Potential issue | 🟡 MinorFreeze
methodchoices inside the migration to avoid runtime app coupling.Line 18–21 and Line 95–99 still make this migration depend on
openwisp_radius.registrationsymbols at runtime. That can break historical migration replay if registration code changes later. Please inline frozen choices (or removechoicesfor this temporary model) so the migration is self-contained.Suggested migration-safe diff
-from openwisp_radius.registration import ( - REGISTRATION_METHOD_CHOICES, - get_registration_choices, -) +_FROZEN_METHOD_CHOICES = [ + ("", "Unspecified"), + ("manual", "Manually created"), + ("email", "Email"), + ("mobile_phone", "Mobile phone"), + ("saml", "SAML"), + ("social_login", "Social Login"), +] @@ - choices=( - REGISTRATION_METHOD_CHOICES - if django.VERSION < (5, 0) - else get_registration_choices - ), + choices=_FROZEN_METHOD_CHOICES,Also applies to: 95-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py` around lines 18 - 21, The migration currently imports REGISTRATION_METHOD_CHOICES and get_registration_choices from openwisp_radius.registration which couples the historical migration to runtime code; to fix, freeze the choices in the migration by replacing references to REGISTRATION_METHOD_CHOICES / get_registration_choices with a literal list/tuple of (value, label) pairs (or remove the choices argument entirely) wherever the temporary model's method field is defined (see the migration file 0032_registered_user_multitenant.py and the field named method around the first import and again near lines 95–99); ensure the frozen choices are self-contained constants inside the migration so replaying the migration does not depend on external symbols.openwisp_radius/social/views.py (1)
50-59:⚠️ Potential issue | 🟠 MajorUse atomic creation for org-scoped
RegisteredUser.Line 50–59 is still a check-then-create flow and can race under concurrent requests, causing IntegrityError on the unique
(user, organization)constraint.Safer atomic pattern
- try: - user.registered_users.get(organization=org) - except RegisteredUser.DoesNotExist: - registered_user = RegisteredUser( - user=user, - organization=org, - method="social_login", - is_verified=False, - ) - registered_user.full_clean() - registered_user.save() + RegisteredUser.objects.get_or_create( + user=user, + organization=org, + defaults={"method": "social_login", "is_verified": False}, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/social/views.py` around lines 50 - 59, Replace the check-then-create pattern (user.registered_users.get(...) except RegisteredUser.DoesNotExist) with an atomic get-or-create call: use RegisteredUser.objects.get_or_create(user=user, organization=org, defaults={"method":"social_login","is_verified":False}) and handle the (instance, created) tuple; if created, run instance.full_clean() and persist or, if validation fails, remove the created instance and surface the error. Wrap the operation in transaction.atomic() and catch IntegrityError to refetch the existing RegisteredUser to avoid races on the unique (user, organization) constraint.openwisp_radius/integrations/monitoring/tasks.py (1)
133-135:⚠️ Potential issue | 🟡 MinorPer-organization signup metrics may undercount "unspecified" users in multi-org setups.
The filter
user__registered_users__isnull=Trueonly catches users with noRegisteredUserrecords at all. After this PR, a user can have aRegisteredUserin organization A but none in organization B. For organization B's metrics, such users should be counted as "unspecified", but this query excludes them because they have aregistered_usersrecord (just not for that organization).Consider filtering for users without a
RegisteredUserfor the specific organization:Proposed approach using Exists subquery
+from django.db.models import Exists, OuterRef users_without_registereduser_query = OrganizationUser.objects.filter( - user__registered_users__isnull=True + ~Exists( + RegisteredUser.objects.filter( + user_id=OuterRef("user_id"), + organization_id=OuterRef("organization_id"), + ) + ) )This checks for users without a
RegisteredUserfor that specific organization, not globally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/integrations/monitoring/tasks.py` around lines 133 - 135, The query users_without_registereduser_query on OrganizationUser incorrectly checks global absence of RegisteredUser; change it to detect users who lack a RegisteredUser for that specific Organization by replacing the current filter with an Exists/OuterRef subquery: build a subquery on RegisteredUser filtering RegisteredUser.user = OuterRef('user') and RegisteredUser.organization = OuterRef('organization'), then filter OrganizationUser with the negation of that Exists (i.e., users where no RegisteredUser exists for that organization). Update the reference in the task where users_without_registereduser_query is used so metrics count per-organization "unspecified" users correctly.openwisp_radius/api/views.py (1)
769-776:⚠️ Potential issue | 🔴 CriticalRemove the invalid
RegisteredUserdefault field.Line 775 passes
is_activeintoRegisteredUser.get_or_create_for_user_and_org(), butRegisteredUserdoes not define that field. The first phone verification for an organization with no existingRegisteredUserwill fail on the create branch instead of persisting the verification result.🐛 Proposed fix
reg_user, __ = RegisteredUser.get_or_create_for_user_and_org( user=user, organization=self.organization, defaults={ "is_verified": True, "method": "mobile_phone", - "is_active": True, }, ) reg_user.is_verified = True reg_user.method = "mobile_phone" @@ - reg_user.save() + reg_user.save(update_fields=["is_verified", "method"])Run this to verify the model fields and the offending call. Expected result: the model excerpt contains
user,organization,method,is_verified, andmodified, but nois_active.#!/bin/bash set -e printf '\nRegisteredUser fields:\n' sed -n '1623,1675p' openwisp_radius/base/models.py printf '\nValidatePhoneTokenView defaults:\n' sed -n '769,777p' openwisp_radius/api/views.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/api/views.py` around lines 769 - 776, The defaults passed to RegisteredUser.get_or_create_for_user_and_org include an invalid field "is_active" which causes creation to fail; update the call in ValidatePhoneTokenView (the reg_user, __ = RegisteredUser.get_or_create_for_user_and_org(...) invocation) by removing "is_active" from the defaults dict so only valid model fields (e.g., "is_verified" and "method") are provided, and if activation state must be set, do so using a separate update/save on the returned reg_user object after creation.openwisp_radius/admin.py (1)
559-566:⚠️ Potential issue | 🟠 MajorAvoid per-row
registered_usersqueries in the user changelist.Line 561 and Line 563 each issue an
EXISTSquery on the reverse FK, so the changelist now does up to two extra queries per user row. Prefetch or annotate the verification state inUserAdmin.get_queryset()and read only in-memory attributes here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/admin.py` around lines 559 - 566, The get_is_verified method is causing two DB EXISTS calls per row; modify the UserAdmin.get_queryset to prefetch/annotate the verification state (e.g., annotate an Exists/Subquery as registered_users_verified or annotate counts/max on registered_users__is_verified, or Prefetch('registered_users') with only the needed fields) and then change get_is_verified to read that in-memory attribute (check for the annotated flag or the prefetched related list) instead of calling obj.registered_users.exists() or filter(...). Ensure you reference get_is_verified to read the new annotated field name (e.g., registered_users_verified) or the prefetched relation so no per-row queries occur.openwisp_radius/migrations/__init__.py (1)
12-18:⚠️ Potential issue | 🟠 MajorVersion this migration logic instead of placing it in
migrations/__init__.py.Released migrations import helpers from this live module. Any future edit here silently rewrites already-shipped migration behavior. Move these CTCR/multitenant helpers into the consuming migration files or into a versioned module that will never be edited again.
Also applies to: 48-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/migrations/__init__.py` around lines 12 - 18, The constants BATCH_SIZE and REGISTERED_USER_ORGANIZATION_HELP_TEXT (and any CTCR/multitenant helpers currently defined in migrations/__init__.py) must be removed from this live importable module and instead be versioned with the migrations that use them: either copy the helpers/constants directly into the specific migration files that reference them or create a new module with a versioned name (e.g., migrations/_vYYYYMMDD_helpers.py) that will never be edited after release; update the consuming migration files to import from that new versioned module (or their local copies) so released migrations do not depend on a mutable top-level migrations.__init__.openwisp_radius/base/models.py (1)
1070-1075:⚠️ Potential issue | 🟠 MajorUpgrade reused registrations to a strong method.
If this branch reuses an existing org-scoped row, it only flips
is_verified. Rows that already havemethod=""or"email"still failis_identity_verified_strong, so batch-imported users can remain blocked in orgs that require strong verification.Suggested fix
if ( not created - and self.organization.radius_settings.needs_identity_verification + and radius_settings.needs_identity_verification ): - registered_user.is_verified = True - registered_user.save() + update_fields = [] + if not registered_user.is_verified: + registered_user.is_verified = True + update_fields.append("is_verified") + if registered_user.method in {"", "email"}: + registered_user.method = "manual" + update_fields.append("method") + if update_fields: + registered_user.save(update_fields=update_fields)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/base/models.py` around lines 1070 - 1075, When reusing an existing organization-scoped RegisteredUser row in the branch guarded by organization.radius_settings.needs_identity_verification, also upgrade its verification method to a strong value so is_identity_verified_strong will pass; in the block that currently sets registered_user.is_verified = True and calls registered_user.save(), additionally set registered_user.method to the strong verification method string used by your model (the value expected by is_identity_verified_strong) before saving (refer to registered_user, is_identity_verified_strong and organization.radius_settings.needs_identity_verification to locate the code).openwisp_radius/migrations/0043_registereduser_add_uuid.py (1)
11-14:⚠️ Potential issue | 🟠 MajorKeep this migration independent from
openwisp_radius.registration.This temporary model still imports live choice definitions from application code. A later refactor in
registration.pycan break fresh installs, and thechoicesargument does not affect the schema of this temporary table anyway. Freeze the value locally or dropchoicesfromRegisteredUserNew.method.Also applies to: 88-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/migrations/0043_registereduser_add_uuid.py` around lines 11 - 14, The migration currently imports REGISTRATION_METHOD_CHOICES and get_registration_choices from openwisp_radius.registration which couples this migration to live app code; update the migration so the temporary model RegisteredUserNew (specifically the field RegisteredUserNew.method) does not import or reference REGISTRATION_METHOD_CHOICES/get_registration_choices — either embed/freeze the choice tuples directly in the migration as a literal list or remove the choices= argument entirely from RegisteredUserNew.method so the migration schema is independent of registration.py changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_radius/api/views.py`:
- Around line 339-349: Wrap the two writes that create OrganizationUser and
RegisteredUser in a single database transaction and make creation idempotent:
use transaction.atomic() around the block that uses OrganizationUser and
RegisteredUser, replace the direct OrganizationUser(...) / save() with an
idempotent get_or_create (e.g. OrganizationUser.objects.get_or_create(user=user,
organization=self.organization, defaults={...}) and only run full_clean when
created), and similarly use get_or_create for RegisteredUser; also handle
IntegrityError as a fallback to query the existing record so concurrent
first-login requests don't raise duplicate-row errors or leave partial state.
Ensure you reference OrganizationUser, RegisteredUser, self.organization and
user in the changed code.
In `@openwisp_radius/migrations/__init__.py`:
- Around line 129-175: The migration currently clones RegisteredUser rows with
organization=None into per-organization rows and then deletes the original
global row (the to_delete_pks logic), which breaks
AbstractRegisteredUser.get_global_or_org_specific; to fix it, stop removing the
legacy global record: remove the line that appends registered_user.pk to
to_delete_pks (and the later delete of RegisteredUser objects) so the original
organization=None row is retained as the global fallback; keep the rest of the
logic that builds to_create and uses _registered_user_extra_kwargs and
_flush_bulk_create, and ensure existing_pairs logic still prevents duplicate
(user_id, organization_id) inserts.
- Around line 194-242: The rollback currently deletes all org-scoped rows
unconditionally and skips recreating a global if one exists (existing_globals),
which can silently drop a stronger verification; change the loop that processes
org_records to, for each registered_user, if registered_user.user_id is in
existing_globals then load that existing global RegisteredUser
(organization__isnull=True) and compare its strength to the org-scoped candidate
using the same method_priority / is_verified / modified criteria used in
org_records; if the existing global is weaker, update that global record’s
fields (is_verified, method, modified and any extra fields from
_registered_user_extra_kwargs) instead of deleting the org row, otherwise skip
creating/updating and allow deletion; adjust to_delete_pks so you only append
pks for rows you intend to remove and perform either a bulk update of existing
globals or a bulk_create for new globals as appropriate.
In `@openwisp_radius/tests/test_admin.py`:
- Around line 1410-1422: The test
test_user_admin_shows_multiple_registered_user_records currently uses a loose
assertion self.assertIn('value="3"', response.rendered_content) which can match
unrelated fields; update it to assert the specific management-form input for the
inline formset (RegisteredUser) by checking the exact input element (e.g., the
hidden TOTAL_FORMS input for the registered_users inline) in
response.rendered_content so the assertion only passes when the
registered_users-TOTAL_FORMS value is "3".
---
Outside diff comments:
In `@openwisp_radius/saml/backends.py`:
- Around line 15-33: The catch for ObjectDoesNotExist is now unreachable because
the code uses user.registered_users.exclude(method="saml").exists(), so remove
the try/except wrapper around the attribute_mapping manipulation in
saml/backends.py; simply run the attribute_mapping.copy() and the has_non_saml
check (user.registered_users.exclude(method="saml").exists()) and the subsequent
loop without catching ObjectDoesNotExist, leaving normal exceptions to surface
or add a more specific guard if needed.
---
Duplicate comments:
In `@openwisp_radius/admin.py`:
- Around line 559-566: The get_is_verified method is causing two DB EXISTS calls
per row; modify the UserAdmin.get_queryset to prefetch/annotate the verification
state (e.g., annotate an Exists/Subquery as registered_users_verified or
annotate counts/max on registered_users__is_verified, or
Prefetch('registered_users') with only the needed fields) and then change
get_is_verified to read that in-memory attribute (check for the annotated flag
or the prefetched related list) instead of calling obj.registered_users.exists()
or filter(...). Ensure you reference get_is_verified to read the new annotated
field name (e.g., registered_users_verified) or the prefetched relation so no
per-row queries occur.
In `@openwisp_radius/api/views.py`:
- Around line 769-776: The defaults passed to
RegisteredUser.get_or_create_for_user_and_org include an invalid field
"is_active" which causes creation to fail; update the call in
ValidatePhoneTokenView (the reg_user, __ =
RegisteredUser.get_or_create_for_user_and_org(...) invocation) by removing
"is_active" from the defaults dict so only valid model fields (e.g.,
"is_verified" and "method") are provided, and if activation state must be set,
do so using a separate update/save on the returned reg_user object after
creation.
In `@openwisp_radius/base/models.py`:
- Around line 1070-1075: When reusing an existing organization-scoped
RegisteredUser row in the branch guarded by
organization.radius_settings.needs_identity_verification, also upgrade its
verification method to a strong value so is_identity_verified_strong will pass;
in the block that currently sets registered_user.is_verified = True and calls
registered_user.save(), additionally set registered_user.method to the strong
verification method string used by your model (the value expected by
is_identity_verified_strong) before saving (refer to registered_user,
is_identity_verified_strong and
organization.radius_settings.needs_identity_verification to locate the code).
In `@openwisp_radius/integrations/monitoring/tasks.py`:
- Around line 133-135: The query users_without_registereduser_query on
OrganizationUser incorrectly checks global absence of RegisteredUser; change it
to detect users who lack a RegisteredUser for that specific Organization by
replacing the current filter with an Exists/OuterRef subquery: build a subquery
on RegisteredUser filtering RegisteredUser.user = OuterRef('user') and
RegisteredUser.organization = OuterRef('organization'), then filter
OrganizationUser with the negation of that Exists (i.e., users where no
RegisteredUser exists for that organization). Update the reference in the task
where users_without_registereduser_query is used so metrics count
per-organization "unspecified" users correctly.
In `@openwisp_radius/migrations/__init__.py`:
- Around line 12-18: The constants BATCH_SIZE and
REGISTERED_USER_ORGANIZATION_HELP_TEXT (and any CTCR/multitenant helpers
currently defined in migrations/__init__.py) must be removed from this live
importable module and instead be versioned with the migrations that use them:
either copy the helpers/constants directly into the specific migration files
that reference them or create a new module with a versioned name (e.g.,
migrations/_vYYYYMMDD_helpers.py) that will never be edited after release;
update the consuming migration files to import from that new versioned module
(or their local copies) so released migrations do not depend on a mutable
top-level migrations.__init__.
In `@openwisp_radius/migrations/0043_registereduser_add_uuid.py`:
- Around line 11-14: The migration currently imports REGISTRATION_METHOD_CHOICES
and get_registration_choices from openwisp_radius.registration which couples
this migration to live app code; update the migration so the temporary model
RegisteredUserNew (specifically the field RegisteredUserNew.method) does not
import or reference REGISTRATION_METHOD_CHOICES/get_registration_choices —
either embed/freeze the choice tuples directly in the migration as a literal
list or remove the choices= argument entirely from RegisteredUserNew.method so
the migration schema is independent of registration.py changes.
In `@openwisp_radius/social/views.py`:
- Around line 50-59: Replace the check-then-create pattern
(user.registered_users.get(...) except RegisteredUser.DoesNotExist) with an
atomic get-or-create call: use RegisteredUser.objects.get_or_create(user=user,
organization=org, defaults={"method":"social_login","is_verified":False}) and
handle the (instance, created) tuple; if created, run instance.full_clean() and
persist or, if validation fails, remove the created instance and surface the
error. Wrap the operation in transaction.atomic() and catch IntegrityError to
refetch the existing RegisteredUser to avoid races on the unique (user,
organization) constraint.
In
`@tests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py`:
- Around line 18-21: The migration currently imports REGISTRATION_METHOD_CHOICES
and get_registration_choices from openwisp_radius.registration which couples the
historical migration to runtime code; to fix, freeze the choices in the
migration by replacing references to REGISTRATION_METHOD_CHOICES /
get_registration_choices with a literal list/tuple of (value, label) pairs (or
remove the choices argument entirely) wherever the temporary model's method
field is defined (see the migration file 0032_registered_user_multitenant.py and
the field named method around the first import and again near lines 95–99);
ensure the frozen choices are self-contained constants inside the migration so
replaying the migration does not depend on external symbols.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 66cc1f42-7971-441f-a202-c28af6013750
📒 Files selected for processing (36)
.github/workflows/ci.ymlopenwisp_radius/admin.pyopenwisp_radius/api/freeradius_views.pyopenwisp_radius/api/serializers.pyopenwisp_radius/api/utils.pyopenwisp_radius/api/views.pyopenwisp_radius/base/admin_filters.pyopenwisp_radius/base/models.pyopenwisp_radius/integrations/monitoring/tasks.pyopenwisp_radius/integrations/monitoring/tests/test_metrics.pyopenwisp_radius/management/commands/base/delete_unverified_users.pyopenwisp_radius/migrations/0043_registereduser_add_uuid.pyopenwisp_radius/migrations/0044_registered_user_multitenant_data.pyopenwisp_radius/migrations/0045_registered_user_multitenant_constraints.pyopenwisp_radius/migrations/__init__.pyopenwisp_radius/saml/backends.pyopenwisp_radius/saml/views.pyopenwisp_radius/settings.pyopenwisp_radius/social/views.pyopenwisp_radius/tests/mixins.pyopenwisp_radius/tests/test_admin.pyopenwisp_radius/tests/test_api/test_api.pyopenwisp_radius/tests/test_api/test_freeradius_api.pyopenwisp_radius/tests/test_api/test_phone_verification.pyopenwisp_radius/tests/test_api/test_rest_token.pyopenwisp_radius/tests/test_batch_add_users.pyopenwisp_radius/tests/test_commands.pyopenwisp_radius/tests/test_models.pyopenwisp_radius/tests/test_saml/test_views.pyopenwisp_radius/tests/test_selenium.pyopenwisp_radius/tests/test_social.pyopenwisp_radius/tests/test_tasks.pyopenwisp_radius/tests/test_token.pyopenwisp_radius/tests/test_users_integration.pyrunteststests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Kilo Code Review
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-06T08:47:54.428Z
Learnt from: stktyagi
Repo: openwisp/openwisp-radius PR: 689
File: .github/workflows/bot-ci-failure.yml:60-60
Timestamp: 2026-03-06T08:47:54.428Z
Learning: In openwisp/openwisp-radius and similar OpenWISP repos, reusable workflow caller files intentionally reference upstream reusable workflows with master (e.g., openwisp/openwisp-utils/.github/workflows/reusable-bot-ci-failure.ymlmaster). This is by design to ensure callers always use the latest changes from the master branch of the upstream project. Do not flag such references as security or pinning issues during code reviews. If applying this pattern broadly, treat any workflow call that uses a remote reusable workflow from a different repository with master as an intentional design choice rather than a vulnerability.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-03-24T21:08:04.331Z
Learnt from: CR
Repo: openwisp/openwisp-radius PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:08:04.331Z
Learning: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior
Applied to files:
runtestsopenwisp_radius/tests/test_api/test_phone_verification.py
🔇 Additional comments (26)
runtests (1)
5-7: Good improvement to test-run observability.Keeping stdout visible in the standard coverage run makes CI/debug output actionable, while preserving the fallback path.
openwisp_radius/tests/test_selenium.py (1)
23-25: Correct isolation for Selenium suite execution.Adding
@tag("no_parallel")toBasicTestis the right move to keep browser tests out of parallel workers and reduce flakiness.openwisp_radius/tests/test_saml/test_views.py (1)
155-157: Good org-scoped test fixture update.This correctly creates the registration record for the target organization and matches the multitenant model behavior.
openwisp_radius/tests/test_batch_add_users.py (1)
146-148: Assertion now targets the correct organization-scoped record.This aligns the test with the new
(user, organization)registration model and avoids false positives from unrelated records.openwisp_radius/tests/mixins.py (1)
100-103: Inline management-form prefix fix is correct.Using
registered_users-*matches the current related name and prevents admin inline formset mismatches.openwisp_radius/tests/test_token.py (1)
68-71: Correct org-aware fixture for verification test.This update matches the new organization-scoped RegisteredUser lookup path.
openwisp_radius/integrations/monitoring/tests/test_metrics.py (1)
25-29: Helper default now correctly reflects org-scoped registrations.Defaulting
organizationhere improves test consistency while preserving override behavior.openwisp_radius/saml/views.py (1)
74-84: Race condition on concurrent SAML logins remains unaddressed.The read-then-create pattern (
get()followed bysave()) is susceptible to race conditions when two concurrent SAML logins occur for the same user and organization. This can raise anIntegrityErroron the unique(user, organization)constraint.Wrap the creation in a try/except for
IntegrityErroror useget_or_createwith appropriate defaults.openwisp_radius/base/admin_filters.py (1)
16-23: LGTM!The filter correctly uses the plural
registered_usersrelationship and adds.distinct()to prevent duplicate rows from the join. The filter semantics are appropriate for the admin list view.openwisp_radius/tests/test_social.py (1)
104-111: LGTM!The test correctly queries the organization-scoped
RegisteredUserviauser.registered_users.get(organization=self.default_org)and handles the specificRegisteredUser.DoesNotExistexception.openwisp_radius/tests/test_admin.py (1)
1424-1487: LGTM!The test setup correctly creates organization-scoped
RegisteredUserrecords to properly exercise the admin filtering and display logic for the multi-tenancy model.openwisp_radius/management/commands/base/delete_unverified_users.py (1)
37-53: LGTM!The refactored query correctly handles multi-tenant users by annotating the count of verified registrations and filtering for users with
num_verified=0. This ensures users verified in any organization are not deleted, addressing the previous review concern.openwisp_radius/tests/test_users_integration.py (1)
96-119: LGTM!The test correctly creates an organization-scoped
RegisteredUserand validates the updated CSV export format that includes organization-specific registration data as a tuple string.openwisp_radius/tests/test_api/test_freeradius_api.py (2)
209-289: LGTM!The new tests provide comprehensive coverage for the organization-scoped verification logic:
test_authorize_verified_user: Verifies both org-specific and global fallback authorization pathstest_multi_org_user_different_verification_states: Confirms verification is isolated per organizationtest_global_fallback_for_orgs_without_specific_records: Validates global records serve as fallback across all organizationsThese tests align well with the
_get_user_query_conditionsimplementation infreeradius_views.py.
343-343: No concerns with this formatting change.openwisp_radius/api/freeradius_views.py (2)
293-293: LGTM -.distinct()prevents duplicate users from joined registered_users.The addition of
.distinct()correctly handles the case where a user may have multipleRegisteredUserrecords (org-specific and global), which would otherwise cause duplicate rows in the join.
412-429: LGTM - Organization-scoped identity verification logic is correct.The org-or-global predicate pattern (
registered_users__organization_id=org_id | registered_users__organization__isnull=True) correctly filters for users who have a verifiedRegisteredUsereither for the specific organization or globally. This aligns with the multitenant model design where:
- Org-specific records take precedence when present
- Global records (
organization=None) serve as fallbackThe same pattern is correctly applied to the
authorize_unverifiedcondition.openwisp_radius/settings.py (1)
235-243: LGTM - Export configuration correctly updated for multitenant RegisteredUser.The changes properly reflect the model updates:
- Using
"registered_users"(plural) matches the newrelated_nameon the ForeignKey- Switching to
prefetch_relatedis correct since users can now have multipleRegisteredUserrecords- The nested fields configuration (
organization_id,method,is_verified) captures the relevant data for each registration recordopenwisp_radius/tests/test_commands.py (2)
279-282: LGTM - Deterministic organization-scoped lookup.Using
.get(organization=self.default_org)ensures the test deterministically targets the correctRegisteredUserrecord for the default organization, avoiding ambiguity when multiple records exist.
377-406: LGTM - Valuable test for cross-organization verification isolation.This subtest properly validates that
delete_unverified_usersrespects per-organization verification status. A user with an unverified record in one org but a verified record in another org should not be deleted, which aligns with the PR's objective of organization-scoped verification.openwisp_radius/tests/test_models.py (1)
1222-1279: LGTM - Comprehensive test coverage for RegisteredUser model methods.The
TestRegisteredUserclass provides thorough coverage of the organization-scoped registration logic:
get_global_or_org_specific: Tests all key scenarios including no records, global fallback, org-specific preference, and the important case where org-specific is returned even when unverified (while global is verified).
cleanvalidation: Tests prevent duplicate records at both org-specific and global levels, matching theUniqueConstraints defined in the model.These tests properly validate the core multitenant behavior that other parts of the codebase depend on.
openwisp_radius/tests/test_api/test_phone_verification.py (2)
222-243: LGTM - Comprehensive "already verified" test coverage.The subtests properly verify that phone token creation is blocked when either:
- An org-specific verified
RegisteredUserexists for the target organization- A global verified
RegisteredUserexists (fallback case)This aligns with the
_validate_already_verifiedlogic in the model that usesget_global_or_org_specific.
960-1007: LGTM - Valuable test for organization-scoped phone verification unverification.This test validates a critical security requirement: changing a phone number only unverifies the
RegisteredUserfor the specific organization where the change occurs, not affecting other organizations' verification status. This ensures proper isolation in multi-org deployments.openwisp_radius/api/utils.py (1)
34-48: LGTM - Clean implementation of organization-scoped identity verification.The method correctly:
- Iterates over
.all()to leverage the prefetch cache (avoiding N+1 queries)- Prefers org-specific
RegisteredUser(breaks early when found)- Falls back to global record (
organization_id is None)- Returns
Falsewhen no matching record existsThis pattern is consistent with the approach used in
RadiusUserSerializer(context snippet 1) and properly handles the org-vs-global precedence.openwisp_radius/migrations/0045_registered_user_multitenant_constraints.py (1)
1-25: LGTM - Migration correctly adds multitenant constraints.The migration properly:
- Depends on the data migration (
0044) to ensure existing data is valid before constraints are applied- Adds both
UniqueConstraints matching the model definition:
unique_registered_user_per_org: Ensures oneRegisteredUserper (user, organization) pairunique_global_registered_user: Ensures at most one global (organization=NULL) record per user via conditional constraintopenwisp_radius/integrations/monitoring/tasks.py (1)
185-200: LGTM - Org-specific preference correctly implemented via ordering.The query now uses
.order_by("-organization_id")which ensures org-specific records (non-nullorganization_id) are returned before global records (nullorganization_id). Combined with.first(), this correctly implements the org-specific-over-global precedence pattern.
| try: | ||
| org_user = OrganizationUser( | ||
| user=user, organization=self.organization | ||
| ) | ||
| org_user.full_clean() | ||
| org_user.save() | ||
| RegisteredUser.objects.get_or_create( | ||
| user=user, | ||
| organization=self.organization, | ||
| defaults={"method": ""}, | ||
| ) |
There was a problem hiding this comment.
Make the first-login org enrollment atomic.
This is a TOCTOU path: Line 335 checks membership, then Lines 340-349 create OrganizationUser and RegisteredUser in separate writes. Two concurrent first-login requests can both pass the check and one of them will either raise an uncaught duplicate-row error or leave only one of the two records persisted. Wrap both inserts in a single transaction and make the membership creation idempotent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/api/views.py` around lines 339 - 349, Wrap the two writes
that create OrganizationUser and RegisteredUser in a single database transaction
and make creation idempotent: use transaction.atomic() around the block that
uses OrganizationUser and RegisteredUser, replace the direct
OrganizationUser(...) / save() with an idempotent get_or_create (e.g.
OrganizationUser.objects.get_or_create(user=user,
organization=self.organization, defaults={...}) and only run full_clean when
created), and similarly use get_or_create for RegisteredUser; also handle
IntegrityError as a fallback to query the existing record so concurrent
first-login requests don't raise duplicate-row errors or leave partial state.
Ensure you reference OrganizationUser, RegisteredUser, self.organization and
user in the changed code.
| existing_globals = set( | ||
| RegisteredUser.objects.filter( | ||
| user_id__in=user_id_batch, | ||
| organization__isnull=True, | ||
| ).values_list("user_id", flat=True) | ||
| ) | ||
| # Annotate each row with an explicit verification priority so that stronger | ||
| # methods (anything that is not '' or 'email') sort before weaker ones. | ||
| # Lexical ordering of 'method' would place '' first, picking the weakest. | ||
| method_priority = Case( | ||
| When(method="", then=Value(0)), | ||
| When(method="email", then=Value(1)), | ||
| default=Value(2), | ||
| output_field=IntegerField(), | ||
| ) | ||
| org_records = ( | ||
| RegisteredUser.objects.filter( | ||
| user_id__in=user_id_batch, | ||
| organization__isnull=False, | ||
| ) | ||
| .annotate(method_priority=method_priority) | ||
| .order_by("user_id", "-is_verified", "-method_priority", "pk") | ||
| ) | ||
|
|
||
| to_create = [] | ||
| to_delete_pks = [] | ||
| current_user_id = None | ||
|
|
||
| for registered_user in org_records.iterator(chunk_size=BATCH_SIZE): | ||
| to_delete_pks.append(registered_user.pk) | ||
| if registered_user.user_id == current_user_id: | ||
| continue | ||
| current_user_id = registered_user.user_id | ||
| if registered_user.user_id in existing_globals: | ||
| continue | ||
| restored = RegisteredUser( | ||
| id=uuid.uuid4(), | ||
| user_id=registered_user.user_id, | ||
| organization=None, | ||
| is_verified=registered_user.is_verified, | ||
| method=registered_user.method, | ||
| **_registered_user_extra_kwargs(registered_user, extra_fields), | ||
| ) | ||
| restored.modified = registered_user.modified | ||
| to_create.append(restored) | ||
|
|
||
| _flush_bulk_create(RegisteredUser, to_create) | ||
| if to_delete_pks: | ||
| RegisteredUser.objects.filter(pk__in=to_delete_pks).delete() |
There was a problem hiding this comment.
Rollback can silently downgrade a user to a weaker global record.
existing_globals skips recreating a global row, but this code still deletes every org-scoped row for that user. If the existing global row is older or weaker than the best org-scoped row, rollback loses the stronger verification state instead of preserving it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/migrations/__init__.py` around lines 194 - 242, The rollback
currently deletes all org-scoped rows unconditionally and skips recreating a
global if one exists (existing_globals), which can silently drop a stronger
verification; change the loop that processes org_records to, for each
registered_user, if registered_user.user_id is in existing_globals then load
that existing global RegisteredUser (organization__isnull=True) and compare its
strength to the org-scoped candidate using the same method_priority /
is_verified / modified criteria used in org_records; if the existing global is
weaker, update that global record’s fields (is_verified, method, modified and
any extra fields from _registered_user_extra_kwargs) instead of deleting the org
row, otherwise skip creating/updating and allow deletion; adjust to_delete_pks
so you only append pks for rows you intend to remove and perform either a bulk
update of existing globals or a bulk_create for new globals as appropriate.
Black Formatting ErrorsHello @pandafy, The CI build failed due to Fix: |
| self.assertContains(response, "id_registered_users-TOTAL_FORMS") | ||
| self.assertIn('value="3"', response.rendered_content) |
There was a problem hiding this comment.
Let's check for this instead
<input type="hidden" name="registered_users-INITIAL_FORMS" value="1" id="id_registered_users-INITIAL_FORMS">
| @capture_any_output() | ||
| @mock.patch("openwisp_radius.utils.SmsMessage.send") | ||
| def test_multi_org_phone_verification_flow(self, *args): |
There was a problem hiding this comment.
Check if we can shorten the code required for this test case. Utlise any pre-existing helper method.
| RegisteredUser.objects.filter(user=user, organization=org2).delete() | ||
| with self.subTest( | ||
| "Test unverified user organization does not need identity verification" | ||
| ): | ||
| registered_user.is_verified = False | ||
| registered_user.save() |
There was a problem hiding this comment.
Let's reverse this change. Set all is_verified=False to all RegisteredUser.
| with self.subTest("global verified record also blocks phone token creation"): | ||
| # Replace org-specific record with a global verified record | ||
| reg_user.delete() | ||
| RegisteredUser.objects.create( | ||
| user=token.user, organization=None, is_verified=True | ||
| ) | ||
| r = self.client.post(url, HTTP_AUTHORIZATION=f"Bearer {token.key}") | ||
| self.assertEqual(r.status_code, 400) | ||
| self.assertEqual(r.json(), {"user": "This user has been already verified."}) |
There was a problem hiding this comment.
I don't think it is correct. Only the org-specific RegisteredUser object should block verification of phone number.
| self.assertEqual(RadiusPostAuth.objects.all().count(), 0) | ||
| params = self._get_postauth_params() | ||
| post_url = f'{reverse("radius:postauth")}{self.token_querystring}' | ||
| post_url = f"{reverse('radius:postauth')}{self.token_querystring}" |
| self._get_org_user() | ||
| token_querystring = f"?token={rad.token}&uuid={str(self.org.pk)}" | ||
| post_url = f'{reverse("radius:authorize")}{token_querystring}' | ||
| post_url = f"{reverse('radius:authorize')}{token_querystring}" |
| }, | ||
| ) | ||
| reg_user.is_verified = True | ||
| reg_user.method = "mobile_phone" |
There was a problem hiding this comment.
Is this really required?
… method in cross-org login
2 participants
Checklist
Reference to Existing Issue
Closes #692
Description of Changes
Blockers