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

feat: add linked accounts page #883

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

michaelhthomas
Copy link

@michaelhthomas michaelhthomas commented Jul 21, 2024

Description

Adds a "Linked Accounts" page to the user settings, allowing managing of linked media server (Plex, Jellyfin) accounts, including linking an account to your existing local user, or removing a linked account to become a local user. This page doesn't add much new functionality (except the ability to unlink accounts), but is intended to lay the groundwork for a new and improved implementation of #183 which allows the secure linking of OIDC accounts with existing local accounts.

Screenshot (if UI-related)

Plex Linked Accounts Page Link Account Dropdown
Plex Linked Accounts page Dropdown menu showing Plex linking option
Jellyfin Linked Accounts Page Link Jellyfin Modal
Jellyfin Linked Accounts page Link Jellyfin Modal
Emby Linked Accounts Page Link Emby Modal
Emby Linked Accounts Page Link Emby Modal

To-Dos

  • Successful build pnpm build
  • Translation keys pnpm i18n:extract
  • Database migration (if required)

Issues Fixed or Closed

  • Fixes #XXXX

@michaelhthomas
Copy link
Author

michaelhthomas commented Jul 21, 2024

Some outstanding questions / TODOs:

  • Should it be possible to link Plex accounts with a different email address? Right now I'm leaning towards no for simplicity's sake, since otherwise we would have to change how the auth linking works as well.
  • Need to make sure you can't lock yourself out of your account. Particularly, need to make sure that users with a media server account do not unlink it without first setting a password
  • Emby? I haven't tested it at all and it currently doesn't have a logo, so will probably just show the jellyfin one
  • Will need help testing this, particularly with Plex since I don't personally use it

Copy link
Collaborator

@gauthier-th gauthier-th left a comment

Choose a reason for hiding this comment

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

Just a few comments for now.
And you will add Emby once the PR with Jellyfin/Emby as different server types will be merged right?
Otherwise it's great I like it, good job!

server/entity/User.ts Outdated Show resolved Hide resolved
src/components/Common/Dropdown/index.tsx Show resolved Hide resolved
@michaelhthomas
Copy link
Author

And you will add Emby once the PR with Jellyfin/Emby as different server types will be merged right?

Yep, that's the plan currently. Hopefully that PR will make emby a lot easier to integrate.

@github-actions github-actions bot added the merge conflict Cannot merge due to merge conflicts label Jul 30, 2024
Copy link

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@github-actions github-actions bot removed the merge conflict Cannot merge due to merge conflicts label Jul 30, 2024
@michaelhthomas
Copy link
Author

Unified the two dropdown components and addressed a few other issues with the frontend.

@gauthier-th gauthier-th linked an issue Jul 31, 2024 that may be closed by this pull request
@michaelhthomas michaelhthomas changed the base branch from develop to feat-server-type-setup August 1, 2024 15:47
@michaelhthomas
Copy link
Author

Rebased on feat-server-type-setup and added support for emby

@gauthier-th gauthier-th mentioned this pull request Aug 12, 2024
15 tasks
Copy link

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@github-actions github-actions bot added the merge conflict Cannot merge due to merge conflicts label Aug 19, 2024
@Fallenbagel
Copy link
Owner

Rebased on feat-server-type-setup and added support for emby

That pr has now been merged

@lucaam lucaam mentioned this pull request Sep 13, 2024
3 tasks
@Gauvino
Copy link
Contributor

Gauvino commented Sep 16, 2024

Why this PR is still in draft any reason for this @michaelhthomas ?

@gauthier-th
Copy link
Collaborator

Hi @michaelhthomas, could you please rebase this on develop and make it ready for review if everything is ok?

Adds a shared component for plain dropdown menus, based on the headlessui Menu component. Updates
the `ButtonWithDropdown` component to use the same inner components, ensuring that the only
difference between the two components is the trigger button, and both use the same components for
the actual dropdown menu.
Prevents the primary administrator from unlinking their media server account (which would break
sync). Additionally, prevents users without a configured local email and password from unlinking
their accounts, which would render them unable to log in.
@michaelhthomas michaelhthomas changed the base branch from feat-server-type-setup to develop October 18, 2024 15:54
@michaelhthomas michaelhthomas marked this pull request as ready for review October 18, 2024 15:56
@michaelhthomas
Copy link
Author

@gauthier-th Rebased on develop. This should be ready for review now, but will probably need some testing. I've personally tested with Jellyfin, Plex, and Emby, but this could definitely use some independent testing.

@github-actions github-actions bot removed the merge conflict Cannot merge due to merge conflicts label Oct 19, 2024
@TheRedCyclops
Copy link

Is there a preview tag for testing on docker?

@gauthier-th
Copy link
Collaborator

gauthier-th commented Nov 1, 2024

Is there a preview tag for testing on docker?

I've just created one: preview-linked-accounts (will be available in ~30min)

Copy link
Collaborator

@gauthier-th gauthier-th left a comment

Choose a reason for hiding this comment

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

Here are a few more remarks after a detailed review. I still have to test it though.
Feel free to challenge me if you disagree on something.

overseerr-api.yml Outdated Show resolved Hide resolved
overseerr-api.yml Outdated Show resolved Hide resolved
server/routes/user/usersettings.ts Outdated Show resolved Hide resolved
server/routes/user/usersettings.ts Outdated Show resolved Hide resolved
server/routes/user/usersettings.ts Outdated Show resolved Hide resolved
src/components/Common/Dropdown/index.tsx Outdated Show resolved Hide resolved
src/components/Common/Dropdown/index.tsx Outdated Show resolved Hide resolved
src/components/Common/Dropdown/index.tsx Outdated Show resolved Hide resolved
@michaelhthomas
Copy link
Author

@gauthier-th Updated to address feedback, thanks for reviewing!

server/routes/user/usersettings.ts Dismissed Show dismissed Hide dismissed
Copy link
Collaborator

@gauthier-th gauthier-th left a comment

Choose a reason for hiding this comment

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

LGTM.
A few more tests are welcome though.

Copy link

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@github-actions github-actions bot added the merge conflict Cannot merge due to merge conflicts label Nov 16, 2024
@gauthier-th
Copy link
Collaborator

@michaelhthomas I think this will be merged soon. Could you please rebase on develop and fix the conflicts? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Cannot merge due to merge conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] SSO
5 participants