Skip to content

Commit

Permalink
Do not allow a deactivated user to login via SSO. (matrix-org#7240)
Browse files Browse the repository at this point in the history
  • Loading branch information
clokep authored and phil-flex committed Jun 16, 2020
1 parent 0d35d0b commit aa275ad
Show file tree
Hide file tree
Showing 8 changed files with 110 additions and 10 deletions.
1 change: 1 addition & 0 deletions changelog.d/7240.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Do not allow a deactivated user to login via SSO.
7 changes: 7 additions & 0 deletions synapse/config/sso.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# 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.
import os
from typing import Any, Dict

import pkg_resources
Expand All @@ -36,6 +37,12 @@ def read_config(self, config, **kwargs):
template_dir = pkg_resources.resource_filename("synapse", "res/templates",)

self.sso_redirect_confirm_template_dir = template_dir
self.sso_account_deactivated_template = self.read_file(
os.path.join(
self.sso_redirect_confirm_template_dir, "sso_account_deactivated.html"
),
"sso_account_deactivated_template",
)

self.sso_client_whitelist = sso_config.get("client_whitelist") or []

Expand Down
34 changes: 30 additions & 4 deletions synapse/handlers/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,9 @@ def __init__(self, hs):
self._sso_auth_confirm_template = load_jinja2_templates(
hs.config.sso_redirect_confirm_template_dir, ["sso_auth_confirm.html"],
)[0]
self._sso_account_deactivated_template = (
hs.config.sso_account_deactivated_template
)

self._server_name = hs.config.server_name

Expand Down Expand Up @@ -644,9 +647,6 @@ def check_user_exists(self, user_id: str):
Returns:
defer.Deferred: (unicode) canonical_user_id, or None if zero or
multiple matches
Raises:
UserDeactivatedError if a user is found but is deactivated.
"""
res = yield self._find_user_id_and_pwd_hash(user_id)
if res is not None:
Expand Down Expand Up @@ -1099,7 +1099,7 @@ def complete_sso_ui_auth(
request.write(html_bytes)
finish_request(request)

def complete_sso_login(
async def complete_sso_login(
self,
registered_user_id: str,
request: SynapseRequest,
Expand All @@ -1113,6 +1113,32 @@ def complete_sso_login(
client_redirect_url: The URL to which to redirect the user at the end of the
process.
"""
# If the account has been deactivated, do not proceed with the login
# flow.
deactivated = await self.store.get_user_deactivated_status(registered_user_id)
if deactivated:
html = self._sso_account_deactivated_template.encode("utf-8")

request.setResponseCode(403)
request.setHeader(b"Content-Type", b"text/html; charset=utf-8")
request.setHeader(b"Content-Length", b"%d" % (len(html),))
request.write(html)
finish_request(request)
return

self._complete_sso_login(registered_user_id, request, client_redirect_url)

def _complete_sso_login(
self,
registered_user_id: str,
request: SynapseRequest,
client_redirect_url: str,
):
"""
The synchronous portion of complete_sso_login.
This exists purely for backwards compatibility of synapse.module_api.ModuleApi.
"""
# Create a login token
login_token = self.macaroon_gen.generate_short_term_login_token(
registered_user_id
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/cas_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,6 @@ async def handle_ticket(
localpart=localpart, default_display_name=user_display_name
)

self._auth_handler.complete_sso_login(
await self._auth_handler.complete_sso_login(
registered_user_id, request, client_redirect_url
)
2 changes: 1 addition & 1 deletion synapse/handlers/saml_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ async def handle_saml_response(self, request):
)

else:
self._auth_handler.complete_sso_login(user_id, request, relay_state)
await self._auth_handler.complete_sso_login(user_id, request, relay_state)

async def _map_saml_response_to_user(
self, resp_bytes: str, client_redirect_url: str
Expand Down
22 changes: 21 additions & 1 deletion synapse/module_api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,13 +220,33 @@ def complete_sso_login(
want their access token sent to `client_redirect_url`, or redirect them to that
URL with a token directly if the URL matches with one of the whitelisted clients.
This is deprecated in favor of complete_sso_login_async.
Args:
registered_user_id: The MXID that has been registered as a previous step of
of this SSO login.
request: The request to respond to.
client_redirect_url: The URL to which to offer to redirect the user (or to
redirect them directly if whitelisted).
"""
self._auth_handler._complete_sso_login(
registered_user_id, request, client_redirect_url,
)

async def complete_sso_login_async(
self, registered_user_id: str, request: SynapseRequest, client_redirect_url: str
):
"""Complete a SSO login by redirecting the user to a page to confirm whether they
want their access token sent to `client_redirect_url`, or redirect them to that
URL with a token directly if the URL matches with one of the whitelisted clients.
Args:
registered_user_id: The MXID that has been registered as a previous step of
of this SSO login.
request: The request to respond to.
client_redirect_url: The URL to which to offer to redirect the user (or to
redirect them directly if whitelisted).
"""
self._auth_handler.complete_sso_login(
await self._auth_handler.complete_sso_login(
registered_user_id, request, client_redirect_url,
)
10 changes: 10 additions & 0 deletions synapse/res/templates/sso_account_deactivated.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>SSO account deactivated</title>
</head>
<body>
<p>This account has been deactivated.</p>
</body>
</html>
42 changes: 39 additions & 3 deletions tests/rest/client/v1/test_login.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ def _delete_device(self, access_token, user_id, password, device_id):
self.assertEquals(channel.code, 200, channel.result)


class CASRedirectConfirmTestCase(unittest.HomeserverTestCase):
class CASTestCase(unittest.HomeserverTestCase):

servlets = [
login.register_servlets,
Expand All @@ -274,6 +274,9 @@ def make_homeserver(self, reactor, clock):
"service_url": "https://matrix.goodserver.com:8448",
}

cas_user_id = "username"
self.user_id = "@%s:test" % cas_user_id

async def get_raw(uri, args):
"""Return an example response payload from a call to the `/proxyValidate`
endpoint of a CAS server, copied from
Expand All @@ -282,10 +285,11 @@ async def get_raw(uri, args):
This needs to be returned by an async function (as opposed to set as the
mock's return value) because the corresponding Synapse code awaits on it.
"""
return """
return (
"""
<cas:serviceResponse xmlns:cas='http://www.yale.edu/tp/cas'>
<cas:authenticationSuccess>
<cas:user>username</cas:user>
<cas:user>%s</cas:user>
<cas:proxyGrantingTicket>PGTIOU-84678-8a9d...</cas:proxyGrantingTicket>
<cas:proxies>
<cas:proxy>https://proxy2/pgtUrl</cas:proxy>
Expand All @@ -294,6 +298,8 @@ async def get_raw(uri, args):
</cas:authenticationSuccess>
</cas:serviceResponse>
"""
% cas_user_id
)

mocked_http_client = Mock(spec=["get_raw"])
mocked_http_client.get_raw.side_effect = get_raw
Expand All @@ -304,6 +310,9 @@ async def get_raw(uri, args):

return self.hs

def prepare(self, reactor, clock, hs):
self.deactivate_account_handler = hs.get_deactivate_account_handler()

def test_cas_redirect_confirm(self):
"""Tests that the SSO login flow serves a confirmation page before redirecting a
user to the redirect URL.
Expand Down Expand Up @@ -370,3 +379,30 @@ def _test_redirect(self, redirect_url):
self.assertEqual(channel.code, 302)
location_headers = channel.headers.getRawHeaders("Location")
self.assertEqual(location_headers[0][: len(redirect_url)], redirect_url)

@override_config({"sso": {"client_whitelist": ["https://legit-site.com/"]}})
def test_deactivated_user(self):
"""Logging in as a deactivated account should error."""
redirect_url = "https://legit-site.com/"

# First login (to create the user).
self._test_redirect(redirect_url)

# Deactivate the account.
self.get_success(
self.deactivate_account_handler.deactivate_account(self.user_id, False)
)

# Request the CAS ticket.
cas_ticket_url = (
"/_matrix/client/r0/login/cas/ticket?redirectUrl=%s&ticket=ticket"
% (urllib.parse.quote(redirect_url))
)

# Get Synapse to call the fake CAS and serve the template.
request, channel = self.make_request("GET", cas_ticket_url)
self.render(request)

# Because the user is deactivated they are served an error template.
self.assertEqual(channel.code, 403)
self.assertIn(b"SSO account deactivated", channel.result["body"])

0 comments on commit aa275ad

Please sign in to comment.