[feature] Made RegisteredUser model support multi-tenancy #692#698
[feature] Made RegisteredUser model support multi-tenancy #692#698pandafy wants to merge 46 commits into
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.
Code Review SummaryStatus: 1 CRITICAL Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Issue: The default value for USER_SETTABLE_REGISTRATION_METHODS includes bank_card, but this method is not defined in REGISTRATION_METHOD_CHOICES in registration.py. The validate_user_settable_registration_methods() function checks that all methods exist in REGISTRATION_METHOD_CHOICES, and will raise an ImproperlyConfigured exception if bank_card is used without being registered first. Fix: Either:
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by kimi-k2.5 · 157,120 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.376% (-0.08%) from 97.453% — 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.
Flake8 E501 Line Too LongHello @pandafy, The CI build failed due to a Fix: |
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
|
|
@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.
pandafy
force-pushed
the
issues/692-different-identity-verification
branch
from
1dcf3ad to
0f904c3
Compare
pandafy
force-pushed
the
issues/692-different-identity-verification
branch
from
7812992 to
494c9fc
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I spotted a few things that need to be improved.
| return apps.get_model(app, model) | ||
|
|
||
|
|
||
| def _batched_iterator(iterator, batch_size=BATCH_SIZE): |
There was a problem hiding this comment.
This is a lot of new logic in this file, is it used only in the last migrations or is it meant to be reusable?
It may make sense to move this to a dedicated file.
There was a problem hiding this comment.
We re-use this in the sample app.
| # and no method should authorize unverified users | ||
| # ensure user is active AND verified | ||
| # Use subqueries to ensure org-specific records take precedence over | ||
| # global (organization=NULL) records. |
| primary_key=True, | ||
| related_name="registered_users", | ||
| ) | ||
| organization = models.ForeignKey( |
There was a problem hiding this comment.
Why not just inherit OrgMixin as we have in pretty much all the django models of OpenWISP?
| # with an org-specific unverified record. | ||
| # | ||
| # Strategy: check if org-specific record exists and satisfies criteria; | ||
| # if not, fall back to checking the global record. This matches the |
There was a problem hiding this comment.
All this complexity for what exactly?
| ) | ||
|
|
||
| # is_verified: user passes if org-specific record is verified, or if | ||
| # no org-specific record exists and the global record is verified. |
There was a problem hiding this comment.
Also this code I think needs to be simplified as we only care for the org specific record and nothing else
| phone_number string | ||
| ============ =========== | ||
|
|
||
| Update Registered User Method |
There was a problem hiding this comment.
| Update Registered User Method | |
| Update user registration method |
| ============ =========== | ||
|
|
||
| Update Registered User Method | ||
| +++++++++++++++++++++++++++++ |
There was a problem hiding this comment.
| +++++++++++++++++++++++++++++ | |
| +++++++++++++++++++++++++++++++ |
| attribute_mapping = attribute_mapping.copy() | ||
| # Check if any of the user's registered_users records | ||
| # were NOT created via SAML. | ||
| # NOTE: This uses a global check (any org) rather than org-specific. |
There was a problem hiding this comment.
Also here the code still mentions the global settings as a real thing.
There was a problem hiding this comment.
Here, global does not mean global RegisteredUser. As you can see that the queryset does not check (organization=None), but it checks if the user was not registered with SAML.
There was a problem hiding this comment.
Tests that would catch the issues mentioend in my other inline comments below:
- Add a multi organization SMS token test: create a token through organization A, then try to validate it through organization B. Organization B must not be verified by organization A's token.
- Add monitoring metric coverage with one user, two organization memberships, and two org specific RegisteredUser rows with different methods. Each organization should be counted only with its own method.
- Add SAML and social tests where an existing
pending_verificationrow is updated after a successful org scoped login. - Add a sample swapped app migration/schema test that confirms
RegisteredUser.organizationis non nullable at the database level. - After optimizing the admin verified column, add a changelist query count test to prevent one
registered_usersquery per row.
| user=OuterRef("pk"), | ||
| organization_id=organization_id, | ||
| ) | ||
| global_only = RegisteredUser.objects.filter( |
There was a problem hiding this comment.
This global fallback should be removed now that organization=None RegisteredUser rows are out of scope. The authorize endpoint can require the row for request.auth directly. That also lets us drop the correlated Exists subqueries from this hot path and use a much simpler organization scoped Q filter.
| ) | ||
| try: | ||
| is_valid = phone_token.is_valid(serializer.data["code"]) | ||
| is_valid = phone_token.is_valid( |
There was a problem hiding this comment.
Passing the organization into validation is not enough because the token selected above is still the latest token for the user, regardless of organization. A token created for org A can be submitted to org B and verify org B's RegisteredUser row. PhoneToken needs to store the requested organization, and token creation, active status, sending, and validation should all filter/use that organization.
| @@ -115,6 +117,8 @@ def _write_user_signup_metrics_for_orgs(metric_key): | |||
| # The query returns a tuple of organization_id, registration_method and | |||
| # count of users who registered with that organization and method. | |||
| registered_users_query = RegisteredUser.objects.exclude( | |||
There was a problem hiding this comment.
The org metrics query below still groups through user__openwisp_users_organizationuser__organization_id. With per organization RegisteredUser rows, it should group by RegisteredUser.organization_id instead. For a user with two memberships and two RegisteredUser rows, the current join can produce every organization/method combination and count methods under the wrong organization. The users_without_registereduser query has the related problem: it only finds users with no RegisteredUser anywhere, not users missing a row for the specific organization.
| orgUser = OrganizationUser(organization=org, user=user) | ||
| orgUser.full_clean() | ||
| orgUser.save() | ||
| registered_user, created = RegisteredUser.get_or_create_for_user_and_org( |
There was a problem hiding this comment.
This only applies the SAML method and SAML_IS_VERIFIED when the org specific row is newly created. If the row already exists as pending_verification, for example after password login added the user to the org, a successful SAML login leaves it pending and possibly unverified. Existing pending rows should be updated to reflect the successful SAML login, while preserving stronger existing states.
| orgUser = OrganizationUser(organization=org, user=user) | ||
| orgUser.full_clean() | ||
| orgUser.save() | ||
| registered_user, created = RegisteredUser.get_or_create_for_user_and_org( |
There was a problem hiding this comment.
Same issue as the SAML path: defaults only apply on create. If this org row already exists as pending_verification, successful social login leaves it pending instead of recording social_login. Please update pending rows explicitly or document why they should remain pending.
| def get_is_verified(self, obj): | ||
| try: | ||
| value = "yes" if obj.registered_user.is_verified else "no" | ||
| is_verifieds = obj.registered_users.values_list("is_verified", flat=True) |
There was a problem hiding this comment.
This introduces one registered_users query per user row in the admin changelist. With the new one to many relation, please prefetch or annotate the verification state in the user admin queryset and read that cached value here.
| ) | ||
| registered_user.full_clean() | ||
| registered_user.save() | ||
| if not created: |
There was a problem hiding this comment.
This branch validates and saves an unchanged RegisteredUser row, so it is a no-op database write. It can be removed, or replaced with explicit pending row update logic if that is the intended behavior.
| registered_user.save() | ||
| # The user is just created, it will not have an email address | ||
| if user.email: | ||
| if created and user.email: |
There was a problem hiding this comment.
created here refers to the RegisteredUser row, not the user. On cross organization SAML login, the user may already have this email address, so creating a new EmailAddress can raise a validation error and log a misleading exception even though the login continues. Please update the comment and use get_or_create() or check for an existing EmailAddress first.
|
|
||
| This endpoint is used during cross-organization login when a user | ||
| authenticates to a new organization. The user must complete verification | ||
| for that organization before they can create account with the new |
There was a problem hiding this comment.
This describes the flow inaccurately. The implementation already creates the OrganizationUser and a pending RegisteredUser row during cross organization login when registration is enabled; access remains denied until org specific verification is completed. Please reword this to avoid saying the account is created only after verification.
| # elsewhere. Since the User model is shared across organizations, | ||
| # updating the username based solely on one org's SAML flow could | ||
| # unexpectedly change the user's identity in other orgs. | ||
| has_non_saml = user.registered_users.exclude(method="saml").exists() |
There was a problem hiding this comment.
This may be intentional because User.username is global, but it deserves an explicit decision with the new per organization registration model. A user who registered with a non SAML method in org A will not have their username updated when logging in with SAML to org B. If this global policy is intended, please make the comment clear that this is about the shared User model, not leftover global RegisteredUser fallback behavior.
<h3>Black, Flake8 Failures, and Syntax Warnings</h3>Hello @pandafy,
|
Multiple CI Failures DetectedHello @pandafy,
|
Test Failures in
|
# Conflicts: # openwisp_radius/tests/test_migrations.py
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I did some testing with the related WLP branch and I haven't found bugs up to now but I still have some concerns which I explained inline.
One general note: I had asked to move the new migration helpers out of migrations/__init__.py, but I realize it's quite futile as there is already a lot there. So there's nothing to do there now, but please keep in mind that this file is becoming huge and we shouldn't keep adding more as it makes maintenance harder. I wrote about squashing old migrations here: #705 (comment)
| super().__init__(*args, **kwargs) | ||
| self.fields["method"].choices = get_registration_choices() | ||
|
|
||
| def validate_method(self, value): |
There was a problem hiding this comment.
I do not think we can blindly trust all registered method values here. Some values are not normal user choices: they represent server side flows which should set the method only after that flow actually happened. Practical examples are saml and social_login.
Right now, could a user submit {"method": "saml"} or {"method": "social_login"} here? If yes, the DB would say the user registered with SAML or social login even though neither flow happened. Maybe this does not lead to an immediate bypass today, but it is still unsafe to let users write values which other parts of the system may treat as trusted registration state.
Please let me know what you think. If you think it cannot be abused, explain why. Otherwise let's discuss the right way to restrict this endpoint before changing it, because a quick allowlist/denylist could also be wrong if we do not define the intended flow clearly.
There was a problem hiding this comment.
@nemesifier the RegisterSerializer already accepts a method field
openwisp-radius/openwisp_radius/api/serializers.py
Lines 568 to 575 in bf61cdb
So the tightening of the allowed registration choices should also apply there.
There was a problem hiding this comment.
Makes sense. We can add another setting which specifies with registration methods are self initiated.
| registered_users_query = RegisteredUser.objects.exclude( | ||
| method="pending_verification" | ||
| ).exclude( | ||
| user__openwisp_users_organizationuser__created__gt=end_time, |
There was a problem hiding this comment.
The grouping was fixed to use RegisteredUser.organization_id, good. But the time filter still goes through user__openwisp_users_organizationuser__created, which can match any membership of the user, not necessarily the membership for the same organization as this RegisteredUser row.
Example: a user joined org A months ago and joins org B now. The current query can count the org A RegisteredUser again in user_signups, because the user has a recent membership in org B.
The users_without_registereduser_query below has the same org scoping problem: it only counts users with no RegisteredUser anywhere, so a user with a row in org A but no row in org B will not be counted as unspecified for org B.
| ) | ||
| if self.value() == "unknown": | ||
| return queryset.filter(registered_user__isnull=True) | ||
| where &= Q(registered_users__isnull=True) |
There was a problem hiding this comment.
This does not look right for organization managers. For non superusers, where already requires registered_users__organization__in=request.user.organizations_managed; adding registered_users__isnull=True means both conditions must be true on the same join, which cannot happen.
This also does not match the admin display logic, where a user is shown as unknown when there is no RegisteredUser in the manager's organizations. The test added around openwisp_radius/tests/test_admin.py:1687-1690 seems to encode the wrong expectation: org2_only is displayed as unknown for the manager, so the unknown filter should include it.
I think unknown means no RegisteredUser exists for this user in the managed organizations, using ~Exists(...) or an equivalent filtered relation. Is my understanding incorrect? If so, please let me know.
| user=user, primary=True | ||
| ) | ||
| email_address, email_created = EmailAddress.objects.get_or_create( | ||
| user=user, |
There was a problem hiding this comment.
EmailAddress.objects.get_or_create() saves the new row before email_address.full_clean() is called below. Is this on purpose and if so, why? It looks wrong to me because an invalid user.email would already have been written by the time validation runs.
Co-authored-by: Copilot <copilot@github.com>
Test Failures in
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I found two outstanding issues that should be addressed before merging.
| choices=(), | ||
| ) | ||
|
|
||
| def __init__(self, instance=None, data=..., **kwargs): |
There was a problem hiding this comment.
This data=... default is not DRF's empty sentinel. Instantiating RegisterSerializer() without data passes Ellipsis as data, so the serializer becomes bound with invalid initial_data. This can break schema generation, browsable API behavior, or any code expecting an unbound serializer. I would use *args, **kwargs or the DRF empty sentinel already imported in this file.
| model = RegisteredUser | ||
| fields = ["method"] | ||
|
|
||
| def __init__(self, instance=None, data=..., **kwargs): |
There was a problem hiding this comment.
Same issue here: data=... makes UpdateRegisteredUserMethodSerializer() bound even when no data was provided. I verified initial_data == Ellipsis and is_valid() == False on an empty serializer instance. Please use *args, **kwargs or DRF's empty sentinel.
| return [(method, available_choices[method]) for method in methods] | ||
|
|
||
|
|
||
| validate_user_settable_registration_methods( |
There was a problem hiding this comment.
This validation runs at module import time, before external apps can register custom methods with register_registration_method(). A project setting OPENWISP_RADIUS_USER_SETTABLE_REGISTRATION_METHODS = ["custom_identity"] and registering that method from an app will fail startup because the method is still unknown here. The test covers manual validation after registering the custom method, but the real startup flow is different. I think this should be delayed until registrations are complete, or moved to a system check.
Co-authored-by: Copilot <copilot@github.com>
Configuration Error in SettingsHello @pandafy, There is a Fix: |
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