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

Fix SSO on workers #9271

Merged
merged 7 commits into from
Feb 1, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions changelog.d/9271.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix single-sign-on when the endpoints are routed to synapse workers.
18 changes: 10 additions & 8 deletions docs/workers.md
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,6 @@ expressions:
^/_matrix/client/(api/v1|r0|unstable)/joined_groups$
^/_matrix/client/(api/v1|r0|unstable)/publicised_groups$
^/_matrix/client/(api/v1|r0|unstable)/publicised_groups/
^/_synapse/client/password_reset/email/submit_token$
Copy link
Member Author

Choose a reason for hiding this comment

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

tangential, but we may as well fix it while we're here.

This line was added in #8227. AFAICT, it has never worked on a worker.


# Registration/login requests
^/_matrix/client/(api/v1|r0|unstable)/login$
Expand Down Expand Up @@ -256,25 +255,28 @@ Additionally, the following endpoints should be included if Synapse is configure
to use SSO (you only need to include the ones for whichever SSO provider you're
using):

# for all SSO providers
^/_matrix/client/(api/v1|r0|unstable)/login/sso/redirect
^/_synapse/client/pick_idp$
^/_synapse/client/pick_username
^/_synapse/client/sso_register$

# OpenID Connect requests.
^/_matrix/client/(api/v1|r0|unstable)/login/sso/redirect$
^/_synapse/oidc/callback$

# SAML requests.
^/_matrix/client/(api/v1|r0|unstable)/login/sso/redirect$
^/_matrix/saml2/authn_response$

# CAS requests.
^/_matrix/client/(api/v1|r0|unstable)/login/(cas|sso)/redirect$
Copy link
Member

Choose a reason for hiding this comment

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

Note that this one was slightly different in that it has (cas|sso) in it. Not sure if any clients still use the cas path though.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeahh, I'm hoping that's old enough that basically nobody will encounter it.

^/_matrix/client/(api/v1|r0|unstable)/login/cas/ticket$

Note that a HTTP listener with `client` and `federation` resources must be
configured in the `worker_listeners` option in the worker config.

Ensure that all SSO logins go to a single process (usually the main process).
Ensure that all SSO logins go to a single process.
Copy link
Member

Choose a reason for hiding this comment

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

This is probably true for the UI auth endpoints too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure it is? afaik nothing is tracked in-memory for UIA (at least for CAS/OIDC; SAML has #7530)

Copy link
Member

Choose a reason for hiding this comment

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

Right, the username mapping sessions and extra attributes only matter on login, not during UI auth. OK.

For multiple workers not handling the SSO endpoints properly, see
[#7530](https://github.com/matrix-org/synapse/issues/7530).

Note that a HTTP listener with `client` and `federation` resources must be
configured in the `worker_listeners` option in the worker config.

#### Load balancing

It is possible to run multiple instances of this worker app, with incoming requests
Expand Down
11 changes: 7 additions & 4 deletions synapse/app/generic_worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from typing_extensions import ContextManager

from twisted.internet import address
from twisted.web.resource import IResource

import synapse
import synapse.events
Expand Down Expand Up @@ -90,9 +91,8 @@
ToDeviceStream,
)
from synapse.rest.admin import register_servlets_for_media_repo
from synapse.rest.client.v1 import events, room
from synapse.rest.client.v1 import events, login, room
from synapse.rest.client.v1.initial_sync import InitialSyncRestServlet
from synapse.rest.client.v1.login import LoginRestServlet
from synapse.rest.client.v1.profile import (
ProfileAvatarURLRestServlet,
ProfileDisplaynameRestServlet,
Expand Down Expand Up @@ -127,6 +127,7 @@
from synapse.rest.client.versions import VersionsRestServlet
from synapse.rest.health import HealthResource
from synapse.rest.key.v2 import KeyApiV2Resource
from synapse.rest.synapse.client import build_synapse_client_resource_tree
from synapse.server import HomeServer, cache_in_self
from synapse.storage.databases.main.censor_events import CensorEventsStore
from synapse.storage.databases.main.client_ips import ClientIpWorkerStore
Expand Down Expand Up @@ -507,7 +508,7 @@ def _listen_http(self, listener_config: ListenerConfig):
site_tag = port

# We always include a health resource.
resources = {"/health": HealthResource()}
resources = {"/health": HealthResource()} # type: Dict[str, IResource]

for res in listener_config.http_options.resources:
for name in res.names:
Expand All @@ -517,7 +518,7 @@ def _listen_http(self, listener_config: ListenerConfig):
resource = JsonResource(self, canonical_json=False)

RegisterRestServlet(self).register(resource)
LoginRestServlet(self).register(resource)
login.register_servlets(self, resource)
ThreepidRestServlet(self).register(resource)
DevicesRestServlet(self).register(resource)
KeyQueryServlet(self).register(resource)
Expand Down Expand Up @@ -557,6 +558,8 @@ def _listen_http(self, listener_config: ListenerConfig):
groups.register_servlets(self, resource)

resources.update({CLIENT_API_PREFIX: resource})

resources.update(build_synapse_client_resource_tree(self))
elif name == "federation":
resources.update({FEDERATION_PREFIX: TransportLayerServer(self)})
elif name == "media":
Expand Down
18 changes: 2 additions & 16 deletions synapse/app/homeserver.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,7 @@
from synapse.rest.admin import AdminRestResource
from synapse.rest.health import HealthResource
from synapse.rest.key.v2 import KeyApiV2Resource
from synapse.rest.synapse.client.pick_idp import PickIdpResource
from synapse.rest.synapse.client.pick_username import pick_username_resource
from synapse.rest.synapse.client.sso_register import SsoRegisterResource
from synapse.rest.synapse.client import build_synapse_client_resource_tree
from synapse.rest.well_known import WellKnownResource
from synapse.server import HomeServer
from synapse.storage import DataStore
Expand Down Expand Up @@ -191,22 +189,10 @@ def _configure_named_resource(self, name, compress=False):
"/_matrix/client/versions": client_resource,
"/.well-known/matrix/client": WellKnownResource(self),
"/_synapse/admin": AdminRestResource(self),
"/_synapse/client/pick_username": pick_username_resource(self),
"/_synapse/client/pick_idp": PickIdpResource(self),
"/_synapse/client/sso_register": SsoRegisterResource(self),
**build_synapse_client_resource_tree(self),
}
)

