From 92a2f05379dc817a8d69d5bdc27b0a5ef0370a3e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 4 Mar 2021 11:29:27 +0000 Subject: [PATCH] Remove support for default values in macaroons --- synapse/handlers/auth.py | 2 +- synapse/handlers/oidc_handler.py | 23 ++++++----------- synapse/util/macaroons.py | 43 +++++++------------------------- tests/handlers/test_oidc.py | 4 +-- 4 files changed, 19 insertions(+), 53 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index e7061da7d598..15a6badb4bfe 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -1614,7 +1614,7 @@ def verify_short_term_login_token(self, token: str) -> LoginTokenAttributes: """ macaroon = pymacaroons.Macaroon.deserialize(token) user_id = get_value_from_macaroon(macaroon, "user_id") - auth_provider_id = get_value_from_macaroon(macaroon, "auth_provider_id", None) + auth_provider_id = get_value_from_macaroon(macaroon, "auth_provider_id") v = pymacaroons.Verifier() v.satisfy_exact("gen = 1") diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 85e502a8d991..b4a74390cca3 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -746,7 +746,7 @@ async def handle_redirect_request( idp_id=self.idp_id, nonce=nonce, client_redirect_url=client_redirect_url.decode(), - ui_auth_session_id=ui_auth_session_id, + ui_auth_session_id=ui_auth_session_id or "", ), ) @@ -1021,10 +1021,9 @@ def generate_oidc_session_token( macaroon.add_first_party_caveat( "client_redirect_url = %s" % (session_data.client_redirect_url,) ) - if session_data.ui_auth_session_id: - macaroon.add_first_party_caveat( - "ui_auth_session_id = %s" % (session_data.ui_auth_session_id,) - ) + macaroon.add_first_party_caveat( + "ui_auth_session_id = %s" % (session_data.ui_auth_session_id,) + ) now = self._clock.time_msec() expiry = now + duration_in_ms macaroon.add_first_party_caveat("time < %d" % (expiry,)) @@ -1058,8 +1057,6 @@ def verify_oidc_session_token( v.satisfy_general(lambda c: c.startswith("nonce = ")) v.satisfy_general(lambda c: c.startswith("idp_id = ")) v.satisfy_general(lambda c: c.startswith("client_redirect_url = ")) - # Sometimes there's a UI auth session ID, it seems to be OK to attempt - # to always satisfy this. v.satisfy_general(lambda c: c.startswith("ui_auth_session_id = ")) satisfy_expiry(v, self._clock.time_msec) @@ -1069,13 +1066,7 @@ def verify_oidc_session_token( nonce = get_value_from_macaroon(macaroon, "nonce") idp_id = get_value_from_macaroon(macaroon, "idp_id") client_redirect_url = get_value_from_macaroon(macaroon, "client_redirect_url") - try: - ui_auth_session_id = get_value_from_macaroon( - macaroon, "ui_auth_session_id" - ) # type: Optional[str] - except KeyError: - ui_auth_session_id = None - + ui_auth_session_id = get_value_from_macaroon(macaroon, "ui_auth_session_id") return OidcSessionData( nonce=nonce, idp_id=idp_id, @@ -1097,8 +1088,8 @@ class OidcSessionData: # The URL the client gave when it initiated the flow. ("" if this is a UI Auth) client_redirect_url = attr.ib(type=str) - # The session ID of the ongoing UI Auth (None if this is a login) - ui_auth_session_id = attr.ib(type=Optional[str], default=None) + # The session ID of the ongoing UI Auth ("" if this is a login) + ui_auth_session_id = attr.ib(type=str) UserAttributeDict = TypedDict( diff --git a/synapse/util/macaroons.py b/synapse/util/macaroons.py index 235130fe89b4..12cdd53327cd 100644 --- a/synapse/util/macaroons.py +++ b/synapse/util/macaroons.py @@ -16,40 +16,13 @@ """Utilities for manipulating macaroons""" -import enum -from typing import Callable, Optional, TypeVar, Union, overload +from typing import Callable, Optional import pymacaroons from pymacaroons.exceptions import MacaroonVerificationFailedException -_TV = TypeVar("_TV") - -class _Sentinel(enum.Enum): - # defining a sentinel in this way allows mypy to correctly handle the typing - sentinel = object() - - -_SENTINEL = object() - - -@overload def get_value_from_macaroon(macaroon: pymacaroons.Macaroon, key: str) -> str: - ... - - -@overload -def get_value_from_macaroon( - macaroon: pymacaroons.Macaroon, key: str, default: _TV -) -> Union[str, _TV]: - ... - - -def get_value_from_macaroon( - macaroon: pymacaroons.Macaroon, - key: str, - default=_SENTINEL, -): """Extracts a caveat value from a macaroon token. Checks that there is exactly one caveat of the form "key = " in the macaroon, @@ -60,12 +33,11 @@ def get_value_from_macaroon( key: the key of the caveat to extract Returns: - The extracted value, or `default` + The extracted value Raises: - KeyError: if `default` was not given, and the caveat was not in the macaroon MacaroonVerificationFailedException: if there are conflicting values for the - caveat in the macaroon + caveat in the macaroon, or if the caveat was not found in the macaroon. """ prefix = key + " = " result = None # type: Optional[str] @@ -86,9 +58,12 @@ def get_value_from_macaroon( if result is not None: return result - if default is _SENTINEL: - raise KeyError("No %s caveat in macaroon" % (key,)) - return default + + # If the caveat is not there, we raise a MacaroonVerificationFailedException. + # Note that it is insecure to generate a macaroon without all the caveats you + # might need (because there is nothing stopping people from adding extra caveats), + # so if the caveat isn't there, something odd must be going on. + raise MacaroonVerificationFailedException("No %s caveat in macaroon" % (key,)) def satisfy_expiry(v: pymacaroons.Verifier, get_time_ms: Callable[[], int]) -> None: diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 1ad0c46eb235..02d4b2de0d24 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -13,7 +13,6 @@ # See the License for the specific language governing permissions and # limitations under the License. import json -from typing import Optional from urllib.parse import parse_qs, urlparse from mock import ANY, Mock, patch @@ -862,7 +861,7 @@ def _generate_oidc_session_token( state: str, nonce: str, client_redirect_url: str, - ui_auth_session_id: Optional[str] = None, + ui_auth_session_id: str = "", ) -> str: from synapse.handlers.oidc_handler import OidcSessionData @@ -905,6 +904,7 @@ async def _make_callback_with_userinfo( idp_id="oidc", nonce="nonce", client_redirect_url=client_redirect_url, + ui_auth_session_id="", ), ) request = _build_callback_request("code", state, session)