Validate API key by tonidero · Pull Request #542 · RevenueCat/purchases-android

tonidero · 2022-06-07T10:50:47Z

Description

Android part of https://revenuecats.atlassian.net/browse/CSDK-57

With this change we log a series of situations:

When a user introduces an API key with an old format, we log a debug message prompting them to update
When a user introduces an API key that is not from Amazon or Google, we log an error
When a user introduces an API key that is from Google but configures the SDK for amazon, we log an error
When a user introduces an API key that is from Amazon but configures the SDK for google, we log an error

This behavior was tested using unit tests and manually through the purchase-tester app

TODO

Check that amazon prefix is correct
Check that we want to log and not crash if invalid API key is found
Merge Extract Purchases creation logic to factory to improve testability #543 before this

Note that this is based on #543

tonidero · 2022-06-07T10:52:53Z

As mentioned, currently I'm just logging these cases. Also lmk if there are other ways to log these messages.

tonidero · 2022-06-07T10:54:33Z

Since this is inside a static function, we could pass this as a parameter to the function, but considering that's a public API, I preferred to leave it like this... It also looks like we don't have tests for this function as of yet so something to think about.

I perform the validation as early as possible, after some other validations

🤔 we don't have a visibleForTesting alternative of the configure, but we do have one for the initializer.
Maybe we could have the apiKeyValidator as a property of Purchases and pass it into the initializer?

It also looks like we don't have tests for this function as of yet so something to think about

Filed as https://revenuecats.atlassian.net/browse/CSDK-101, let's take care of it once this PR gets merged!

tonidero · 2022-06-07T10:59:41Z

This could be abstracted to a @Before block... I just wasn't sure about adding one just for this. We could also use parameterized tests for these tests, but I don't like them as much since you lose the custom test name (and not sure if they are available in JUnit4..)

I think you just volunteered to upgrade to JUnit 5 😈

aboedo · 2022-06-07T12:14:40Z

I think you just volunteered to upgrade to JUnit 5 😈

aboedo · 2022-06-07T12:37:40Z

🤔 we don't have a visibleForTesting alternative of the configure, but we do have one for the initializer.
Maybe we could have the apiKeyValidator as a property of Purchases and pass it into the initializer?

aboedo · 2022-06-07T12:42:34Z

It also looks like we don't have tests for this function as of yet so something to think about

Filed as https://revenuecats.atlassian.net/browse/CSDK-101, let's take care of it once this PR gets merged!

aboedo · 2022-06-07T12:43:15Z

Looking great @tonidero! I left a few comments regarding testing, and we should update the prefix

tonidero · 2022-06-07T14:56:15Z

The API key wasn't exposed to the Purchases object itself. I thought about passing it to the constructor of the APIKeyValidator so we wouldn't need to pass it to Purchases, but it felt like that was a function parameter and not a class property. Lmk if you think otherwise

I wonder if this is a smell. Maybe it means that the validation should be outside by the configure method like in iOS? Then you wouldn't need to inject these 2 things in here.

Right, on a first iteration I had it there. However, the configure method is not currently tested in Android, which would leave a gap on our test to make sure that the validation actually happened. After discussing with Andy, he suggested moving the validation to the purchases constructor, which would allow for easier testing than on the configure method.

Having said that, I agree it's not ideal having to add 2 extra parameters only for this and we have already created a task to add tests for the configure method (https://revenuecats.atlassian.net/browse/CSDK-101). So we could potentially move this back there after those tests have been added. I'm working on this task today, so we can hold this to after that's done

Did the refactor for the configure method in #543 and made this PR point to that one. Lmk what you think!

vegaro · 2022-06-07T19:12:28Z

good touch on the \ns

In all honesty, I copied the message from iOS, but yeah, it improves readability I would say 👍

vegaro · 2022-06-07T19:15:46Z

I need to double check something with you... If we don't make this class internal, is it accessible for devs? I don't think so right? They would have to do implementation on the common library, or we would have to be doing api on common from the purchases module, right?

Sorry I am asking this but I always get confused haha

No worries! And that's correct, devs shouldn't have access to this class since purchases uses implementation on common. Devs shouldn't have access to it unless we used api instead of implementation on the purchases module.

aboedo · 2022-06-08T12:15:41Z

+        "configure the SDK to use Google.\nSee https://rev.cat/auth for more details."
+    const val INVALID_API_KEY = "The specified API Key is not recognized.\n" +
+        "Ensure that you are using the public app-specific API key, " +
+        " which should look like 'goog_1a2b3c4d5e6f7h' or 'amzn_1a2b3c4d5e6f7h'.\n" +


