DEVEXP-201: Verification webhooks #26
Conversation
JPPortier
requested review from
Dovchik,
650elx,
krogers0607 and
asein-sinch
as code owners
JPPortier
changed the title
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned Manifest Files.github/workflows/samples-compilation.yaml
sample-app/pom.xml
|
|
Please ignore failures for samples |
JPPortier
force-pushed
the
DEVEXP-201-verification-webhooks
branch
from
ce29d56
to
781d129
Compare
…hey are store at SinchClient level and a SinchClient instance can handle a single one application (like for single one project id)
| * see <a href="https://developers.sinch.com/docs/verification/api-reference/authentication/callback-signed-request">https://developers.sinch.com/docs/verification/api-reference/authentication/callback-signed-request</a> | ||
| * @since 1.0 | ||
| */ | ||
| boolean checkAuthentication( |
There was a problem hiding this comment.
I'm wondering if it makes sense to have this method in this service, knowing that the voice domain will define the same
There was a problem hiding this comment.
Right.
When Voice will arise, it seems refactoring will have sense (something synch with what we have to do related to Application spreading across Voice/Verification)
| String[] newSplit = computedAuthorization.split(" "); | ||
| String computedHash = newSplit.length > 1 ? newSplit[1] : ""; | ||
|
|
||
| return computedHash.equals(authorizationHash); |
There was a problem hiding this comment.
This function may be refactored to just be about the comparison between the received against the computed value and put the implementation details about how the extract the data in a private function
|
|
||
| Map<String, String> headers = | ||
| Stream.of( | ||
| new AbstractMap.SimpleEntry<>("authorization", "application foo="), |
There was a problem hiding this comment.
Maybe you can test against a value that starts with the application key but with a bad signature only such as 789:foo= so that you are really checking the tampering of the payload
| @@ -42,7 +41,7 @@ public void resetToken() { | |||
|
|
|||
| @Override | |||
| public Collection<Pair<String, String>> getAuthorizationHeaders( | |||
| String method, String httpContentType, String path, String body) { | |||
| String timestamp, String method, String httpContentType, String path, String body) { | |||
There was a problem hiding this comment.
You may remove the toString() when setting the x-timestamp header at line https://github.com/sinch/sinch-sdk-java/pull/26/files#diff-feb9f88279fca9ebde2e5c47af9da62694ad1d41b00f3e6f5f3672d6b51e3fa2L63
sample-app/src/main/java/com/sinch/sample/webhooks/verification/VerificationController.java
Show resolved
Hide resolved
…tials-at-sinchclient-configuration-level DEVEXP-201: store application credentials at SinchClient configuration level
This PR is adding helpers form SDK for webhooks support to:
checkAuthentication: validate authentication has received from request. User do not have to write it own implementation related to hashcode computation/validationunserializeVerificationEvent&&serializeVerificationResponseserialize/de-serialize payloads for webhooks callbacks and provide to user ready-to-use classes instance. So do not have to implement it owns outside of SDKTo provide these helpers, we need to be able to handle both Authentication validation mechanism (
BasicandApplication) because this setting is managed by user from his dashboard.So even if we only use
Applicationauthentication when SDK is sending request; webhooks validation handle both of them on received requests.