-
Notifications
You must be signed in to change notification settings - Fork 13
Add checksum field to location in database and GraphQL #747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c16cee0
77e26f6
b5905b1
2306d97
c7b67a8
6f383df
a0afe52
e5b003a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| ALTER TABLE public.location | ||
| DROP CONSTRAINT IF EXISTS location_checksum_and_algorithm_all_or_none, | ||
| DROP COLUMN IF EXISTS checksum, | ||
| DROP COLUMN IF EXISTS checksum_algorithm; | ||
|
|
||
| DROP TYPE IF EXISTS public.checksum_algorithm; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| CREATE TYPE public.checksum_algorithm AS ENUM ( | ||
| 'MD5', | ||
| 'SHA256', | ||
| 'SHA1' | ||
| ); | ||
|
|
||
| ALTER TABLE public.location | ||
| ADD COLUMN checksum TEXT, | ||
| ADD COLUMN checksum_algorithm public.checksum_algorithm, | ||
| ADD CONSTRAINT location_checksum_and_algorithm_all_or_none CHECK ((checksum IS NULL AND checksum_algorithm IS NULL) OR (checksum IS NOT NULL AND checksum_algorithm IS NOT NULL)); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,11 @@ impl CreatePolicy<NewLocation> for LocationPolicy { | |
| return Err(ThothError::ThothLocationError); | ||
| } | ||
|
|
||
| // Only superusers can add a checksum. | ||
| if !user.is_superuser() && data.checksum.is_some() { | ||
| return Err(ThothError::CreateLocationChecksumError); | ||
| } | ||
|
|
||
| // Canonical locations must be complete; non-canonical locations must satisfy rules. | ||
| if data.canonical { | ||
| data.canonical_record_complete(ctx.db())?; | ||
|
|
@@ -74,6 +79,20 @@ impl UpdatePolicy<Location, PatchLocation> for LocationPolicy { | |
| return Err(ThothError::ThothUpdateCanonicalError); | ||
| } | ||
|
|
||
| // Only superusers can add a checksum. | ||
| if current.checksum.is_none() && patch.checksum.is_some() && !user.is_superuser() { | ||
| return Err(ThothError::UpdateLocationChecksumError); | ||
| } | ||
|
|
||
| // Only superusers can update or delete an existing checksum. | ||
| if ((current.checksum.is_some() && current.checksum != patch.checksum) | ||
| || (current.checksum_algorithm.is_some() | ||
| && current.checksum_algorithm != patch.checksum_algorithm)) | ||
|
Comment on lines
+88
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Behaviour is desired: the patch will overwrite whatever is in the existing record, so we don't want to let any client submit |
||
| && !user.is_superuser() | ||
| { | ||
| return Err(ThothError::UpdateLocationChecksumError); | ||
| } | ||
|
|
||
| // If setting canonical to true, require record completeness. | ||
| if patch.canonical { | ||
| patch.canonical_record_complete(ctx.db())?; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This upload path now forwards
cdn_checksumintoupsert_thoth_location, which persistslocation.checksumviaLocation::create/updatewithout callingLocationPolicy::can_createorcan_update. In practice,complete_file_uploadis 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 inLocationPolicy.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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