Use pypitoken to generate, check, introspect tokens #9264
Conversation
| def test_find_userid_no_macaroon(self, macaroon_service): | ||
| assert macaroon_service.find_userid(None) is None |
There was a problem hiding this comment.
The MacaroonService.find_userid method was never called with None. I'm not sure it makes sense to leave this codepath.
| def test_deserialize_raw_macaroon_when_none(self, macaroon_service): | ||
| raw_macaroon = pretend.stub() | ||
| macaroon_service._extract_raw_macaroon = pretend.call_recorder(lambda a: None) | ||
|
|
||
| with pytest.raises(services.InvalidMacaroon): | ||
| macaroon_service._deserialize_raw_macaroon(raw_macaroon) | ||
|
|
||
| assert macaroon_service._extract_raw_macaroon.calls == [ | ||
| pretend.call(raw_macaroon), | ||
| ] |
There was a problem hiding this comment.
_extract_raw_macaroon doesn't exist anymore, and the load function will raise when no token is extracted, and test_deserialize_raw_macaroon tests that.
| @pytest.mark.parametrize( | ||
| "exception", | ||
| [ | ||
| IndexError, | ||
| TypeError, | ||
| UnicodeDecodeError, | ||
| ValueError, | ||
| binascii.Error, | ||
| struct.error, | ||
| MacaroonDeserializationException, | ||
| Exception, # https://github.com/ecordell/pymacaroons/issues/50 | ||
| ], | ||
| ) | ||
| def test_deserialize_raw_macaroon(self, monkeypatch, macaroon_service, exception): | ||
| raw_macaroon = pretend.stub() | ||
| macaroon_service._extract_raw_macaroon = pretend.call_recorder( | ||
| lambda a: raw_macaroon | ||
| ) | ||
| monkeypatch.setattr( | ||
| pymacaroons.Macaroon, "deserialize", pretend.raiser(exception) | ||
| ) |
There was a problem hiding this comment.
pypitoken has a test for various types of errors https://github.com/ewjoachim/pypitoken/blob/master/tests/test_token.py#L325-L344 and a catchall because of ecordell/pymacaroons#50 : https://github.com/ewjoachim/pypitoken/blob/master/pypitoken/token.py#L328-L332
| @@ -431,6 +431,7 @@ def test_validate_token_scope_valid_user(self): | |||
| ) | |||
|
|
|||
| assert form.validate() | |||
| assert form.validated_restrictions == {} | |||
There was a problem hiding this comment.
This wasn't tested, I added a test, it's free :)
| identifier = uuid.uuid4() | ||
| key = _generate_key() |
There was a problem hiding this comment.
The previous flow was
- we generate the caveats object
- we create the database macaroon, letting the DB choose the identifier and have the key computed by a python function set as model default
- we read the id and key from the object after the DB flush
- we generate the pymacaroons object from those
Because we want pypitoken to generate the caveats object and pypitoken needs the key and id, the new flow is:
- we generate a random key and id
- we generate the pypitoken object
- we extract the caveats
- we create the database macaroon with the already computer key, id and caveats
There was a problem hiding this comment.
The comment above the model field should be updated (it discusses having the default factory in database vs python).
warehouse/macaroons/services.py
Outdated
| raise InvalidMacaroon("malformed macaroon") | ||
| return pypitoken.Token.load(raw_macaroon) | ||
| except pypitoken.LoaderError as exc: | ||
| raise InvalidMacaroon(str(exc)) |
There was a problem hiding this comment.
It's one of the promises of the pypitoken API that the exception message is displayable to the user (same when calling check())
|
PR updated following the merge of the token scanning feature :) |
ewjoachim
force-pushed
the
pypitoken
branch
2 times, most recently
from
06597ac to
18cc3f3
Compare
| @@ -496,7 +496,7 @@ <h3 id="apitoken">{{ apitoken() }}</h3> | |||
| Where you edit or add these values will depend on your individual use case. For example, some users may need to edit <a href="{{ pypirc_href }}" title="{{ title }}" target="_blank" rel="noopener">their <code>.pypirc</code> file</a>, while others may need to update their CI configuration file (e.g. <a href="{{ travis_href }}" title="{{ title }}" target="_blank" rel="noopener"><code>.travis.yml</code> if you are using Travis</a>). | |||
| {% endtrans %} | |||
| </p> | |||
| <p>{% trans %}Advanced users may wish to inspect their token by decoding it with base64, and checking the output against the unique identifier displayed on PyPI.{% endtrans %}</p> | |||
| <p>{% trans trimmed pypitoken_href='https://pypitoken.readthedocs.io/', title=gettext('External link') %}Advanced users may wish to inspect their token by decoding it with <a href="{{ pypitoken_href }}" title="{{ title }}" target="_blank" rel="noopener">the <code>pypitoken</code> library</a>, and checking the identifier against the unique identifier displayed on PyPI. Please make sure to keep your token identifier private, though. Knowing just the identifier of a token is not enough to impersonate the user, but it's sufficient to have a token be disabled.{% endtrans %}</p> | |||
There was a problem hiding this comment.
Added a little bit more info than strictly required but I thought it was interesting :)
ewjoachim
force-pushed
the
pypitoken
branch
2 times, most recently
from
2b1ceca to
9459275
Compare
|
Rebased but blocked by #10230 |
ewjoachim
force-pushed
the
pypitoken
branch
3 times, most recently
from
735423f to
9ee2782
Compare
Fixes #9184
Fixes #9018
Ah, a PR where we remove code without removing features, I love those :D
So, normally after this PR, the warehouse codebase won't know about the format of the caveats anymore, and we should be able to add new restriction types with much more ease :)