Skip to content

Conversation

prandla
Copy link
Member

@prandla prandla commented Aug 25, 2025

This is 3 different changes (one in each commit), each building on top of the previous one. I can submit them as separate PRs if you'd prefer that.

  1. Added an opaque_id column to user tests, which acts just like the opaque_id of submissions. I think this is good for consistency with submissions in its own right, regardless of the other agenda i'm pushing in this PR :)
  2. Use the opaque_ids in the functional test suite, to map CWS-visible identifiers to AWS pages. Previously this was done by encrypting the real submission ID with the webserver secret key. Instead, I made the relevant AWS handler additionally accept an opaque ID in place of the submission ID.
  3. Point 2 was the last (or possibly only?) user of encrypt_number / decrypt_number. These functions were themselves the only users of pycryptodome. So I deleted the functions and dropped the cryptodome dependency. Also, the encrypt/decrypt functions were basically the only users of cmscommon.binary. That module itself is trivial to implement with python builtin functions, so I replaced the remaining 3 uses of it with builtins and deleted it too.

One issue I realized just now is that the opaque_ids aren't guaranteed to be globally unique, so technically it's possible for the test suite to fail if it's ran with a pre-existing DB (this will never happen in docker, and well, statistically it's quite unlikely to ever happen to anyone in development either). I guess I could make the AWS endpoint also take a participation ID in the URL and thread the participation ID through the test runner to pass it in the right place, but it seems annoying...

Another alternative is to make opaque_ids a bit longer and globally unique.

@veluca93 veluca93 requested a review from gollux August 25, 2025 13:14
@prandla prandla force-pushed the cleanup-encryption-code branch from f1ca285 to 0d04853 Compare August 27, 2025 03:15
@prandla
Copy link
Member Author

prandla commented Aug 27, 2025

Another alternative is to make opaque_ids a bit longer and globally unique.

Does anyone have any objections to this? I think having a globally unique opaque ID might come in handy in the future as well.

@wil93
Copy link
Member

wil93 commented Aug 29, 2025

Another alternative is to make opaque_ids a bit longer and globally unique.

Does anyone have any objections to this? I think having a globally unique opaque ID might come in handy in the future as well.

If we go in that direction, I think it would be wise to use UUIDs. They are natively supported by Postgres and I feel like it would be safer to use that rather than reinventing our own solution (which we already kinda did in #1419)

@prandla prandla force-pushed the cleanup-encryption-code branch from 0d04853 to 116ed60 Compare August 31, 2025 08:18
@prandla
Copy link
Member Author

prandla commented Aug 31, 2025

I think I would be in favor of just using UUIDs. The arguments made by @gollux in #1419 don't sound very convincing to me, I don't think the submissions table is ever going to grow big enough for index size/performance to be a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants