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

Use http by default on ConnectToServerView #971

Merged
merged 7 commits into from
Apr 17, 2024

Conversation

alasclar
Copy link
Contributor

Change ConnectToServerView to use http by default, without including in it the text field - as specified by @LePips in #967. Retains previous behaviour of the connect button being disabled when text field is empty.

Swiftfin/Views/ConnectToServerView.swift Outdated Show resolved Hide resolved
Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

I just forgot this last time. Additionally, does with actually work with an https 302 redirect? I recall a while ago people asking about it but can't remember if I did work for it. We need to make sure that works beforehand.

Swiftfin tvOS/Views/ConnectToServerView.swift Outdated Show resolved Hide resolved
Swiftfin/Views/ConnectToServerView.swift Outdated Show resolved Hide resolved
@alasclar
Copy link
Contributor Author

I just forgot this last time. Additionally, does with actually work with an https 302 redirect? I recall a while ago people asking about it but can't remember if I did work for it. We need to make sure that works beforehand.

I can tell you that it is working with my setup - I am using nginx proxy manager to both direct my jellyfin domain to the correct port on the server and to create a certificate for https. Both before this change and after, the app would successfully connect to my server whether I specify https or not. I have assumed that the final connection would be over https regardless, the same way a browser redirects to https if available anyway, but I don't actually have any proof.

Outputting the serverState upon connection gives an http url, which isn't a good sign. I will have a look at the code further to see if I can find any proof either way.

@alasclar
Copy link
Contributor Author

@LePips
I haven't had much luck with this, I haven't found any evidence that the https redirect is actually happening. If someone with more networking expertise knows that'd be great, but otherwise I don't think it makes sense to assume http in the text entry - as if I don't have to specify the prefix I, as a user, would assume that https is being used if available.

To be honest if a redirect isn't happening I think my previous PR #967 doesn't look like a bad option, but it could also make sense to just add that redirect functionality in. I could achieve that by just calling connectToServer twice and silencing the thrown errors (first thing that came into my head), but that's definitely not the most efficient way of doing that.

@LePips
Copy link
Member

LePips commented Feb 20, 2024

No, we just need to figure out what actually happens with a 301/302 redirect. I remember doing work for it but can't remember if I broke it. Let's not over-complicate things since we should handle that anyways.

I'm not able to personally set anything up for a while but I'll get to it when I can.

@LePips
Copy link
Member

LePips commented Apr 16, 2024

Sorry it's been a while but I took at this today and sadly a lot more post-processing was required in order to make this possible. We actually are fine on properly handling redirects, however the issue came from how we store the connection URL, which should be the final URL from the response if it's different (I think, idk if this has some weird RFC technicalities).

URL manipulation via strings always feels hacky but this works with a basic http -> https redirect. I wasn't able to test with a redirect to a completely different URL, or from/to a server with the server living at a subpath. However, we can deal with that if it comes up.

If you would like to continue this work, you can see my branch below and merge it into this PR and fix for tvOS since you started this work. If not, I can open a different PR to merge.

@LePips LePips added the enhancement New feature or request label Apr 16, 2024
Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

Actually don't need the url check in the views, but it doesn't break anything and I can clean that up since I intend to work on these views soon.

@LePips LePips merged commit 272799d into jellyfin:main Apr 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants