Conversation
nicolaskempf57
left a comment
There was a problem hiding this comment.
Thanks for this PR ! I think we are ready on udata side, but it will require some changes in cdata and maybe other projects like front-kit if they also have an interface for the api key
Done in datagouv/cdata#962 |
maudetes
left a comment
There was a problem hiding this comment.
🎉 I think it will be a very nice improvement for security and usability !
|
|
||
| Tokens are stored as HMAC-SHA256 hashes in a dedicated `api_token` MongoDB collection. Revoked tokens are kept for audit (soft-delete via `revoked_at`). | ||
|
|
||
| ## Previous system |
There was a problem hiding this comment.
I wouldn't document past system in this documentation?
| from udata.errors import ConfigError | ||
| from udata.models import datastore | ||
|
|
||
| if not app.config.get("API_TOKEN_SECRET"): |
There was a problem hiding this comment.
Agreed it may be better to raise instead of leaving it by default. However, we should do the same for a list of settings for coherence?
for config in ["SECRET_KEY", "API_TOKEN_SECRET"]:
...
And I would add a API_TOKEN_SECRET in the different udata.cfg samples in docs.
| @me.route("/tokens/", endpoint="my_tokens") | ||
| class TokenListAPI(API): |
There was a problem hiding this comment.
Can we use the wording api key instead of vague token?
In my understanding, other potential token usage would have distinct routes anyway?
| @api.marshal_list_with(ApiToken.__read_fields__) | ||
| def get(self): | ||
| """List all my active API tokens""" | ||
| return list(ApiToken.objects(user=current_user._get_current_object(), revoked_at=None)) |
There was a problem hiding this comment.
Could we do
| return list(ApiToken.objects(user=current_user._get_current_object(), revoked_at=None)) | |
| return list(ApiToken.objects(user=current_user.id, revoked_at=None)) |
| @api.marshal_list_with(ApiToken.__read_fields__) | ||
| def get(self): | ||
| """List all my active API tokens""" | ||
| return list(ApiToken.objects(user=current_user._get_current_object(), revoked_at=None)) |
There was a problem hiding this comment.
Should it return expired tokens but not revoked?
| self.save() | ||
| from udata.core.user.api_tokens import ApiToken | ||
|
|
||
| ApiToken.objects(user=self, revoked_at=None).update( |
There was a problem hiding this comment.
I think we have a reverse rule on user = db.ReferenceField("User", required=True, reverse_delete_rule=db.CASCADE), thus the token will be deleted anyway?
| if not login_user(user, False): | ||
| if not login_user(api_token.user, False): | ||
| self.abort(401, "Inactive user") | ||
| api_token.update_usage(request.headers.get("User-Agent")) |
There was a problem hiding this comment.
Don't we want to update usage in authenticate directly?
| def test_header_auth(self): | ||
| """Should handle header API Key authentication""" | ||
| with self.api_user() as user: # API Key auth | ||
| response = self.post(url_for("api.fake"), headers={"X-API-KEY": user.apikey}) | ||
| with self.api_user(): | ||
| response = self.post(url_for("api.fake")) |
There was a problem hiding this comment.
I'm confused, it doesn't test anything anymore?
| token = cls.objects(token_hash=token_hash, revoked_at=None).first() | ||
| if token is None: | ||
| return None | ||
| if token.expires_at and token.expires_at < datetime.now(timezone.utc): |
There was a problem hiding this comment.
I think we have an issue of comparison between timezone naive and aware datetimes here?
I think we're missing a test for this.
|
|
||
| @classmethod | ||
| def authenticate(cls, plaintext_token): | ||
| """Lookup a token by hashing the plaintext. Returns ApiToken or None.""" |
There was a problem hiding this comment.
Don't we want to return appropriate error (revoked vs expired vs invalid?
Uh oh!
There was an error while loading. Please reload this page.