Add checksum field to location in database and GraphQL#747
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c16cee0fb3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| publication_id, | ||
| work.landing_page.clone(), | ||
| cdn_url, | ||
| Some(cdn_checksum.to_string()), | ||
| )?; |
There was a problem hiding this comment.
This upload path now forwards cdn_checksum into upsert_thoth_location, which persists location.checksum via Location::create/update without calling LocationPolicy::can_create or can_update. In practice, complete_file_upload is authorized by file/CDN permissions (not superuser-only), so a non-superuser who can complete a publication upload can still add or change a checksum, bypassing the superuser restriction introduced in LocationPolicy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Behaviour is desired: checksums should not be manually entered/changed by non-superuser, but automated file uploads should be able to set them for Thoth locations
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f383df401
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| // Only superusers can update or delete an existing checksum. | ||
| if current.checksum.is_some() && current.checksum != patch.checksum && !user.is_superuser() |
There was a problem hiding this comment.
The new checksum permission gate only compares patch.checksum to current.checksum, so a non-superuser can still change patch.checksum_algorithm (for example MD5 → SHA256) while keeping the checksum string unchanged, and can_update will allow it. This bypasses the intended restriction that checksum metadata is superuser-controlled and can leave checksum data semantically incorrect for downstream consumers that trust the algorithm field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Oh yes, I had been thinking that would be covered by the database constraints, but I forgot about that case.
| @@ -0,0 +1,9 @@ | |||
| CREATE TYPE public.checksum_algorithm AS ENUM ( | |||
| 'MD5', | |||
| 'SHA256' | |||
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5b003a6a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ((current.checksum.is_some() && current.checksum != patch.checksum) | ||
| || (current.checksum_algorithm.is_some() | ||
| && current.checksum_algorithm != patch.checksum_algorithm)) |
There was a problem hiding this comment.
PatchLocation uses nullable fields with treat_none_as_null, so clients that do not send the newly added checksum/checksum_algorithm fields produce None in patch even when they are not trying to edit checksum metadata. This condition then interprets omitted fields as a checksum change and rejects non-superuser updates (or clears checksum for superusers), which breaks ordinary update_location calls that only edit unrelated fields on records that already have a checksum.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Behaviour is desired: the patch will overwrite whatever is in the existing record, so we don't want to let any client submit None for these fields if they have already been set.
2 participants
No description provided.