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.

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

Trusted entitlements: Support signing static endpoints #1119

Conversation

tonidero
Copy link
Contributor

@tonidero tonidero commented Jul 3, 2023

Description

This adds support for static endpoint signing. Currently, that would be the offerings and product-entitlement mapping endpoints.

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Merging #1119 (6575c19) into new-trusted-entitlements-signature-format (77e8fb3) will increase coverage by 0.22%.
The diff coverage is 100.00%.

❗ Current head 6575c19 differs from pull request most recent head 784f8d9. Consider uploading reports for the commit 784f8d9 to get more accurate results

@@                              Coverage Diff                              @@
##           new-trusted-entitlements-signature-format    #1119      +/-   ##
=============================================================================
+ Coverage                                      84.91%   85.13%   +0.22%     
=============================================================================
  Files                                            186      186              
  Lines                                           6614     6586      -28     
  Branches                                         947      944       -3     
=============================================================================
- Hits                                            5616     5607       -9     
+ Misses                                           622      603      -19     
  Partials                                         376      376              
Impacted Files Coverage Δ
...java/com/revenuecat/purchases/common/HTTPClient.kt 90.07% <100.00%> (+0.07%) ⬆️
...revenuecat/purchases/common/networking/Endpoint.kt 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@tonidero tonidero force-pushed the toniricodiez/sdk-2990-add-signing-to-the-entitlementmapping-endpoint-android branch from 6575c19 to d29e3a5 Compare July 4, 2023 08:10
@tonidero tonidero marked this pull request as ready for review July 4, 2023 08:14
@tonidero tonidero requested a review from a team July 4, 2023 08:14
@tonidero
Copy link
Contributor Author

tonidero commented Jul 4, 2023

Backend integration tests are expected to fail until the backend changes have been deployed. I'm also pointing these changes to an integration branch until those changes are deployed.

false
}

val needsNonceToPerformSigning: Boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great 👍🏻

@@ -87,14 +96,12 @@ abstract class BaseBackendIntegrationTest {
every { edit() } returns sharedPreferencesEditor
}
eTagManager = ETagManager(sharedPreferences)
signingManager = SigningManager(SignatureVerificationMode.Disabled, appConfig, apiKey())
signingManager = spyk(SigningManager(signatureVerificationMode, appConfig, apiKey()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I changed to a spy so we can see and verify the calls more easily, but it's using the actual implementation. Since there is no way to know that the offerings/product-entitlement mapping calls were verified or not, this made easier to make sure we were verifying those endpoints

Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is no way to know that the offerings/product-entitlement mapping calls were verified or not

I checked that by using the "fake invalid signature" thing. Did you add a way to do that in Android too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I meant in the "success" case, we should make sure we are verifying the signature. But right now there is no way to detect whether a SUCCESS or a NOT_REQUESTED happened. We could maybe if we could verify the logs, but I haven't added that in Android yet. Until then, this makes sure we are at least calling the signingManager.verify method, so it shouldn't be NOT_REQUESTED.

Copy link
Contributor

@NachoSoto NachoSoto left a comment

Choose a reason for hiding this comment

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

Love it.

@@ -142,6 +143,7 @@ class SigningManager(
)

return if (verificationResult) {
verboseLog(NetworkStrings.VERIFICATION_SUCCESS.format(urlPath))
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

@@ -87,14 +96,12 @@ abstract class BaseBackendIntegrationTest {
every { edit() } returns sharedPreferencesEditor
}
eTagManager = ETagManager(sharedPreferences)
signingManager = SigningManager(SignatureVerificationMode.Disabled, appConfig, apiKey())
signingManager = spyk(SigningManager(signatureVerificationMode, appConfig, apiKey()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is no way to know that the offerings/product-entitlement mapping calls were verified or not

I checked that by using the "fake invalid signature" thing. Did you add a way to do that in Android too?

}

@Test
fun `needsNonceToPerformSigning is true for expected values`() {
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests are great.

@@ -33,6 +33,22 @@ sealed class Endpoint(val pathTemplate: String, val name: String) {
}

val supportsSignatureValidation: Boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

Also this isn't used now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this isn't used now?

It's used in the signing manager:

return endpoint.supportsSignatureValidation && signatureVerificationMode.shouldVerify

@tonidero
Copy link
Contributor Author

tonidero commented Jul 6, 2023

Will merge this and rerun the backend integration tests there once the backend is deployed.

@tonidero tonidero merged commit 467003f into new-trusted-entitlements-signature-format Jul 6, 2023
@tonidero tonidero deleted the toniricodiez/sdk-2990-add-signing-to-the-entitlementmapping-endpoint-android branch July 6, 2023 09:00
NachoSoto added a commit to RevenueCat/purchases-ios that referenced this pull request Jul 6, 2023
tonidero added a commit that referenced this pull request Jul 7, 2023
### Description
Integration branch for the changes in trusted entitlements. Includes
changes from:
- #1111 
- #1114 
- #1118 
- #1119 
- #1124
tonidero added a commit that referenced this pull request Jul 11, 2023
### Description
This is based on #1119. Looks like the merge commit for that PR was lost
in a rebase in the base PR #1117 😬.
 
This adds support for static endpoint signing. Currently, that would be
the offerings and product-entitlement mapping endpoints.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants