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

Add UI Page for OIDC Configuration #18

Merged
merged 3 commits into from
Apr 27, 2022
Merged

Add UI Page for OIDC Configuration #18

merged 3 commits into from
Apr 27, 2022

Conversation

strazto
Copy link
Collaborator

@strazto strazto commented Apr 23, 2022

Partially resolves #2

For now, this only implements OID configuration, though gives a solid reference for SAML configuration, which will be similar.

Presently, options that take json objects require json to be given on the UI - For list items, this would be better as a multiline text field that splits on newlines, and more more complex mappings, something actually good needs to be made - but this will do for now.

@strazto strazto marked this pull request as ready for review April 23, 2022 07:32
@strazto strazto changed the title Add UI Page for OAuth2 Configuration Add UI Page for OIDC Configuration Apr 23, 2022
@9p4
Copy link
Owner

9p4 commented Apr 23, 2022

Could you make the CI happy (it's all style)?

@strazto
Copy link
Collaborator Author

strazto commented Apr 24, 2022

Could you make the CI happy (it's all style)?

Yep, I've updated my own build command to use --warnaserror too

@strazto strazto force-pushed the config_page branch 2 times, most recently from 8a81abc to 827b292 Compare April 24, 2022 08:34
@strazto
Copy link
Collaborator Author

strazto commented Apr 24, 2022

Looks like it should pass CI checks now, seems to on my fork:

strazto#2

strazto added a commit to strazto/jellyfin-plugin-sso that referenced this pull request Apr 24, 2022
@strazto

This comment was marked as resolved.

@strazto
Copy link
Collaborator Author

strazto commented Apr 24, 2022

Should be RTM - The frontend issues are resolved now

SSO-Auth/Config/config.js Outdated Show resolved Hide resolved
SSO-Auth/Config/configPage.html Show resolved Hide resolved
For now, this only implements OID configuration.

Add configuration

More frontend additions

implement more frontend for save / load config

Fully implement save + load

implement delete provider
@strazto
Copy link
Collaborator Author

strazto commented Apr 24, 2022

This'll probably be annoying to rebase but that's what you get for not formatting your code, I've thrown both the html & js at prettier so far

@9p4
Copy link
Owner

9p4 commented Apr 24, 2022

I'm going to take this for a test run and merge once it all feels good :)

Thanks for the PR!

@strazto
Copy link
Collaborator Author

strazto commented Apr 24, 2022

I'll wait to rebase #19 until I wait & see if you have more changes to request.

Happy to say that I've successfully used this to configure my dev server to authorize against my authelia instance, and was able to login with the most basic settings:

---
authelia:
  OidEndpoint: https://authelia.example.com/.well-known/openid-configuration/
  OidClientId: jellyfin
  OidSecret: <omitted>
  Enabled: true
  EnableAuthorization: true
  EnableAllFolders: true
  EnabledFolders: []
  Roles: []
  EnableFolderRoles: false
  FolderRoleMapping: []
  RoleClaim: a.b.d

@strazto
Copy link
Collaborator Author

strazto commented Apr 24, 2022

So far I've noticed that there's not as much validation of configuration as there probably should be, particularly the json fields, whereby if they are left empty, there'll be runtime errors when you try to run your flow.

For now, the json fields that you want to leave unset seem to need some parse-able placeholder, [] or {}

@strazto
Copy link
Collaborator Author

strazto commented Apr 24, 2022

Also, future work to make this actually feel nice is going to involve:

  • client (or server) side mapping of folders to their respective ids
  • a nice(er) UI for selecting folders, using LDAP auth as a reference:
    image

Copy link
Owner

@9p4 9p4 left a comment

Choose a reason for hiding this comment

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

A lot of nitpicky UX changes. Most are spelling/grammar/consistency but a few are for how data is input and may take a while to change.

SSO-Auth/Config/configPage.html Outdated Show resolved Hide resolved
SSO-Auth/Config/configPage.html Outdated Show resolved Hide resolved
SSO-Auth/Config/configPage.html Outdated Show resolved Hide resolved
SSO-Auth/Config/configPage.html Outdated Show resolved Hide resolved
SSO-Auth/Config/configPage.html Outdated Show resolved Hide resolved
SSO-Auth/Config/configPage.html Outdated Show resolved Hide resolved
SSO-Auth/Config/configPage.html Outdated Show resolved Hide resolved
SSO-Auth/Config/configPage.html Outdated Show resolved Hide resolved
SSO-Auth/Config/configPage.html Outdated Show resolved Hide resolved
SSO-Auth/Config/configPage.html Outdated Show resolved Hide resolved
@strazto
Copy link
Collaborator Author

strazto commented Apr 24, 2022

@94p although I'd like to get the input changes in (json input is def not user friendly), I'd still call this an improvement over the previous system (no config page).
Will you consider pulling this as good-enough-for-now if I get the spelling / punctuation / help string comments fixed?

@9p4
Copy link
Owner

9p4 commented Apr 24, 2022

Yep!

@9p4
Copy link
Owner

9p4 commented Apr 24, 2022

I would prefer for an "Add" button to be added before I merge, however.

@strazto
Copy link
Collaborator Author

strazto commented Apr 24, 2022

I would prefer for an "Add" button to be added before I merge, however.

If you mean to add a new oidc config, that's implemented- you fill out the form and press "add", at the bottom of the form.
If there's an entry loaded up, it updates that entry, but really it just writes to whatever entry has the corresponding "provider name"

I think I know what you're saying, though - to make the pattern more obvious, I could add a button up top "add" that just clears the form and maybe puts "new provider" into the name field, so the flow to add a new provider feels more obvious

@strazto
Copy link
Collaborator Author

strazto commented Apr 24, 2022

When I get a second tomorrow, I'll address the style and spelling nits and then it'll be good to merge, with a clear path for how to get it usable enough to someday be beta-ready (I'll probably implement SAML config around the same time)

@9p4
Copy link
Owner

9p4 commented Apr 24, 2022

Again, thank you so much for this contribution!

@strazto
Copy link
Collaborator Author

strazto commented Apr 25, 2022

Okay, awake now, I'll address those things - Not fully clear on the "Add" functionality I described here

I think I know what you're saying, though - to make the pattern more obvious, I could add a button up top "add" that just clears the form and maybe puts "new provider" into the name field, so the flow to add a new provider feels more obvious

I think if I improve the labeling of the add / update form, then it shouldn't be as necessary to implement a specific "Add" button, it should be clear what form does what.

@strazto
Copy link
Collaborator Author

strazto commented Apr 25, 2022

Current UI

image

By default, "select existing" is collapsed.

image

@strazto
Copy link
Collaborator Author

strazto commented Apr 25, 2022

Should be RTM. Feel free to squash on merge, it'll be a hassle to rebase regardless (I'd rather the two cleanup commits be squashed anyway)

Documenting comments / requests that won't be actioned this PR in #2 (comment)

@strazto strazto requested a review from 9p4 April 25, 2022 07:31
@9p4
Copy link
Owner

9p4 commented Apr 26, 2022

I'll do some more testing this week but looks very good!

@9p4 9p4 merged commit 3a8e89d into 9p4:main Apr 27, 2022
@strazto strazto deleted the config_page branch May 12, 2022 08:53
@strazto strazto mentioned this pull request May 13, 2022
13 tasks
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.

Admin page
2 participants