SSO: redirect to public URL before setting cookies by richvdh · Pull Request #9436 · matrix-org/synapse

richvdh · 2021-02-18T15:15:07Z

... otherwise, we don't get the cookie back.

synapse/http/site.py

clokep · 2021-02-18T15:20:06Z

When would requests arrive at the wrong URL? (Is this due to delegation of some kind?)

richvdh · 2021-02-18T15:47:56Z

When would requests arrive at the wrong URL?

The canonical CS API (and hence public_baseurl) for matrix.org is https://matrix-client.matrix.org, which goes directly to the synapse cluster, rather than via the general matrix.org cluster.

Mostly for historical reasons, it's also possible to log in via https://matrix.org. It seems like some clients default to that, which might be a thing we should look into getting them to change.

synapse/http/__init__.py

synapse/http/site.py

synapse/rest/client/v1/login.py

Hit synapse on the public_baseurl to avoid a redirect.

richvdh · 2021-02-24T18:17:50Z

tests/rest/client/v1/test_login.py

-BASE_URL = "https://synapse/"
+# synapse server name: used to populate public_base_url in some tests
+SYNAPSE_SERVER_PUBLIC_HOSTNAME = "synapse"
+BASE_URL = "http://%s/" % (SYNAPSE_SERVER_PUBLIC_HOSTNAME,)


swapping to http here, because FakeChannel.isSecure() returns False, so synapse will see the requested uri as http://synapse/.... Configuring that as the public_baseurl avoids the redirect.

Might be good to include this comment in the code. Just looking at the file, I'd be a bit confused why other URLs here were https, but not the public base url.

fair enough. I've updated some comments.

richvdh · 2021-02-24T18:19:20Z

tests/rest/client/v1/utils.py

+        # that should 302 us to the public base url
+        assert channel.code == 302
+        location = channel.headers.getRawHeaders("Location")[0]
+        parts = urllib.parse.urlsplit(location)
+        channel = make_request(
+            self.hs.get_reactor(),
+            self.site,
+            "GET",
+            urllib.parse.urlunsplit(("", "") + parts[2:]),
+            custom_headers=[
+                ("Host", parts[1]),
+            ],
+        )
+


this was the easiest way of figuring out what the Host header should be set to.

richvdh · 2021-02-24T18:19:29Z

tests/rest/client/v2_alpha/test_auth.py

    def default_config(self):
        config = super().default_config()
-        config["public_baseurl"] = "https://synapse.test"
+        config["public_baseurl"] = "http://synapse.test"


richvdh · 2021-02-24T18:26:15Z

I think this is finally ready-for-review after a week of banging my head against various infrastructure-related problems. The commits should be reviewable independently.

anoadragon453

This looks pretty reasonable to me.

anoadragon453 · 2021-02-26T10:28:08Z

tests/rest/client/v1/test_login.py

-BASE_URL = "https://synapse/"
+# synapse server name: used to populate public_base_url in some tests
+SYNAPSE_SERVER_PUBLIC_HOSTNAME = "synapse"
+BASE_URL = "http://%s/" % (SYNAPSE_SERVER_PUBLIC_HOSTNAME,)


Might be good to include this comment in the code. Just looking at the file, I'd be a bit confused why other URLs here were https, but not the public base url.

This reverts commit 5ee8a1c. This has now been superceded on develop by PR #9436.

@dklimpel

Synapse 1.29.0 (2021-03-08) =========================== Note that synapse now expects an `X-Forwarded-Proto` header when used with a reverse proxy. Please see [UPGRADE.rst](UPGRADE.rst#upgrading-to-v1290) for more details on this change. No significant changes. Synapse 1.29.0rc1 (2021-03-04) ============================== Features -------- - Add rate limiters to cross-user key sharing requests. ([\#8957](matrix-org/synapse#8957)) - Add `order_by` to the admin API `GET /_synapse/admin/v1/users/<user_id>/media`. Contributed by @dklimpel. ([\#8978](matrix-org/synapse#8978)) - Add some configuration settings to make users' profile data more private. ([\#9203](matrix-org/synapse#9203)) - The `no_proxy` and `NO_PROXY` environment variables are now respected in proxied HTTP clients with the lowercase form taking precedence if both are present. Additionally, the lowercase `https_proxy` environment variable is now respected in proxied HTTP clients on top of existing support for the uppercase `HTTPS_PROXY` form and takes precedence if both are present. Contributed by Timothy Leung. ([\#9372](matrix-org/synapse#9372)) - Add a configuration option, `user_directory.prefer_local_users`, which when enabled will make it more likely for users on the same server as you to appear above other users. ([\#9383](matrix-org/synapse#9383), [\#9385](matrix-org/synapse#9385)) - Add support for regenerating thumbnails if they have been deleted but the original image is still stored. ([\#9438](matrix-org/synapse#9438)) - Add support for `X-Forwarded-Proto` header when using a reverse proxy. ([\#9472](matrix-org/synapse#9472), [\#9501](matrix-org/synapse#9501), [\#9512](matrix-org/synapse#9512), [\#9539](matrix-org/synapse#9539)) Bugfixes -------- - Fix a bug where users' pushers were not all deleted when they deactivated their account. ([\#9285](matrix-org/synapse#9285), [\#9516](matrix-org/synapse#9516)) - Fix a bug where a lot of unnecessary presence updates were sent when joining a room. ([\#9402](matrix-org/synapse#9402)) - Fix a bug that caused multiple calls to the experimental `shared_rooms` endpoint to return stale results. ([\#9416](matrix-org/synapse#9416)) - Fix a bug in single sign-on which could cause a "No session cookie found" error. ([\#9436](matrix-org/synapse#9436)) - Fix bug introduced in v1.27.0 where allowing a user to choose their own username when logging in via single sign-on did not work unless an `idp_icon` was defined. ([\#9440](matrix-org/synapse#9440)) - Fix a bug introduced in v1.26.0 where some sequences were not properly configured when running `synapse_port_db`. ([\#9449](matrix-org/synapse#9449)) - Fix deleting pushers when using sharded pushers. ([\#9465](matrix-org/synapse#9465), [\#9466](matrix-org/synapse#9466), [\#9479](matrix-org/synapse#9479), [\#9536](matrix-org/synapse#9536)) - Fix missing startup checks for the consistency of certain PostgreSQL sequences. ([\#9470](matrix-org/synapse#9470)) - Fix a long-standing bug where the media repository could leak file descriptors while previewing media. ([\#9497](matrix-org/synapse#9497)) - Properly purge the event chain cover index when purging history. ([\#9498](matrix-org/synapse#9498)) - Fix missing chain cover index due to a schema delta not being applied correctly. Only affected servers that ran development versions. ([\#9503](matrix-org/synapse#9503)) - Fix a bug introduced in v1.25.0 where `/_synapse/admin/join/` would fail when given a room alias. ([\#9506](matrix-org/synapse#9506)) - Prevent presence background jobs from running when presence is disabled. ([\#9530](matrix-org/synapse#9530)) - Fix rare edge case that caused a background update to fail if the server had rejected an event that had duplicate auth events. ([\#9537](matrix-org/synapse#9537)) Improved Documentation ---------------------- - Update the example systemd config to propagate reloads to individual units. ([\#9463](matrix-org/synapse#9463)) Internal Changes ---------------- - Add documentation and type hints to `parse_duration`. ([\#9432](matrix-org/synapse#9432)) - Remove vestiges of `uploads_path` configuration setting. ([\#9462](matrix-org/synapse#9462)) - Add a comment about systemd-python. ([\#9464](matrix-org/synapse#9464)) - Test that we require validated email for email pushers. ([\#9496](matrix-org/synapse#9496)) - Allow python to generate bytecode for synapse. ([\#9502](matrix-org/synapse#9502)) - Fix incorrect type hints. ([\#9515](matrix-org/synapse#9515), [\#9518](matrix-org/synapse#9518)) - Add type hints to device and event report admin API. ([\#9519](matrix-org/synapse#9519)) - Add type hints to user admin API. ([\#9521](matrix-org/synapse#9521)) - Bump the versions of mypy and mypy-zope used for static type checking. ([\#9529](matrix-org/synapse#9529))

richvdh commented Feb 18, 2021

View reviewed changes

synapse/http/site.py Outdated Show resolved Hide resolved

richvdh requested a review from a team February 18, 2021 15:49

clokep reviewed Feb 18, 2021

View reviewed changes

synapse/http/__init__.py Outdated Show resolved Hide resolved

synapse/http/site.py Outdated Show resolved Hide resolved

synapse/rest/client/v1/login.py Show resolved Hide resolved

richvdh added the A-Social Login Login via external identity providers label Feb 19, 2021

richvdh self-assigned this Feb 23, 2021

richvdh force-pushed the rav/fix_cookie_path branch from 33ce200 to 8299db8 Compare February 23, 2021 15:21

richvdh added 3 commits February 24, 2021 18:15

test_login: Factor out a utility function

00a3399

Update some SSO tests to use public url

761e5aa

Hit synapse on the public_baseurl to avoid a redirect.

Redirect redirect requests if they arrive on the wrong URI

7167082

richvdh force-pushed the rav/fix_cookie_path branch from 8299db8 to 7167082 Compare February 24, 2021 18:15

richvdh commented Feb 24, 2021

View reviewed changes

newsfile

694ba54

richvdh requested a review from a team February 24, 2021 18:25

anoadragon453 approved these changes Feb 26, 2021

View reviewed changes

update comments

1b436be

richvdh merged commit 15090de into develop Feb 26, 2021

richvdh deleted the rav/fix_cookie_path branch February 26, 2021 14:02

richvdh added a commit that referenced this pull request Feb 26, 2021

Revert "Redirect redirect requests if they arrive on the wrong URI"

0e56f02

This reverts commit 5ee8a1c. This has now been superceded on develop by PR #9436.

warricksothr mentioned this pull request Jul 28, 2021

Infinte Redirect Loop For SSO Login When Behind A Reverse Proxy #10492

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SSO: redirect to public URL before setting cookies#9436

SSO: redirect to public URL before setting cookies#9436
richvdh merged 5 commits intodevelopfrom
rav/fix_cookie_path

richvdh commented Feb 18, 2021

Uh oh!

Uh oh!

clokep commented Feb 18, 2021

Uh oh!

richvdh commented Feb 18, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richvdh Feb 24, 2021

Uh oh!

anoadragon453 Feb 26, 2021

Uh oh!

richvdh Feb 26, 2021

Uh oh!

richvdh Feb 24, 2021

Uh oh!

richvdh Feb 24, 2021

Uh oh!

richvdh commented Feb 24, 2021

Uh oh!

anoadragon453 left a comment

Uh oh!

anoadragon453 Feb 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

richvdh commented Feb 18, 2021

Uh oh!

Uh oh!

clokep commented Feb 18, 2021

Uh oh!

richvdh commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richvdh Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

anoadragon453 Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

richvdh Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

richvdh Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

richvdh Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

richvdh commented Feb 24, 2021

Uh oh!

anoadragon453 left a comment

Choose a reason for hiding this comment

Uh oh!

anoadragon453 Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

richvdh commented Feb 18, 2021 •

edited

Loading