Skip to content

Commit

Permalink
refactor(jellyfin): abstract jellyfin hostname, updated ui to reflect…
Browse files Browse the repository at this point in the history
… it, better validation (#773)

* refactor(jellyfinsettings): abstract jellyfin hostname, updated ui to reflect it, better validation

This PR refactors and abstracts jellyfin hostname into, jellyfin ip, jellyfin port, jellyfin useSsl,
and jellyfin urlBase. This makes it more consistent with how plex settings are stored as well. In
addition, this improves validation as validation can be applied seperately to them instead of as one
whole regex doing the work to validate the url.
UI was updated to reflect this.

BREAKING CHANGE: Jellyfin settings now does not include a hostname. Instead it abstracted it to ip,
port, useSsl, and urlBase. However, migration of old settings to new settings should work
automatically.

* refactor: remove console logs and use getHostname and ApiErrorCodes

* fix: store req.body jellyfin settings temporarily and store only if valid

This should fix the issue where settings are saved even if the url
was invalid. Now the settings will only be saved if the url is
valid. Sort of like a test connection.

* refactor: clean up commented out code

* refactor(i18n): extract translation keys

* fix(auth): auth failing with jellyfin login is disabled

* fix(settings): jellyfin migrations replacing the rest of the settings

* fix(settings): jellyfin hostname should be carried out if hostname exists

* fix(settings): merging the wrong settings source

* refactor(settings): use migrator for dynamic settings migrations

* refactor(settingsmigrator): settings migration handler and the migrations

* test(cypress): fix cypress tests failing

cypress settings were lacking some of the jobs so when the startJobs() is called when the app
starts, it was failing to schedule the jobs where their cron timings were not specified in the
cypress settings. Therefore, this commit adds those jobs back. In addition, other setting options
were added to keep cypress settings consistent with a normal user.

* chore(prettierignore): ignore cypress/config/settings.cypress.json as it does not need prettier

* chore(prettier): ran formatter on cypress config to fix format check error

format check locally passes on this file. However, it fails during the github actions format check.
Therefore, json language features formatter was run instead of prettier to see if that fixes the
issue.

* test(cypress): add only missing jobs to the cypress settings

* ci: attempt at trying to get formatter to pass on cypress config json file

* refactor: revert the changes brought to try and fix formatter

added back the rest of the cypress settings and removed cypress settings from .prettierignore

* refactor(settings): better erorr logging when jellyfin connection test fails in settings page
  • Loading branch information
fallenbagel authored Jun 13, 2024
1 parent a9741fa commit 38ad875
Show file tree
Hide file tree
Showing 16 changed files with 529 additions and 118 deletions.
27 changes: 27 additions & 0 deletions cypress/config/settings.cypress.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"region": "",
"originalLanguage": "",
"trustProxy": false,
"mediaServerType": 1,
"partialRequestsEnabled": true,
"locale": "en"
},
Expand All @@ -37,6 +38,17 @@
],
"machineId": "test"
},
"jellyfin": {
"name": "",
"ip": "",
"port": 8096,
"useSsl": false,
"urlBase": "",
"externalHostname": "",
"jellyfinForgotPasswordUrl": "",
"libraries": [],
"serverId": ""
},
"tautulli": {},
"radarr": [],
"sonarr": [],
Expand Down Expand Up @@ -139,11 +151,26 @@
"sonarr-scan": {
"schedule": "0 30 4 * * *"
},
"plex-watchlist-sync": {
"schedule": "0 */10 * * * *"
},
"availability-sync": {
"schedule": "0 0 5 * * *"
},
"download-sync": {
"schedule": "0 * * * * *"
},
"download-sync-reset": {
"schedule": "0 0 1 * * *"
},
"jellyfin-recently-added-scan": {
"schedule": "0 */5 * * * *"
},
"jellyfin-full-scan": {
"schedule": "0 0 3 * * *"
},
"image-cache-cleanup": {
"schedule": "0 0 5 * * *"
}
}
}
10 changes: 10 additions & 0 deletions server/api/jellyfin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,16 @@ class JellyfinAPI extends ExternalAPI {
return;
}

