feat(appcheck): Add replay protection feature #641
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
weixifan
left a comment
weixifan
left a comment
There was a problem hiding this comment.
Thank you for this contribution! I have left comments addressing App Check domain-specific concerns, but I will leave it to other reviewers to address Go language style and library use concerns.
|
Hi @weixifan and @lahirumaramba Thank you for taking your time to review this implementation. Your guidance on this is much appreciated. I have update the implementation as per @weixifan recommendations. Please review and advise. Happy coding |
|
Hi @weixifan. I hope all is well. I have refactor token verifier url generation as recommended. I agree, It looks cleaner than before. Please review Thanks |
|
Hi @weixifan and @lahirumaramba. Any feedback on this PR? |
weixifan
left a comment
weixifan
left a comment
There was a problem hiding this comment.
Sorry about the delay. This is approved from App Check's point of view. A Golang reviewer will review your PR for style and consistency with existing code.
Thank you for your contribution!
|
Hi – This is a really important feature for securing backends, and I was wondering if there’s any estimated timeline for when this might be reviewed and merged? |
|
Hello, Any updates on this? Thanks |
lahirumaramba
changed the title
lahirumaramba
left a comment
lahirumaramba
left a comment
There was a problem hiding this comment.
Thank you for you contribution and thank you so much for your patience on this. We have prioritized this feature. I added a few comments. Let me know if you have any questions.
|
Hello @lahirumaramba. Thank you very much for your feedback. I have added another commit with include changes from your comments. I had to also update Thanks |
|
I have updated as per the above feedback. Thank you for your assistance |
|
@gemini-code-assist review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces replay protection for App Check tokens by adding a VerifyOneTimeToken method. The implementation correctly reuses the existing VerifyToken logic for initial validation and then makes an API call to a new endpoint to consume the token. The changes are well-structured, and the necessary modifications to propagate client options for testing purposes are included. The new functionality is covered by a table-driven test, which is great. I've suggested an improvement to the test to cover more failure scenarios.
| appCheckVerifyTestsTable := []struct { | ||
| label string | ||
| mockServerResponse string | ||
| expectedError error | ||
| }{ | ||
| {label: "testWhenAlreadyConsumedResponseIsTrue", mockServerResponse: `{"alreadyConsumed": true}`, expectedError: ErrTokenAlreadyConsumed}, | ||
| {label: "testWhenAlreadyConsumedResponseIsFalse", mockServerResponse: `{"alreadyConsumed": false}`, expectedError: nil}, | ||
| } |
There was a problem hiding this comment.
The test cases for TestVerifyOneTimeToken cover the successful API calls well. To make the tests more comprehensive, it would be beneficial to add cases for API call failures, such as network errors or non-200 HTTP status codes from the backend. This will ensure that errors from the underlying httpClient.DoAndUnmarshal are correctly propagated and handled.
For example, you could add cases to your test table for:
- A network error during the API call (by setting
ErronmockHTTPResponse). - An unsuccessful HTTP status code like 500 from the backend (by setting
StatusCodeonmockHTTPResponse).
|
Hi @lahirumaramba and team. Kindly advise on the failed test stage. Thanks |
4 participants
Hello Team.
Kindly assist to review this initial implementation of App Check Replay Protection API as requested in issue #632
Your feedback and guidance will be highly appreciated.