-
Notifications
You must be signed in to change notification settings - Fork 146
feat: improve login page flexibility #1227
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
base: main
Are you sure you want to change the base?
feat: improve login page flexibility #1227
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (42.85%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1227 +/- ##
==========================================
- Coverage 84.20% 84.09% -0.11%
==========================================
Files 68 68
Lines 2938 2943 +5
Branches 374 374
==========================================
+ Hits 2474 2475 +1
- Misses 404 408 +4
Partials 60 60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@andypols Here's a first attempt at improving the login page flexibility/error handling. Let me know if this works with your SSO setup - I'm not sure I understand the differences between that and the current OIDC (if it's simply an extra auth method, it might work out of the box). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, but I think it needs a few tweaks.
There are really only two flows depending on the strategy:
- Provide a UI to collect the username for the strategy (example 2)
- Delegate to use the stragagies login UI (example 3). In this case the user should not have to click a button, it should automatically call
handleAuthMethodLogin
.
I think the title "Local Authentication" might be confusing for users who don't know the implementation details. I'd suggest just using "Login" instead - clearer and more user-friendly.
@andypols Just fixed this so that it logs in automatically if a single non-user/password method is available. I also fixed up the "${authName} Authentication" label and changed it for "Login", which is only actually shown if a username/password auth method (such as Local or AD) is available. The login page still supports multiple auth methods in the off-chance that some organizations need it. |
Perhaps we should raise a separate issue about single vs. multiple authentication methods? It feels inconsistent that the code and config seem to allow multiple, but the schema prevents it. The current setup is also creating a poor experience for users: Seeing a message like “No username/password authentication method is configured. Please contact an administrator to enable this feature.” is confusing. It makes it look like the organisation is misusing git-proxy rather than just not having that method enabled. It is perfectly valid not to have username/password authentication. A button that says “LOGIN WITH OPENIDCONNECT” isn’t very user-friendly. Most users won’t know what OpenID Connect means. Even if a button is required, why not just “Login”? Ideally, SSO methods would simply redirect without needing a button—but that only works if there’s just one method enabled. @kriswest What do you think? A second opinion would be welcome :) |
In the schema there are two authentication elements, one in definitions and one in the main config structure. The one in the main config structure is an array that references the definition for its elements: Lines 117 to 123 in 7030df1
Each element in that array is This structure allows multiple auth methods to be enabled and has done since before I added the I'm all for the code auto-redirecting if only one auth method is enabled and it doesn't need the form. That sounds like what JUan has done (haven't looked yet) according to his last comment: #1227 (comment) |
Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
@andypols The issue with the additional auth methods is that we still need to be able to identify what service we're using to authenticate. I think the best solution would be an extra My interim solution is to display "Login with X" only if multiple methods are present, and "Login" otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #1206.
This PR adds a
/api/auth/config
endpoint to fetch the enabled username/password method, as well as the enabled one-click methods such as OIDC. It also fixes the display for the available config types on the Login page, and improves error handling and visibility of auth methods.Screenshots
When both local* and a custom, non-username/password method
When only local* is enabled
When only a custom, non-username/password method is enabled
*Defaults to AD if enabled, see
/src/routes/auth
for more details**Note that it's not currently possible to have no enabled authentication methods (see
getAuthMethods()
)