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.

@ysmilda
Copy link

ysmilda commented Oct 14, 2024

As this is a feature I'd love to have, I installed it on my local instance. The steps I took are:

git clone https://github.com/pXius/jellyseerr.git
cd jellyseerr
git checkout feature-emby-connect-login
docker build . -t fallenbagel/jellyseerr:embyconnectfix -f Dockerfile.local

After changing the tag on my docker-compose.yml and a docker compose up -d it seems to work great! Going to keep the development version up for a while to see if I run into any issues down the line.

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

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

@gauthier-th
Copy link
Collaborator

gauthier-th commented Nov 12, 2024

@pXius can you please fix the merge conflict?
I'd also like to have one or two more people testing this before we can merge.
Edit: i'll ask on the Jellyseerr Discord if any Emby user could test this.

@ysmilda
Copy link

ysmilda commented Nov 15, 2024

After a month of running I've had no issues. Would love to see this merged as using the dev server introduces some performance degradation.

@gauthier-th
Copy link
Collaborator

After a month of running I've had no issues. Would love to see this merged as using the dev server introduces some performance degradation.

You should have built it instead of using the dev server: https://docs.jellyseerr.dev/getting-started/buildfromsource
I just created a preview tag, preview-emby-connect, so it can be tested with Docker. It will be available in ~30min.

@ysmilda
Copy link

ysmilda commented Nov 15, 2024

Initially tried to build a non dev version, but ran into problems with the version tag and the ui reloading because of it.

Thanks for the build, will switch over to that one for further testing.

@markschrik
Copy link

Is there any clue when or if this will be merged into main? For now I am fine running the docker tag, but it would be great to use LATEST.

@gauthier-th
Copy link
Collaborator

We are waiting for @pXius to fix the merge conflict. Once it's done it should be merged within a couple weeks.

@markschrik
Copy link

Hm, sad that I can't see the conflict without making a request myself. I would love to help.

@Fallenbagel
Copy link
Owner

Hm, sad that I can't see the conflict without making a request myself. I would love to help.

5 comments above
Screenshot_20241129_141914_GitHub.jpg

@markschrik
Copy link

markschrik commented Nov 29, 2024

I know but I cannot see the actual conflict because I am not a member of this PR / reviewer.

@markschrik
Copy link

For now I am testing the docker tag, it works good :)

Updated the DB myself to convert my existing local accounts to work with Emby accounts, just had to add the JellyfinUsername and JellyfinUserId to the db and it worked.

@markschrik
Copy link

markschrik commented Nov 30, 2024

Suddenly it does not work anymore.

[debug][EmbyConnect API]: Failed to authenticate using EmbyConnect: 401 Unauthorized {"ip":"REDACTED"}
[debug]: Emby Connect authentication failed: apiError 

But also 


[info][API]: Found matching Emby user; updating user with Emby {"ip":"REDACTED","jellyfinUsername":"REDACTED"}

But then it does not give an error, but just stays at the login page.

As a sidenote, just emby local account gives the same issue now.

Edit:

It seems that migrating this way seems to break something I guess.
Is it possible to add a way to connect a local account and emby connect account if the email address is the same? Right now if the account already exists it gives a sql error. Logical, but might be nice to just connect them then.

@pXius
Copy link
Author

pXius commented Dec 6, 2024

Hey guys, apologies for a late response. I was away for work.

I'll pull in main and fix conflicts this weekend. 👌

@markschrik
Copy link

Hey guys, apologies for a late response. I was away for work.

I'll pull in main and fix conflicts this weekend. 👌

Nice. That would be amazing.

Can you please try to make it work for local accounts that already have Emby connect, but the email is identical?

right now, the behaviour is, a user logs in with Emby connect, an a sql error gets logged for duplicate user.

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.

Add authentication with Emby Connect
5 participants