public async getSystemInfo(): Promise<any> {
try {
const systemInfoResponse = await this.get<any>('/System/Info');

return systemInfoResponse;
} catch (e) {
throw new ApiError(e.response?.status, ApiErrorCode.InvalidAuthToken);
}
}

public async getServerName(): Promise<string> {
try {
const serverResponse = await this.get<JellyfinUserResponse>(
Expand Down
2 changes: 2 additions & 0 deletions server/constants/error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,7 @@ export enum ApiErrorCode {
InvalidCredentials = 'INVALID_CREDENTIALS',
InvalidAuthToken = 'INVALID_AUTH_TOKEN',
NotAdmin = 'NOT_ADMIN',
SyncErrorGroupedFolders = 'SYNC_ERROR_GROUPED_FOLDERS',
SyncErrorNoLibraries = 'SYNC_ERROR_NO_LIBRARIES',
Unknown = 'UNKNOWN',
}
12 changes: 5 additions & 7 deletions server/entity/Media.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type { DownloadingItem } from '@server/lib/downloadtracker';
import downloadTracker from '@server/lib/downloadtracker';
import { getSettings } from '@server/lib/settings';
import logger from '@server/logger';
import { getHostname } from '@server/utils/getHostname';
import {
AfterLoad,
Column,
Expand Down Expand Up @@ -211,15 +212,12 @@ class Media {
} else {
const pageName =
process.env.JELLYFIN_TYPE === 'emby' ? 'item' : 'details';
const { serverId, hostname, externalHostname } = getSettings().jellyfin;
let jellyfinHost =
const { serverId, externalHostname } = getSettings().jellyfin;

const jellyfinHost =
externalHostname && externalHostname.length > 0
? externalHostname
: hostname;

jellyfinHost = jellyfinHost.endsWith('/')
? jellyfinHost.slice(0, -1)
: jellyfinHost;
: getHostname();

if (this.jellyfinMediaId) {
this.mediaUrl = `${jellyfinHost}/web/index.html#!/${pageName}?id=${this.jellyfinMediaId}&context=home&serverId=${serverId}`;
Expand Down
3 changes: 2 additions & 1 deletion server/lib/availabilitySync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { User } from '@server/entity/User';
import type { RadarrSettings, SonarrSettings } from '@server/lib/settings';
import { getSettings } from '@server/lib/settings';
import logger from '@server/logger';
import { getHostname } from '@server/utils/getHostname';

class AvailabilitySync {
public running = false;
Expand Down Expand Up @@ -84,7 +85,7 @@ class AvailabilitySync {
) {
if (admin) {
this.jellyfinClient = new JellyfinAPI(
settings.jellyfin.hostname ?? '',
getHostname(),
admin.jellyfinAuthToken,
admin.jellyfinDeviceId
);
Expand Down
5 changes: 4 additions & 1 deletion server/lib/scanners/jellyfin/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type { Library } from '@server/lib/settings';
import { getSettings } from '@server/lib/settings';
import logger from '@server/logger';
import AsyncLock from '@server/utils/asyncLock';
import { getHostname } from '@server/utils/getHostname';
import { randomUUID as uuid } from 'crypto';
import { uniqWith } from 'lodash';

Expand Down Expand Up @@ -594,8 +595,10 @@ class JellyfinScanner {
return this.log('No admin configured. Jellyfin sync skipped.', 'warn');
}

const hostname = getHostname();

this.jfClient = new JellyfinAPI(
settings.jellyfin.hostname ?? '',
hostname,
admin.jellyfinAuthToken,
admin.jellyfinDeviceId
);
Expand Down
26 changes: 17 additions & 9 deletions server/lib/settings.ts → server/lib/settings/index.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { MediaServerType } from '@server/constants/server';
import { Permission } from '@server/lib/permissions';
import { runMigrations } from '@server/lib/settings/migrator';
import { randomUUID } from 'crypto';
import fs from 'fs';
import { merge } from 'lodash';
import path from 'path';
import webpush from 'web-push';
import { Permission } from './permissions';

export interface Library {
id: string;
Expand Down Expand Up @@ -38,7 +39,10 @@ export interface PlexSettings {

export interface JellyfinSettings {
name: string;
hostname: string;
ip: string;
port: number;
useSsl?: boolean;
urlBase?: string;
externalHostname?: string;
jellyfinForgotPasswordUrl?: string;
libraries: Library[];
Expand Down Expand Up @@ -130,7 +134,6 @@ interface FullPublicSettings extends PublicSettings {
region: string;
originalLanguage: string;
mediaServerType: number;
jellyfinHost?: string;
jellyfinExternalHost?: string;
jellyfinForgotPasswordUrl?: string;
jellyfinServerName?: string;
Expand Down Expand Up @@ -274,7 +277,7 @@ export type JobId =
| 'image-cache-cleanup'
| 'availability-sync';

interface AllSettings {
export interface AllSettings {
clientId: string;
vapidPublic: string;
vapidPrivate: string;
Expand All @@ -291,7 +294,7 @@ interface AllSettings {

const SETTINGS_PATH = process.env.CONFIG_DIRECTORY
? `${process.env.CONFIG_DIRECTORY}/settings.json`
: path.join(__dirname, '../../config/settings.json');
: path.join(__dirname, '../../../config/settings.json');

class Settings {
private data: AllSettings;
Expand Down Expand Up @@ -331,7 +334,10 @@ class Settings {
},
jellyfin: {
name: '',
hostname: '',
ip: '',
port: 8096,
useSsl: false,
urlBase: '',
externalHostname: '',
jellyfinForgotPasswordUrl: '',
libraries: [],
Expand Down Expand Up @@ -547,8 +553,6 @@ class Settings {
region: this.data.main.region,
originalLanguage: this.data.main.originalLanguage,
mediaServerType: this.main.mediaServerType,
jellyfinHost: this.jellyfin.hostname,
jellyfinExternalHost: this.jellyfin.externalHostname,
partialRequestsEnabled: this.data.main.partialRequestsEnabled,
cacheImages: this.data.main.cacheImages,
vapidPublic: this.vapidPublic,
Expand Down Expand Up @@ -637,7 +641,11 @@ class Settings {
const data = fs.readFileSync(SETTINGS_PATH, 'utf-8');

if (data) {
this.data = merge(this.data, JSON.parse(data));
const parsedJson = JSON.parse(data);
this.data = runMigrations(parsedJson);

this.data = merge(this.data, parsedJson);

this.save();
}
return this;
Expand Down
30 changes: 30 additions & 0 deletions server/lib/settings/migrations/0001_migrate_hostname.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import type { AllSettings } from '@server/lib/settings';

const migrateHostname = (settings: any): AllSettings => {
const oldJellyfinSettings = settings.jellyfin;
if (oldJellyfinSettings && oldJellyfinSettings.hostname) {
const { hostname } = oldJellyfinSettings;
const protocolMatch = hostname.match(/^(https?):\/\//i);
const useSsl = protocolMatch && protocolMatch[1].toLowerCase() === 'https';
const remainingUrl = hostname.replace(/^(https?):\/\//i, '');
const urlMatch = remainingUrl.match(/^([^:]+)(:([0-9]+))?(\/.*)?$/);

delete oldJellyfinSettings.hostname;
if (urlMatch) {
const [, ip, , port, urlBase] = urlMatch;
settings.jellyfin = {
...settings.jellyfin,
ip,
port: port || (useSsl ? 443 : 80),
useSsl,
urlBase: urlBase ? urlBase.replace(/\/$/, '') : '',
};
}
}
if (settings.jellyfin && settings.jellyfin.hostname) {
delete settings.jellyfin.hostname;
}
return settings;
};

export default migrateHostname;
21 changes: 21 additions & 0 deletions server/lib/settings/migrator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import type { AllSettings } from '@server/lib/settings';
import fs from 'fs';
import path from 'path';

const migrationsDir = path.join(__dirname, 'migrations');

export const runMigrations = (settings: AllSettings): AllSettings => {
const migrations = fs
.readdirSync(migrationsDir)
.filter((file) => file.endsWith('.js') || file.endsWith('.ts'))
// eslint-disable-next-line @typescript-eslint/no-var-requires
.map((file) => require(path.join(migrationsDir, file)).default);

let migrated = settings;

for (const migration of migrations) {
migrated = migration(migrated);
}

return migrated;
};
43 changes: 29 additions & 14 deletions server/routes/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { getSettings } from '@server/lib/settings';
import logger from '@server/logger';
import { isAuthenticated } from '@server/middleware/auth';
import { ApiError } from '@server/types/error';
import { getHostname } from '@server/utils/getHostname';
import * as EmailValidator from 'email-validator';
import { Router } from 'express';
import gravatarUrl from 'gravatar-url';
Expand Down Expand Up @@ -222,30 +223,39 @@ authRoutes.post('/jellyfin', async (req, res, next) => {
username?: string;
password?: string;
hostname?: string;
port?: number;
urlBase?: string;
useSsl?: boolean;
email?: string;
};

//Make sure jellyfin login is enabled, but only if jellyfin is not already configured
if (
settings.main.mediaServerType !== MediaServerType.JELLYFIN &&
settings.jellyfin.hostname !== ''
settings.main.mediaServerType != MediaServerType.NOT_CONFIGURED
) {
return res.status(500).json({ error: 'Jellyfin login is disabled' });
} else if (!body.username) {
return res.status(500).json({ error: 'You must provide an username' });
} else if (settings.jellyfin.hostname !== '' && body.hostname) {
} else if (settings.jellyfin.ip !== '' && body.hostname) {
return res
.status(500)
.json({ error: 'Jellyfin hostname already configured' });
} else if (settings.jellyfin.hostname === '' && !body.hostname) {
} else if (settings.jellyfin.ip === '' && !body.hostname) {
return res.status(500).json({ error: 'No hostname provided.' });
}

try {
const hostname =
settings.jellyfin.hostname !== ''
? settings.jellyfin.hostname
: body.hostname ?? '';
settings.jellyfin.ip !== ''
? getHostname()
: getHostname({
useSsl: body.useSsl,
ip: body.hostname,
port: body.port,
urlBase: body.urlBase,
});

const { externalHostname } = getSettings().jellyfin;

// Try to find deviceId that corresponds to jellyfin user, else generate a new one
Expand All @@ -261,17 +271,14 @@ authRoutes.post('/jellyfin', async (req, res, next) => {
'base64'
);
}

// First we need to attempt to log the user in to jellyfin
const jellyfinserver = new JellyfinAPI(hostname ?? '', undefined, deviceId);
let jellyfinHost =
const jellyfinserver = new JellyfinAPI(hostname, undefined, deviceId);
const jellyfinHost =
externalHostname && externalHostname.length > 0
? externalHostname
: hostname;

jellyfinHost = jellyfinHost.endsWith('/')
? jellyfinHost.slice(0, -1)
: jellyfinHost;

const ip = req.ip;
let clientIp;

Expand Down Expand Up @@ -328,8 +335,11 @@ authRoutes.post('/jellyfin', async (req, res, next) => {
const serverName = await jellyfinserver.getServerName();

settings.jellyfin.name = serverName;
settings.jellyfin.hostname = body.hostname ?? '';
settings.jellyfin.serverId = account.User.ServerId;
settings.jellyfin.ip = body.hostname ?? '';
settings.jellyfin.port = body.port ?? 8096;
settings.jellyfin.urlBase = body.urlBase ?? '';
settings.jellyfin.useSsl = body.useSsl ?? false;
settings.save();
startJobs();

Expand Down Expand Up @@ -444,7 +454,12 @@ authRoutes.post('/jellyfin', async (req, res, next) => {
label: 'Auth',
error: e.errorCode,
status: e.statusCode,
hostname: body.hostname,
hostname: getHostname({
useSsl: body.useSsl,
ip: body.hostname,
port: body.port,
urlBase: body.urlBase,
}),
}
);
return next({
Expand Down
Loading

0 comments on commit 38ad875

Please sign in to comment.