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 Plex OAuth #413

Merged
merged 28 commits into from
Jul 1, 2023
Merged

Add Plex OAuth #413

merged 28 commits into from
Jul 1, 2023

Conversation

JVT038
Copy link
Collaborator

@JVT038 JVT038 commented Jun 27, 2023

This PR adds the ability for users to link their Plex account to Movary.
When the user logs in, the following happens:

  1. The user will be redirected to the Plex login system, where they will enter their credentials and login.
  2. The user will grant permission to Movary.
  3. The user will be returned to Movary and Movary will receive the X-Plex-Token, which gives Movary access to the user's data through the Plex API.

This PR adds multiple columns:

  • plex_client_id: Necessary for the OAuth flow. It's the Plex identifier, so maybe we should change the column's name. This is provided by Plex after fetching an Authentication URL.
  • plex_client_temporary_code: As the name suggests, this is a temporary code that is fetched alongside with the Plex client ID / identifier. It will be valid for 15 minutes and during those 15min, it can be used to start a login process.
  • plex_account_id: This is the ID o fthe Plex acount in the Plex system. This ID is necessary if you want to reach the https://plex.tv API, and not the local (http://localhost:32400) API. A lot of the API calls made to the central Plex API require the account ID, which is why it's stored in the database.
  • plex_server_url: The server URL of the local Plex server. For some reason, some of the Plex API calls go to the local Plex server (mainly the data about the Plex server, such as which movies does the server have, which libraries, etc.), which is why this is stored in the database.

When an user adds a local server, Movary will first check whether the user has access to the server. That's basically the only validation that currently exists.

When an users wants to 'log out', the values from the plex_client_id, plex_client_temporary_code, plex_access_token, and the plex_account_id are deleted. (Actually, they're updated to the value of null).

Feel free to leave any feedback and/or refactor my code; I haven't really changed it tbh; I just copied most of it from the old PR, changed it so that it doesn't cause errors and/or merge conflicts with the new code.

Question:

How will we validate the Plex server URL? It has to look like this http://localhost:32400 (especially the port is important). But it can also be like this https://plex.domain.com if the user is running a reverse proxy.

@JVT038 JVT038 requested a review from leepeuker as a code owner June 27, 2023 19:45
@JVT038
Copy link
Collaborator Author

JVT038 commented Jun 27, 2023

For more info on the technical details of the Plex OAuth flow, check out this, this and this.

@JVT038
Copy link
Collaborator Author

JVT038 commented Jun 27, 2023

BTW, I hope I did the dependency injection right, because I still don't really understand what dependency injection means or does, so I just tried some stuff until the errors disappeared lol.

@leepeuker leepeuker linked an issue Jun 27, 2023 that may be closed by this pull request
src/Factory.php Outdated Show resolved Hide resolved
public/js/settings-integration-plex.js Outdated Show resolved Hide resolved
Comment on lines 93 to 97
try {
$plexAuthenticationData = $this->plexTvClient->sendPostRequest($relativeUrl);
} catch (PlexNoClientIdentifier) {
return null;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why did you remove this?

Copy link
Owner

Choose a reason for hiding this comment

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

because I do not want to return null here, I always expect a authentication url from this method.
if this is not possible an exception should be thrown and the caller should decide what to do about it

Comment on lines +101 to +107
<input type="text"
name="plexServerUrlInput"
id="plexServerUrlInput"
class="form-control {% if plexTokenExists == false %} disabled {% endif %}"
value="{{ plexServerUrl }}"
aria-describedby="plexServerUrlInputPrefix" {% if plexTokenExists == false %} disabled {% endif %}
>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly, I personally prefer to have all the HTML attributes on one line, regardless of how big this line may become.

Copy link
Owner

Choose a reason for hiding this comment

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

I enforce a 180 character limit per line via IDE, I much prefer scrolling vertically vs horizontally 😅

@leepeuker
Copy link
Owner

leepeuker commented Jun 29, 2023

I see one open issue left, if PLEX_IDENTIFIER is not set, the Plex Authentication settings page section should be disabled similar to how it is when no application url is set

@JVT038
Copy link
Collaborator Author

JVT038 commented Jun 29, 2023

Looks good to me.
One question: The help button (next to the input for the server URL) is a question mark.
In the Netflix import page, there's a help button for the date format input, and that isn't a question mark, but the word 'help'.

Screenshot:
image

I think we should stay consistent with these things. Either use a question mark everywhere for help modals or the word 'help'.

@leepeuker
Copy link
Owner

I aggree, I think I prefer the question mark, let's use this on the netflix page to? Maybe even together with the same button styling?

@leepeuker leepeuker merged commit 9d78346 into main Jul 1, 2023
@leepeuker leepeuker deleted the add-plex-oauth branch July 1, 2023 09:45
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 Plex Oauth token authentication
2 participants