[fix] Added setting to disable cross-organization login#674
[fix] Added setting to disable cross-organization login#674
Conversation
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
WalkthroughAdds a global setting OPENWISP_RADIUS_CROSS_ORGANIZATION_LOGIN_ENABLED (default True) and a per-organization field cross_organization_login_enabled on OrganizationRadiusSettings with migration. ObtainAuthTokenView.validate_membership now requires both the global setting and the organization’s cross_organization_login_enabled (in addition to registration_enabled) before creating an OrganizationUser link for an existing user authenticating to an organization. Tests, admin, settings, migrations, and docs were updated to reflect this behavior. Sequence Diagram(s)sequenceDiagram
participant Client
participant APIView as ObtainAuthTokenView
participant Global as OPENWISP_RADIUS_CROSS_ORGANIZATION_LOGIN_ENABLED
participant OrgSettings as OrganizationRadiusSettings
participant OrgUser as OrganizationUser
participant DB as Database
Client->>APIView: POST /api-token-auth (credentials + target org)
APIView->>Global: read setting
Global-->>APIView: True/False
alt Global = True
APIView->>OrgSettings: read registration_enabled & cross_organization_login_enabled
OrgSettings-->>APIView: settings
alt org.registration_enabled AND org.cross_organization_login_enabled
APIView->>OrgUser: attempt to create OrganizationUser for user & org
OrgUser->>DB: validate & save
DB-->>OrgUser: success / error
APIView-->>Client: 200 OK (token) or 400 ValidationError
else
APIView-->>Client: 403 Forbidden (self-registration not allowed)
end
else
APIView-->>Client: 403 Forbidden (cross-organization login disabled)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pandafy
force-pushed
the
fix-registration-to-multiple-organizations
branch
from
a596fdb to
c1bf421
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_radius/api/views.py (1)
321-339:⚠️ Potential issue | 🟠 MajorAvoid KeyError when ValidationError lacks
__all__.
error.message_dict.pop("__all__")can raise a KeyError and turn a validation error into a 500.✅ Safer error propagation
- except ValidationError as error: - raise serializers.ValidationError( - {"non_field_errors": error.message_dict.pop("__all__")} - ) + except ValidationError as error: + error_dict = error.message_dict.copy() + if "__all__" in error_dict: + error_dict["non_field_errors"] = error_dict.pop("__all__") + elif not error_dict: + error_dict = {"non_field_errors": error.messages} + raise serializers.ValidationError(error_dict)
🤖 Fix all issues with AI agents
In `@docs/user/settings.rst`:
- Around line 631-660: The docs imply cross-organization login by
OPENWISP_RADIUS_CROSS_ORGANIZATION_LOGIN_ENABLED alone is sufficient; update the
text (the section for OPENWISP_RADIUS_CROSS_ORGANIZATION_LOGIN_ENABLED and its
references to radius_login_obtain_user_auth_token and
radius_registering_to_multiple_organizations) to explicitly state that the
target organization's "allow registrations" (registration-enabled) setting must
also be enabled for automatic cross-organization login/registration to occur,
and add a short example sentence clarifying that if registration is disabled on
the target org the login will be blocked even when
OPENWISP_RADIUS_CROSS_ORGANIZATION_LOGIN_ENABLED is True.
In
`@openwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py`:
- Around line 14-25: The verbose_name on the AddField migration for model_name
"organizationradiussettings" and name "cross_organization_login_enabled" uses
"registration" but should match the field and setting terminology "login";
update the verbose_name value in this migration (the
openwisp_utils.fields.FallbackBooleanChoiceField declaration) to
"Cross-organization login enabled" so the admin UI wording is consistent with
the field name and setting.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/user/rest-api.rstdocs/user/settings.rstopenwisp_radius/api/views.pyopenwisp_radius/base/models.pyopenwisp_radius/migrations/0038_clean_fallbackfields.pyopenwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.pyopenwisp_radius/settings.pyopenwisp_radius/tests/test_api/test_rest_token.pyopenwisp_radius/tests/test_api/test_utils.py
🧰 Additional context used
🧬 Code graph analysis (5)
openwisp_radius/tests/test_api/test_utils.py (1)
openwisp_radius/utils.py (1)
get_organization_radius_settings(222-231)
openwisp_radius/tests/test_api/test_rest_token.py (2)
openwisp_radius/api/serializers.py (2)
create(239-258)create(416-419)tests/openwisp2/sample_users/models.py (1)
OrganizationUser(37-39)
openwisp_radius/api/views.py (1)
openwisp_radius/utils.py (1)
get_organization_radius_settings(222-231)
openwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py (1)
openwisp_radius/migrations/0038_clean_fallbackfields.py (1)
Migration(48-60)
openwisp_radius/migrations/0038_clean_fallbackfields.py (1)
openwisp_radius/utils.py (1)
get_model(37-39)
🪛 Ruff (0.14.14)
openwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py
[warning] 10-12: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 14-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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.10 | django~=4.2.0
- GitHub Check: Python==3.10 | 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.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (5)
openwisp_radius/migrations/0038_clean_fallbackfields.py (1)
16-18: LGTM — migration now uses apps.get_model directly.
This keeps the data migration aligned with Django’s recommended pattern.openwisp_radius/settings.py (1)
97-99: Looks good — setting follows existing configuration pattern.docs/user/rest-api.rst (1)
516-521: Docs update looks clear and aligned with behavior.Also applies to: 631-635
openwisp_radius/tests/test_api/test_utils.py (1)
83-130: LGTM — solid coverage for the new setting.
The test mirrors the established pattern for settings fallback and misconfiguration handling.openwisp_radius/tests/test_api/test_rest_token.py (1)
254-274: LGTM — clear negative-path coverage.
Good assertion of the 403 response and the absence of unintended OrganizationUser creation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py
Show resolved
Hide resolved
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: 1
🤖 Fix all issues with AI agents
In `@docs/user/rest-api.rst`:
- Around line 632-637: Update the phrase "cross organization login" to use
consistent hyphenation "cross-organization login" in the sentence that reads
"only if the organization has both registration and cross organization login
enabled" (and any other occurrences of that exact phrase) so it matches the rest
of the docs/settings; search for the string "cross organization login" and
replace with "cross-organization login" to keep style consistent.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/user/rest-api.rstdocs/user/settings.rstopenwisp_radius/api/views.pyopenwisp_radius/base/models.pyopenwisp_radius/migrations/0038_clean_fallbackfields.pyopenwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.pyopenwisp_radius/settings.pyopenwisp_radius/tests/test_api/test_rest_token.pyopenwisp_radius/tests/test_api/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (5)
- openwisp_radius/base/models.py
- openwisp_radius/tests/test_api/test_utils.py
- openwisp_radius/migrations/0038_clean_fallbackfields.py
- docs/user/settings.rst
- openwisp_radius/api/views.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_radius/tests/test_api/test_rest_token.py (1)
tests/openwisp2/sample_users/models.py (1)
OrganizationUser(37-39)
openwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py (1)
openwisp_radius/migrations/0038_clean_fallbackfields.py (1)
Migration(48-60)
🪛 Ruff (0.14.14)
openwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py
[warning] 10-12: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 14-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (9)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (3)
openwisp_radius/settings.py (1)
97-99: Looks good: defaulted global toggle is clear and consistent.
The new setting is wired through the standard config helper and defaults to True, preserving existing behavior.openwisp_radius/tests/test_api/test_rest_token.py (1)
254-274: Solid coverage for org-level disable flag.
Test asserts status, message, and absence of membership creation—good behavioral coverage.docs/user/rest-api.rst (1)
516-522: Doc note is clear and aligned with the new setting.
Nice addition describing the global disable behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pandafy
force-pushed
the
fix-registration-to-multiple-organizations
branch
from
c1bf421 to
5102e89
Compare
pandafy
force-pushed
the
fix-registration-to-multiple-organizations
branch
from
5102e89 to
58b8c1a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py (1)
1-1: Migration filename uses "registration" while the field uses "login".The filename
0043_organizationradiussettings_cross_organization_registration_enabled.pysays "registration" but the actual field iscross_organization_login_enabled. This is a cosmetic inconsistency — it won't break anything, but it could confuse developers browsing migrations later. Consider renaming for clarity if no migration has been applied in production yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py` at line 1, The migration filename mentions "registration" but the field is cross_organization_login_enabled; rename the migration file from 0043_organizationradiussettings_cross_organization_registration_enabled.py to 0043_organizationradiussettings_cross_organization_login_enabled.py for consistency, ensure the new filename is committed, and check/update any migration dependency references in other migrations (if any refer to the old module name) so imports/dependencies still point to the renamed migration; no code changes to the Migration class are needed unless you have hard-coded the old filename somewhere.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/images/organization_cross_login.pngis excluded by!**/*.png
📒 Files selected for processing (10)
docs/user/rest-api.rstdocs/user/settings.rstopenwisp_radius/admin.pyopenwisp_radius/api/views.pyopenwisp_radius/base/models.pyopenwisp_radius/migrations/0038_clean_fallbackfields.pyopenwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.pyopenwisp_radius/settings.pyopenwisp_radius/tests/test_api/test_rest_token.pyopenwisp_radius/tests/test_api/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/user/settings.rst
- docs/user/rest-api.rst
- openwisp_radius/base/models.py
- openwisp_radius/settings.py
🧰 Additional context used
🧬 Code graph analysis (5)
openwisp_radius/api/views.py (1)
openwisp_radius/utils.py (1)
get_organization_radius_settings(222-231)
openwisp_radius/tests/test_api/test_rest_token.py (1)
tests/openwisp2/sample_users/models.py (1)
OrganizationUser(37-39)
openwisp_radius/tests/test_api/test_utils.py (1)
openwisp_radius/utils.py (1)
get_organization_radius_settings(222-231)
openwisp_radius/migrations/0038_clean_fallbackfields.py (1)
openwisp_radius/utils.py (1)
get_model(37-39)
openwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py (1)
openwisp_radius/migrations/0038_clean_fallbackfields.py (1)
Migration(48-60)
🪛 Ruff (0.15.1)
openwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py
[warning] 10-12: Mutable default value for class attribute
(RUF012)
[warning] 14-27: Mutable default value for class attribute
(RUF012)
⏰ 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.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | 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.12 | django~=5.1.0
🔇 Additional comments (6)
openwisp_radius/tests/test_api/test_rest_token.py (1)
254-274: LGTM!The test correctly validates that when
cross_organization_login_enabled=False, the endpoint returns 403 and noOrganizationUseris created. It follows the same patterns as the existingtest_user_auth_token_different_organization_registration_settingstest.openwisp_radius/tests/test_api/test_utils.py (1)
83-128: LGTM!The test faithfully mirrors the existing
test_is_registration_enabledandtest_is_sms_verification_enabledpatterns, covering all four states (True, False, None/fallback, and missing settings).openwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py (1)
14-26: Field definition looks correct.The
FallbackBooleanChoiceFieldwithfallback=Trueanddefault=Nonealigns with the global settingCROSS_ORGANIZATION_LOGIN_ENABLEDdefaulting toTrue. The verbose_name is now consistent with the field name (addressed from prior review).openwisp_radius/migrations/0038_clean_fallbackfields.py (1)
16-18: Good fix: usingapps.get_modelinstead ofload_modelin a data migration.This is the correct Django pattern for data migrations. Using
apps.get_modelensures the model reflects the historical schema state at this migration point, avoiding issues if the model gains new fields (likecross_organization_login_enabled) in later migrations.openwisp_radius/admin.py (1)
582-582: LGTM!Logical placement of the new field right after
registration_enabledin the admin fieldset.openwisp_radius/api/views.py (1)
328-354: LGTM — correctly gates cross-org login on both settings.The updated
validate_membershipnow requires bothcross_organization_login_enabledandregistration_enabledto beTruebefore auto-creating anOrganizationUser. Short-circuit evaluation meansregistration_enabledis only checked whencross_organization_login_enabledisTrue, which is efficient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@openwisp_radius/migrations/0043_organizationradiussettings_cross_organization_registration_enabled.py`:
- Line 1: The migration filename mentions "registration" but the field is
cross_organization_login_enabled; rename the migration file from
0043_organizationradiussettings_cross_organization_registration_enabled.py to
0043_organizationradiussettings_cross_organization_login_enabled.py for
consistency, ensure the new filename is committed, and check/update any
migration dependency references in other migrations (if any refer to the old
module name) so imports/dependencies still point to the renamed migration; no
code changes to the Migration class are needed unless you have hard-coded the
old filename somewhere.
2 participants
Checklist