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

SSO Support - alternate UI #2731

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

SSO Support - alternate UI #2731

wants to merge 22 commits into from

Conversation

SleeplessOne1917
Copy link
Member

@SleeplessOne1917 SleeplessOne1917 commented Oct 2, 2024

The main difference between this and #2578 - UI wise at least - is the admin tab for managing SSO providers. Before, is was a table of forms. Since the forms had many fields, even 1 or 2 providers caused a lot of cognitive load trying to read through. In this PR, existing providers are in a list with collapsable entries, and their values are listed in a more space-efficient list of key-value pairs. Adding and editing providers is handled with a modal now.

There are stilll several changes that need to be done. Chief among them is the need for translations; best to hold off on merging until those are decided on and provided. There is also the matter of helpful information about adding OAuth providers that I raised in the other PR. I'm unsure what approach would be best here: Little help circles like we use with some fields in other parts of the UI? A link to a learning resource so admins know what the values they're setting do (perhaps privacy portal has a good resource for this, or maybe we can add something about it to the Lemmy docs)? Some combination of both?

cc. @privacyguard

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

The UI seems fine, but I don't know how to test this properly.

And ya we'll have to wait on @privacyguard for the correct i18n translations of these ones: which should get added as a PR to this file

@dessalines
Copy link
Member

There is also the matter of helpful information about adding OAuth providers that I raised in the other PR. I'm unsure what approach would be best here: Little help circles like we use with some fields in other parts of the UI?

I think linking to an external page describing these would be best.

@privacyguard
Copy link

Settings Page

We tested adding / removing / editing Providers in the admin page. The changes look good. There are minor UI improvements that could be made such as text overflow.

Concerning the help part

With time, most providers would be added as preset providers making the task much simpler for admins (these would also serve as examples).

Most items in the form are standard and provided by the OAuth provider. The special fields that could use some help (or maybe a clear title are):

  • oauth_scopes: While this is a standard field. It's useful to explain which user data Lemmy expects from the provider. In this case, it's an ID + an email. So the scopes that would include these values should be provided.
  • oauth_id_claim: This is a custom field that tells Lemmy which property should be used as user_id.
  • oauth_auto_verify_email: This is Lemmy specific (but commonly used in other applications)
  • oauth_account_linking_enabled: This is Lemmy specific (but commonly used in other applications)

Authentication

We tested the authentication, it worked as expected. There's the mismatch in error names we mentioned in the review.

Concerning the use of the Modal to login, it's not very common except by apps that usually have a large number of providers configured. Do we expect Lemmy admins to have 5+ providers configured? Otherwise it would be much cleaner to simply have the providers directly in the page (as a single column under the user/pass login). That said, this is a style preference so you guys should decide on that. We're just giving our opinion.

@privacyguard
Copy link

privacyguard commented Oct 3, 2024

Concerning the translations, is this PR blocked by the translations? Could it wait 2-3 days?

@SleeplessOne1917
Copy link
Member Author

There are minor UI improvements that could be made such as text overflow.

Where are you seeing the text overflow issues?

Concerning the use of the Modal to login, it's not very common except by apps that usually have a large number of providers configured. Do we expect Lemmy admins to have 5+ providers configured? Otherwise it would be much cleaner to simply have the providers directly in the page (as a single column under the user/pass login). That said, this is a style preference so you guys should decide on that. We're just giving our opinion.

I am not sure how many providers the average instance would have configured tbh. When there are fewer I agree that that the modal is less clean. The non-modal starts to messier once the number of providers gets big but, as stated, I'm not sure what a realistic number would be. I'm fine not using the modal, though I think they should be in a section with their own heading.

I think linking to an external page describing these would be best.

Makes sense. We should probably add a page for it to the Lemmy docs before releasing 0.20.0.

Concerning the translations, is this PR blocked by the translations? Could it wait 2-3 days?

We usually handle translations for a feature as part of the same PR that adds the feature, although strictly speaking it is not. I find that having them be added as part of the same PR makes it easier to track which translations are new in case we forget to add one.

@privacyguard
Copy link

Sounds good. For the translations, we'll have a PR ready for it tomorrow.

@SleeplessOne1917
Copy link
Member Author

@dessalines @privacyguard The translations are included now and the PR is ready for final review before merging.

@SleeplessOne1917 SleeplessOne1917 marked this pull request as ready for review October 7, 2024 17:21
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.

3 participants