It looks like we have a double space here - one after the comma, one before the next line

Good eye! @NachoSoto I copied this message from iOS, so we probably would need to fix this there as well.

NachoSoto

Much cleaner! I love it.

NachoSoto · 2022-06-08T16:02:47Z


            require(context.applicationContext is Application) { "Needs an application context." }
+
+            apiKeyValidator.validateAndLog(apiKey, store)


NachoSoto · 2022-06-08T16:03:19Z

+        OTHER_PLATFORM
+    }
+
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)


This annotation is really useful since it gives you warnings if you try to go against the marked visibility. Though I think we should avoid using it as much as possible, since I believe we should test classes only with their public API. In this case, since the side effects of the public API are only logging, this was much easier though.

NachoSoto · 2022-06-08T16:03:43Z

+private const val GOOGLE_API_KEY_PREFIX = "goog_"
+private const val AMAZON_API_KEY_PREFIX = "amzn_"


Can these be namespaced inside of APIKeyValidator? Not sure if that's the Kotlin thing to do though.

So in order to make this a compile time constant, the options are to declare it as a top level constant (like this) or in a companion object inside the class. Both would work, but the companion object approach is slightly dirtier IMO.

If we had to make a public compile time constant and tie it to the class, then using a companion object would be the way to go but for
a private one I prefer this approach. Don't know of any best practices about this though, but this answer thinks the same way I do https://stackoverflow.com/a/49977253

tonidero · 2022-06-10T06:22:12Z

Bump to this for review @aboedo @beylmk (this is based on #543)

aboedo

:chefskiss:

tonidero · 2022-06-13T13:50:48Z

Will hold merging this until #543 is reviewed as well, since this is based on that PR @aboedo

tonidero requested a review from a team June 7, 2022 10:50

tonidero commented Jun 7, 2022

View reviewed changes

aboedo reviewed Jun 7, 2022

View reviewed changes

tonidero commented Jun 7, 2022

View reviewed changes

tonidero requested a review from aboedo June 7, 2022 15:00

aboedo requested a review from vegaro June 7, 2022 15:10

aboedo assigned tonidero Jun 7, 2022

vegaro reviewed Jun 7, 2022

View reviewed changes

tonidero force-pushed the csdk-57 branch from dec8740 to f1aaceb Compare June 8, 2022 09:35

tonidero changed the base branch from main to csdk-101 June 8, 2022 09:35

tonidero requested review from NachoSoto and vegaro June 8, 2022 09:54

aboedo reviewed Jun 8, 2022

View reviewed changes

NachoSoto reviewed Jun 8, 2022

View reviewed changes

tonidero requested review from aboedo and beylmk June 10, 2022 16:10

aboedo approved these changes Jun 10, 2022

View reviewed changes

tonidero force-pushed the csdk-101 branch from a6864eb to 1bd7945 Compare June 14, 2022 14:35

Base automatically changed from csdk-101 to main June 14, 2022 14:48

tonidero force-pushed the csdk-57 branch from 9d999f9 to ce38e82 Compare June 14, 2022 15:05

tonidero added 4 commits June 14, 2022 17:06

Validate API key

5c30bab

Rename amazon_ to amzn_ and small refactor in tests

34f9936

Move APIKeyValidator to purchases module

243e4dc

Remove extra space in error message

f273bb6

tonidero force-pushed the csdk-57 branch from ce38e82 to f273bb6 Compare June 14, 2022 15:06

tonidero merged commit 75ac90f into main Jun 14, 2022

tonidero deleted the csdk-57 branch June 14, 2022 15:19

tonidero mentioned this pull request Jun 22, 2022

Release/5.2.0 #549

Merged

1 task

tonidero mentioned this pull request Jul 7, 2022

test-Release/5.2.0 #556

Closed

aboedo mentioned this pull request Jul 13, 2022

Release: 5.2.1 #563

Merged


		require(context.applicationContext is Application) { "Needs an application context." }

		apiKeyValidator.validateAndLog(apiKey, store)

		private const val GOOGLE_API_KEY_PREFIX = "goog_"
		private const val AMAZON_API_KEY_PREFIX = "amzn_"

Uh oh!

Conversation

tonidero commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboedo Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboedo Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboedo commented Jun 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero commented Jun 10, 2022

Uh oh!

aboedo left a comment

Choose a reason for hiding this comment

Uh oh!

tonidero commented Jun 13, 2022

Uh oh!

tonidero commented Jun 7, 2022 •

edited

Loading

aboedo Jun 7, 2022 •

edited

Loading

aboedo Jun 7, 2022 •

edited

Loading