Skip to content

[auth] Display login form when OIDC auth is enabled along with other auth backends #4153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

idzikovsky
Copy link
Contributor

What changes were proposed in this pull request?

Without this fix Hue skips login form when desktop.auth.backend set to desktop.auth.backend.OIDCBackend (and does not include desktop.auth.backend.AllowFirstUserDjangoBackend).

But this does not cover a lot of cases, e.g.:

[desktop]
[[auth]]
backend=desktop.auth.backend.LdapBackend,desktop.auth.backend.OIDCBackend

or:

[desktop]
[[auth]]
backend=desktop.auth.backend.AllowFirstUserDjangoBackend,desktop.auth.backend.PamBackend,desktop.auth.backend.OIDCBackend

So in my opinion it's better to skip that form only when only OIDCBackend is used (pretty much exactly like the comment on the first line of that if statement says).

How was this patch tested?

Manual

Copy link

github-actions bot commented May 30, 2025

⚠️ No test files modified. Please ensure that changes are properly tested. ⚠️

Copy link

Python Code Coverage

Python Coverage Report •
FileStmtsMissCoverMissing
TOTAL541852707850% 
report-only-changed-files is enabled. No files were changed during this commit :)

Pytest Report

Tests Skipped Failures Errors Time
1186 106 💤 0 ❌ 0 🔥 5m 55s ⏱️

Copy link

github-actions bot commented Jun 2, 2025

UI Coverage Report

Lines Statements Branches Functions
Coverage: 32%
39.15% (30527/77959) 31.01% (14247/45936) 23.89% (2130/8915)

Copy link

github-actions bot commented Jun 2, 2025

Python Code Coverage

Python Coverage Report •
FileStmtsMissCoverMissing
TOTAL541852707850% 
report-only-changed-files is enabled. No files were changed during this commit :)

Pytest Report

Tests Skipped Failures Errors Time
1186 106 💤 0 ❌ 0 🔥 6m 1s ⏱️

@bjornalm bjornalm requested review from Harshg999 and Copilot June 3, 2025 14:57
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@Harshg999 Harshg999 requested a review from Copilot June 3, 2025 15:10
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refines when the OIDC login form is displayed by introducing a helper that detects if OIDC is the only authentication backend and updating the login URL logic accordingly.

  • Add only_oidc_configured to check if the backends list contains only OIDC
  • Replace the previous backend check with only_oidc_configured in the login URL condition
  • Ensure mozilla_django_oidc is added to INSTALLED_APPS when OIDC is enabled
Comments suppressed due to low confidence (2)

desktop/core/src/desktop/settings.py:597

  • [nitpick] The loop variable b is ambiguous; consider renaming it to backend for better readability.
return not [b for b in AUTHENTICATION_BACKENDS if b != 'desktop.auth.backend.OIDCBackend']

desktop/core/src/desktop/settings.py:596

  • Introduce unit tests for only_oidc_configured covering cases with single OIDC backend, multiple backends including OIDC, and no OIDC backend to ensure correct behavior.
def only_oidc_configured():

Copy link
Collaborator

@Harshg999 Harshg999 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @idzikovsky for contributing a fix!

There are some nits from the copilot review, can you please take a look if possible and rebase your PR to latest master?

@idzikovsky idzikovsky force-pushed the fix-oidc-multiauth branch from 54d72a8 to 1047664 Compare June 4, 2025 19:25
@idzikovsky
Copy link
Contributor Author

I did additional testing on this and it appeared that we also need to ignore the axes.backends.AxesBackend entry added here:
https://github.com/cloudera/hue/blob/84d4002d/desktop/core/src/desktop/settings.py#L530

Copy link

github-actions bot commented Jun 4, 2025

Python Code Coverage

Python Coverage Report •
FileStmtsMissCoverMissing
TOTAL541852707850% 
report-only-changed-files is enabled. No files were changed during this commit :)

Pytest Report

Tests Skipped Failures Errors Time
1186 106 💤 0 ❌ 0 🔥 5m 58s ⏱️

@idzikovsky
Copy link
Contributor Author

idzikovsky commented Jun 4, 2025

Or another variant here is to remove axes backend in a separate step to make code clearer:

def only_oidc_configured():
    """Check if only the OIDC Auth Backend is enabled."""
    backends = filter(lambda backend: backend != 'axes.backends.AxesBackend', AUTHENTICATION_BACKENDS)  # ignore implicitly added backends
    return all(backend == 'desktop.auth.backend.OIDCBackend' for backend in backends)

@idzikovsky idzikovsky requested a review from wing2fly June 7, 2025 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants