Improve OpenPGP keyring test coverage - #7947
Conversation
| data = self.packet() | ||
| # note: because these queries aren't ordered, the result may be nondetermininistic | ||
| for signature in self.openpgp_signatures.filter(**content_filter): | ||
| for signature in self.openpgp_signatures.filter(**content_filter).order_by("pk"): |
There was a problem hiding this comment.
Should we try to order by something different than PK? Problem is we don't really have access to much else.
| "permissions": [ | ||
| ( | ||
| "manage_roles_openpgpdistribution", | ||
| "Can manage roles on openpgp distributions", |
There was a problem hiding this comment.
So irritating that just fixing a typo requires a migration. I will probably try to see if we can fold this one into another one, should another one come along soon.
| ) | ||
| assert keys.count == 1 | ||
| key = keys.results[0] | ||
| assert len(key.fingerprint) == 64 |
There was a problem hiding this comment.
This condition is implicit in the regex on the next line.
Also this looks kind of repetitive maybe you could @pytest.mark.parametrize these upload tests.
There was a problem hiding this comment.
Already ahead of you
| result = verify(bytes=test_data, store=lambda key_ids: served_certs, signature=sig) | ||
| assert result.valid_sigs | ||
|
|
||
| def test_verify_signature_multiple_keys_in_keyring( |
There was a problem hiding this comment.
This could perhaps be consolidated with the previous one, but parameterize it is a bit trickier than the others.
|
@mdellweg Sorry for requesting review prematurely, I wanted to try to get some early feedback before EOD. But yes, the initial state was very rough, and now it's much more cleaned up. |
|
Also I expect some of these tests to fail, since PySequoia isn't quite PQC ready yet. edit: also I found a bug. Either way we have to wait for the next release. |
|
@mdellweg Do we need an access policy on the OpenPGPDistributionViewSet? Or is the "default" one fine? I'm not sure if the commented out |
426c99e to
34c797c
Compare
|
@mdellweg So, here's one issue. Some of the failures are caused because So we could either migrate it to a separate app, or change the implementation of |
- v4/v6 key uploads (RSA, Ed25519) - PQC keys (ML-DSA-65, ML-DSA-87) - content listing/filtering - serialization round-trips via distribution - signature verification using keys served from the keyring - key revocation - idempotent uploads - private key rejection Also fix three bugs: - Typo: distribution permission said "gem" instead of "openpgp" - content_handler_list_directory crashed when no repository version exists - represent() querysets were unordered, producing nondeterministic output Assisted-By: Claude Opus 4.6
A public key packet needs to be before any other packets
And store fingerprints uppercase consistently in the DB.
Add a viewset for repository versions. Assisted-By: Claude Opus 4.6
The `expired` property used short-circuit evaluation (`self.expiration_time and ...`) which returns None when expiration_time is None. The serializer declares `expired` as a non-nullable BooleanField, so None causes a pydantic ValidationError in the generated client bindings. Wrap the expression in bool() so it always returns False when expiration_time is unset. Assisted-By: Claude Opus 4.6
| # Multiple viewsets for the same model (e.g. RepositoryVersionViewSet | ||
| # and OpenPGPKeyringVersionViewSet both use RepositoryVersion). | ||
| # Prefer the base class — subclass viewsets exist for URL routing | ||
| # and custom access policies, not for model-to-viewset reverse lookups. |
There was a problem hiding this comment.
This is theoretically correct (I think), but it's a huge hack that Claude came up with, and I'd prefer to just move the openpgp types to a new app. There's also conflicts with implementing import/export for types in pulpcore.
If we were to actually move forwards with this I'd want to review this very very heavily. I have not really done so yet, I just want to see the impact it has on CI.
When an app registers multiple NamedModelViewSet subclasses for the same model (e.g. RepositoryVersionViewSet and OpenPGPKeyringVersionViewSet both use RepositoryVersion), the lookup previously skipped the model entirely, causing LookupError in the import/export system. Resolve the ambiguity by preferring the base class in the inheritance hierarchy. Subclass viewsets still participate in URL routing and provide their own access policies — they just aren't the canonical answer for model-to-viewset reverse lookups. Assisted-By: Claude Opus 4.6
The field exists on the model but was missing from the serializer's fields tuple, causing an AttributeError in the client bindings when tests accessed it. Assisted-By: Claude Opus 4.6
2 participants
Also fix three bugs:
Assisted-By: Claude Opus 4.6
📜 Checklist
See: Pull Request Walkthrough