Skip to content
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

Fix auth in UI with LDAP if password authentication is disabled #3912

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

minurmin
Copy link
Contributor

@minurmin minurmin commented Jan 28, 2025

References

Description

If LDAP authentication is enabled but Password authentication is not, Angular UI shows a blank login dialog. This PR fixes the issue by forcing Password dialog to be present (as the same password dialog is used in both password- and ldap-based authentication) as part of the enumeration and sorting of login methods.

Instructions for Reviewers

Details on functionality: When enumerating the authentication methods for sorting, additional local variables to record whether password auth and/or ldap auth are present. In the special case where ldap auth is present and password auth is not, password auth is added anyway. This is needed because there is no renderAuthMethodFor decorator related to ldap auth (and overall, ldap auth is barely referenced anywhere outside the auth.method.ts as the UI is the same for password authentication). Because of the new variables the form will appear only once compared to the situation where a decorator would be present both password and ldap auth (as mentioned in discussion of #2701).

So far, the code has only been tested with DSpace 7.6.x. However, there seems to be no essential changes in auth.interceptor.ts after introducing the reordering functionality #2414 so it's at least mergeable to main branch as well.

Steps to test the issue (i.e. adjusting values of plugin.sequence.org.dspace.authenticate.AuthenticationMethod in DSpace server settings) are described in DSpace/DSpace#8312

Checklist

  • My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using npm run lint
  • My PR doesn't introduce circular dependencies (verified via npm run check-circ-deps)
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • My PR aligns with Accessibility guidelines if it makes changes to the user interface.
  • My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
  • If my PR fixes an issue ticket, I've linked them together.

@tdonohue tdonohue added bug high priority authentication: LDAP related to integration with LDAP authentication: password related to built in password authentication 1 APPROVAL pull request only requires a single approval to merge port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Jan 28, 2025
@tdonohue
Copy link
Member

@minurmin : This has some lint errors which you can see from the "Files changed" tab https://github.com/DSpace/dspace-angular/pull/3912/files It might be possible to fix them automatically by running npm run lint-fix

@minurmin
Copy link
Contributor Author

minurmin commented Jan 28, 2025

Ok, it should pass lint validation now.

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @minurmin ! I reviewed and tested this today, and verified it works. It's perhaps not the perfect solution (as it requires some extra logic to check which auth methods are enabled specific to LDAP), but it seems "good enough" because it solves the immediate bug.

So, I'm going to go ahead and merge this & port to 8.x and 7.x

@tdonohue tdonohue added this to the 9.0 milestone Jan 30, 2025
@tdonohue tdonohue merged commit 7e69b44 into DSpace:main Jan 30, 2025
15 checks passed
@dspace-bot
Copy link
Contributor

Successfully created backport PR for dspace-7_x:

@dspace-bot
Copy link
Contributor

Successfully created backport PR for dspace-8_x:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge authentication: LDAP related to integration with LDAP authentication: password related to built in password authentication bug high priority port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Couldn't auth in UI with LDAP if password authentication disabled
3 participants