-
Notifications
You must be signed in to change notification settings - Fork 18
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
[V3] Encode Pending Browser Switch Request as Base64 String #90
Changes from all commits
70ff25f
a66ed89
bacf539
e5c67b2
c52bb8e
0e2b07d
52ddf2b
dbb0e96
4d80d70
3e6fc66
06ead6c
8466f63
3b8bf62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
package com.braintreepayments.api | ||
|
||
import android.net.Uri | ||
import org.json.JSONObject | ||
|
||
/** | ||
* The result of a browser switch obtained from [BrowserSwitchClient.parseResult] | ||
*/ | ||
sealed class BrowserSwitchParseResult { | ||
|
||
/** | ||
* The browser switch was successfully completed. See [resultInfo] for details. | ||
*/ | ||
class Success internal constructor( | ||
val deepLinkUrl: Uri, | ||
val requestCode: Int, | ||
val requestUrl: Uri, | ||
val requestMetadata: JSONObject?, | ||
) : BrowserSwitchParseResult() { | ||
internal constructor(deepLinkUrl: Uri, originalRequest: BrowserSwitchRequest) : this( | ||
deepLinkUrl, | ||
originalRequest.requestCode, | ||
originalRequest.url, | ||
originalRequest.metadata | ||
) | ||
} | ||
|
||
/** | ||
* The browser switch failed. | ||
* @property [error] Error detailing the reason for the browser switch failure. | ||
*/ | ||
class Failure internal constructor(val error: BrowserSwitchException) : | ||
BrowserSwitchParseResult() | ||
|
||
/** | ||
* No browser switch result was found. This is the expected result when a user cancels the | ||
* browser switch flow without completing by closing the browser, or navigates back to the app | ||
* without completing the browser switch flow. | ||
*/ | ||
object NoResult : BrowserSwitchParseResult() | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,39 +1,58 @@ | ||
package com.braintreepayments.api; | ||
|
||
import android.net.Uri; | ||
import android.util.Base64; | ||
|
||
import androidx.annotation.NonNull; | ||
import androidx.annotation.VisibleForTesting; | ||
|
||
import org.json.JSONException; | ||
import org.json.JSONObject; | ||
|
||
import java.nio.charset.StandardCharsets; | ||
|
||
// 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 | ||
Comment on lines
+14
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we remove these TODOs before merging the PR in? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, we may be able to convert these to JIRA tickets. |
||
public class BrowserSwitchRequest { | ||
|
||
private static final String KEY_REQUEST_CODE = "requestCode"; | ||
private static final String KEY_URL = "url"; | ||
private static final String KEY_RETURN_URL_SCHEME = "returnUrlScheme"; | ||
private static final String KEY_METADATA = "metadata"; | ||
|
||
private final Uri url; | ||
private final int requestCode; | ||
private final JSONObject metadata; | ||
@VisibleForTesting | ||
final String returnUrlScheme; | ||
private boolean shouldNotifyCancellation; | ||
|
||
static BrowserSwitchRequest fromJson(String json) throws JSONException { | ||
JSONObject jsonObject = new JSONObject(json); | ||
int requestCode = jsonObject.getInt("requestCode"); | ||
String url = jsonObject.getString("url"); | ||
String returnUrlScheme = jsonObject.getString("returnUrlScheme"); | ||
JSONObject metadata = jsonObject.optJSONObject("metadata"); | ||
boolean shouldNotify = jsonObject.optBoolean("shouldNotify", true); | ||
return new BrowserSwitchRequest(requestCode, Uri.parse(url), metadata, returnUrlScheme, shouldNotify); | ||
|
||
@NonNull | ||
static BrowserSwitchRequest fromBase64EncodedJSON(@NonNull String base64EncodedRequest) throws BrowserSwitchException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we should weigh the benefits of base64encoding compared to the extra merchant integration lift of having to handle the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
byte[] data = Base64.decode(base64EncodedRequest, Base64.DEFAULT); | ||
String requestJSONString = new String(data, StandardCharsets.UTF_8); | ||
try { | ||
JSONObject requestJSON = new JSONObject(requestJSONString); | ||
return new BrowserSwitchRequest( | ||
requestJSON.getInt(KEY_REQUEST_CODE), | ||
Uri.parse(requestJSON.getString(KEY_URL)), | ||
requestJSON.optJSONObject(KEY_METADATA), | ||
requestJSON.getString(KEY_RETURN_URL_SCHEME) | ||
); | ||
} catch (JSONException e) { | ||
throw new BrowserSwitchException("Unable to deserialize browser switch state.", e); | ||
} | ||
} | ||
|
||
BrowserSwitchRequest(int requestCode, Uri url, JSONObject metadata, String returnUrlScheme, boolean shouldNotifyCancellation) { | ||
BrowserSwitchRequest(int requestCode, Uri url, JSONObject metadata, String returnUrlScheme) { | ||
this.url = url; | ||
this.requestCode = requestCode; | ||
this.metadata = metadata; | ||
this.returnUrlScheme = returnUrlScheme; | ||
this.shouldNotifyCancellation = shouldNotifyCancellation; | ||
} | ||
|
||
Uri getUrl() { | ||
|
@@ -48,24 +67,20 @@ JSONObject getMetadata() { | |
return metadata; | ||
} | ||
|
||
boolean getShouldNotifyCancellation() { | ||
return shouldNotifyCancellation; | ||
} | ||
|
||
void setShouldNotifyCancellation(boolean shouldNotifyCancellation) { | ||
this.shouldNotifyCancellation = shouldNotifyCancellation; | ||
} | ||
@NonNull | ||
String toBase64EncodedJSON() throws BrowserSwitchException { | ||
try { | ||
JSONObject requestJSON = new JSONObject() | ||
.put(KEY_REQUEST_CODE, requestCode) | ||
.put(KEY_URL, url.toString()) | ||
.put(KEY_RETURN_URL_SCHEME, returnUrlScheme) | ||
.putOpt(KEY_METADATA, metadata); | ||
|
||
String toJson() throws JSONException { | ||
JSONObject result = new JSONObject(); | ||
result.put("requestCode", requestCode); | ||
result.put("url", url.toString()); | ||
result.put("returnUrlScheme", returnUrlScheme); | ||
result.put("shouldNotify", shouldNotifyCancellation); | ||
if (metadata != null) { | ||
result.put("metadata", metadata); | ||
byte[] requestJSONBytes = requestJSON.toString().getBytes(StandardCharsets.UTF_8); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return Base64.encodeToString(requestJSONBytes, Base64.DEFAULT); | ||
} catch (JSONException e) { | ||
throw new BrowserSwitchException("Unable to serialize browser switch state.", e); | ||
} | ||
return result.toString(); | ||
} | ||
|
||
boolean matchesDeepLinkUrlScheme(@NonNull Uri url) { | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the state/token naming here, those don't align in my mind. Maybe
startResultDetails
orstartResultInfo
orstoredStartResult
and removetoken
from the description?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
storedStartResult
makes sense. "result" seems generic enough for a merchant to not want to inspect its value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withresult.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 isString
, and it would work nicely in both contexts (start()
andparse()
).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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we had separate naming for the
start()
andparse()
steps? It might give a better description for each for both steps.Start -
startResultToStore
Parse -
storedStartResult
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toStarted
andFailure
to better indicate that the request hasn't completed successfully yet?Also maybe something like
completeRequest
would make more sense thanparse
to indicate what is required by the merchant?Can't decide what would be most clear for merchants. Tagging in @scannillo @jaxdesmarais @saperi22 for more opinions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withpendingRequest
.I also think
completeRequest()
is more clear thanparse()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on
pendingRequest
andcompleteRequest
, those both sound like the most clear options for both us and merchantsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR for renaming
parseResult()
tocompleteRequest()
: #101