Add ExpiryCaveat (#11122)

* warehouse, tests: add and test `ExpiryCaveat`

* warehouse, tests: refactor macaroon failure handling

Add tests for success/failure conditions in the top-level verifier.

* tests: lint and format

* macaroons/caveats: remove `_fail` API

This was a misdesign.

Co-authored-by: Ee Durbin <ewdurbin@gmail.com>

Author: woodruffw

tests/unit/macaroons/test_caveats.py

@@ -11,13 +11,16 @@
 # limitations under the License.
 
 import json
+import os
+import time
 
 import pretend
+import pymacaroons
 import pytest
 
 from pymacaroons.exceptions import MacaroonInvalidSignatureException
 
-from warehouse.macaroons.caveats import Caveat, InvalidMacaroonError, V1Caveat, Verifier
+from warehouse.macaroons.caveats import Caveat, ExpiryCaveat, V1Caveat, Verifier
 
 from ...common.db.packaging import ProjectFactory
 
@@ -28,10 +31,8 @@ def test_creation(self):
         caveat = Caveat(verifier)
 
         assert caveat.verifier is verifier
-        with pytest.raises(InvalidMacaroonError):
-            caveat.verify(pretend.stub())
-        with pytest.raises(InvalidMacaroonError):
-            caveat(pretend.stub())
+        assert caveat.verify(pretend.stub()) is False
+        assert caveat(pretend.stub()) is False
 
 
 class TestV1Caveat:
@@ -47,8 +48,7 @@ def test_verify_invalid_predicates(self, predicate, result):
         verifier = pretend.stub()
         caveat = V1Caveat(verifier)
 
-        with pytest.raises(InvalidMacaroonError):
-            caveat(predicate)
+        assert caveat(predicate) is False
 
     def test_verify_valid_predicate(self):
         verifier = pretend.stub()
@@ -62,17 +62,17 @@ def test_verify_project_invalid_context(self):
         caveat = V1Caveat(verifier)
 
         predicate = {"version": 1, "permissions": {"projects": ["notfoobar"]}}
-        with pytest.raises(InvalidMacaroonError):
-            caveat(json.dumps(predicate))
+
+        assert caveat(json.dumps(predicate)) is False
 
     def test_verify_project_invalid_project_name(self, db_request):
         project = ProjectFactory.create(name="foobar")
         verifier = pretend.stub(context=project)
         caveat = V1Caveat(verifier)
 
         predicate = {"version": 1, "permissions": {"projects": ["notfoobar"]}}
-        with pytest.raises(InvalidMacaroonError):
-            caveat(json.dumps(predicate))
+
+        assert caveat(json.dumps(predicate)) is False
 
     def test_verify_project_no_projects_object(self, db_request):
         project = ProjectFactory.create(name="foobar")
@@ -83,8 +83,8 @@ def test_verify_project_no_projects_object(self, db_request):
             "version": 1,
             "permissions": {"somethingthatisntprojects": ["blah"]},
         }
-        with pytest.raises(InvalidMacaroonError):
-            caveat(json.dumps(predicate))
+
+        assert caveat(json.dumps(predicate)) is False
 
     def test_verify_project(self, db_request):
         project = ProjectFactory.create(name="foobar")
@@ -95,6 +95,56 @@ def test_verify_project(self, db_request):
         assert caveat(json.dumps(predicate)) is True
 
 
+class TestExpiryCaveat:
+    @pytest.mark.parametrize(
+        "predicate",
+        [
+            # invalid JSON
+            "invalid json",
+            # missing nbf and exp
+            '{"missing": "values"}',
+            # nbf and exp present, but null
+            '{"nbf": null, "exp": null}',
+            # nbf and exp present, but empty
+            '{"nbf": "", "exp": ""}',
+            # valid JSON, but wrong type
+            "[]",
+        ],
+    )
+    def test_verify_invalid_predicates(self, predicate):
+        verifier = pretend.stub()
+        caveat = ExpiryCaveat(verifier)
+
+        assert caveat(predicate) is False
+
+    def test_verify_not_before(self):
+        verifier = pretend.stub()
+        caveat = ExpiryCaveat(verifier)
+
+        not_before = int(time.time()) + 60
+        expiry = not_before + 60
+        predicate = json.dumps({"exp": expiry, "nbf": not_before})
+        assert caveat(predicate) is False
+
+    def test_verify_already_expired(self):
+        verifier = pretend.stub()
+        caveat = ExpiryCaveat(verifier)
+
+        not_before = int(time.time()) - 10
+        expiry = not_before - 5
+        predicate = json.dumps({"exp": expiry, "nbf": not_before})
+        assert caveat(predicate) is False
+
+    def test_verify_ok(self):
+        verifier = pretend.stub()
+        caveat = ExpiryCaveat(verifier)
+
+        not_before = int(time.time()) - 10
+        expiry = int(time.time()) + 60
+        predicate = json.dumps({"exp": expiry, "nbf": not_before})
+        assert caveat(predicate)
+
+
 class TestVerifier:
     def test_creation(self):
         macaroon = pretend.stub()
@@ -108,7 +158,7 @@ def test_creation(self):
         assert verifier.principals is principals
         assert verifier.permission is permission
 
-    def test_verify(self, monkeypatch):
+    def test_verify_invalid_signature(self, monkeypatch):
         verify = pretend.call_recorder(
             pretend.raiser(MacaroonInvalidSignatureException)
         )
