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

Use Chrome Custom Tabs when possible #95

Merged
merged 9 commits into from
Jul 3, 2017
Merged

Conversation

lbalmaceda
Copy link
Contributor

This PR adds support for Chrome Custom Tabs to use with the WebAuthProvider class when not launching a WebView. The user API remains the same, but now they need a different configuration and they can skip the call to WebAuthProvider.resume(intent) as it's handled internally by the library. This also means that they don't need to capture the onNewIntent or onActivityResult method calls.

Proposed configuration changes

  1. Remove the custom Intent-Filter that captures the Callback URI from the AndroidManifest.xml file.

2a. Add both manifestPlaceholders to the app/build.gradle file so that the library knows which domain and scheme to use. The host is set automatically using the applicationId placeholder:

apply plugin: 'com.android.application'

android {
    compileSdkVersion 25
    buildToolsVersion "25.0.3"
    defaultConfig {
        applicationId "com.auth0.samples"
        minSdkVersion 15
        targetSdkVersion 25
        manifestPlaceholders = [auth0Domain: "@string/auth0_domain", auth0Scheme: "https"]
    }
//...
}

2b. Alternatively, don't set the manifestPlaceholders but replace the RedirectActivity definition in the AndroidManifest.xml file with one that defines the custom Intent-Filter.

<activity android:name="com.auth0.android.provider.RedirectActivity">
<intent-filter>
                <action android:name="android.intent.action.VIEW" />
                <category android:name="android.intent.category.DEFAULT" />
                <category android:name="android.intent.category.BROWSABLE" />

                <data
                    android:host="@string/auth0_domain"
                    android:pathPrefix="/android/{YOUR_APP_PACKAGE_NAME}/callback"
                    android:scheme="https" />
            </intent-filter>
</activity>

Not setting the placeholders like in (2a) or not overriding the Intent-Filter declaration like in (2b) will result in the library's defined Intent-Filter to not find the expected placeholder values and the code to not compile.

Discussion

The trade-off is between setting a default https scheme in the library's Intent-Filter (so the user doesn't need to set an extra manifest placeholder) or setting a placeholder that the user must always define.

If a normal user will use more https over a custom scheme, we can let them override the whole Intent-Filter in the manifest like in (2b). If a normal user will prefer custom schemes, then we can keep the 2 manifest placeholders proposal.

@lbalmaceda lbalmaceda requested review from nikolaseu and hzalaz June 13, 2017 19:32
README.md Outdated
Also register the intent filters inside your activity's tag, so you can receive the call in your activity. Note that you will have to specify the callback url inside the `data` tag.
It's a good practice to define reusable resources like `@string/auth0_domain` but you can also hard code the value. In case you're using a [custom scheme](#a-note-about-app-deep-linking) you must update the `auth0Scheme` property.

Alternatively you can define your own **intent-filter** for the `RedirectActivity` in the `AndroidManifest.xml` file replacing the one defined by the library. If you choose to do this, the `manifestPlaceholders` don't need to be set. In your manifest inside your application's tag add the `RedirectActivity` declaration:
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively you can declare the RedirectActivity in the AndroidManifest.xml file with your own intent-filter so it overrides the library's default.

@hzalaz
Copy link
Member

hzalaz commented Jun 14, 2017

If we promote https and is the best UX we can make it the default in the library, remove the placeholder and let the user declare the whole activity if he needs custom schemes.

@lbalmaceda
Copy link
Contributor Author

Another thing with the usage of placeholders: If the user doesn't want to use WebAuthProvider and just uses the AuthenticationAPIClient (i.e. for createUser/login) then the compiler will force him to set a valid placeholder anyway.

public boolean bindServiceAndLaunchUri(@NonNull Uri uri) {
nextUri = uri;
boolean boundRequestSuccess = bindService();
if (isBound || !boundRequestSuccess) {
Copy link
Contributor

@nikolaseu nikolaseu Jun 15, 2017

Choose a reason for hiding this comment

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

I think the negation is wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean
"launch uri if service was already bound or was just bounded successfully"
but then.. why call bindService if it was already bound?

Copy link
Contributor

Choose a reason for hiding this comment

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

anybody cares if it was already bound or was bound in this call?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok I get it.
but from the way it's used, this can never be "already bound" because a new controller is created each time. maybe something to optimize in the activity that uses this in https://github.com/auth0/Auth0.Android/pull/95/files#diff-101f8235cd64f6d21cfc3fc82d610bd5R105

if (context != null) {
context.unbindService(this);
}
nextUri = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be also cleared when the uri is launched?

@lbalmaceda
Copy link
Contributor Author

I decided to go with the default https scheme option. On my tests with the different emulators, the longer it took to bind and connect to the Custom Tabs Service was ~600ms. I've set a max non-blocking wait time of 1 second for the service to connect, that IMHO could be raised to 1.5 or 2 safely. If the service is not connected in that given time, a call to launchUri will use a simple Browser instead of the Custom Tabs Browser.

@lbalmaceda lbalmaceda force-pushed the add-chrome-custom-tabs branch from fdc27aa to c4a9e67 Compare June 30, 2017 21:56
@lbalmaceda lbalmaceda force-pushed the add-chrome-custom-tabs branch from c4a9e67 to 9ad1e0d Compare July 3, 2017 16:12
@lbalmaceda lbalmaceda merged commit e7f06e7 into master Jul 3, 2017
@lbalmaceda lbalmaceda deleted the add-chrome-custom-tabs branch July 3, 2017 16:24
@lbalmaceda lbalmaceda modified the milestones: v1-Next, 1.9.0 Jul 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants