invidious icon indicating copy to clipboard operation
invidious copied to clipboard

Published 20 hours ago • verified publisher icon iv-org

support for private instances

Open stonerl opened this issue 1 year ago • 21 comments

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.

stonerl avatar Nov 13 '23 10:11 stonerl

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

unixfox avatar Nov 13 '23 11:11 unixfox

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

stonerl avatar Nov 13 '23 12:11 stonerl

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?

unixfox avatar Nov 13 '23 14:11 unixfox

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.

stonerl avatar Nov 13 '23 14:11 stonerl

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.

unixfox avatar Nov 13 '23 15:11 unixfox

Also Security by obscurity will not help against real threats.

The rest of the feature is looking good

rix1337 avatar Nov 13 '23 15:11 rix1337

Okay, I removed redirect_login.

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

stonerl avatar Nov 13 '23 21:11 stonerl

The before_all handler isn't called for the error routes. See https://github.com/kemalcr/kemal/issues/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.

syeopite avatar Nov 17 '23 09:11 syeopite

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.

stonerl avatar Nov 17 '23 10:11 stonerl

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

rix1337 avatar Nov 18 '23 12:11 rix1337

From my side, it is good to go.

stonerl avatar Nov 18 '23 13:11 stonerl

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.

syeopite avatar Nov 20 '23 19:11 syeopite

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…

stonerl avatar Nov 21 '23 09:11 stonerl

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

rix1337 avatar Nov 27 '23 14:11 rix1337

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

syeopite avatar Nov 27 '23 20:11 syeopite

I'll take a look at this.

stonerl avatar Nov 27 '23 22:11 stonerl

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

sigulete avatar Dec 11 '23 01:12 sigulete

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[bot] avatar Mar 18 '24 12:03 github-actions[bot]

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

rix1337 avatar Mar 18 '24 12:03 rix1337

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: https://github.com/iv-org/invidious/pull/4259#issuecomment-1828573067

unixfox avatar Mar 18 '24 14:03 unixfox

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

stonerl avatar Mar 18 '24 16:03 stonerl