if self.get_config().oidc_enabled:
from synapse.rest.oidc import OIDCResource

resources["/_synapse/oidc"] = OIDCResource(self)

if self.get_config().saml2_enabled:
from synapse.rest.saml2 import SAML2Resource

resources["/_matrix/saml2"] = SAML2Resource(self)

if self.get_config().threepid_behaviour_email == ThreepidBehaviour.LOCAL:
from synapse.rest.synapse.client.password_reset import (
PasswordResetSubmitTokenResource,
Expand Down
49 changes: 48 additions & 1 deletion synapse/rest/synapse/client/__init__.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# -*- coding: utf-8 -*-
# Copyright 2020 The Matrix.org Foundation C.I.C.
# Copyright 2021 The Matrix.org Foundation C.I.C.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand All @@ -12,3 +12,50 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from typing import TYPE_CHECKING, Mapping

from twisted.web.resource import Resource

from synapse.rest.synapse.client.pick_idp import PickIdpResource
from synapse.rest.synapse.client.pick_username import pick_username_resource
from synapse.rest.synapse.client.sso_register import SsoRegisterResource

if TYPE_CHECKING:
from synapse.server import HomeServer


def build_synapse_client_resource_tree(hs: "HomeServer") -> Mapping[str, Resource]:
"""Builds a resource tree to include synapse-specific client resources

These are resources which should be loaded on all workers which expose a C-S API:
ie, the main process, and any generic workers so configured.

Returns:
map from path to Resource.
"""
resources = {
# SSO bits. These are always loaded, whether or not SSO login is actually
# enabled (they just won't work very well if it's not)
"/_synapse/client/pick_idp": PickIdpResource(hs),
"/_synapse/client/pick_username": pick_username_resource(hs),
"/_synapse/client/sso_register": SsoRegisterResource(hs),
}

# provider-specific SSO bits. Only load these if they are enabled, since they
# rely on optional dependencies.
if hs.config.oidc_enabled:
from synapse.rest.oidc import OIDCResource

resources["/_synapse/oidc"] = OIDCResource(hs)

if hs.config.saml2_enabled:
from synapse.rest.saml2 import SAML2Resource

# This is mounted under '/_matrix' for backwards-compatibility.
resources["/_matrix/saml2"] = SAML2Resource(hs)
clokep marked this conversation as resolved.
Show resolved Hide resolved

return resources


__all__ = ["build_synapse_client_resource_tree"]
40 changes: 20 additions & 20 deletions synapse/storage/databases/main/registration.py
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,26 @@ def f(txn):

return await self.db_pool.runInteraction("get_users_by_id_case_insensitive", f)

async def record_user_external_id(
Copy link
Member Author

Choose a reason for hiding this comment

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

this is moving from RegistrationStore to RegistrationWorkerStore. There's no caching on this table so I think it's safe to move.

self, auth_provider: str, external_id: str, user_id: str
) -> None:
"""Record a mapping from an external user id to a mxid

Args:
auth_provider: identifier for the remote auth provider
external_id: id on that system
user_id: complete mxid that it is mapped to
"""
await self.db_pool.simple_insert(
table="user_external_ids",
values={
"auth_provider": auth_provider,
"external_id": external_id,
"user_id": user_id,
},
desc="record_user_external_id",
)

async def get_user_by_external_id(
self, auth_provider: str, external_id: str
) -> Optional[str]:
Expand Down Expand Up @@ -1371,26 +1391,6 @@ def _register_user(

self._invalidate_cache_and_stream(txn, self.get_user_by_id, (user_id,))

async def record_user_external_id(
self, auth_provider: str, external_id: str, user_id: str
) -> None:
"""Record a mapping from an external user id to a mxid

Args:
auth_provider: identifier for the remote auth provider
external_id: id on that system
user_id: complete mxid that it is mapped to
"""
await self.db_pool.simple_insert(
table="user_external_ids",
values={
"auth_provider": auth_provider,
"external_id": external_id,
"user_id": user_id,
},
desc="record_user_external_id",
)

async def user_set_password_hash(
self, user_id: str, password_hash: Optional[str]
) -> None:
Expand Down
15 changes: 3 additions & 12 deletions tests/rest/client/v1/test_login.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,7 @@
from synapse.rest.client.v1 import login, logout
from synapse.rest.client.v2_alpha import devices, register
from synapse.rest.client.v2_alpha.account import WhoamiRestServlet
from synapse.rest.synapse.client.pick_idp import PickIdpResource
from synapse.rest.synapse.client.pick_username import pick_username_resource
from synapse.rest.synapse.client.sso_register import SsoRegisterResource
from synapse.rest.synapse.client import build_synapse_client_resource_tree
from synapse.types import create_requester

from tests import unittest
Expand Down Expand Up @@ -424,11 +422,8 @@ def default_config(self) -> Dict[str, Any]:
return config

def create_resource_dict(self) -> Dict[str, Resource]:
from synapse.rest.oidc import OIDCResource

d = super().create_resource_dict()
d["/_synapse/client/pick_idp"] = PickIdpResource(self.hs)
d["/_synapse/oidc"] = OIDCResource(self.hs)
d.update(build_synapse_client_resource_tree(self.hs))
return d

def test_get_login_flows(self):
Expand Down Expand Up @@ -1212,12 +1207,8 @@ def default_config(self):
return config

def create_resource_dict(self) -> Dict[str, Resource]:
from synapse.rest.oidc import OIDCResource

d = super().create_resource_dict()
d["/_synapse/client/pick_username"] = pick_username_resource(self.hs)
d["/_synapse/client/sso_register"] = SsoRegisterResource(self.hs)
d["/_synapse/oidc"] = OIDCResource(self.hs)
d.update(build_synapse_client_resource_tree(self.hs))
return d

def test_username_picker(self):
Expand Down
6 changes: 2 additions & 4 deletions tests/rest/client/v2_alpha/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from synapse.handlers.ui_auth.checkers import UserInteractiveAuthChecker
from synapse.rest.client.v1 import login
from synapse.rest.client.v2_alpha import auth, devices, register
from synapse.rest.oidc import OIDCResource
from synapse.rest.synapse.client import build_synapse_client_resource_tree
from synapse.types import JsonDict, UserID

from tests import unittest
Expand Down Expand Up @@ -173,9 +173,7 @@ def default_config(self):

def create_resource_dict(self):
resource_dict = super().create_resource_dict()
if HAS_OIDC:
# mount the OIDC resource at /_synapse/oidc
resource_dict["/_synapse/oidc"] = OIDCResource(self.hs)
resource_dict.update(build_synapse_client_resource_tree(self.hs))
return resource_dict

def prepare(self, reactor, clock, hs):
Expand Down