gristlabs / grist-core

Grist is the evolution of spreadsheets.
https://www.getgrist.com/
Apache License 2.0
6.94k stars 304 forks source link

Grist doesn't respect changes in FORWARD_AUTH_HEADER when doing SSO #207

Open helmut72 opened 2 years ago

helmut72 commented 2 years ago

Based on following how-tos: https://community.getgrist.com/t/a-template-for-self-hosting-grist-with-traefik-and-docker-compose/856 https://community.getgrist.com/t/grist-authelia-custom-logout-path/967

... I have setup Grist with Caddy as Reverse Proxy and Authelia for authentication. But I think there is an error on Grist when it comes to single sign on.

For showing the problem I have installed 2 different whoami, whoami.example.com and whoami2.example.com. Both secured with Authelia. Also Grist is secured.

Logout will be catched on all 3 apps by Caddy when calling /signed-out:

1. open in a tab whoami.example.com, login with Authelia
   Result: Remote-Email: myuser@example.com, Cookie: 123

2. open in a tab whoami2.example.com
   Result: Remote-Email: myuser@example.com, Cookie: 123

3. open in a tab grist.example.com, press sign-in
   Result: signed in automatically, Auth[GET]: id 6 email myuser@example.com, Cookie: 123

4. open whoami.example.com/signed-out
   Result: Authelia login page returns to both whoami and whoami2
           on grist Auth[GET]: id 6 email myuser@example.com, Cookie: 123, stil logged in!!

5. login to whoami.example.com with anotheruser
   Result: whoami and whoami2 are logged in with Remote-Email: anotheruser@example.com, Cookie: 789
           on grist still Auth[GET]: id 6 email myuser@example.com, but Cookie is: 789

Everything works fine as long as I only use Grist for Login/Logout. When I login on another Webapp first, Grist doesn't respect the new login from another user.

paulfitz commented 2 years ago

Thanks for spelling this out @helmut72. It makes sense, the ForwardAuthLogin mechanism as implemented doesn't play well with peer sites. I think the fix could be fairly straightforward.

helmut72 commented 2 years ago

Thank you. That would be great!

paulfitz commented 2 years ago

Hi @helmut72, a recent commit cleaned up behavior and gave some more options https://github.com/gristlabs/grist-core/commit/561d9696aa43a7cd4efc4c349d9868b5b33bd678. Should be included in the latest grist-core docker image now. I'd be interested if it helps with your situation.

helmut72 commented 2 years ago

My intention no to use authentication on all paths was the feature to share some tables with anonymous users. Using authentication on all paths even works great with 0.7.9. Now with the latest docker image and to have shared links, I need to set GRIST_IGNORE_SESSION=false, right?

When it's set to false, nothing really changes. The old user is still logged in. I need to logout from Grist after Step 5. In the meantime another user is logged in on whoami.example.com and whoami2.example.com.

One other question. What do you mean with GRIST_PROXY_AUTH_HEADER? Do I also need to set this variable or should I leave it empty?

Thank you!

paulfitz commented 2 years ago

GRIST_IGNORE_SESSION will default to false, no need to set it.

That's too bad, in that case the updates won't help you. Yes, if you do need to set a session on Grist, then you are going to have a user on Grist that could be inconsistent with the user elsewhere. There's no signal I know of that would let Grist know to remove/update it (since in your situation you're specifically omitting to pass on the auth header). Is it possible to ask Caddy to hit an endpoint to let Grist know it should remove the user?

I wonder if it would be useful to have a distinct url available for sharing with anonymous users. Something with a common prefix that can be easily excluded from auth by reverse proxies.

The GRIST_PROXY_AUTH_HEADER environment variable is something that already existed, from a user contributed feature https://github.com/gristlabs/grist-core/pull/165/files. You don't need it when using forward auth.

helmut72 commented 2 years ago

I wonder if it would be useful to have a distinct url available for sharing with anonymous users. Something with a common prefix that can be easily excluded from auth by reverse proxies.

An own prefix is common for API access or shared links (for example Nextcloud). This would solve the problem.

Grist works perfect when the reverse proxy handles session management if authentication is on all paths, but then I lost sharing with others.

helmut72 commented 2 years ago

Any update?

My intention is dropping SAML/Keycloak for Authelia and using sharing also with header auth, because it would be cool to integrate Grist tables to Outline like it's possible with Airtable: https://www.getoutline.com

Tables are better placed into something like Grist and Text is better placed into something like Outline.

Thank you.

paulfitz commented 2 years ago

No update, sorry. One thing I did want to mention is a currently undocumented feature where you can assign a custom id to a document, like https://templates.getgrist.com/doc/afterschool-program - see how the url is /doc/ instead of /GeNeRatED-Id/? You can set this kind of id via the api, using https://support.getgrist.com/api/#tag/docs/paths/~1docs~1{docId}/patch and supplying a urlId value. You could choose to do this with docs you want to share, and leave the /doc prefix open for anonymous access.

Otherwise, I think implementing the feature you're looking for would mostly involve adding a new endpoint family here https://github.com/gristlabs/grist-core/blob/main/app/server/lib/AppEndpoint.ts#L301-L303 and tweaking docHandler to treat docs that are shared with the public as having a distinct canonical url. There might need to be some refactoring throughout the app to make sure that any code issuing urls for a doc is aware of its current sharing status.

helmut72 commented 2 years ago

One thing I did want to mention is a currently undocumented feature

Thanks! This is enough for personal use, because I'm being able to include a tables/URLs in Markdown documents and no one need an account on my Grist installation to view/open the table.

There might need to be some refactoring throughout the app

I understand that this takes a longer time, also needs testing. Will ask in some months again. ;-) Thanks for sharing this undocumented feature. Unfortunately I'm busy these days and need to test it, but this should help.