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

support for private instances #4259

Closed
wants to merge 4 commits into from
Closed

Conversation

stonerl
Copy link

@stonerl stonerl commented Nov 13, 2023

This is a modification of PR #3728. And addresses #446

Server admins can set the instance to be private. Which means it is only accessible with a registered user account.

The endpoints /api/v1/popular and /api/v1/trending are whitelisted because some clients expect them to be open.

This is a modification of PR iv-org#3728. And addresses iv-org#446

Server admins can set the instance to be private. Which means it is only accessible with a registered user account.

The endpoints `/api/v1/popular` and `/api/v1/trending` are whitelisted because some clients expect them to be open.
@unixfox
Copy link
Member

unixfox commented Nov 13, 2023

What is the exact difference between "private_instance" and "redirect_login"?

@stonerl
Copy link
Author

stonerl commented Nov 13, 2023

What is the exact difference between "private_instance" and "redirect_login"?

When an instance is set to private, the administrator can choose whether they want to redirect to the login page.

E.g. A user accesses the page https://my.invidious.instance/, since / is not whitelisted, The user gets redirected to https://my.invidious.instance/login when redirect_login is set to true, if set to false the user just sees a white page and does not get redirected. Without private_instance: true redirect_login has no effect.

redirect_login: true
redirect

redirect_login: false
no_redirect

@unixfox
Copy link
Member

unixfox commented Nov 13, 2023

Thank you for the feedback with the differences.

But what's the use case for the parameter "private_instance" here? It returns a blank page. How are the registered users supposed to use the instance if it returns a blank page for the home page?

@stonerl
Copy link
Author

stonerl commented Nov 13, 2023

Foremost. Sorry, for the failing workflows. That was me being stupid …

Regarding your question. Registered users can still access the page via https://my.invidious.instance/login since the login route is on the whitelist.

It's more or less: Security through obscurity. People end up on one's instance for whatever reason. But it is not clear from the beginning whether the site is working or not. So they might think, “Oh, it's broken” and go their merry way. I know that this isn't really secure, but it obfuscates the instance at least a little.

@unixfox
Copy link
Member

unixfox commented Nov 13, 2023

It's more or less: Security through obscurity. People end up on one's instance for whatever reason. But it is not clear from the beginning whether the site is working or not. So they might think, “Oh, it's broken” and go their merry way. I know that this isn't really secure, but it obfuscates the instance at least a little.

Well for me this functionality shouldn't be in invidious. It's something that you can do on the reverse proxy side.

I do understand why the log in functionality is implemented in invidious because you can't do this on the reverse proxy side. But the idea to hide the home page is something so niche that I don't think this should be included into the core of invidious.

@rix1337
Copy link

rix1337 commented Nov 13, 2023

Also Security by obscurity will not help against real threats.

The rest of the feature is looking good

@stonerl
Copy link
Author

stonerl commented Nov 13, 2023

Okay, I removed redirect_login.

And linting should work now 😆 I ran: crystal tool format

@syeopite
Copy link
Member

The before_all handler isn't called for the error routes. See kemalcr/kemal#663. Is that likely to impact this PR?

On those endpoints, Invidious will always be open to those without an account. I'm not sure if that that has any implications.

@stonerl
Copy link
Author

stonerl commented Nov 17, 2023

The before_all handler isn't called for the error routes. See kemalcr/kemal#663. Is that likely to impact this PR?

On those endpoints, Invidious will always be open to those without an account. I'm not sure if that that has any implications.

No, since non-existing routes are not whitelisted, these requests will always be forwarded to the login page.

@rix1337
Copy link

rix1337 commented Nov 18, 2023

What further changes are expected here?
Are we ready to merge?

@stonerl
Copy link
Author

stonerl commented Nov 18, 2023

From my side, it is good to go.

@syeopite
Copy link
Member

syeopite commented Nov 20, 2023

The user is being redirected but it seems like the handlers for the various endpoints are still being called in the end.

Try increase the log level to trace and query the /search endpoint. You'd see that the user got redirected to the /login page but the handler is still called and the query is still sent to YouTube, parsed, etc.

@stonerl
Copy link
Author

stonerl commented Nov 21, 2023

The user is being redirected but it seems like the handlers for the various endpoints are still being called in the end.

Okay, but I assume that is something that needs to be handled in another PR? Since this is an issue not directly related to private instances…

@rix1337
Copy link

rix1337 commented Nov 27, 2023

Agreed. Let’s not creep more features into this and instead release.

@syeopite
Copy link
Member

syeopite commented Nov 27, 2023

It very much is directly related and not a feature creep. It is a bug of the way private instances are implemented in this PR.

This PR redirects to /login for all endpoints when the user is not logged in. But the logic for those routes are still being ran anyways.

Not only is this not right for how a redirect should work, this bug:

  • Increases the load to what a public instance would see even on private instances as the same logic and requests are being done even when the user is not allowed to access the page

  • Increases the chances of Invidious being blocked due to all of the requests getting sent to YouTube on each redirect. Once again even when all the user sees is a /login page.

This is something that needs to be fixed for this PR to be merged

@stonerl
Copy link
Author

stonerl commented Nov 27, 2023

I'll take a look at this.

rix1337 added a commit to rix1337/docker-utube that referenced this pull request Dec 3, 2023
@sigulete
Copy link

I'm looking for this patch. Is it going to be merged soon?

Copy link

This pull request has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely abandoned or outdated. If you think this pull request is still relevant and applicable, you just have to post a comment and it will be unmarked.

@github-actions github-actions bot added the stale label Mar 18, 2024
@rix1337
Copy link

rix1337 commented Mar 18, 2024

Had this running I production for months now.
It’s very much still relevant

@unixfox
Copy link
Member

unixfox commented Mar 18, 2024

I'm looking for this patch. Is it going to be merged soon?

this comment is still the reason why this PR hasn't been merged yet: #4259 (comment)

@stonerl
Copy link
Author

stonerl commented Mar 18, 2024

I'm currently short on time. I'll update the PR as soon as possible.

@github-actions github-actions bot removed the stale label Mar 19, 2024
@stonerl stonerl marked this pull request as draft March 25, 2024 16:26
Copy link

This pull request has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely abandoned or outdated. If you think this pull request is still relevant and applicable, you just have to post a comment and it will be unmarked.

@github-actions github-actions bot added the stale label Jun 24, 2024
@rix1337
Copy link

rix1337 commented Jun 24, 2024

Activity

@unixfox unixfox closed this Jun 24, 2024
@unixfox
Copy link
Member

unixfox commented Jun 24, 2024

We will reopen the PR once #4259 (comment) has been tackled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants