login_only #3728

johnwmail · 2023-04-07T13:11:30Z

PR refer to #446

unixfox · 2023-04-08T13:23:59Z

Don't push other changes that have nothing to do with the feature.

You have added docker related things

johnwmail · 2023-04-08T14:27:17Z

Sorry, this is my first time submit PR, meanwhile I am learning CI/CD things.
Anyway, should be reverted, thank you.

johnwmail · 2023-04-08T14:32:15Z

Would you please review it again, thank you.

src/invidious/routes/before_all.cr

SamantazFox · 2023-05-06T15:13:51Z

Thanks for the PR, and sorry for the delay it took me to review.

This subject is a tough one: By adding that check at the bottom of before_all, you still allow an external user to load images and videos. If you want to entirely restrict access, you'd need to put that code before return if (on line 64), which invloves re-thinking how auth is done (at the moment, we don't check if the user is authenticated on content proxy routes for performance reasons, at is saves us some DB calls).

Ideally, we should use a local cache to store user sessions (see the kemalcr/kemal-session shard), so that we don't have to call the DB all the time on every request.

SamantazFox · 2023-07-14T16:49:29Z

bump?

johnwmail · 2023-07-14T23:17:52Z

Sorry for late reply, I have updated the code before "return if (line 64)", please review, thank you.

rix1337 · 2023-11-01T20:28:54Z

I built the patch from this specific pull request and its working: rix1337/docker-utube/general (arm image only)

Its broken however on the popular iOS client Yattee (even if logged in inside the app):

To help understand the error, I built Yattee locally and debugged the request that causes the it:

requestDescription String "GET https://HOSTNAME/api/v1/videos/e50028LAtAo"

This requests results in a http 500 error with the patch applied.

This is easily reproducible by requesting the URL when logged out:

When logged in, there is no 500 error.

I therefore assume, this patch is working correctly. To work with this new parameter, Yattee needs to request the videos Endpoint with the session cookie.

rix1337 · 2023-11-01T20:37:44Z

Also the health check from docker compose has now switched to unhealthy permanently:

    healthcheck:
      test: wget -nv --tries=1 --spider http://127.0.0.1:3000/api/v1/comments/jNQXAC9IVRw || exit 1
      interval: 30s
      timeout: 5s
      retries: 2

This endpoint should probably be replaced by another one, that response properly even when logged out.

Any suggestions?

rix1337 · 2023-11-01T22:37:29Z

Related Yattee issue: yattee/yattee#552

Robiqaz

please release. Was looking forward to this.

thank you.

Daviey

The change appears functionally good to me, except it does introduce an unhanded error when loading the loading page whilst not logged in:

2023-11-09 14:59:59 UTC [info] 302 GET / 441.37µs
Exception: Closed stream (IO::Error)
  from /usr/share/crystal/src/io.cr:121:5 in 'check_open'
  from /usr/share/crystal/src/compress/gzip/writer.cr:74:15 in 'write'
  from /usr/share/crystal/src/http/server/response.cr:97:7 in 'write'
  from /usr/share/crystal/src/io.cr:484:7 in 'write_string'
  from /usr/share/crystal/src/string.cr:5263:5 in 'to_s'
  from /usr/share/crystal/src/io.cr:177:5 in '<<'
  from /usr/share/crystal/src/io.cr:191:5 in 'print'
  from src/invidious/helpers/handlers.cr:40:5 in 'process_request'
  from lib/kemal/src/kemal/route_handler.cr:17:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/websocket_handler.cr:13:14 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/filter_handler.cr:21:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:212:5 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:94:12 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:145:12 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:70:5 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/ext/kemal_static_file_handler.cr:162:16 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/exception_handler.cr:8:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/logger.cr:17:35 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/init_handler.cr:12:7 in 'call'
  from /usr/share/crystal/src/http/server/request_processor.cr:51:11 in 'process'
  from /usr/share/crystal/src/http/server.cr:517:5 in 'handle_client'
  from /usr/share/crystal/src/http/server.cr:470:13 in '->'
  from /usr/share/crystal/src/fiber.cr:146:11 in 'run'
  from /usr/share/crystal/src/fiber.cr:98:34 in '->'
  from ???

