[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: adds a UUID PK, changes the user relation to ForeignKey with related_name="registered_users", and introduces a nullable organization FK plus uniqueness constraints (per-org and single global). It adds migrations (CT/CTCR and multitenant data/constraints), updates model APIs and helpers, makes identity-verification and registration org-scoped, adjusts admin, views, serializers, monitoring/accounting, management commands, SAML/social hooks, and many tests to use organization-scoped RegisteredUser records and updated query/prefetch logic. Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API
participant View as "View / OrgResolver"
participant DB as "RegisteredUser(s) DB"
rect rgba(150,200,100,0.5)
Note over Client,API: Org-scoped user lookup and authorization
Client->>API: Request with org context (e.g., token, org slug)
API->>View: Resolve organization (view.organization)
API->>DB: Query registered_users filtered by (organization=view.organization) OR organization IS NULL, ordered to prefer org-specific
DB-->>API: Return matching RegisteredUser (org-specific or global) or none
API->>API: Compute is_verified and method from selected RegisteredUser (or None)
API->>Client: Respond (authorization decision, serialized user with is_verified/method)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 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.
github-project-automation
Bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
|
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.
Code Review SummaryStatus: 1 Critical Issue Found | Recommendation: Address before merge SummaryThis PR implements multi-tenancy support for the RegisteredUser model with critical security fixes for authorization bypass vulnerabilities. Critical Security Fixes (NEW)
New Critical Issue FoundLogic Inversion in social/views.py:56 Other Changes
CI DependencyThe temporary tarball dependency on openwisp-users/issues/497-export-users branch (line 77) should be updated to a proper release before merging. Review these changes in Kilo Cloud Reviewed by kimi-k2.5-0127 · 1,451,364 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.338% (-0.1%) 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.
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.
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?
CI Failure: Test Failures and Coverage IssueHello @pandafy, There are multiple test failures and a coverage reporting issue:
Actionable Steps:
|
Test Failures in
|
Since openwisp-users==1.2.1, openwisp-users is responsible for registering SocialAppAdmin. Thus, re-registering the admin class here raises error.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openwisp_radius/api/freeradius_views.py (1)
412-429:⚠️ Potential issue | 🔴 CriticalOrg-specific registration must take precedence in the verification predicate.
The query at lines 413-429 accepts any verified or allowed
registered_usersrow from either the requesting organization or the global fallback. A user with an organization-specific unverified record and a global verified record will be authorized, reintroducing cross-organization verification reuse. The predicate must first check for org-specific registration and only apply global fallback when no org-specific record exists—or use the same precedence logic asRegisteredUser.get_global_or_org_specific().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/api/freeradius_views.py` around lines 412 - 429, The predicate currently mixes org-specific and global registered_users rows (org_or_global) causing global verified rows to override org-specific unverified rows; change the logic in the view that builds is_verified/authorize_unverified so org-specific records take precedence—mirror RegisteredUser.get_global_or_org_specific(): first build a predicate that matches registered_users with registered_users__organization_id=organization_id and check verified/method there, then only apply the global (registered_users__organization__isnull=True) fallback when no org-specific match exists (implement this fallback using an Exists/Subquery against RegisteredUser or by excluding global matches when an org-specific match exists), and use these two-tier predicates instead of the current org_or_global combination; update references to organization_id, is_verified, AUTHORIZE_UNVERIFIED, and authorize_unverified accordingly.openwisp_radius/integrations/monitoring/tasks.py (1)
204-208:⚠️ Potential issue | 🟠 MajorGuard
clean_registration_method(...)before writing metric tags.
clean_registration_methodnow returnsNonefor non-reportable states such aspending_verification, but this path still writes that value intoextra_tags["method"]. That can produce malformed metric series or fail metric creation for pending registrations. Handle theNonecase here the same way you already do in the signup-metric paths.🤖 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 204 - 208, Call clean_registration_method(registration_method) and do not blindly write its return into extra_tags["method"]; instead mirror the signup-metric paths' behavior by checking the cleaned value and only adding the "method" tag when the cleaned result is not None (or use the same fallback they use in signup metrics). Update the block that builds extra_tags (the registration_method variable, extra_tags dict) so that None from clean_registration_method is guarded against and does not get written as a metric tag.openwisp_radius/api/serializers.py (1)
568-575:⚠️ Potential issue | 🟠 MajorRefresh
methodchoices at serializer initialization.
get_registration_choices()is evaluated once when the serializer class is defined (at import time), freezing the registry for the lifetime of the process. This breaks the dynamic registration-method behavior preserved by the model field and causes newly registered methods to fail validation until server restart. Override__init__to populateself.fields["method"].choiceswith fresh values per instance (or move the logic to a shared mixin).Also applies to: 768-775
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/api/serializers.py` around lines 568 - 575, The ChoiceField for "method" currently calls get_registration_choices() at class-definition time, freezing available options; fix by overriding the serializer's __init__ to set self.fields["method"].choices = get_registration_choices() so choices are refreshed per instance (do this for both occurrences where the field is defined), ensuring you reference the existing field name ("method") and the helper function get_registration_choices() when updating the code.
♻️ Duplicate comments (12)
openwisp_radius/saml/views.py (1)
75-84:⚠️ Potential issue | 🟠 MajorHandle concurrent create race for SAML
RegisteredUser.Line 75 still performs read-then-create. Parallel SAML logins can race and fail on the unique
(user, organization)constraint.Proposed race-safe change
+from django.db import IntegrityError, transaction @@ - 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: + with transaction.atomic(): + registered_user.save() + except IntegrityError: + 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 - 84, The current read-then-create for RegisteredUser (user.registered_users.get(organization=org) + creating RegisteredUser(...).save()) is racy; replace it with a race-safe pattern such as using RegisteredUser.objects.get_or_create(user=user, organization=org, defaults={...}) or, if you need full_clean, wrap the create in a transaction and handle IntegrityError by fetching the existing RegisteredUser; update the code paths that reference user.registered_users.get(...) and the RegisteredUser(...) creation so the unique (user, organization) constraint cannot cause a crash on concurrent SAML logins.openwisp_radius/social/views.py (1)
50-59:⚠️ Potential issue | 🟠 MajorMake org-scoped
RegisteredUsercreation race-safe.Line 50 still uses a check-then-create pattern; concurrent requests can race and hit the unique
(user, organization)constraint with anIntegrityError.Proposed race-safe change
+from django.db import IntegrityError, transaction @@ - 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() + 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() + try: + with transaction.atomic(): + registered_user.save() + except IntegrityError: + 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/social/views.py` around lines 50 - 59, The current check-then-create using user.registered_users.get(...) is not race-safe and can cause IntegrityError on concurrent requests; change this to a race-safe create by using RegisteredUser.objects.get_or_create(...) scoped to the organization (or wrap creation in a transaction.atomic block and catch IntegrityError, then re-fetch the instance with RegisteredUser.objects.get(user=user, organization=org)). Ensure you still call registered_user.full_clean() for new instances before saving (or run full_clean when creating via get_or_create by validating beforehand), and update the code paths that reference user.registered_users.get, RegisteredUser.full_clean, and RegisteredUser.save to use the new get_or_create or the atomic + IntegrityError then-get flow.openwisp_radius/integrations/monitoring/tasks.py (2)
119-137:⚠️ Potential issue | 🟠 MajorPer-org signup metrics are still attributed to the wrong organization.
This query groups by
user__openwisp_users_organizationuser__organization_id, so one org-scopedRegisteredUserrow is counted once for every org the same user belongs to. The fallback query has the complementary bug:user__registered_users__isnull=Trueonly finds users with no registrations anywhere, not users missing a registration for this specificOrganizationUser.organization. In multi-org setups that makes the org-level metrics inaccurate.Please derive org-scoped counts from
RegisteredUser.organization_id, then handle the “global fallback” / “no org-specific record” cases fromOrganizationUserwith an org-awareExists(...)check against the same organization.🤖 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 119 - 137, The per-org signup aggregation incorrectly groups by user__openwisp_users_organizationuser__organization_id causing duplicate counts for users in multiple orgs; change the main aggregation to group by RegisteredUser.organization_id (use RegisteredUser.organization_id and method) so each RegisteredUser row is counted only once per its own organization, and update the fallback users_without_registereduser_query (OrganizationUser) to filter per-organization by using an Exists(...) subquery that checks for a RegisteredUser with the same user_id and the OrganizationUser.organization_id (i.e. only include OrganizationUser rows where no RegisteredUser exists for that specific organization), keeping the metric_key logic intact.
189-195:⚠️ Potential issue | 🟠 Major
-organization_idcan still pick the global record before the org-specific one.
order_by("-organization_id").first()is not a safe “prefer org-specific” lookup. On PostgreSQL,NULLsorts first for descending order, so a globalorganization=Nonerow can win over the org-specific row and leak the wrong registration method into accounting metrics.Suggested fix
- registration_method = ( - RegisteredUser.objects.only("method") - .filter(user__username=username) - .filter(Q(organization_id=organization_id) | Q(organization__isnull=True)) - .order_by("-organization_id") - .first() - ) + registration_method = ( + RegisteredUser.objects.only("method") + .filter(user__username=username, organization_id=organization_id) + .first() + ) + if registration_method is None: + registration_method = ( + RegisteredUser.objects.only("method") + .filter(user__username=username, organization__isnull=True) + .first() + )🤖 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 189 - 195, The current lookup that builds registration_method using RegisteredUser.objects...order_by("-organization_id").first() can prefer NULL organization_id on Postgres; change the query to explicitly prefer org-specific rows by ordering with NULLs last or by annotating a flag so non-global rows sort first (e.g. use models.F('organization_id').desc(nulls_last=True) in order_by or annotate a Case/When is_global and order_by('is_global', '-organization_id')); update the query that assigns registration_method to use that ordering so the row for the given organization_id is chosen before the global (organization__isnull=True) record.openwisp_radius/admin.py (1)
559-566:⚠️ Potential issue | 🟠 Major
get_is_verifiednow adds up to two extra queries per user row.Both
obj.registered_users.exists()andobj.registered_users.filter(...).exists()hit the database, so the changelist regresses to O(n) extra queries. On larger user tables this will slow the admin noticeably. Compute the aggregate once inUserAdmin.get_queryset()withExists(...)/annotations or prefetch and read the in-memory result here instead.🤖 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 executes two DB queries per row (obj.registered_users.exists() and filter(...).exists()), causing O(n) extra queries; instead, annotate the UserAdmin queryset once in UserAdmin.get_queryset() using an Exists/Subquery (or prefetch_related with Prefetch for registered_users) to compute a boolean like is_verified_exists, and then have get_is_verified read that in-memory annotated attribute (e.g., getattr(obj, 'is_verified_exists', False) or check the prefetched related list) to return "yes"/"no"/"unknown" without making new DB calls.tests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py (1)
18-21:⚠️ Potential issue | 🟠 MajorMake the sample-app migration self-contained.
This migration still depends on
openwisp_radius.registrationat runtime just to populatechoicesfor a temporary table. That makes test database rebuilds and fresh installs fragile, while providing no schema benefit. Freeze the choices inside the migration or dropchoices=fromRegisteredUserNew.method.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 imports get_registration_choices/REGISTRATION_METHOD_CHOICES at runtime to populate the temporary table choices; make the migration self-contained by removing the external dependency: either freeze the choices into a literal list and use that literal when defining the temporary table or simply drop the choices= argument from the field RegisteredUserNew.method (also update the other occurrence around the similar block at the second location referencing lines 95-99), ensuring no runtime imports from openwisp_radius.registration remain in this migration.openwisp_radius/api/views.py (2)
767-783:⚠️ Potential issue | 🔴 CriticalPersist
method="mobile_phone"on the existing-row path.When this reuses the org-scoped row created during first login (
method="pending_verification"),defaultsare ignored. This code only flipsis_verified, so the record stays weak andis_identity_verified_strongremains false after a successful SMS check.Suggested fix
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" @@ user.phone_number = phone_token.phone_number user.save() - reg_user.save() + reg_user.save(update_fields=["is_verified", "method"])Please add a regression test for the org-join → SMS-verify flow as well.
🤖 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 767 - 783, When reusing the org-scoped RegisteredUser row from RegisteredUser.get_or_create_for_user_and_org, the defaults are ignored so you must explicitly set the stronger identity method and persist it; update the handler around reg_user (the reg_user variable returned by RegisteredUser.get_or_create_for_user_and_org) to set reg_user.method = "mobile_phone" (and any related flags such as reg_user.is_identity_verified_strong if present) before calling reg_user.save(), while continuing to update user.phone_number from phone_token.phone_number and saving user; add a regression test covering the org-join → SMS-verify flow that first creates a pending_verification RegisteredUser and then runs the SMS verification path to assert reg_user.method becomes "mobile_phone" and the strong verification flag is set.
341-351:⚠️ Potential issue | 🟠 MajorMake first-login org enrollment atomic.
Two concurrent auth requests can both pass the membership check here, then race while creating
OrganizationUserandRegisteredUser. One request can 500 on a duplicate row, or the user can be left with only one of the two records persisted. Wrap both writes in a singletransaction.atomic()block and make both creations 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 341 - 351, Wrap the two writes in a database transaction and make them idempotent: import transaction from django.db, run a transaction.atomic() block around the OrganizationUser and RegisteredUser creations, and use get_or_create for both OrganizationUser and RegisteredUser (e.g. OrganizationUser.objects.get_or_create(user=user, organization=self.organization) and RegisteredUser.objects.get_or_create(user=user, organization=self.organization, defaults={"method":"pending_verification"})) so concurrent requests won’t raise duplicate-row errors; keep references to OrganizationUser, RegisteredUser, self.organization and user when applying the change.openwisp_radius/migrations/0043_registereduser_add_uuid.py (1)
11-14:⚠️ Potential issue | 🔴 CriticalMake this migration self-contained.
This temporary-table migration still imports
REGISTRATION_METHOD_CHOICES/get_registration_choicesfrom application code. Ifopenwisp_radius.registrationchanges later, fresh installs can no longer replay0043. Sincechoicesdo not affect the schema here, freeze them locally or removechoices=fromRegisteredUserNew.method.Minimal fix
-from openwisp_radius.registration import ( - REGISTRATION_METHOD_CHOICES, - get_registration_choices, -) @@ ( "method", models.CharField( blank=True, - choices=( - REGISTRATION_METHOD_CHOICES - if django.VERSION < (5, 0) - else get_registration_choices - ), default="", help_text=( "users can sign up in different ways, some "Also applies to: 88-95
🤖 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 imports REGISTRATION_METHOD_CHOICES / get_registration_choices from application code making it non-self-contained; update the migration to avoid runtime app imports by either freezing the choices locally (copy the current REGISTRATION_METHOD_CHOICES tuple into the migration and use that) or simply remove the choices= argument from the RegisteredUserNew.method field so the migration only defines the schema changes; ensure no references to get_registration_choices or REGISTRATION_METHOD_CHOICES remain in the migration (also update the other affected block around RegisteredUserNew where choices were used).openwisp_radius/base/models.py (1)
1070-1075:⚠️ Potential issue | 🟠 MajorUpgrade reused batch registrations to a strong method.
get_or_create_user()can return an existing user for duplicate CSV emails, so this branch is reachable. If that org-scopedRegisteredUseralready exists withmethod=""or"email", this code only marks it verified; it still failsis_identity_verified_strongand can remain blocked in orgs that require strong verification.Suggested 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 1070 - 1075, The branch that marks registered_user as verified can leave an existing RegisteredUser with a weak/empty method still failing is_identity_verified_strong; update the logic inside the block that checks not created and organization.radius_settings.needs_identity_verification so that when you set registered_user.is_verified = True you also upgrade registered_user.method to a strong verification method if it is empty or a weak value (use the RegisteredUser method enum/constant, e.g. the EMAIL/STRONG method defined on RegisteredUser), then save the instance; ensure this change occurs in the same place that calls get_or_create_user() so duplicate CSV-created users are upgraded atomically.openwisp_radius/migrations/__init__.py (2)
194-242:⚠️ Potential issue | 🟠 MajorDon't drop org-scoped rows just because a global row already exists.
This reverse path skips recreating a global record for
existing_globals, but it still deletes every org-scoped row for those users. If the retained global row is older or weaker than the best org-scoped record, rollback silently downgrades the user state instead of preserving the strongest available data.🤖 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 migration currently appends every org-scoped row PK into to_delete_pks before deciding whether to recreate a global row, causing org rows to be deleted even when an existing global should be kept; change the logic in the loop over org_records so you only mark an org-scoped row for deletion when you actually intend to replace it (i.e., only append registered_user.pk to to_delete_pks inside the branch where you create/restore a global record for that user and not when registered_user.user_id is in existing_globals or skipped), and leave org rows untouched otherwise; update references to RegisteredUser, existing_globals, org_records, to_delete_pks, and _flush_bulk_create accordingly.
149-175:⚠️ Potential issue | 🟠 MajorKeep the legacy
organization=Nonerow in the forward migration.Deleting the original global record after cloning it into current memberships breaks the compatibility contract for preexisting users. After this migration, a user who later joins a different organization no longer has the legacy fallback record the PR objectives require.
Suggested fix
- to_delete_pks = [] for registered_user in batch: organization_ids = sorted(memberships.get(registered_user.user_id, ())) if not organization_ids: continue - to_delete_pks.append(registered_user.pk) extra_kwargs = _registered_user_extra_kwargs(registered_user, extra_fields) for organization_id in organization_ids: pair = (registered_user.user_id, organization_id) if pair in existing_pairs: continue @@ _flush_bulk_create(RegisteredUser, to_create) - if to_delete_pks: - RegisteredUser.objects.filter(pk__in=to_delete_pks).delete()🤖 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 149 - 175, The migration currently deletes the original global (organization=None) RegisteredUser rows after cloning them into per-organization records; change the logic in the loop that builds to_delete_pks so that any RegisteredUser with organization_id is None is not appended (i.e., only mark for deletion when registered_user.organization_id is not None), or simply stop collecting to_delete_pks entirely so legacy organization=None rows are preserved; update the code around the RegisteredUser creation block (symbols: RegisteredUser, _registered_user_extra_kwargs, _flush_bulk_create, to_delete_pks, existing_pairs, batch) to ensure legacy fallback rows remain and only truly duplicate/non-legacy rows are removed (and adjust the subsequent RegisteredUser.objects.filter(pk__in=to_delete_pks).delete() 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 541-545: The RegisteredUser admin marks "organization" in
readonly_fields which prevents org-scoped RegisteredUser instances from being
created/edited in the User admin inline; remove "organization" from
readonly_fields (or conditionally only make it readonly in contexts where
editing is disallowed) and ensure "organization" remains in fields so the inline
form can set it; update the RegisteredUserAdmin (and any InlineModelAdmin
referencing RegisteredUser) to allow editing/setting organization when adding
rows (or override get_readonly_fields to return readonly only for change view
where needed) so new inline rows do not default to organization=None.
In `@openwisp_radius/integrations/monitoring/tests/test_metrics.py`:
- Around line 433-455: The tests call self._read_chart for both all_points and
org_points but then mistakenly assert against all_points in the org-scoped
branches; update the assertions after reading org_points to assert against
org_points (e.g., replace references to all_points["traces"] and
all_points["summary"] with org_points["traces"] and org_points["summary"]) for
both user_signup_chart and total_user_signup_chart checks so the
organization-filtered series returned by _read_chart is actually validated.
- Around line 505-526: The test currently asserts that pending_verification is
excluded only from the org-scoped chart; update it to also verify the global
"__all__" series is free of pending_verification data: after calling
write_user_registration_metrics.delay(), read both the org chart and the global
chart (use _read_chart with organization_id=[str(org.pk)] and
organization_id=["__all__"]) for the user_signup_metric and/or
total_user_signup_metric and assert that the global chart's traces (and points)
do not include the pending_verification series; reference
write_user_registration_metrics, user_signup_metric, total_user_signup_metric,
and _read_chart to locate where to add the additional assertion.
In `@openwisp_radius/migrations/__init__.py`:
- Around line 101-103: The current rollback ordering on RegisteredUserNew uses
"pk" as a tie-breaker which is effectively random because new PKs are UUIDs;
change the queryset ordering in the
RegisteredUserNew.annotate(...).order_by(...) call to use "-modified" as the
semantic tie-breaker before falling back to "pk" (i.e., order_by("user_id",
"-is_verified", "-method_priority", "-modified", "pk")) so the most recently
modified record wins; apply the same change to the other similar ordering
instance later in the file where RegisteredUserNew is ordered.
In `@openwisp_radius/tests/test_api/test_phone_verification.py`:
- Around line 951-995: The test test_phone_change_unverifies_only_specific_org
never creates a RegisteredUser for org2 so it doesn't assert the other
organization's verification survives; fix it by creating a registration for org2
(e.g. call the existing helper _create_org_user with organization=org2 and the
same user or create RegisteredUser for user and org2 and set is_verified=True)
before performing the phone change, then after the change assert
RegisteredUser.objects.get(user=user, organization=org2).is_verified is still
True; this targets the test function
test_phone_change_unverifies_only_specific_org and the RegisteredUser model to
ensure isolation is verified.
---
Outside diff comments:
In `@openwisp_radius/api/freeradius_views.py`:
- Around line 412-429: The predicate currently mixes org-specific and global
registered_users rows (org_or_global) causing global verified rows to override
org-specific unverified rows; change the logic in the view that builds
is_verified/authorize_unverified so org-specific records take precedence—mirror
RegisteredUser.get_global_or_org_specific(): first build a predicate that
matches registered_users with registered_users__organization_id=organization_id
and check verified/method there, then only apply the global
(registered_users__organization__isnull=True) fallback when no org-specific
match exists (implement this fallback using an Exists/Subquery against
RegisteredUser or by excluding global matches when an org-specific match
exists), and use these two-tier predicates instead of the current org_or_global
combination; update references to organization_id, is_verified,
AUTHORIZE_UNVERIFIED, and authorize_unverified accordingly.
In `@openwisp_radius/api/serializers.py`:
- Around line 568-575: The ChoiceField for "method" currently calls
get_registration_choices() at class-definition time, freezing available options;
fix by overriding the serializer's __init__ to set self.fields["method"].choices
= get_registration_choices() so choices are refreshed per instance (do this for
both occurrences where the field is defined), ensuring you reference the
existing field name ("method") and the helper function
get_registration_choices() when updating the code.
In `@openwisp_radius/integrations/monitoring/tasks.py`:
- Around line 204-208: Call clean_registration_method(registration_method) and
do not blindly write its return into extra_tags["method"]; instead mirror the
signup-metric paths' behavior by checking the cleaned value and only adding the
"method" tag when the cleaned result is not None (or use the same fallback they
use in signup metrics). Update the block that builds extra_tags (the
registration_method variable, extra_tags dict) so that None from
clean_registration_method is guarded against and does not get written as a
metric tag.
---
Duplicate comments:
In `@openwisp_radius/admin.py`:
- Around line 559-566: get_is_verified currently executes two DB queries per row
(obj.registered_users.exists() and filter(...).exists()), causing O(n) extra
queries; instead, annotate the UserAdmin queryset once in
UserAdmin.get_queryset() using an Exists/Subquery (or prefetch_related with
Prefetch for registered_users) to compute a boolean like is_verified_exists, and
then have get_is_verified read that in-memory annotated attribute (e.g.,
getattr(obj, 'is_verified_exists', False) or check the prefetched related list)
to return "yes"/"no"/"unknown" without making new DB calls.
In `@openwisp_radius/api/views.py`:
- Around line 767-783: When reusing the org-scoped RegisteredUser row from
RegisteredUser.get_or_create_for_user_and_org, the defaults are ignored so you
must explicitly set the stronger identity method and persist it; update the
handler around reg_user (the reg_user variable returned by
RegisteredUser.get_or_create_for_user_and_org) to set reg_user.method =
"mobile_phone" (and any related flags such as
reg_user.is_identity_verified_strong if present) before calling reg_user.save(),
while continuing to update user.phone_number from phone_token.phone_number and
saving user; add a regression test covering the org-join → SMS-verify flow that
first creates a pending_verification RegisteredUser and then runs the SMS
verification path to assert reg_user.method becomes "mobile_phone" and the
strong verification flag is set.
- Around line 341-351: Wrap the two writes in a database transaction and make
them idempotent: import transaction from django.db, run a transaction.atomic()
block around the OrganizationUser and RegisteredUser creations, and use
get_or_create for both OrganizationUser and RegisteredUser (e.g.
OrganizationUser.objects.get_or_create(user=user,
organization=self.organization) and
RegisteredUser.objects.get_or_create(user=user, organization=self.organization,
defaults={"method":"pending_verification"})) so concurrent requests won’t raise
duplicate-row errors; keep references to OrganizationUser, RegisteredUser,
self.organization and user when applying the change.
In `@openwisp_radius/base/models.py`:
- Around line 1070-1075: The branch that marks registered_user as verified can
leave an existing RegisteredUser with a weak/empty method still failing
is_identity_verified_strong; update the logic inside the block that checks not
created and organization.radius_settings.needs_identity_verification so that
when you set registered_user.is_verified = True you also upgrade
registered_user.method to a strong verification method if it is empty or a weak
value (use the RegisteredUser method enum/constant, e.g. the EMAIL/STRONG method
defined on RegisteredUser), then save the instance; ensure this change occurs in
the same place that calls get_or_create_user() so duplicate CSV-created users
are upgraded atomically.
In `@openwisp_radius/integrations/monitoring/tasks.py`:
- Around line 119-137: The per-org signup aggregation incorrectly groups by
user__openwisp_users_organizationuser__organization_id causing duplicate counts
for users in multiple orgs; change the main aggregation to group by
RegisteredUser.organization_id (use RegisteredUser.organization_id and method)
so each RegisteredUser row is counted only once per its own organization, and
update the fallback users_without_registereduser_query (OrganizationUser) to
filter per-organization by using an Exists(...) subquery that checks for a
RegisteredUser with the same user_id and the OrganizationUser.organization_id
(i.e. only include OrganizationUser rows where no RegisteredUser exists for that
specific organization), keeping the metric_key logic intact.
- Around line 189-195: The current lookup that builds registration_method using
RegisteredUser.objects...order_by("-organization_id").first() can prefer NULL
organization_id on Postgres; change the query to explicitly prefer org-specific
rows by ordering with NULLs last or by annotating a flag so non-global rows sort
first (e.g. use models.F('organization_id').desc(nulls_last=True) in order_by or
annotate a Case/When is_global and order_by('is_global', '-organization_id'));
update the query that assigns registration_method to use that ordering so the
row for the given organization_id is chosen before the global
(organization__isnull=True) record.
In `@openwisp_radius/migrations/__init__.py`:
- Around line 194-242: The migration currently appends every org-scoped row PK
into to_delete_pks before deciding whether to recreate a global row, causing org
rows to be deleted even when an existing global should be kept; change the logic
in the loop over org_records so you only mark an org-scoped row for deletion
when you actually intend to replace it (i.e., only append registered_user.pk to
to_delete_pks inside the branch where you create/restore a global record for
that user and not when registered_user.user_id is in existing_globals or
skipped), and leave org rows untouched otherwise; update references to
RegisteredUser, existing_globals, org_records, to_delete_pks, and
_flush_bulk_create accordingly.
- Around line 149-175: The migration currently deletes the original global
(organization=None) RegisteredUser rows after cloning them into per-organization
records; change the logic in the loop that builds to_delete_pks so that any
RegisteredUser with organization_id is None is not appended (i.e., only mark for
deletion when registered_user.organization_id is not None), or simply stop
collecting to_delete_pks entirely so legacy organization=None rows are
preserved; update the code around the RegisteredUser creation block (symbols:
RegisteredUser, _registered_user_extra_kwargs, _flush_bulk_create,
to_delete_pks, existing_pairs, batch) to ensure legacy fallback rows remain and
only truly duplicate/non-legacy rows are removed (and adjust the subsequent
RegisteredUser.objects.filter(pk__in=to_delete_pks).delete() accordingly).
In `@openwisp_radius/migrations/0043_registereduser_add_uuid.py`:
- Around line 11-14: The migration imports REGISTRATION_METHOD_CHOICES /
get_registration_choices from application code making it non-self-contained;
update the migration to avoid runtime app imports by either freezing the choices
locally (copy the current REGISTRATION_METHOD_CHOICES tuple into the migration
and use that) or simply remove the choices= argument from the
RegisteredUserNew.method field so the migration only defines the schema changes;
ensure no references to get_registration_choices or REGISTRATION_METHOD_CHOICES
remain in the migration (also update the other affected block around
RegisteredUserNew where choices were used).
In `@openwisp_radius/saml/views.py`:
- Around line 75-84: The current read-then-create for RegisteredUser
(user.registered_users.get(organization=org) + creating
RegisteredUser(...).save()) is racy; replace it with a race-safe pattern such as
using RegisteredUser.objects.get_or_create(user=user, organization=org,
defaults={...}) or, if you need full_clean, wrap the create in a transaction and
handle IntegrityError by fetching the existing RegisteredUser; update the code
paths that reference user.registered_users.get(...) and the RegisteredUser(...)
creation so the unique (user, organization) constraint cannot cause a crash on
concurrent SAML logins.
In `@openwisp_radius/social/views.py`:
- Around line 50-59: The current check-then-create using
user.registered_users.get(...) is not race-safe and can cause IntegrityError on
concurrent requests; change this to a race-safe create by using
RegisteredUser.objects.get_or_create(...) scoped to the organization (or wrap
creation in a transaction.atomic block and catch IntegrityError, then re-fetch
the instance with RegisteredUser.objects.get(user=user, organization=org)).
Ensure you still call registered_user.full_clean() for new instances before
saving (or run full_clean when creating via get_or_create by validating
beforehand), and update the code paths that reference user.registered_users.get,
RegisteredUser.full_clean, and RegisteredUser.save to use the new get_or_create
or the atomic + IntegrityError then-get flow.
In
`@tests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py`:
- Around line 18-21: The migration imports
get_registration_choices/REGISTRATION_METHOD_CHOICES at runtime to populate the
temporary table choices; make the migration self-contained by removing the
external dependency: either freeze the choices into a literal list and use that
literal when defining the temporary table or simply drop the choices= argument
from the field RegisteredUserNew.method (also update the other occurrence around
the similar block at the second location referencing lines 95-99), ensuring no
runtime imports from openwisp_radius.registration remain in 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: 65871474-e0e1-4bb8-ab58-2d3dd0ed1dad
📒 Files selected for processing (42)
.github/workflows/ci.ymldocs/user/rest-api.rstdocs/user/settings.rstopenwisp_radius/admin.pyopenwisp_radius/api/freeradius_views.pyopenwisp_radius/api/serializers.pyopenwisp_radius/api/urls.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/integrations/monitoring/utils.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/registration.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/api/views.pytests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/{docs,documentation}/**/*.{md,rst,adoc,html}
📄 CodeRabbit inference engine (Custom checks)
**/{docs,documentation}/**/*.{md,rst,adoc,html}: Documentation must be updated if a change affects behavior that is already documented
Feature documentation must mention the new feature. If the feature is heavily UI-related, a new section or page is appropriate
Files:
docs/user/settings.rstdocs/user/rest-api.rst
🧠 Learnings (6)
📚 Learning: 2026-04-17T13:53:45.430Z
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 .github/workflows/ci.yml, the CI step that runs `pip install --upgrade --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"` is an intentionally temporary blocker dependency (to use a pre-release branch until openwisp-users PR `#498` is merged). Do not flag this pip install as an issue in future reviews as long as it appears in this workflow with the same purpose.
Applied to files:
.github/workflows/ci.yml
📚 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/api/views.py
📚 Learning: 2026-04-17T13:53:48.176Z
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 698
File: .github/workflows/ci.yml:77-77
Timestamp: 2026-04-17T13:53:48.176Z
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.
Applied to files:
openwisp_radius/migrations/0043_registereduser_add_uuid.pytests/openwisp2/sample_radius/migrations/0032_registered_user_multitenant.pyopenwisp_radius/migrations/__init__.py
📚 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: Bug fixes must include a regression test that reproduces the bug scenario
Applied to files:
openwisp_radius/api/views.py
📚 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: Regression tests must fail if the patch is removed or commented out
Applied to files:
openwisp_radius/api/views.py
🔇 Additional comments (18)
.github/workflows/ci.yml (1)
77-77: Temporary blocker install is acceptable as-is.No change needed here; this forced reinstall from
issues/497-export-usersis consistent with the current temporary upstream dependency strategy for this PR.Based on learnings: this exact CI install command in
.github/workflows/ci.ymlis intentionally temporary untilopenwisp-usersPR#498is merged.openwisp_radius/tests/test_selenium.py (1)
24-24: Good isolation for Selenium live-server tests.Adding
no_parallelhere is the right guardrail to avoid flaky cross-test interference during parallel runs.runtests (1)
6-6: Nice improvement for CI debugging.Keeping output visible and merging stderr into stdout makes failures easier to diagnose without changing the fallback behavior.
openwisp_radius/tests/mixins.py (1)
100-103: Inline formset keys correctly updated to plural relation.The
registered_users-*management keys now align with theRegisteredUserreverse relation and multi-record inline behavior.openwisp_radius/tests/test_batch_add_users.py (1)
146-148: Organization-scoped RegisteredUser assertion is correct.Using
registered_users.get(organization=organization)validates the exact tenant-specific verification state created by the batch flow.openwisp_radius/settings.py (1)
235-243: Export config update correctly reflects one-to-many RegisteredUser data.Switching to a structured
registered_usersfield plusprefetch_related=["registered_users"]is consistent with the new model shape and export expectations.openwisp_radius/tests/test_users_integration.py (1)
99-119: Integration test now validates the new export contract accurately.The fixture and CSV assertions are aligned with org-scoped
RegisteredUserexport (registered_userswith(organization_id, method, is_verified)payload).docs/user/settings.rst (1)
699-701: Documentation update forpending_verificationis clear and necessary.This addition makes the transitional org-specific verification state explicit for users configuring identity-verification methods.
As per coding guidelines, Documentation must be updated if a change affects behavior that is already documented, and feature documentation must mention the new feature.
openwisp_radius/tests/test_token.py (1)
68-71: Test fixture now correctly scopes verification to the organization.Adding
organization=self.default_orgmakes the already-verified path deterministic under the multi-tenant model.openwisp_radius/tests/test_saml/test_views.py (1)
155-157: SAML test setup now correctly creates tenant-specific registration state.Using the default organization in
RegisteredUser.objects.create(...)matches the org-aware lookup used by the SAML ACS flow.openwisp_radius/integrations/monitoring/utils.py (1)
54-55:pending_verificationexclusion logic is correctly implemented.Returning
Nonehere cleanly integrates with downstream metric writers that skip non-reportable registration states.openwisp_radius/registration.py (1)
13-13:pending_verificationregistration choice is correctly added.This aligns with the new org-scoped pending state and keeps the label translatable.
openwisp_radius/saml/backends.py (1)
17-20: The non-SAML check is correctly adapted to the plural relation.Using
user.registered_users.exclude(method="saml").exists()fits the multi-record model and preserves the username-protection intent.docs/user/rest-api.rst (1)
806-847: Docs for the new registration-method endpoint are complete and clear.Path, auth, allowed input, and response contract are documented in line with the new feature behavior.
openwisp_radius/api/urls.py (1)
80-84: New org-scoped URL route is wired correctly.The endpoint path and view binding are consistent with the surrounding API URL structure.
openwisp_radius/base/admin_filters.py (1)
18-22: Filter update for multi-row relation looks correct.Using
registered_users__...with.distinct()is the right adjustment after moving to a one-to-many relation.openwisp_radius/tests/test_social.py (1)
16-16: Test adjustments correctly reflect org-scopedRegisteredUserbehavior.The model loading, exception handling, and org-filtered lookup are aligned with the new relation design.
Also applies to: 105-111
openwisp_radius/tests/test_api/test_api.py (1)
1658-1671: This review comment is incorrect. The empty string""is explicitly a valid choice inREGISTRATION_METHOD_CHOICES(labeled "Unspecified"), so the test correctly expects HTTP 200 for{"method": ""}. DRF'sChoiceFieldaccepts valid choice values regardless ofallow_blank; the field's validation only rejects blank input when the value is not in the available choices. Since""is included in the choices, the test is working as intended.> Likely an incorrect or invalid review comment.
| readonly_fields = ( | ||
| "organization", | ||
| "modified", | ||
| ) | ||
| fields = ("organization", "method", "is_verified", "modified") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the RegisteredUserInline class in admin.py
rg -n "class RegisteredUserInline" openwisp_radius/Repository: openwisp/openwisp-radius
Length of output: 139
🏁 Script executed:
# Get context around the specified lines 541-545
sed -n '535,555p' openwisp_radius/admin.py | cat -nRepository: openwisp/openwisp-radius
Length of output: 677
🏁 Script executed:
# Find RegisteredUser model definition
rg -n "class RegisteredUser" openwisp_radius/Repository: openwisp/openwisp-radius
Length of output: 301
🏁 Script executed:
# Examine RegisteredUser model definition
sed -n '103,150p' openwisp_radius/models.py | cat -nRepository: openwisp/openwisp-radius
Length of output: 286
🏁 Script executed:
# Find AbstractRegisteredUser to understand the model structure
rg -n "class AbstractRegisteredUser" openwisp_radius/Repository: openwisp/openwisp-radius
Length of output: 144
🏁 Script executed:
# Check the base models to understand organization field
rg -A 20 "class AbstractRegisteredUser" openwisp_radius/base/models.py | head -40Repository: openwisp/openwisp-radius
Length of output: 775
🏁 Script executed:
# Examine how organization is defined in RegisteredUser
rg -n "organization" openwisp_radius/models.py | head -20Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check if there's any save logic or form methods in RegisteredUserInline
rg -B 5 -A 30 "class RegisteredUserInline" openwisp_radius/admin.pyRepository: openwisp/openwisp-radius
Length of output: 959
🏁 Script executed:
# Check AlwaysHasChangedForm and see how it handles readonly fields
rg -B 5 -A 20 "class AlwaysHasChangedForm" openwisp_radius/admin.pyRepository: openwisp/openwisp-radius
Length of output: 667
🏁 Script executed:
# Look for any custom save methods that might set organization
rg -n "def save" openwisp_radius/admin.py | head -10Repository: openwisp/openwisp-radius
Length of output: 125
🏁 Script executed:
# Check the User model to see if it has an organization relationship
rg -n "class User" openwisp_radius/models.pyRepository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check if User/UserAdmin has organization context
rg -B 2 -A 10 "class UserAdmin" openwisp_radius/admin.pyRepository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check the User model structure and if it has organization
rg -B 5 -A 30 "class User" openwisp_users/models.py 2>/dev/null | head -60Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check if there's a custom save method in RegisteredUserInline or UserAdmin
sed -n '1,600p' openwisp_radius/admin.py | grep -A 20 "def save"Repository: openwisp/openwisp-radius
Length of output: 1049
🏁 Script executed:
# Look for any inline form handling or custom logic related to organization
rg -B 3 -A 10 "organization" openwisp_radius/admin.py | head -80Repository: openwisp/openwisp-radius
Length of output: 1758
🏁 Script executed:
# Check if extra=0 is indeed set (limiting creation of new records)
sed -n '537,565p' openwisp_radius/admin.py | cat -nRepository: openwisp/openwisp-radius
Length of output: 1035
🏁 Script executed:
# Check git history of RegisteredUserInline to see if extra was changed
git log --oneline -p openwisp_radius/admin.py | grep -B 5 -A 10 "RegisteredUserInline" | head -60Repository: openwisp/openwisp-radius
Length of output: 803
🏁 Script executed:
# Check if RegisteredUser can be created from its own admin page
rg -n "@admin.register.*RegisteredUser" openwisp_radius/admin.pyRepository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Search for any admin class for RegisteredUser
rg -B 5 -A 20 "RegisteredUser" openwisp_radius/admin.py | grep -A 20 "admin.register"Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Look at the actual user editing workflow - see UserAdmin definition
rg -B 10 -A 40 "class UserAdmin" openwisp_radius/admin.pyRepository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check if there are any tests that show the intended workflow
rg -n "RegisteredUserInline" openwisp_radius/tests/Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check the full git history to see if extra was different before
git log --all --oneline -p -- openwisp_radius/admin.py | grep -B 20 -A 20 "extra = " | grep -A 20 -B 20 "RegisteredUserInline"Repository: openwisp/openwisp-radius
Length of output: 5938
🏁 Script executed:
# Look for the current state of UserAdmin to see all its inlines
sed -n '1,550p' openwisp_radius/admin.py | tail -100 | head -50Repository: openwisp/openwisp-radius
Length of output: 1923
🏁 Script executed:
# Check if RegisteredUserInline.has_change_permission is defined (relevant for editing)
rg -A 5 "has_delete_permission" openwisp_radius/admin.py | grep -B 5 -A 5 "RegisteredUserInline"Repository: openwisp/openwisp-radius
Length of output: 311
🏁 Script executed:
# Check the broader context of UserAdmin inlines to understand intended use
rg -B 5 "RegisteredUserInline" openwisp_radius/admin.pyRepository: openwisp/openwisp-radius
Length of output: 323
🏁 Script executed:
# Check if there are any signals or other mechanisms that auto-set organization
rg -n "organization" openwisp_radius/signals.py 2>/dev/null || echo "signals.py not found or no matches"Repository: openwisp/openwisp-radius
Length of output: 102
🏁 Script executed:
# Check if there were any commits where extra was > 0 for RegisteredUserInline
git log --all -p -- openwisp_radius/admin.py | grep -B 30 "class RegisteredUserInline" | grep "extra" | head -10Repository: openwisp/openwisp-radius
Length of output: 241
🏁 Script executed:
# Understand the intended behavior - check if has_add_permission was ever defined
git log --all -p -- openwisp_radius/admin.py | grep -B 5 -A 15 "RegisteredUserInline" | grep -A 10 "has_add_permission" | head -20Repository: openwisp/openwisp-radius
Length of output: 432
🏁 Script executed:
# Check the model's organization field definition more carefully
sed -n '1623,1670p' openwisp_radius/base/models.py | cat -nRepository: openwisp/openwisp-radius
Length of output: 2218
🏁 Script executed:
# Verify: with extra=0, can new rows even be created? Check Django inline behavior
rg -B 10 -A 10 "extra = 0" openwisp_radius/admin.py | head -60Repository: openwisp/openwisp-radius
Length of output: 1576
🏁 Script executed:
# Search for RegisteredUser admin registration
rg -n "@admin.register.*RegisteredUser|class.*RegisteredUser.*Admin" openwisp_radius/admin.pyRepository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check the full fields tuple definition in RegisteredUserInline
sed -n '537,560p' openwisp_radius/admin.py | cat -nRepository: openwisp/openwisp-radius
Length of output: 815
🏁 Script executed:
# Verify the change: was organization just added to readonly_fields and also to fields?
git log --all -p -- openwisp_radius/admin.py | grep -B 25 -A 25 "readonly_fields.*organization" | head -80Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Get the specific commit that added organization to readonly_fields
git log --all --oneline -- openwisp_radius/admin.py | head -20Repository: openwisp/openwisp-radius
Length of output: 1307
🏁 Script executed:
# Check the specific commit where organization was added to readonly
git show --stat $(git log --all --format="%H" -- openwisp_radius/admin.py | head -1)Repository: openwisp/openwisp-radius
Length of output: 520
🏁 Script executed:
# Look for the commit message that explains why organization was added to readonly
git log --all -p -- openwisp_radius/admin.py | grep -B 50 "readonly_fields = (" | grep -A 5 "organization" | head -40Repository: openwisp/openwisp-radius
Length of output: 1401
🏁 Script executed:
# Check the current HEAD to see what we're reviewing
git log --oneline -1Repository: openwisp/openwisp-radius
Length of output: 110
🏁 Script executed:
# Find the commit that added organization to readonly_fields
git log --all -p -- openwisp_radius/admin.py | grep -B 100 'readonly_fields = (' | grep -B 100 "organization" | tail -150 | head -80Repository: openwisp/openwisp-radius
Length of output: 3267
🏁 Script executed:
# More targeted: search for commit that changed readonly_fields for RegisteredUserInline
git log --all -p -- openwisp_radius/admin.py | awk '/class RegisteredUserInline/,/^commit/ {print}' | head -200Repository: openwisp/openwisp-radius
Length of output: 6713
🏁 Script executed:
# Check if there's a separate admin for RegisteredUser (not just inline)
rg -n "RegisteredUser" openwisp_radius/admin.py | grep -i adminRepository: openwisp/openwisp-radius
Length of output: 257
🏁 Script executed:
# Final check: what is the current commit we're reviewing?
git log -1 --format="%H %s"Repository: openwisp/openwisp-radius
Length of output: 143
🏁 Script executed:
# Check the diff that contains the organization readonly field addition
git log --all --reverse -p -- openwisp_radius/admin.py | grep -B 50 -A 10 '"organization",' | grep -B 50 -A 10 "RegisteredUserInline" | head -100Repository: openwisp/openwisp-radius
Length of output: 2586
🏁 Script executed:
# Find the exact commit that changed organization from non-readonly to readonly
git log --all --reverse -p -- openwisp_radius/admin.py | grep -B 20 -A 20 "organization.*readonly\|readonly.*organization" | head -100Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check what the previous version had for readonly_fields
git show HEAD~5:openwisp_radius/admin.py 2>/dev/null | grep -A 10 "class RegisteredUserInline" || echo "Can't show older version"Repository: openwisp/openwisp-radius
Length of output: 92
🏁 Script executed:
# Check the current PR/branch to see if organization being readonly is part of the change
git diff HEAD~10 -- openwisp_radius/admin.py | grep -B 5 -A 5 "organization"Repository: openwisp/openwisp-radius
Length of output: 97
🏁 Script executed:
# Check the full recent commit history more carefully
git log --oneline | head -5Repository: openwisp/openwisp-radius
Length of output: 110
🏁 Script executed:
# Look at the initial RegisteredUserInline commit in detail
git log --all --reverse --oneline -- openwisp_radius/admin.py | grep -i "registered"Repository: openwisp/openwisp-radius
Length of output: 217
🏁 Script executed:
# Check if there's a way to set organization elsewhere - perhaps via signals or a separate form
rg -n "RegisteredUser" openwisp_radius/base/models.py | head -20Repository: openwisp/openwisp-radius
Length of output: 505
🏁 Script executed:
# Verify the critical concern: with extra=0 and readonly organization, check if new rows can be added at all
# Look for any custom formset or form handling that might override this
rg -n "AlwaysHasChangedForm\|RegisteredUserInline" openwisp_radius/admin.py -A 20 | head -60Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check what the initial commit for RegisteredUserInline intended
git log --all --reverse --oneline -- openwisp_radius/admin.py | head -1 | cut -d' ' -f1 | xargs git show | grep -A 30 "RegisteredUserInline" | head -40Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check if there's documentation or comments about why extra=0
rg -B 10 -A 5 "extra = 0" openwisp_radius/admin.py | grep -B 10 -A 5 "RegisteredUserInline"Repository: openwisp/openwisp-radius
Length of output: 458
🏁 Script executed:
# Most important: verify Django inline behavior - with extra=0, can admin add new records?
# Check if there are any tests showing the intended usage
fd -e py test openwisp_radius/tests | head -5 | xargs rg -l "RegisteredUserInline" 2>/dev/null | head -3Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Examine if has_add_permission is explicitly missing or intentionally so
sed -n '537,560p' openwisp_radius/admin.py | grep "has_add_permission" || echo "Not found - intentionally missing"Repository: openwisp/openwisp-radius
Length of output: 101
Readonly organization prevents creating org-scoped registrations from the inline.
RegisteredUser is multitenant, but the organization field is marked readonly. This blocks admins from creating or modifying per-organization verification records from the User admin inline—even though the inline permits adding new rows (no has_add_permission override), newly added RegisteredUser rows will default to organization=None because the field cannot be edited.
Suggested fix
- readonly_fields = (
- "organization",
- "modified",
- )
+ readonly_fields = ("modified",)
fields = ("organization", "method", "is_verified", "modified")📝 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.
| readonly_fields = ( | |
| "organization", | |
| "modified", | |
| ) | |
| fields = ("organization", "method", "is_verified", "modified") | |
| readonly_fields = ("modified",) | |
| fields = ("organization", "method", "is_verified", "modified") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/admin.py` around lines 541 - 545, The RegisteredUser admin
marks "organization" in readonly_fields which prevents org-scoped RegisteredUser
instances from being created/edited in the User admin inline; remove
"organization" from readonly_fields (or conditionally only make it readonly in
contexts where editing is disallowed) and ensure "organization" remains in
fields so the inline form can set it; update the RegisteredUserAdmin (and any
InlineModelAdmin referencing RegisteredUser) to allow editing/setting
organization when adding rows (or override get_readonly_fields to return
readonly only for change view where needed) so new inline rows do not default to
organization=None.
| def test_phone_change_unverifies_only_specific_org(self, *args): | ||
| org2 = self._create_org(name="org2", slug="org2") | ||
| org2_settings = OrganizationRadiusSettings.objects.get_or_create( | ||
| organization=org2 | ||
| )[0] | ||
| org2_settings.sms_verification = True | ||
| org2_settings.needs_method = True | ||
| org2_settings.sms_sender = "+595972157632" | ||
| org2_settings.full_clean() | ||
| org2_settings.save() | ||
| self._create_org_user(organization=org2) | ||
|
|
||
| self._register_user(expect_users=None) | ||
| user = User.objects.get(email=self._test_email) | ||
| user_token = Token.objects.get(user=user) | ||
|
|
||
| phone_token = PhoneToken.objects.create( | ||
| user=user, | ||
| ip="127.0.0.1", | ||
| phone_number="+393664255801", | ||
| ) | ||
| url = reverse("radius:phone_token_validate", args=[self.default_org.slug]) | ||
| response = self.client.post( | ||
| url, | ||
| json.dumps({"code": phone_token.token}), | ||
| content_type="application/json", | ||
| HTTP_AUTHORIZATION=f"Bearer {user_token.key}", | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
|
|
||
| reg_org1 = RegisteredUser.objects.get(user=user, organization=self.default_org) | ||
| self.assertEqual(reg_org1.is_verified, True) | ||
|
|
||
| url = reverse("radius:phone_number_change", args=[self.default_org.slug]) | ||
| with mock.patch("openwisp_radius.utils.SmsMessage.send"): | ||
| response = self.client.post( | ||
| url, | ||
| json.dumps({"phone_number": "+595972157444"}), | ||
| content_type="application/json", | ||
| HTTP_AUTHORIZATION=f"Bearer {user_token.key}", | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
|
|
||
| reg_org1.refresh_from_db() | ||
| self.assertEqual(reg_org1.is_verified, False) |
There was a problem hiding this comment.
This isolation test never creates the second org’s RegisteredUser.
Line 961 creates a different org member, and the test never adds an org2 registration for user. As written, it only proves the default-org row is unset; it does not prove another organization’s verified row survives the phone change.
Suggested fix
def test_phone_change_unverifies_only_specific_org(self, *args):
org2 = self._create_org(name="org2", slug="org2")
@@
- self._create_org_user(organization=org2)
-
self._register_user(expect_users=None)
user = User.objects.get(email=self._test_email)
user_token = Token.objects.get(user=user)
+ self._create_org_user(organization=org2, user=user)
+ reg_org2 = RegisteredUser.objects.create(
+ user=user,
+ organization=org2,
+ method="mobile_phone",
+ is_verified=True,
+ )
@@
reg_org1.refresh_from_db()
+ reg_org2.refresh_from_db()
self.assertEqual(reg_org1.is_verified, False)
+ self.assertEqual(reg_org2.is_verified, True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/tests/test_api/test_phone_verification.py` around lines 951 -
995, The test test_phone_change_unverifies_only_specific_org never creates a
RegisteredUser for org2 so it doesn't assert the other organization's
verification survives; fix it by creating a registration for org2 (e.g. call the
existing helper _create_org_user with organization=org2 and the same user or
create RegisteredUser for user and org2 and set is_verified=True) before
performing the phone change, then after the change assert
RegisteredUser.objects.get(user=user, organization=org2).is_verified is still
True; this targets the test function
test_phone_change_unverifies_only_specific_org and the RegisteredUser model to
ensure isolation is verified.
There was a problem hiding this comment.
@nemesifier when I thought more about test_phone_change_unverifies_only_specific_org, the test looked wrong. Since, the phone number is saved in the User model changing the phone number will change it for all organizations. So, technically, we should unverify the user from all Organizations.
But, doing so will make the user verify to every organization they had previously registered to.
Do you think that the result is acceptable?
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!
There was a problem hiding this comment.
If a user changes their phone number during cross-organization login, then we will flag the user as unverified in all Organizations where the user has registered using mobile_phone method. As soon as they verify the phone number in one organization, we will mark them as verified.
pandafy
force-pushed
the
issues/692-different-identity-verification
branch
from
1dcf3ad to
0f904c3
Compare
|
@coderabbitai review! Summarize if any previously flagged comment has been unresolved. Focus on real problems |
Flake8 F401 Failure in Multiple JobsHello @pandafy, There is a
Fix: Remove the unused import statement from the specified file. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 225511 🏁 Script executed: Length of output: 21136
Here’s the focused status on previously flagged items. I verified the current code in the repo and called out only real problems that still need changes. Resolved since prior reviews
Unresolved, high-impact issues
diff --git a/openwisp_radius/api/views.py b/openwisp_radius/api/views.py
@@
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.is_verified = True
+ reg_user.method = "mobile_phone"
@@
- user.save()
- reg_user.save()
+ user.save()
+ reg_user.save(update_fields=["is_verified", "method"])
diff --git a/openwisp_radius/integrations/monitoring/tasks.py b/openwisp_radius/integrations/monitoring/tasks.py
@@
- registration_method = (
- RegisteredUser.objects.only("method")
- .filter(user__username=username)
- .filter(Q(organization_id=organization_id) | Q(organization__isnull=True))
- .order_by("-organization_id")
- .first()
- )
+ registration_method = (
+ RegisteredUser.objects.only("method")
+ .filter(user__username=username, organization_id=organization_id)
+ .first()
+ )
+ if registration_method is None:
+ registration_method = (
+ RegisteredUser.objects.only("method")
+ .filter(user__username=username, organization__isnull=True)
+ .first()
+ )
@@
- registration_method = clean_registration_method(registration_method)
+ registration_method = clean_registration_method(registration_method)
@@
- extra_tags = {
- "method": registration_method,
+ extra_tags = {
+ **({"method": registration_method} if registration_method is not None else {}),
"organization_id": organization_id,
"calling_station_id": sha1_hash(calling_station_id),
"called_station_id": called_station_id,
}
diff --git a/openwisp_radius/integrations/monitoring/tasks.py b/openwisp_radius/integrations/monitoring/tasks.py
@@
- users_without_registereduser_query = OrganizationUser.objects.filter(
- user__registered_users__isnull=True
- )
+ from django.db.models import Exists, OuterRef
+ users_without_registereduser_query = OrganizationUser.objects.filter(
+ ~Exists(
+ RegisteredUser.objects.filter(
+ user_id=OuterRef("user_id"),
+ organization_id=OuterRef("organization_id"),
+ )
+ )
+ )Optional follow-up (if you want to de-duplicate signups across orgs): derive org from RegisteredUser.organization_id and ensure the time window matches your business definition, but that is a product decision.
diff --git a/openwisp_radius/migrations/__init__.py b/openwisp_radius/migrations/__init__.py
@@ def migrate_registered_users_multitenant_forward(...):
- to_delete_pks = []
for registered_user in batch:
organization_ids = sorted(memberships.get(registered_user.user_id, ()))
if not organization_ids:
continue
- to_delete_pks.append(registered_user.pk)
extra_kwargs = _registered_user_extra_kwargs(registered_user, extra_fields)
@@
- _flush_bulk_create(RegisteredUser, to_create)
- if to_delete_pks:
- RegisteredUser.objects.filter(pk__in=to_delete_pks).delete()
+ _flush_bulk_create(RegisteredUser, to_create)For the reverse path, prefer updating an existing weaker global instead of deleting org rows blindly; if you want, I can draft that patch next.
diff --git a/openwisp_radius/admin.py b/openwisp_radius/admin.py
@@
-from django.contrib.auth.admin import UserAdmin
+from django.contrib.auth.admin import UserAdmin
+from django.db.models import Exists, OuterRef, Q
@@
+def _annotated_user_qs(self, request):
+ from openwisp_radius.base.models import RegisteredUser
+ base = super(UserAdmin, self).get_queryset(request)
+ ru = RegisteredUser.objects.filter(user_id=OuterRef("pk"))
+ ru_verified = RegisteredUser.objects.filter(user_id=OuterRef("pk"), is_verified=True)
+ return base.annotate(
+ has_ru=Exists(ru),
+ has_ru_verified=Exists(ru_verified),
+ )
+UserAdmin.get_queryset = _annotated_user_qs
@@
def get_is_verified(self, obj):
- try:
- if not obj.registered_users.exists():
- value = "unknown"
- elif obj.registered_users.filter(is_verified=True).exists():
- value = "yes"
- else:
- value = "no"
- except Exception:
- value = "unknown"
+ value = "unknown"
+ if getattr(obj, "has_ru", False) is False:
+ value = "unknown"
+ elif getattr(obj, "has_ru_verified", False):
+ value = "yes"
+ else:
+ value = "no"
icon_url = static(f"admin/img/icon-{value}.svg")
return mark_safe(f'<img src="{icon_url}" alt="{value}">')
diff --git a/openwisp_radius/base/models.py b/openwisp_radius/base/models.py
@@
if (
not created
and self.organization.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)Medium/low-priority items to consider
diff --git a/openwisp_radius/api/serializers.py b/openwisp_radius/api/serializers.py
@@ class UpdateRegisteredUserMethodSerializer(serializers.ModelSerializer):
+ def __init__(self, *args, **kwargs):
+ super().__init__(*args, **kwargs)
+ self.fields["method"].choices = get_registration_choices()
Items I did not re-verify in this pass (flagged earlier)
Would you like me to open follow-up GitHub issues for the migration helper adjustments and the monitoring fixes? I can file them with links back to this PR and comment thread. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Below are the items that I think still need to be addressed before merge.
Bugs / security
Cross-org verification leak in the FreeRADIUS authorize query
openwisp_radius/api/freeradius_views.py:405-429 builds the verified-user
filter as:
org_or_global = Q(registered_users__organization_id=organization_id) | Q(
registered_users__organization__isnull=True
)
is_verified = Q(registered_users__is_verified=True) & org_or_globalBecause this is a JOIN-based filter, a user with any registered_users row
satisfying the conditions passes. In particular, a user that has a global
record (organization=NULL) marked is_verified=True plus an org-specific
record for the org being authenticated marked is_verified=False is wrongly
authorized — exactly the bypass scenario the issue is trying to prevent.
api/utils.py:IDVerificationHelper.is_identity_verified_strong already has
the right behavior (prefer org-specific, fall back to global). The FreeRADIUS
side should match it: do an explicit lookup that uses the org-specific row
when present and only falls back to the global row when no org-specific row
exists. The same applies to the authorize_unverified branch a few lines
below.
test_global_fallback_for_orgs_without_specific_records and
test_multi_org_user_different_verification_states cover the "only one
record" cases but not the conflicting "global verified + org-specific
unverified" case.
ValidatePhoneTokenView leaves the method as pending_verification
openwisp_radius/api/views.py:766-782:
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.save()defaults is only applied when the row is created. In the cross-org path the
row already exists with method="pending_verification" (created by
ObtainAuthTokenView.validate_membership), so after a successful SMS
verification the row ends up is_verified=True but method= "pending_verification". Because pending_verification is in
_weak_verification_methods, is_identity_verified_strong keeps returning
False and the user remains locked out. The expected flow currently relies on
the client calling update_registered_user_registration_method first; if
they don't, validation appears to succeed but login keeps returning 401 with
no clear remediation.
Either flip the method to mobile_phone here when the existing one is
pending_verification, or reject the validation request until the method has
been chosen. There is no test covering the "skipped UpdateRegisteredUserMethod
step" path.
post_save_radiusaccounting orders NULLs the wrong way (Postgres)
openwisp_radius/integrations/monitoring/tasks.py:189-198:
RegisteredUser.objects
.only("method")
.filter(user__username=username)
.filter(Q(organization_id=organization_id) | Q(organization__isnull=True))
.order_by("-organization_id")
.first()The intent (per surrounding code) is to prefer the org-specific row over the
global one. But on PostgreSQL the default for ORDER BY x DESC is NULLS FIRST, so when both rows are present .first() returns the global one.
SQLite/MySQL actually behave the opposite way, so the result also depends on
the backend, which is worse. Use an explicit ordering — e.g.
F("organization_id").desc(nulls_last=True) — or two queries (org-specific
first, fall back to global).
Resulting metric tag can be None
clean_registration_method was changed to return None for
pending_verification (integrations/monitoring/utils.py). The two
_write_user_signup_metric_* helpers correctly skip the None case, but
post_save_radiusaccounting does not — it assigns
extra_tags["method"] = registration_method regardless. A user whose
registration is still pending will produce metric points with a literal
method=None tag. Either skip writing the metric, or fall back to
"unspecified".
social/views.py saves twice after get_or_create
openwisp_radius/social/views.py:51-58:
registered_user, created = RegisteredUser.objects.get_or_create(
user=user, organization=org,
defaults={"method": "social_login", "is_verified": False},
)
if created:
registered_user.full_clean()
registered_user.save()get_or_create already persists the new row, so the full_clean()/save()
runs against an already-saved object. If full_clean() were to raise, the
record would already exist in the DB — the validation has no chance to block
insertion. The matching block in saml/views.py was already cleaned up; this
one should follow the same pattern (drop the redundant full_clean() and
save(), or do validate_constraints/full_clean() before the
get_or_create).
Code quality / dead code
Dead except in UpdateRegisteredUserMethodView.post()
openwisp_radius/api/views.py:855-866:
try:
reg_user = get_object_or_404(
RegisteredUser, user_id=user.pk, organization=self.organization,
)
except RegisteredUser.DoesNotExist:
raise NotFound(...)get_object_or_404 raises Http404, not DoesNotExist, so the except
branch is unreachable. Either drop the try/except entirely (Http404 already
yields a 404 response with the swagger-documented message) or replace it with
RegisteredUser.objects.get(...) and catch DoesNotExist to produce the
custom message.
Dead except ObjectDoesNotExist in OpenwispRadiusSaml2Backend
openwisp_radius/saml/backends.py:14-33. The old code accessed
user.registered_user.method (the OneToOne could raise
ObjectDoesNotExist); the new code uses
user.registered_users.exclude(method="saml").exists(), which can never
raise that exception. The try/except ObjectDoesNotExist: pass wrapper is
dead.
Behavior change in OpenwispRadiusSaml2Backend._update_user
While in there: the new check is "skip the SAML username update if any
record on this user is non-SAML." With multi-tenancy this is more
conservative than the old "skip if this user's method is non-SAML". For a
user that signed up via mobile_phone in org A and via SAML in org B, the
SAML login on org B will no longer update their username, even though for
that org they did sign up via SAML. If that is intentional it deserves a
comment; otherwise the check should target the org being authenticated (the
SAML login already knows the org from the relay state).
AbstractRegisteredUser.clean() mostly duplicates the unique constraint
openwisp_radius/base/models.py:1693-1702 re-implements the per-org
uniqueness check in Python. The two UniqueConstraints declared on the
model are validated by Django's validate_constraints() during
full_clean() (including the partial one with the Q(organization__isnull=True)
condition), so the dedicated clean() runs an extra query without
catching anything new. The one thing it does add is a custom error
message; if that's the goal, consider customizing the constraint
violation message via the violation_error_message argument on the
UniqueConstraint instead and dropping the extra query. Not blocking.
Documentation / compatibility
export_users CSV output format changed
openwisp_radius/settings.py:232-241 now contributes a single
registered_users column whose value is rendered as
((organization_id,method,is_verified))(...) instead of the previous two
flat columns registered_user.method / registered_user.is_verified. The
test in tests/test_users_integration.py was rewritten to match. Anything
external that consumes that CSV will break, so this should be called out
in the release notes / upgrade guide.
REST API doc has an awkward sentence
docs/user/rest-api.rst (Update Registered User Method section) says:
"The user must complete verification for that organization before they can
create account with the new organization." Reads oddly — should be e.g.
"before they can finish registering with the new organization" or similar.
Tests / coverage gaps
- No test for the cross-org leak above (global verified + org-specific
unverified) on the FreeRADIUS authorize path. - No test for
ValidatePhoneTokenViewbeing called without first calling
update_registered_user_registration_method(the bug above). post_save_radiusaccountingis not tested with both an org-specific and a
globalRegisteredUserfor the same user.- The end-to-end multi-org flow test
(test_multi_org_phone_verification_flow) only verifies the happy path
where the client follows the prescribed order (login → update method →
verify); error/edge orderings are not covered.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ) | ||
| registered_user.full_clean() | ||
| registered_user.save() | ||
| if not created: |
There was a problem hiding this comment.
CRITICAL: Potential logic inversion. The if not created condition means full_clean() and save() are only called when the record ALREADY EXISTS, not when it's newly created. This may be intentional to update existing records, but verify this is the desired behavior - typically you'd want to validate NEW records.
3 participants
Checklist
Reference to Existing Issue
Closes #692
Description of Changes
Breaking Changes
registered_usersCSV export column format from two flatcolumns (
registered_user.method,registered_user.is_verified)to a single nested column (
registered_users) containing((organization_id,method,is_verified))tuples. External integrationsconsuming this CSV output will need to be updated.
Blockers