Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding SHA-1 hash validation support. #1140

Merged
merged 8 commits into from
Aug 31, 2022
Merged

Adding SHA-1 hash validation support. #1140

merged 8 commits into from
Aug 31, 2022

Conversation

MewX
Copy link
Contributor

@MewX MewX commented Aug 12, 2022

The Photos upload API can take a SHA-1 hash in the HTTP header. If the hash does not match after the final payload finishes, the API server will return 400 error.

This feature is still experimental. It works only for single-payload upload.

@MewX MewX force-pushed the hash-support branch 4 times, most recently from e4212d8 to a8ba71d Compare August 13, 2022 00:59
MewX
MewX commented Aug 15, 2022
View reviewed changes
Copy link
Contributor Author

@MewX MewX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I've updated the commit. Mind having another look?

...ypes-common/src/main/java/org/datatransferproject/types/common/models/photos/PhotoModel.java Show resolved Hide resolved
.../src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosInterface.java Outdated Show resolved Hide resolved
.../src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosInterface.java Outdated Show resolved Hide resolved
@MewX MewX force-pushed the hash-support branch 3 times, most recently from e525742 to 6958930 Compare August 16, 2022 23:49
seehamrun
seehamrun previously approved these changes Aug 17, 2022
View reviewed changes
@MewX
Copy link
Contributor Author

MewX commented Aug 19, 2022

Added an empty check for the new sha1 field:

    if (sha1 != null && !sha1.isEmpty()) {

Most users seem to like passing empty string in json as well. The albumId in the PhotoModel is one of the cases. It accepts empty string as null album.

@MewX
Copy link
Contributor Author

MewX commented Aug 19, 2022

Added minal change to cast SHA-1 string to upper case so that base16 decoder can decode the SHA1 into bytes.

      headers.put("X-Goog-Hash", "sha1=" + Base64.getEncoder()
          .encodeToString(BaseEncoding.base16().decode(sha1.toUpperCase())));

otherwise we will receive this error if there's any lower case in the SHA-1 string:

Exception in thread "main" java.lang.IllegalArgumentException: com.google.common.io.BaseEncoding$DecodingException: Unrecognized character: a
	at com.google.common.io.BaseEncoding.decode(BaseEncoding.java:219)
	at org.datatransferproject.datatransfer.google.photos.GooglePhotosInterface.uploadPhotoContent(GooglePhotosInterface.java:182)
	at org.datatransferproject.datatransfer.google.photos.GooglePhotosImporter.importPhotoBatch(GooglePhotosImporter.java:226)
	at org.datatransferproject.datatransfer.google.photos.GooglePhotosImporter.importPhotos(GooglePhotosImporter.java:197)
	at org.datatransferproject.datatransfer.google.photos.GooglePhotosImporter.importItem(GooglePhotosImporter.java:138)

@MewX
Adding SHA-1 hash validation support.
d326db9 
The Photos upload API can take a SHA-1 hash in the HTTP header. If the
hash does not match after the final payload finishes, the API server
will return 400 error.

This feature is still experimental. It works only for single-payload
upload.
@MewX
Copy link
Contributor Author

MewX commented Aug 19, 2022 •
edited
Loading

Fixed the wrong POST request header builder. (Was using parameter builder instead of header to add the new header.)

seehamrun
seehamrun previously approved these changes Aug 22, 2022
View reviewed changes
MewX and others added 2 commits August 23, 2022 08:40
@MewX
Merge branch 'master' into hash-support
c372e59
@MewX
Fixing the error check for hash mismatch.
e784fa2 
Any hash mismatch will result in the batch import request failure with an UploadErrorException.

This matches the current behavior where any network issue will raise an IOExeption and fail the whole batch.
@jzacsh
Update extensions/data-transfer/portability-data-transfer-google/src/…
0efdfec 
…main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosInterface.java


actually `maybeRethrowFileMismatchError` (and consider moving somewhere common if this isn't specific to Photos APIs)
@jzacsh
Copy link
Collaborator

jzacsh commented Aug 26, 2022

wrt 0efdfec - whoa sorry about that! I didn't mean to commit onto your branch - I thought I was updating a comment in the Github review flow. Please ignore (feel free to force-push to your own branch as usual!)

chaaransen and others added 2 commits August 28, 2022 21:57
@chaaransen @xokker @MewX
Junit Migration: runner to extension migration (dtinit#1143)
55b0eb9 
* Migrated from Runners to Extensions (@RunWith -> @ExtendWith)
* Migrated @test annotations from Junit 4 -> 5
* Migrated @before & @after to @beforeeach & @AfterEach, @BeforeClass to @BeforeAll

Co-authored-by: Ernest Sadykov <[email protected]>
@MewX
Resolve comments in pull request dtinit#1140.
f481f0e
@MewX
Copy link
Contributor Author

MewX commented Aug 29, 2022

wrt 0efdfec - whoa sorry about that! I didn't mean to commit onto your branch - I thought I was updating a comment in the Github review flow. Please ignore (feel free to force-push to your own branch as usual!)

All good! I accepted your change and pushed forward. PTAL.

jzacsh
jzacsh requested changes Aug 29, 2022
View reviewed changes
Copy link
Collaborator

@jzacsh jzacsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks for making the changes; Nothing major in this pass.

...e/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosImporter.java Show resolved Hide resolved
...e/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosImporter.java Show resolved Hide resolved
.../src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosInterface.java Outdated Show resolved Hide resolved
jzacsh
jzacsh requested changes Aug 31, 2022
View reviewed changes
Copy link
Collaborator

@jzacsh jzacsh left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please hold off for a moment - I'll get you an alternative PR by end of the day

edit: nevermind

...e/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosImporter.java Outdated Show resolved Hide resolved
@jzacsh jzacsh merged commit 23fd34b into dtinit:master Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzacsh jzacsh jzacsh approved these changes

@seehamrun seehamrun seehamrun left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MewX @jzacsh @seehamrun @chaaransen