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

Changed from username and password to api key for jellyfin #1816

Closed
wants to merge 3 commits into from

Conversation

conner-replogle
Copy link

Category

Feature

Overview

Changed Jellyfin from username & password to api as some people wanted including myself. I am not aware of why this wasn't done at first, so let me know if this is breaking something or if there was a reason not to do this.

Related Issue

#1635

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi 👋. Thank you for making your first contribution to Homarr. Please ensure that you've completed all the points in the TODO checklist. We'll review your changes shortly.

@manuel-rw
Copy link
Collaborator

Hi, thanks for the contribution. I think this is a nice idea - you're right that API keys should be the default.
However, this isn't a good idea to implement it like this, as it will be a breaking change.
It means, that every user will have to redo their setup. Many will just not bother with it.

One solution would be to have fields for username, password and API key, but currently, they are required.
I'd suggest that we add the option to make fields required or optional in the widget definition.
WDYT @Meierschlumpf @ajnart @Tagaishi

This way, the user could choose between API key and user / PW + existing installations wouldn't break.

@manuel-rw manuel-rw added 🏛️ Architecture An idea for the project's architecture that could help it grow 🚨 Security Risk Outdated APIs, bad security practice labels Jan 14, 2024
@Meierschlumpf
Copy link
Collaborator

Hi, thanks for the contribution. I think this is a nice idea - you're right that API keys should be the default. However, this isn't a good idea to implement it like this, as it will be a breaking change. It means, that every user will have to redo their setup. Many will just not bother with it.

One solution would be to have fields for username, password and API key, but currently, they are required. I'd suggest that we add the option to make fields required or optional in the widget definition. WDYT @Meierschlumpf @ajnart @Tagaishi

This way, the user could choose between API key and user / PW + existing installations wouldn't break.

Maybe it makes more sense to have multiple options of combinations. Then we could like make that one of them is still required (either username / password or apiKey)

@conner-replogle
Copy link
Author

We would need to rework how the options are setup to allow that, I just pushed a commit with an attempt to do so, though I am not finished with it. It is not perfect since all it does is make the fields optional and it fails to enforce username/password or api Key

@conner-replogle conner-replogle force-pushed the jellyfin-apikey branch 2 times, most recently from 4c95865 to 1304144 Compare January 15, 2024 06:09
@manuel-rw
Copy link
Collaborator

I'm confused. The diff here doesn't show any additional changes. However, I can see that you did make when I click "Compare" on your force-push: https://github.com/ajnart/homarr/compare/4c958653a943f3f6eb9803dc47296facfee9e26f..130414491d999cdde73bca18e1406c667f717fe1
@Meierschlumpf can you confirm this?
Did you maybe push it incorrectly?

@conner-replogle
Copy link
Author

Oh shoot, I had realized I accidentally pushed my config with some username/passwords in it so i tried to find a way to delete it but I guess I can't. I'll repush the changes in a minute.

@ajnart
Copy link
Owner

ajnart commented Jan 16, 2024

Oh shoot, I had realized I accidentally pushed my config with some username/passwords in it so i tried to find a way to delete it but I guess I can't. I'll repush the changes in a minute.

Yeah unfortunately that's not possible on GitHub, you'll need to change it 😅. Or you can open a GitHub support ticket to remove it. I've already done it in the past

@conner-replogle
Copy link
Author

I changed the password so I guess its fine but man I gotta keep and eye out for that

@manuel-rw
Copy link
Collaborator

manuel-rw commented Jan 16, 2024

Can confirm, I looked at your commits. If your domain ends with .dev, change your credentials immediately.
Your commit is public to everyone.
There are hundreds of automated scanners always looking for passwords - so your account is most likely compromised.

@conner-replogle
Copy link
Author

Okay my recent commits username/password works and Api key but there is not enforcement that you do either it basically is all optional fields I don't believe this will be a breaking change for configs but not entirely sure. I managed to commit this time without doxxing myself lol. I changed my password btw

