Remove support for default values in macaroons

Author: richvdh

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")

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(

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 = <val>" 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:

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)