login_only

[johnwmail]

PR refer to https://github.com/iv-org/invidious/issues/446

[unixfox]

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

You have added docker related things

[johnwmail]

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

[johnwmail]

Would you please review it again, thank you.

[SamantazFox]

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]

bump?

[johnwmail]

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

[rix1337]

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]

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]

Related Yattee issue: https://github.com/yattee/yattee/issues/552

[stonerl]

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?

[stonerl]

There is another unhandled exeption:

2023-11-09 18:02:26 UTC [info] 302 POST /feed/webhook/v1:1699275109:79a13f9f:051c7b138f7ac06da40a268a20537f3039d1810a 37.35ms
Unhandled exception in spawn: Nil assertion failed (NilAssertionError)
  from /usr/share/crystal/src/nil.cr:113:7 in 'not_nil!'
  from /usr/share/crystal/src/nil.cr:109:3 in 'not_nil!'
  from src/invidious/routes/feeds.cr:412:14 in '->'
  from /usr/share/crystal/src/fiber.cr:146:11 in 'run'
  from /usr/share/crystal/src/fiber.cr:98:34 in '->'
  from ???

[stonerl]

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]

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]

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

[Daviey]

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]

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]

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.

[stonerl]

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

[pievalentin]

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

[github-actions[bot]]

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]

This pr is still very much relevant