@@ -32,11 +32,11 @@ export const IntegrationSelector = ({ form }: IntegrationSelectorProps) => {

const requiredProperties = Object.entries(integrationFieldDefinitions).filter(([k, v]) => {
const val = integrationFieldProperties[integrationType];
return val.includes(k as IntegrationField);
return val.map((a) => a.type).includes(k as IntegrationFieldType);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename a->propertyPair

let sessionApi;


if (apiKey && apiKey.length > 5 ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the length 5? Why not just >0?

Comment on lines 81 to 95
let api;
let infoApi;
let sessionApi;


if (apiKey && apiKey.length > 5 ) {
api = jellyfin.createApi(app.url, apiKey);
infoApi = await getSystemApi(api).getPublicSystemInfo();
sessionApi = getSessionApi(api);
}else if (username && password){
api = jellyfin.createApi(app.url);
await api.authenticateUserByName(username, password);
infoApi = await getSystemApi(api).getPublicSystemInfo();
sessionApi = getSessionApi(api);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of doing this, can you extract it to a function? This way it should get easier to read

src/types/app.ts Outdated
Comment on lines 95 to 119
lidarr: [{ type: 'apiKey', isRequired: true }],
radarr: [{ type: 'apiKey', isRequired: true }],
sonarr: [{ type: 'apiKey', isRequired: true }],
sabnzbd: [{ type: 'apiKey', isRequired: true }],
readarr: [{ type: 'apiKey', isRequired: true }],
overseerr: [{ type: 'apiKey', isRequired: true }],
jellyseerr: [{ type: 'apiKey', isRequired: true }],
deluge: [{ type: 'password', isRequired: true }],
nzbGet: [
{ type: 'username', isRequired: true},
{ type: 'password', isRequired: true },
],
qBittorrent: [
{ type: 'username', isRequired: true },
{ type: 'password', isRequired: true },
],
transmission: [
{ type: 'username', isRequired: true },
{ type: 'password', isRequired: true },
],
jellyfin: [{ type: 'username', isRequired: false },
{ type: 'password', isRequired: false },{ type: 'apiKey', isRequired: false }],
plex: [{ type: 'apiKey', isRequired: true }],
pihole: [{ type: 'apiKey', isRequired: true }],
adGuardHome: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Format using Prettier

@ajnart
Copy link
Owner

ajnart commented Feb 15, 2024

@conner-replogle did you see the review ?

@manuel-rw
Copy link
Collaborator

@conner-replogle

@conner-replogle
Copy link
Author

Oh i forgot I even opened this yeah Ill implement those changes later today :0

Copy link
Owner

@ajnart ajnart left a comment

Choose a reason for hiding this comment

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

@conner-replogle It seems that you formatted the whole codebase. Can you undo the changes to the files not in the scope of this PR ?

@ajnart
Copy link
Owner

ajnart commented Mar 15, 2024

Also as a sidenote, if the isRequired is added that means we can probably add it to the torrent client that doesn't require user/pass, I don't remember which one it is but a bunch of people said "I don't have a username / password" and we told them to put anything inside of the fields

@conner-replogle
Copy link
Author

To be honest I'm a rust developer not a js/ts so I really don't know how to use prettier just ran the command in package.json can you send me a command to use to only do the files

@SeDemal
Copy link
Collaborator

SeDemal commented Mar 15, 2024

To be honest I'm a rust developer not a js/ts so I really don't know how to use prettier just ran the command in package.json can you send me a command to use to only do the files

if you use VS.code, just hit f1 and type Format Document. Or Shift Alt F.
It might prompt you to select which formatter you want to use so be careful to click on prettier if it does.

@ajnart
Copy link
Owner

ajnart commented Apr 18, 2024

I'll close this. There are wayyyy too many changes and it's been stale for quite a while. If anyone's motivated to do this feel free to open a new PR (with just the related changes)

@ajnart ajnart closed this Apr 18, 2024
@Meierschlumpf
Copy link
Collaborator

Alrady implemented in v1.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ Architecture An idea for the project's architecture that could help it grow 🚨 Security Risk Outdated APIs, bad security practice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants