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(new auth api): add EmbyConnect authentication API #943

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

Conversation

pXius
Copy link

@pXius pXius commented Aug 24, 2024

Description

This PR is to serve as an RFC to add EmbyConnect authentication to Jellyseerr.

If follows the exact flow used when login in via https://emby.media, but creates a JellyfinLoginResponse as the last step using the acquired data.

graph TD
    A{Start flow IF username <br> is an email address <br> && <br> Jellyfin auth failed} --> B[Authenticate towards EmbyConnect <br> with email and password]
    B --> C[Get a list of Emby servers linked to user]
    C --> D{Compare if a linked <br> server matches the Emby <br> instance in Jellyseerr config}
    D -->|No Match| F[Throw No matching server Exception]
    D -->|Match Found| E[Do an authentication exchange with the hosted <br> Emby server to get a local userId and authToken]
    E --> G[Construct a JellyfinLoginResponse object <br> and return to existing Jellyfin auth login flow]
Loading

Changes to Code

  • Created an emby connect api with the required methods to orchestrate a set of calls towards EmbyConnect to build a JellyseerrLoginResponse object
  • Add an EmbyConnect auth call to the Jellyfin login() method as the last auth attempt IF the username is an email address
  • Update the post() method on the ExternalApi class to handle request bodies that need to be x-www-form-urlencoded

Notes

  • This flow will NOT work as first time setup login as there is no existing configured Emby server to match to. First auth has to use a local account as it is today.
  • I attempted to encapsulate as much of the logic in one place to avoid any possible conflicts with feat: Jellyfin/Emby server type setup #685 and other future in-progress work.
  • JS is not my strong suit; any comments for improvement are welcome.

Additional Improvements?

To-Dos

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

Issues Fixed or Closed

Created an Emby Connect API with the required methods to orchestrate a set of calls towards
EmbyConnect to build a JellyseerrLoginResponse object. Added an EmbyConnect authentication call to
the Jellyfin login() method as the last authentication attempt, but only if the username is an email
address. Updated the post() method on the ExternalApi class to handle request bodies that need to be
x-www-form-urlencoded.

Fallenbagel#749
server/api/embyconnect.ts Fixed Show fixed Hide fixed
server/api/jellyfin.ts Fixed Show fixed Hide fixed
server/api/jellyfin.ts Outdated Show resolved Hide resolved
server/api/jellyfin.ts Outdated Show resolved Hide resolved
server/api/embyconnect.ts Outdated Show resolved Hide resolved
server/api/embyconnect.ts Outdated Show resolved Hide resolved
This refactor includes a change that adds a conditional arg to the ExternalApi get() method to
override the base url. This method was pre-existing and already used in the calls and method.

Fallenbagel#943
@pXius
Copy link
Author

pXius commented Aug 27, 2024

@gauthier-th

Thanks for the feedback.
I've resolved all comments as requested, please confirm that this solution is acceptable.

server/api/jellyfin.ts Dismissed Show dismissed Hide dismissed
Reverting get() baseUrl overwrite in favour of a vanilla fetch() call

Fallenbagel#943
@pXius
Copy link
Author

pXius commented Aug 28, 2024

@gauthier-th
Thanks again for the guidance.

I have resolved the final comment by reverting the baseUrl override in favour of a vanilla fetch() call as requested.

Please let me know if any other changes are required.

@pXius pXius requested a review from gauthier-th August 30, 2024 22:02
@pXius
Copy link
Author

pXius commented Sep 15, 2024

Hey @gauthier-th
Anything outstanding to get this merged?

@gauthier-th
Copy link
Collaborator

Hey @gauthier-th Anything outstanding to get this merged?

This will be merged, in time.
We also need more people reviewing and testing this, as I don't use EmbyConnect.

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.
This still needs to be tested, though.

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.

Add authentication with Emby Connect
2 participants