2023-11-09 14:59:59 UTC [info] 302 GET /feed/popular 10.91ms
2023-11-09 14:59:59 UTC [info] 200 GET /login 839.7µs

Daviey · 2023-11-09T15:01:39Z

src/invidious/config.cr

@@ -84,6 +84,7 @@ class Config

  # Used to tell Invidious it is behind a proxy, so links to resources should be https://
  property https_only : Bool?
+  property login_only : Bool?


It would be good to add a descriptive comment on what this does, similar to the others.

stonerl · 2023-11-09T17:23:18Z

One more thing I noticed. When using a third-party client like Yattee. Adding the instance and logging into an account is only possible when login_only: false. Are there some API endpoints that can be made accessible to allow adding an instance and logging into an existing account with login_only: true?

src/invidious/routes/before_all.cr

@@ -100,6 +88,24 @@ module Invidious::Routes::BeforeAll
      end
    end

+    unregistered_path_whitelist = {"/", "/login", "/licenses", "/privacy"}


stonerl · 2023-11-09T19:30:32Z

The change appears functionally good to me, except it does introduce an unhanded error when loading the loading page whilst not logged in:

2023-11-09 14:59:59 UTC [info] 302 GET / 441.37µs
Exception: Closed stream (IO::Error)
  from /usr/share/crystal/src/io.cr:121:5 in 'check_open'
  from /usr/share/crystal/src/compress/gzip/writer.cr:74:15 in 'write'
  from /usr/share/crystal/src/http/server/response.cr:97:7 in 'write'
  from /usr/share/crystal/src/io.cr:484:7 in 'write_string'
  from /usr/share/crystal/src/string.cr:5263:5 in 'to_s'
  from /usr/share/crystal/src/io.cr:177:5 in '<<'
  from /usr/share/crystal/src/io.cr:191:5 in 'print'
  from src/invidious/helpers/handlers.cr:40:5 in 'process_request'
  from lib/kemal/src/kemal/route_handler.cr:17:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/websocket_handler.cr:13:14 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/filter_handler.cr:21:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:212:5 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:94:12 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:145:12 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:70:5 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/ext/kemal_static_file_handler.cr:162:16 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/exception_handler.cr:8:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/logger.cr:17:35 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/init_handler.cr:12:7 in 'call'
  from /usr/share/crystal/src/http/server/request_processor.cr:51:11 in 'process'
  from /usr/share/crystal/src/http/server.cr:517:5 in 'handle_client'
  from /usr/share/crystal/src/http/server.cr:470:13 in '->'
  from /usr/share/crystal/src/fiber.cr:146:11 in 'run'
  from /usr/share/crystal/src/fiber.cr:98:34 in '->'
  from ???

2023-11-09 14:59:59 UTC [info] 302 GET /feed/popular 10.91ms
2023-11-09 14:59:59 UTC [info] 200 GET /login 839.7µs

This unhandled exception also happens when logged in.

Daviey · 2023-11-10T09:10:21Z

src/invidious/routes/before_all.cr

+    unregistered_path_whitelist = {"/", "/login", "/licenses", "/privacy"}
+    if CONFIG.login_only && !env.get?("user") && !unregistered_path_whitelist.includes?(env.request.path)


This is good, but I also found that Clipious client requires /api/v1/stat when adding a new server to the client. It loads this to check it is a valid server, and refuses if it doesn't work (currently it returns a 500 error, which should be a bug anyway). However, even if this additional endpoint is added the client doesn't load Popular and Trending (default pane) because it makes these requests unauthenticated (will need to raise this as an issue with Clipious)

Added your input to a new suggestion.

src/invidious/routes/before_all.cr

