[V3] Encode Pending Browser Switch Request as Base64 String #90

sshropshire · 2024-04-02T15:56:26Z

Summary of changes

Rename BrowserSwitchPendingRequest to BrowserSwitchStartResult
Support ComponentActivity for BrowserSwitchClient#start() method
Rename BrowserSwitchResult to BrowserSwitchParseResult
Remove BrowserSwitchInfo helper class

Checklist

Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.

@sshropshire

sshropshire · 2024-04-02T16:11:00Z

browser-switch/src/main/java/com/braintreepayments/api/BrowserSwitchRequest.java

-        result.put("shouldNotify", shouldNotifyCancellation);
-        if (metadata != null) {
-            result.put("metadata", metadata);
+            byte[] requestJSONBytes = requestJSON.toString().getBytes(StandardCharsets.UTF_8);


Base 64 encoding is technically optional, though it does make the state string appear to the merchant as an opaque data type to discourage tampering. Base64 encoding also has an added benefit of masking the data to prevent casual observation of browser switch metadata e.g. intercepting an ec-token over a screen share.

sarahkoop · 2024-04-02T21:18:47Z

browser-switch/src/main/java/com/braintreepayments/api/BrowserSwitchClient.java

+     *
+     * @param intent              the intent to return to your application containing a deep link result from the
+     *                            browser flow
+     * @param pendingRequestState the {@link BrowserSwitchStartResult.Success} token returned from


I'm not sure about the state/token naming here, those don't align in my mind. Maybe startResultDetails or startResultInfo or storedStartResult and remove token from the description?

I agree the naming can be improved. Choosing the right names for Browser Switch will maximize the Developer Experience and influence the DX in core.

For me the only thing with "details" and "info" that's conflicting is an implication that the string we return can provide information at runtime. Technically it's an opaque string that we don't want merchants to inspect.

We should definitely remove the word token, that's a holdover from what I called it originally. But in a way this "state" is similar to a BT nonce–we don't expect merchants to inspect the value for details because the structure of the data is undefined i.e. doesn't follow semantic versioning.

Could we maybe shorten it to requestState? Or continue to ideate as a group this might be something to get some 👀's on. I'd be interested in seeing how this bubbles up to the features in the Core SDK too that could help us pick the right names.

I think storedStartResult makes sense. "result" seems generic enough for a merchant to not want to inspect its value.

I like storedStartResult for this method. The only thing where the naming doesn't scale as nicely is when the result is unpacked, we'd end up with result.storedStartResult.

I'm wondering now if we should just go with something like originalRequest to keep it simple? It'll still be opaque because the type is String, and it would work nicely in both contexts (start() and parse()).

Also reiterating the main reason we make this value a String is because strings work nicely with all Android persistence mechanisms e.g. SharedPrefs, DataStore, etc. We only need the merchant to hold on to this value for us to remove the responsibility of state restoration out of the SDK. We've gotten a good number of inbounds on that in the past, which makes us think that state restoration logic may be unique to each individual merchant app.

What if we had separate naming for the start() and parse() steps? It might give a better description for each for both steps.

Start - startResultToStore

when (val result = browserSwitchClient.start(this, browserSwitchOptions)) { is BrowserSwitchStartResult.Success -> PendingRequestStore.put(this, result.startResultToStore) ... }

Parse - storedStartResult

PendingRequestStore.get(this)?.let { storedStartRequest -> when (val result = browserSwitchClient.parseResult(intent, storedStartResult)) { ... } }

Honestly I still kind of think the pendingRequest naming is a little more clear for what it is - a request that hasn't been completed yet. Maybe we could change the types to Started and Failure to better indicate that the request hasn't completed successfully yet?

when (val pendingRequest = browserSwitchClient.start(this, browserSwitchOptions)) { is BrowserSwitchPendingRequest.Started -> PendingRequestStore.put(this, pendingRequest.requestToStore) ... }

Also maybe something like completeRequest would make more sense than parse to indicate what is required by the merchant?

PendingRequestStore.get(this)?.let { storedPendingRequest -> when (val result = browserSwitchClient.completeRequest(intent, storedPendingRequest)) { ... } }

Can't decide what would be most clear for merchants. Tagging in @scannillo @jaxdesmarais @saperi22 for more opinions

I like that suggestion. I think the consumer of Browser Switch could name the return value of browserSwitchClient.start(this, browserSwitchOptions) anything they wanted. But I agree with pendingRequest.

I also think completeRequest() is more clear than parse().

Agree on pendingRequest and completeRequest, those both sound like the most clear options for both us and merchants

PR for renaming parseResult() to completeRequest(): #101

sarahkoop · 2024-04-02T21:22:08Z

browser-switch/src/main/java/com/braintreepayments/api/BrowserSwitchRequest.java

-        return new BrowserSwitchRequest(requestCode, Uri.parse(url), metadata, returnUrlScheme, shouldNotify);
+
+    @NonNull
+    static BrowserSwitchRequest fromBase64EncodedJSON(@NonNull String base64EncodedRequest) throws BrowserSwitchException {


I guess we should weigh the benefits of base64encoding compared to the extra merchant integration lift of having to handle the Failure type for all browser based flows. I could go either way. What do others think?

Oh yeah def. We can't really enforce JSON tampering though. We may still need the failure scenario for the case where we're unable to reconstruct the original BrowserSwitchRequest.

tdchow · 2024-04-04T19:25:05Z

browser-switch/src/main/java/com/braintreepayments/api/BrowserSwitchClient.java

+     *
+     * @param intent              the intent to return to your application containing a deep link result from the
+     *                            browser flow
+     * @param pendingRequestState the {@link BrowserSwitchStartResult.Success} token returned from


I think storedStartResult makes sense. "result" seems generic enough for a merchant to not want to inspect its value.

tdchow · 2024-04-04T19:28:04Z

browser-switch/src/main/java/com/braintreepayments/api/BrowserSwitchRequest.java

+// Links
+// Base64 Encode a String in Android: https://stackoverflow.com/a/7360440
+
+// TODO: consider encryption
+// Ref: https://medium.com/fw-engineering/sharedpreferences-and-android-keystore-c4eac3373ac7
+
+// TODO: Rename to `BrowserSwitchStartRequest` and remove `BrowserSwitchOptions` in favor this class


Should we remove these TODOs before merging the PR in?

I agree, we may be able to convert these to JIRA tickets.

sarahkoop · 2024-06-21T16:05:45Z

This PR might need to be closed and re-worked after all the changes in browser switch - @sshropshire can you revert the naming changes or open a new PR with just the base64 encoding work?

sshropshire · 2024-06-24T21:48:50Z

Closing this after discussion with @sarahkoop. Will create a new PR with the Base64 encoding changes.

sshropshire added 11 commits March 26, 2024 12:51

Return base64 encoded string from browser switch.

70ff25f

Fix opt metadata bug.

a66ed89

Update tests and use token externally.

bacf539

Add error state to browser switching.

e5c67b2

Add Failure type to BrowserSwitchResult parser.

c52bb8e

Refactor naming.

0e2b07d

Refactor naming again.

52ddf2b

Rename local variable for consistency.

dbb0e96

Remove unecessary TODO.

4d80d70

Rename BrowserSwitchPendingRequest to BrowserSwitchStartResult.

3e6fc66

Revert no result naming.

06ead6c

sshropshire commented Apr 2, 2024

View reviewed changes

sshropshire added 2 commits April 2, 2024 13:13

Add TODO and update CHANGELOG.

8466f63

Update v3 MIGRATION GUIDE.

3b8bf62

sshropshire marked this pull request as ready for review April 2, 2024 18:35

sshropshire requested a review from a team as a code owner April 2, 2024 18:35

sarahkoop reviewed Apr 2, 2024

View reviewed changes

tdchow reviewed Apr 4, 2024

View reviewed changes

sshropshire closed this Jun 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[V3] Encode Pending Browser Switch Request as Base64 String #90

[V3] Encode Pending Browser Switch Request as Base64 String #90

sshropshire commented Apr 2, 2024 •

edited

Loading

sshropshire Apr 2, 2024

sarahkoop Apr 2, 2024

sshropshire Apr 3, 2024

tdchow Apr 4, 2024

sshropshire Apr 8, 2024

tdchow May 21, 2024 •

edited

Loading

sarahkoop May 23, 2024

tdchow May 23, 2024

jaxdesmarais May 28, 2024

tdchow May 29, 2024

sarahkoop Apr 2, 2024

sshropshire Apr 3, 2024

tdchow Apr 4, 2024

tdchow Apr 4, 2024

sshropshire Apr 8, 2024

sarahkoop commented Jun 21, 2024

sshropshire commented Jun 24, 2024

[V3] Encode Pending Browser Switch Request as Base64 String #90

[V3] Encode Pending Browser Switch Request as Base64 String #90

Conversation

sshropshire commented Apr 2, 2024 • edited Loading

Summary of changes

Checklist

Authors

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdchow May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahkoop commented Jun 21, 2024

sshropshire commented Jun 24, 2024

sshropshire commented Apr 2, 2024 •

edited

Loading

tdchow May 21, 2024 •

edited

Loading