Add validation to catch blank ephemeral keys in CustomerConfiguration #2517
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sfriedman-stripe
force-pushed
the
sfriedman/validate-ephemeral-key
branch
2 times, most recently
from
2bd7d0d
to
debae23
Compare
yuki-stripe
previously approved these changes
Apr 27, 2023
StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/PaymentSheetConfiguration.swift
Outdated
Show resolved
Hide resolved
Comment on lines
14
to
19
| func test_customerConfigurationInit_assertsWhenEphemeralKeyIsBlank() throws { | ||
| let exception = catchBadInstruction { | ||
| _ = PaymentSheet.CustomerConfiguration(id: "foo", ephemeralKeySecret: "") | ||
| } | ||
|
|
||
| XCTAssertNotNil(exception) |
sfriedman-stripe
force-pushed
the
sfriedman/validate-ephemeral-key
branch
from
93dabc2
to
8fd3dac
Compare
CwlPreconditionTesting allows us to unit test traps (i.e. fatalError, assert, precondition). We currently don't have any way to test these in the repo, so engineers simply don't test our assertions. This will improve the overall state of our unit tests by encouraging testing of more code paths. Further reading on how CwlPreconditionTesting works: https://www.cocoawithlove.com/blog/2016/02/02/partial-functions-part-two-catching-precondition-failures.html
We currently don't validate that an ephemeral key is non-empty when a user includes it in the `CustomerConfiguration`. This ends up giving users a pretty obscure error in the API response, and is difficult to debug today. Including this validation will catch this issue early and provide users a better debugging experience.
Co-authored-by: Yuki <[email protected]>
sfriedman-stripe
force-pushed
the
sfriedman/validate-ephemeral-key
branch
from
b36b25f
to
6a71a2e
Compare
|
@yuki-stripe Had to fix a couple tests that I broke. Could you PTAL and make sure I'm not breaking the expected behavior for those tests? |
yuki-stripe
previously approved these changes
Apr 27, 2023
eurias-stripe
previously approved these changes
Apr 28, 2023
CHANGELOG.md
Outdated
| @@ -2,6 +2,9 @@ | |||
| ### Payments | |||
| * [Fixed] STPPaymentHandler.handleNextAction allows payment methods that are delayed or require further customer action like like SEPA Debit or OXXO. | |||
|
|
|||
| ### PaymentSheet | |||
| * [Added] Improved validation for ephemeral keys | |||
There was a problem hiding this comment.
Does this need a changelog entry? This is not a user facing change.
In case we do need the entry, tiny nit:
Suggested change
| * [Added] Improved validation for ephemeral keys | |
| * [Added] Improved validation for ephemeral keys. |
There was a problem hiding this comment.
I'm not sure what we normally do/do not include in the changelog. It does improve user-facing error messages that users have complained about in the past, so sorta-kinda user facing?
There was a problem hiding this comment.
I can go either way with this one. If users have specifically mentioned this then it is worth calling it out.
sfriedman-stripe
dismissed stale reviews from eurias-stripe and yuki-stripe
via
e9c42e0
Co-authored-by: eurias-stripe <[email protected]>
eurias-stripe
approved these changes
May 1, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Added an assertion to catch blank ephemeral keys when a user instantiates a
CustomerConfigurationMotivation
We currently don't validate that an ephemeral key is non-empty when a
user includes it in the
CustomerConfiguration. This ends up givingusers a pretty obscure error in the API response, and is difficult to
debug today.
Including this validation will catch this issue early and provide users
a better debugging experience.
As part of this change, I also added a dependency on CwlPreconditionTesting.
CwlPreconditionTesting allows us to unit test traps (i.e. fatalError,
assert, precondition). We currently don't have any way to test these in
the repo, so engineers simply don't test our assertions. This will
improve the overall state of our unit tests by encouraging testing of
more code paths.
Testing
Changelog