+    unregistered_path_whitelist = {"/", "/login", "/licenses", "/privacy"}
+    if CONFIG.login_only && !env.get?("user") && !unregistered_path_whitelist.includes?(env.request.path)
+      env.response.headers["Location"] = "/login"
+      haltf env, status_code: 302
+    end


Daviey · 2023-11-10T13:05:51Z

Third party apps using the API are depending on some endpoints being unauthenticated such as /api/v1/popular and /api/v1/trending, currently there are giving a 500 error (but no traceback), and I haven't investigated the cause. These endpoints in https://github.com/iv-org/invidious/tree/master/src/invidious/routes/api/v1 are divided between Authenticated and others, where the others are implied to be Public. I tried adding authentication headers to the requests in Clipious, but the server side component doesn't seem to be expecting auth'.

Whilst adding "/api/v1/stats" facilitates login, the actual app' isn't really functional. So I think we state that api/v1 has a contract of having relatively open access, which makes me think that to implement this change, we either need to disable API access or create a v2. But someone closer to the project should really add their informed view of this!

stonerl · 2023-11-10T13:24:57Z

Well, these two endpoints could be added to the whitelist as well. Since video playback still wouldn't be possible without logging in.

Daviey · 2023-11-10T13:30:29Z

Well, these two endpoints could be added to the whitelist as well. Since video playback still wouldn't be possible without logging in.

True, I assumed the intent of login was to reduce data leak... but popular and trending isn't really sensitive.

rix1337 · 2023-11-10T14:59:36Z

Third party apps using the API are depending on some endpoints being unauthenticated such as /api/v1/popular and /api/v1/trending, currently there are giving a 500 error (but no traceback), and I haven't investigated the cause. These endpoints in https://github.com/iv-org/invidious/tree/master/src/invidious/routes/api/v1 are divided between Authenticated and others, where the others are implied to be Public. I tried adding authentication headers to the requests in Clipious, but the server side component doesn't seem to be expecting auth'.

Whilst adding "/api/v1/stats" facilitates login, the actual app' isn't really functional. So I think we state that api/v1 has a contract of having relatively open access, which makes me think that to implement this change, we either need to disable API access or create a v2. But someone closer to the project should really add their informed view of this!

Just adding that after I raised my issue with Yattee they were able to quickly produce a fix, so your findings may be isolated to Clipious.

The patch from this pr works perfectly in combination with the patch from Yattee, when I enable login_only.

stonerl · 2023-11-10T15:05:56Z

Well, these two endpoints could be added to the whitelist as well. Since video playback still wouldn't be possible without logging in.

True, I assumed the intent of login was to reduce data leak... but popular and trending isn't really sensitive.

I added them to the suggestion, as @rix1337 pointed out, Yattee does show the Trending and Popular (if enabled) when login_only: true. So I guess Clipious might need to make some modifications to allow for this to work without having these endpoints exposed via the whitelist.

src/invidious/routes/before_all.cr

+      env.response.headers["Location"] = "/login"
+      haltf env, status_code: 302


src/invidious/config.cr

@@ -84,6 +84,7 @@ class Config

  # Used to tell Invidious it is behind a proxy, so links to resources should be https://
  property https_only : Bool?
+  property login_only : Bool?


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.

stonerl · 2023-11-13T11:09:51Z

I opened a separate PR (#4259), where I address all the missing bits of this one.

pievalentin · 2023-11-29T16:55:42Z

That would be a cool feature to have! Yattee for instance doesn't support basic auth

github-actions · 2024-02-28T01:10:46Z

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.

rix1337 · 2024-02-28T07:55:37Z

This pr is still very much relevant

AlecKinnear · 2024-05-15T13:06:42Z

Invidious is great software, just discovered it. I'm a YouTube Premium user who can't stand all the extras and autoplay and visual noise any more (even Premium isn't enough).

Please guys make Invidious for logged in users only. We are running it on a small VPS and if even a small group of five people were to come and start watching 4K videos, our VPS would collapse.

At this point, having read the related issue and the [other commits](#4259, it seems more a question of ill-will than a technical obstacle. What I mean by ill-will is that some of the core developers don't believe that creating private Invidious instances should not be easy. Yes, I know about the http auth alternative but that effectively creates double login. I might be able to get by with whitelisted IP's but it's all a barrier to use, which a simple login barrier would be enough to solve.

That someone's stats program won't work when logged-in does not seem relevant. Mr Stats could just work on his own patch or not use the logged-in only feature.

Why don't you make logged-in only a paid feature? I'd happily pay $20 to support the project and to have this option. Yes, I could code it myself or hand apply these patches. But that doesn't help the project or others. It just means a whole lot of wasted duplicate effort.

Larslol140 · 2024-07-28T10:16:29Z

src/invidious/routes/before_all.cr

@@ -100,6 +88,24 @@ module Invidious::Routes::BeforeAll
      end
    end

+    unregistered_path_whitelist = {"/", "/login", "/licenses", "/privacy"}


I think /videoplayback needs to be added, otherwise I was not able to watch videos (DASH).

Larslol140 · 2024-08-21T15:50:06Z

I noticed that comments appear to be missing, but I couldn't narrow down the issue.

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.

johnwmail requested a review from a team as a code owner April 7, 2023 13:11

johnwmail requested review from SamantazFox and removed request for a team April 7, 2023 13:11

johnwmail requested a review from unixfox as a code owner April 8, 2023 11:11

johnwmail closed this Apr 8, 2023

johnwmail force-pushed the master branch from 4d5ec6f to 525e4bd Compare April 8, 2023 14:20

login_only

c676272

johnwmail reopened this Apr 8, 2023

LennyPenny reviewed Apr 9, 2023

View reviewed changes

src/invidious/routes/before_all.cr Outdated Show resolved Hide resolved

check the config flag first

b3637f2

tristan-k mentioned this pull request May 24, 2023

handle http basic auth in piped instance yattee/yattee#1

Closed

github-actions bot added the stale label Jul 9, 2023

johnwmail and others added 2 commits July 15, 2023 06:47

Merge branch 'iv-org:master' into master

af86cdb

@SamantazFox: Put that code before return if (on line 64)

f524405

Set the cookie first

d3e6d7b

github-actions bot removed the stale label Jul 15, 2023

github-actions bot added the stale label Aug 29, 2023

SamantazFox removed the stale label Sep 10, 2023

iv-org deleted a comment from github-actions bot Sep 10, 2023

unixfox mentioned this pull request Nov 1, 2023

Private Invidious #4222

Closed

rix1337 mentioned this pull request Nov 1, 2023

[Feature request] private invidious #446

Open

rix1337 mentioned this pull request Nov 1, 2023

Broken /videos/ request for private Invidious instances yattee/yattee#552

Closed

1 task

Robiqaz approved these changes Nov 6, 2023

View reviewed changes

Daviey suggested changes Nov 9, 2023

View reviewed changes

stonerl reviewed Nov 9, 2023

View reviewed changes

src/invidious/routes/before_all.cr

@@ -100,6 +88,24 @@ module Invidious::Routes::BeforeAll

end

end

unregistered_path_whitelist = {"/", "/login", "/licenses", "/privacy"}

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

stonerl reviewed Nov 9, 2023

View reviewed changes

stonerl reviewed Nov 10, 2023

View reviewed changes

src/invidious/routes/before_all.cr

Comment on lines +93 to +94

env.response.headers["Location"] = "/login"

haltf env, status_code: 302

This comment was marked as outdated.

Sign in to view

stonerl reviewed Nov 10, 2023

View reviewed changes

src/invidious/config.cr

@@ -84,6 +84,7 @@ class Config

# Used to tell Invidious it is behind a proxy, so links to resources should be https://

property https_only : Bool?

property login_only : Bool?

This comment was marked as outdated.

Sign in to view

stonerl mentioned this pull request Nov 13, 2023

support for private instances #4259

Closed

github-actions bot added the stale label Feb 28, 2024

github-actions bot removed the stale label Feb 28, 2024

Larslol140 reviewed Jul 28, 2024

View reviewed changes

Merge branch 'iv-org:master' into master

c3b06e7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

login_only #3728

login_only #3728

johnwmail commented Apr 7, 2023

unixfox commented Apr 8, 2023

johnwmail commented Apr 8, 2023

johnwmail commented Apr 8, 2023

SamantazFox commented May 6, 2023

SamantazFox commented Jul 14, 2023

johnwmail commented Jul 14, 2023

rix1337 commented Nov 1, 2023 •

edited

Loading

rix1337 commented Nov 1, 2023 •

edited

Loading

rix1337 commented Nov 1, 2023

Robiqaz left a comment

Daviey left a comment

Daviey Nov 9, 2023

stonerl commented Nov 9, 2023 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

stonerl commented Nov 9, 2023

This comment was marked as outdated.

Daviey Nov 10, 2023

stonerl Nov 10, 2023

This comment was marked as outdated.

Daviey commented Nov 10, 2023

stonerl commented Nov 10, 2023

Daviey commented Nov 10, 2023

rix1337 commented Nov 10, 2023

stonerl commented Nov 10, 2023

This comment was marked as outdated.

This comment was marked as outdated.

stonerl commented Nov 13, 2023

pievalentin commented Nov 29, 2023

github-actions bot commented Feb 28, 2024

rix1337 commented Feb 28, 2024

AlecKinnear commented May 15, 2024 •

edited

Loading

Larslol140 Jul 28, 2024

Larslol140 commented Aug 21, 2024

		unregistered_path_whitelist = {"/", "/login", "/licenses", "/privacy"}
		if CONFIG.login_only && !env.get?("user") && !unregistered_path_whitelist.includes?(env.request.path)

		env.response.headers["Location"] = "/login"
		haltf env, status_code: 302

login_only #3728

Are you sure you want to change the base?

login_only #3728

Conversation

johnwmail commented Apr 7, 2023

unixfox commented Apr 8, 2023

johnwmail commented Apr 8, 2023

johnwmail commented Apr 8, 2023

SamantazFox commented May 6, 2023

SamantazFox commented Jul 14, 2023

johnwmail commented Jul 14, 2023

rix1337 commented Nov 1, 2023 • edited Loading

rix1337 commented Nov 1, 2023 • edited Loading

rix1337 commented Nov 1, 2023

Robiqaz left a comment

Choose a reason for hiding this comment

Daviey left a comment

Choose a reason for hiding this comment

Daviey Nov 9, 2023

Choose a reason for hiding this comment

stonerl commented Nov 9, 2023 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

stonerl commented Nov 9, 2023

This comment was marked as outdated.

Daviey Nov 10, 2023

Choose a reason for hiding this comment

stonerl Nov 10, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

Daviey commented Nov 10, 2023

stonerl commented Nov 10, 2023

Daviey commented Nov 10, 2023

rix1337 commented Nov 10, 2023

stonerl commented Nov 10, 2023

This comment was marked as outdated.

This comment was marked as outdated.

stonerl commented Nov 13, 2023

pievalentin commented Nov 29, 2023

github-actions bot commented Feb 28, 2024

rix1337 commented Feb 28, 2024

AlecKinnear commented May 15, 2024 • edited Loading

Larslol140 Jul 28, 2024

Choose a reason for hiding this comment

Larslol140 commented Aug 21, 2024

rix1337 commented Nov 1, 2023 •

edited

Loading

rix1337 commented Nov 1, 2023 •

edited

Loading

stonerl commented Nov 9, 2023 •

edited

Loading

AlecKinnear commented May 15, 2024 •

edited

Loading