Add per-user Cargo token authentication - #40
Conversation
f23fca6 to
5b349bf
Compare
| model = models.RustCargoToken | ||
| fields = core_serializers.ModelSerializer.Meta.fields + ( | ||
| "name", | ||
| "token", |
There was a problem hiding this comment.
This isn't displaying the token values to any API user, right? Even the user that created it should probably not be able to see it again after initial creation.
It also doesn't map directly to any field name
There was a problem hiding this comment.
Yes, in this implementation there is no way to retrieve token value. It's shown just once, right after creation in the response. So this field exists only for this one response. Any reads on this field will return None.
There was a problem hiding this comment.
So you're saying that the token value is filled out on creation, but afterwards (list, detail, etc.) it shows as None?
Do you think it would be better to use two serializers so that the field is ommitted entirely in the post-creation lookup cases, rather than set null?
There was a problem hiding this comment.
Yeah I guess you are right, that makes more sense
| # Authentication disabled for now | ||
| authentication_classes = [] | ||
| permission_classes = [] | ||
| authentication_classes = [CargoTokenAuthentication] |
There was a problem hiding this comment.
Does this prevent users without tokens from downloading packages, yes? If so, I feel like that needs to at least be configurable behavior, normally there's no requirement to pass a token just to download packages from a registry, though the use case is permitted
There was a problem hiding this comment.
There is a get_permission() override in that class, that just returns [] for GET requests so unauthenticated users are still able to download crates.
| assert response.status_code == 403 | ||
| errors = response.json()["errors"] | ||
| assert any("authorization token" in e["detail"] for e in errors) | ||
| unyank_url = urljoin(base, "api/v1/crates/fake/0.0.1/unyank") |
There was a problem hiding this comment.
We should probably create a helper for the yank API, or else use the bindings
|
|
||
| response = cargo_api_request("GET", urljoin(base, "me")) | ||
| assert response.status_code == 403 | ||
| response = cargo_api_request("GET", urljoin(base, "me"), headers={"Authorization": token.token}) |
|
At minimum this section of the docs needs updating. But probably others do as well. https://github.com/pulp/pulp_rust/blob/main/docs/user/guides/private-registry.md#authentication edit: also https://github.com/pulp/pulp_rust/blob/main/docs/index.md#features And maybe authorization should have it's own separate document to be referenced in the other docs. I'm not sure, probably depends on how much detail we need. It should be clear what auth Pulp supports and how it differs from crates.io, e.g. no package ownership at the moment. |
ceede98 to
fce4315
Compare
|
Please make changes in temporary commits so that it's easier to review what changed, it can be squashed at the end. |
closes: pulp#24 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
||
| class Meta: | ||
| model = models.RustCargoToken | ||
| fields = core_serializers.ModelSerializer.Meta.fields + ("name", "last_used") |
There was a problem hiding this comment.
The name and last_used fields don't seem to be declared? I recall them being here, maybe they got lost in the shuffle?
There was a problem hiding this comment.
I actually do not recall them being there, it's possible that claude removed them while rolling back some of the changes. If they actually were there, it must have been for the help text
| ], | ||
| } | ||
|
|
||
| def get_serializer_class(self): |
There was a problem hiding this comment.
I think this wouldn't be necessary?
2 participants
Replaces the hardcoded stub token with per-user Cargo token authentication. Users create tokens via the REST API (
POST /api/v3/cargo/tokens/), then use them withcargo login/cargo publish/cargo yankas normal. Tokens are stored as SHA-256 hashes and scoped to the requesting user.closes: #24