Skip to content

[fix] Allowed managing social auth secrets when needed#500

Merged
nemesifier merged 1 commit intomasterfrom
fix-socialauth
Apr 22, 2026
Merged

[fix] Allowed managing social auth secrets when needed#500
nemesifier merged 1 commit intomasterfrom
fix-socialauth

Conversation

@nemesifier
Copy link
Copy Markdown
Member

Before, OpenWISP Users removed the allauth.socialaccount admin sections to keep the admin UI simple, but over time more users need this to support OAuth/SAML authentication to the admin interface (manage app secrets).
With this change, the admin allows managing app secrets if any allauth.socialaccount.provider is installed, eg: microsoft, google, openid, etc.

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • N/A I have updated the documentation.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

📝 Walkthrough

Walkthrough

The changes introduce a new setting SOCIALACCOUNT_ADMIN_NEEDED that determines whether Allauth socialaccount models should remain registered in the Django admin. When disabled, the SocialApp model is conditionally added to an unregistration list that is then processed to remove these models from the admin site. The new setting is automatically set based on the presence of any OAuth/SAML provider apps in INSTALLED_APPS.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Bug Fixes ❌ Error The PR fixes a bug enabling OAuth/SAML admin management but lacks regression tests to verify the fix or prevent regressions. Add tests verifying SOCIALACCOUNT_ADMIN_NEEDED setting behavior and SocialApp admin registration/unregistration based on installed providers.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately summarizes the main change: conditionally enabling management of social auth secrets based on provider installation, matching the PR objectives.
Description check ✅ Passed The description includes the required sections with context about the change, but test coverage is incomplete (tests not written) and documentation is marked N/A without full checklist completion.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-socialauth

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@openwisp_users/settings.py`:
- Around line 54-61: Respect an explicit settings override before auto-detecting
providers: first check getattr(settings, "SOCIALACCOUNT_ADMIN_NEEDED", None) and
if it is not None use that boolean value; otherwise fall back to the existing
detection logic on settings.INSTALLED_APPS (the any(...) check for
app.startswith("allauth.socialaccount.providers")) but also support an optional
settings.SOCIALACCOUNT_PROVIDER_APPS list (if present) and treat
SOCIALACCOUNT_ADMIN_NEEDED as True if any entry from that list is present in
INSTALLED_APPS; update the assignment to SOCIALACCOUNT_ADMIN_NEEDED 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: 6f7a1925-6150-4ef4-89dc-549660687229

📥 Commits

Reviewing files that changed from the base of the PR and between bc00bf7 and 5b656ab.

📒 Files selected for processing (2)
  • openwisp_users/admin.py
  • openwisp_users/settings.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). (17)
  • GitHub Check: Python==3.11 | django~=5.2.0
  • 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.11 | django~=5.0.0
  • GitHub Check: Python==3.12 | django~=5.0.0
  • GitHub Check: Python==3.10 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=4.2.0
  • GitHub Check: Python==3.10 | django~=5.0.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=4.2.0
  • GitHub Check: Python==3.11 | django~=5.1.0
  • GitHub Check: Analyze (python)
  • GitHub Check: Analyze (actions)
  • GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (Custom checks)

For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework (django.utils.translation.gettext, gettext_lazy, or ugettext aliases)

Files:

  • openwisp_users/admin.py
  • openwisp_users/settings.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:00.871Z
Learning: In openwisp/openwisp-users, the `password` field (Django hashed password) has been included in the default `EXPORT_USERS_COMMAND_CONFIG["fields"]` since before PR `#498`. Flagging it as a security concern in the context of this PR is out of scope as it is a pre-existing configuration choice.
🔇 Additional comments (1)
openwisp_users/admin.py (1)

655-662: LGTM: SocialApp is now preserved only when credentials need admin management.

The unregister loop remains safe via is_registered, and this keeps the admin UI simplified when no provider admin is needed.

Comment thread openwisp_users/settings.py Outdated
@openwisp-companion
Copy link
Copy Markdown

Flake8 F401 Errors in Multiple Jobs

Hello @nemesifier,
(Analysis for commit 5b656ab)

The CI failed due to flake8 reporting an "imported but unused" error (F401). This indicates that the allauth.app_settings module was imported in openwisp_users/settings.py but its contents (allauth_settings) were not used.

To fix this:

  1. Remove the unused import: Delete the line from allauth import app_settings as allauth_settings from openwisp_users/settings.py.
  2. Remove the unused variable: If allauth_settings is not used elsewhere, remove it from the file.

If the import is still needed for other reasons, ensure that allauth_settings is used or remove the alias if it's not being used.

Before, OpenWISP Users removed the ``allauth.socialaccount``
admin sections to keep the admin UI simple, but over time
more users need this to support OAuth/SAML authentication
to the admin interface (manage app secrets).
With this change, the admin allows managing app secrets
if any ``allauth.socialaccount.provider`` is installed,
eg: microsoft, google, openid, etc.
@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 97.899% (+0.005%) from 97.894% — fix-socialauth into master

@nemesifier nemesifier merged commit 8c19f71 into master Apr 22, 2026
24 checks passed
@nemesifier nemesifier deleted the fix-socialauth branch April 22, 2026 21:54
@nemesifier
Copy link
Copy Markdown
Member Author

/backport 1.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants