Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

SSO: redirect to public URL before setting cookies #9436

Merged
merged 5 commits into from
Feb 26, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 32 additions & 24 deletions tests/rest/client/v1/test_login.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

import time
import urllib.parse
from typing import Any, Dict, List, Union
from typing import Any, Dict, List, Optional, Union
from urllib.parse import urlencode

from mock import Mock
Expand Down Expand Up @@ -47,8 +47,9 @@
HAS_JWT = False


# public_base_url used in some tests
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,)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough. I've updated some comments.


# CAS server used in some tests
CAS_SERVER = "https://fake.test"
Expand Down Expand Up @@ -480,11 +481,7 @@ def test_get_msc2858_login_flows(self):
def test_multi_sso_redirect(self):
"""/login/sso/redirect should redirect to an identity picker"""
# first hit the redirect url, which should redirect to our idp picker
channel = self.make_request(
"GET",
"/_matrix/client/r0/login/sso/redirect?redirectUrl="
+ urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL),
)
channel = self._make_sso_redirect_request(False, None)
self.assertEqual(channel.code, 302, channel.result)
uri = channel.headers.getRawHeaders("Location")[0]

Expand Down Expand Up @@ -628,41 +625,52 @@ def test_multi_sso_redirect_to_unknown(self):

def test_client_idp_redirect_msc2858_disabled(self):
"""If the client tries to pick an IdP but MSC2858 is disabled, return a 400"""
channel = self.make_request(
"GET",
"/_matrix/client/unstable/org.matrix.msc2858/login/sso/redirect/oidc?redirectUrl="
+ urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL),
)
channel = self._make_sso_redirect_request(True, "oidc")
self.assertEqual(channel.code, 400, channel.result)
self.assertEqual(channel.json_body["errcode"], "M_UNRECOGNIZED")

@override_config({"experimental_features": {"msc2858_enabled": True}})
def test_client_idp_redirect_to_unknown(self):
"""If the client tries to pick an unknown IdP, return a 404"""
channel = self.make_request(
"GET",
"/_matrix/client/unstable/org.matrix.msc2858/login/sso/redirect/xxx?redirectUrl="
+ urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL),
)
channel = self._make_sso_redirect_request(True, "xxx")
self.assertEqual(channel.code, 404, channel.result)
self.assertEqual(channel.json_body["errcode"], "M_NOT_FOUND")

@override_config({"experimental_features": {"msc2858_enabled": True}})
def test_client_idp_redirect_to_oidc(self):
"""If the client pick a known IdP, redirect to it"""
channel = self.make_request(
"GET",
"/_matrix/client/unstable/org.matrix.msc2858/login/sso/redirect/oidc?redirectUrl="
+ urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL),
)

channel = self._make_sso_redirect_request(True, "oidc")
self.assertEqual(channel.code, 302, channel.result)
oidc_uri = channel.headers.getRawHeaders("Location")[0]
oidc_uri_path, oidc_uri_query = oidc_uri.split("?", 1)

# it should redirect us to the auth page of the OIDC server
self.assertEqual(oidc_uri_path, TEST_OIDC_AUTH_ENDPOINT)

def _make_sso_redirect_request(
self, unstable_endpoint: bool = False, idp_prov: Optional[str] = None
):
"""Send a request to /_matrix/client/r0/login/sso/redirect

... or the unstable equivalent

... possibly specifying an IDP provider
"""
endpoint = (
"/_matrix/client/unstable/org.matrix.msc2858/login/sso/redirect"
if unstable_endpoint
else "/_matrix/client/r0/login/sso/redirect"
)
if idp_prov is not None:
endpoint += "/" + idp_prov
endpoint += "?redirectUrl=" + urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL)

return self.make_request(
"GET",
endpoint,
custom_headers=[("Host", SYNAPSE_SERVER_PUBLIC_HOSTNAME)],
)

@staticmethod
def _get_value_from_macaroon(macaroon: pymacaroons.Macaroon, key: str) -> str:
prefix = key + " = "
Expand Down
2 changes: 1 addition & 1 deletion tests/rest/client/v2_alpha/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ class UIAuthTests(unittest.HomeserverTestCase):

def default_config(self):
config = super().default_config()
config["public_baseurl"] = "https://synapse.test"
config["public_baseurl"] = "http://synapse.test"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above


if HAS_OIDC:
# we enable OIDC as a way of testing SSO flows
Expand Down