@@ -120,6 +170,53 @@ def test_verify(self, monkeypatch):
         verifier = Verifier(macaroon, context, principals, permission)
 
         monkeypatch.setattr(verifier.verifier, "verify", verify)
-        with pytest.raises(InvalidMacaroonError):
-            verifier.verify(key)
+        assert verifier.verify(key) is False
         assert verify.calls == [pretend.call(macaroon, key)]
+
+    @pytest.mark.parametrize(
+        ["caveats", "valid"],
+        [
+            # Both V1 and expiry present and valid.
+            (
+                [
+                    {"permissions": "user", "version": 1},
+                    {"exp": int(time.time()) + 3600, "nbf": int(time.time()) - 1},
+                ],
+                True,
+            ),
+            # V1 only present and valid.
+            ([{"permissions": "user", "version": 1}], True),
+            # V1 and expiry present but V1 invalid.
+            ([{"permissions": "bad", "version": 1}], False),
+            # V1 and expiry present but expiry invalid.
+            (
+                [
+                    {"permissions": "user", "version": 1},
+                    {"exp": int(time.time()) + 1, "nbf": int(time.time()) + 3600},
+                ],
+                False,
+            ),
+        ],
+    )
+    def test_verify(self, monkeypatch, caveats, valid):
+        key = os.urandom(32)
+        m = pymacaroons.Macaroon(
+            location="fakelocation",
+            identifier="fakeid",
+            key=key,
+            version=pymacaroons.MACAROON_V2,
+        )
+
+        for caveat in caveats:
+            m.add_first_party_caveat(json.dumps(caveat))
+
+        # Round-trip through serialization to ensure we're not clinging to any state.
+        serialized_macaroon = m.serialize()
+        deserialized_macaroon = pymacaroons.Macaroon.deserialize(serialized_macaroon)
+
+        context = pretend.stub()
+        principals = pretend.stub()
+        permission = pretend.stub()
+
+        verifier = Verifier(deserialized_macaroon, context, principals, permission)
+        assert verifier.verify(key) is valid

warehouse/macaroons/caveats.py

@@ -11,6 +11,7 @@
 # limitations under the License.
 
 import json
+import time
 
 import pymacaroons
 
@@ -24,55 +25,86 @@ class InvalidMacaroonError(Exception):
 class Caveat:
     def __init__(self, verifier):
         self.verifier = verifier
+        # TODO: Surface this failure reason to the user.
+        # See: https://github.com/pypa/warehouse/issues/9018
+        self.failure_reason = None
 
-    def verify(self, predicate):
-        raise InvalidMacaroonError
+    def verify(self, predicate) -> bool:
+        return False
 
     def __call__(self, predicate):
         return self.verify(predicate)
 
 
 class V1Caveat(Caveat):
-    def verify_projects(self, projects):
+    def verify_projects(self, projects) -> bool:
         # First, ensure that we're actually operating in
         # the context of a package.
         if not isinstance(self.verifier.context, Project):
-            raise InvalidMacaroonError(
+            self.failure_reason = (
                 "project-scoped token used outside of a project context"
             )
+            return False
 
         project = self.verifier.context
         if project.normalized_name in projects:
             return True
 
-        raise InvalidMacaroonError(
+        self.failure_reason = (
             f"project-scoped token is not valid for project '{project.name}'"
         )
+        return False
 
-    def verify(self, predicate):
+    def verify(self, predicate) -> bool:
         try:
             data = json.loads(predicate)
         except ValueError:
-            raise InvalidMacaroonError("malformatted predicate")
+            self.failure_reason = "malformatted predicate"
+            return False
 
         if data.get("version") != 1:
-            raise InvalidMacaroonError("invalidate version in predicate")
+            self.failure_reason = "invalid version in predicate"
+            return False
 
         permissions = data.get("permissions")
         if permissions is None:
-            raise InvalidMacaroonError("invalid permissions in predicate")
+            self.failure_reason = "invalid permissions in predicate"
+            return False
 
         if permissions == "user":
             # User-scoped tokens behave exactly like a user's normal credentials.
             return True
 
         projects = permissions.get("projects")
         if projects is None:
-            raise InvalidMacaroonError("invalid projects in predicate")
+            self.failure_reason = "invalid projects in predicate"
+            return False
 
         return self.verify_projects(projects)
 
 
+class ExpiryCaveat(Caveat):
+    def verify(self, predicate):
+        try:
+            data = json.loads(predicate)
+            expiry = data["exp"]
+            not_before = data["nbf"]
+        except (KeyError, ValueError, TypeError):
+            self.failure_reason = "malformatted predicate"
+            return False
+
+        if not expiry or not not_before:
+            self.failure_reason = "missing fields"
+            return False
+
+        now = int(time.time())
+        if now < not_before or now >= expiry:
+            self.failure_reason = "token is expired"
+            return False
+
+        return True
+
+
 class Verifier:
     def __init__(self, macaroon, context, principals, permission):
         self.macaroon = macaroon
@@ -83,11 +115,12 @@ def __init__(self, macaroon, context, principals, permission):
 
     def verify(self, key):
         self.verifier.satisfy_general(V1Caveat(self))
+        self.verifier.satisfy_general(ExpiryCaveat(self))
 
         try:
             return self.verifier.verify(self.macaroon, key)
         except (
             pymacaroons.exceptions.MacaroonInvalidSignatureException,
             Exception,  # https://github.com/ecordell/pymacaroons/issues/50
         ):
-            raise InvalidMacaroonError("invalid macaroon signature")
+            return False