Problem with getValidAccessToken()

[simonnorberg]

We call ConnectSdk.getValidAccessToken(AccessTokenCallback callback) in onResume() in our launch activity to check if we are logged in with ConnectID. If we get a onSuccess() callback we assume that we are logged in and have a valid IdToken. However, this is not always the case. Sometimes ConnectSdk.getIdToken().getSerializedSignedJwt() and ConnectSdk.getIdToken().getSubject() returns null if we call them in the AccessTokenCallback onSuccess.

Is this working as intended? Our understanding is that if we have a valid access token we should always have an IdToken? Is there a recommended way to verify that a user is logged in?

We are using version 1.7.1.

[jorunfa]

There might of course be something wrong in our solution, but there is one possibility that would explain this without there being an error.

This happens for some users not all, some of the time, or have I misunderstood that bit?

I'm not sure how it might have happened, but is it possible that some of these users have signed in without openid in the scope? From http://docs.telenordigital.com/apis/connect/id/authentication.html:

[refering to the ID Token] This field is only present if the openid scope value was provided to the user authorization call.

If they at one time, perhaps in a earlier version of the app, signed in without this scope value. Is that possible?

If not, we can try to recreate the problem.

[simonnorberg]

This happens for some users not all, some of the time, or have I misunderstood that bit?

Yes, this happens only for some users. But it seems to happen all or most of the time for those users.

If they at one time, perhaps in a earlier version of the app, signed in without this scope value. Is that possible?

We always set openid in the scope using ConnectLoginButton.setLoginScopeTokens in all versions of the app. So that should not be the cause.

[jorunfa]

I will see if I can recreate it.

[jorunfa]

I haven't had the time to look into this issue to try to recreate it yet, due to other, more pressing, issues. I will see if we can find the time today or Monday.

Any additional information to help recreate the issue is appreciated, if any is available.

[jorunfa]

We weren't able to reproduce, could you compare implementation to the example implementation? https://github.com/telenordigital/connect-android-sdk/commit/9cbf492d3083e9133fad395ac1e4345e6bfa7841

Available in getvalidaccesstoken-example branch

[jorunfa]

Is there any other information you could give us that would help us debug the problem? Do you have any stats on which Android version is being used or more information about the context this is being used? If needed you can always send me some source code (privately).

[simonnorberg]

Hi,

regarding the branch getvalidaccesstoken-example:

We use AppCompatActivity instead of Activity.

We use the "SignInActivity" as our launch activity.

We don't do this check in onCreate.

if (ConnectSdk.getAccessToken() == null) {
    goToLogin();
    return;
}

Our onResume actually looks more like this:

if (ConnectSdk.hasValidRedirectUrlCall(getIntent())) {
    ConnectSdk.handleRedirectUriCallIfPresent(getIntent(), new ConnectCallback() {
            @Override
            public void onSuccess(Object successData) {
                goToSignedInActivity();
            }

            @Override
            public void onError(Object errorData) {
                showEnabledButton();
            }
        });
} else {
    ConnectSdk.getValidAccessToken(new AccessTokenCallback() {
            @Override
            public void onSuccess(String accessToken) {
                goToSignedInActivity();
            }

            @Override
            public void onError(Object errorData) {
                showEnabledButton();
            }
        });
}

Another scenario that happens often in our onResume is that

1. (onResume) ConnectSdk.hasValidRedirectUrlCall() returns true
2. We get ConnectSdk.handleRedirectUriCallIfPresent onError
3. User clicks login button.
4. Login webview/custom tabs opens and closes immediately
5. (onResume again) ConnectSdk.hasValidRedirectUrlCall() returns true
6. We get ConnectSdk.handleRedirectUriCallIfPresent onSuccess and launch SignedInActivity.

---

Most of our users have Samsung galaxy s7/s8/s9 and Android 7-8.

---

Many users only opens the app weekly or monthly (to pay bills) so these problems might only happen when you are not using the app often.

[jorunfa]

I'm a bit confused, here... ConnectSdk.getValidAccessToken will not authenticate the user, only get the access token if there is a signed in user.

I think you perhaps can change the SignInActivity logic to something like this: https://gist.github.com/jorunfa/2931b42583fbea10f864c74771111acd

[simonnorberg]

Ok, my understanding was that if (ConnectSdk.getAccessToken() != null) { is the same as ConnectSdk.getValidAccessToken but that it will also try to refresh the token automatically?

[jorunfa]

No, not quite, it throws if there are no signed in users: https://github.com/telenordigital/connect-android-sdk/blob/master/connect/src/com/telenor/connect/ConnectSdk.java#L175 https://github.com/telenordigital/connect-android-sdk/blob/master/connect/src/com/telenor/connect/id/ConnectIdService.java#L44

1. I understand the confusion (it's very reasonable)
2. I see there is a spelling error Request Token -> Refresh Token.
3. The exception is unchecked (inherits RuntimeException) - a decision I think was wrong.

I guess the proper solution might be to have a different callback, something like this, and never throw: https://gist.github.com/jorunfa/eca797d6a01e3d9b2a68d4305665393e What do you think?

[simonnorberg]

Yeah, FailureType would be better 👍

Is there a difference (for the user experience) between these examples? We're using Example 1 at the moment.

Example 1:

if (ConnectSdk.hasValidRedirectUrlCall(getIntent())) {
    ConnectSdk.handleRedirectUriCallIfPresent(getIntent(), new ConnectCallback() {
            @Override
            public void onSuccess(Object successData) {
                goToSignedInActivity();
            }

            @Override
            public void onError(Object errorData) {
                showEnabledButton();
            }
        });
} else {
   try {
        ConnectSdk.getValidAccessToken(new AccessTokenCallback() {
                @Override
                public void onSuccess(String accessToken) {
                    goToSignedInActivity();
                }

                @Override
                public void onError(Object errorData) {
                    showEnabledButton();
                }
            });
    catch (ConnectRefreshTokenMissingException e) {
        showEnabledButton();
    }
}

Example 2:

if (ConnectSdk.hasValidRedirectUrlCall(getIntent())) {
    ConnectSdk.handleRedirectUriCallIfPresent(getIntent(), new ConnectCallback() {
            @Override
            public void onSuccess(Object successData) {
                goToSignedInActivity();
            }

            @Override
            public void onError(Object errorData) {
                showEnabledButton();
            }
        });
} else {
    if (ConnectSdk.getAccessToken() != null) {
        goToSignedInActivity();
    } else {
        showEnabledButton();
    }
}

[jorunfa]

Sorry for the slow reply... we're trying to hire more people so we can be less busy. I think your two scenarios are close to equivalent, except in example 2, it should be != null instead of == null.

if if (ConnectSdk.getAccessToken() != null) is called then you have a signed in user (since there are tokens).

One other thing: ConnectSdk.handleRedirectUriCallIfPresent calls ConnectSdk.hasValidRedirectUrlCall internally, so you don't need to wrap it in that. ConnectSdk.hasValidRedirectUrlCall was originally meant to be a helper for setting UI element state (like showing a loading spinner while the no-UI network call for exchanging authorization code for access and refresh tokens runs).

[jorunfa]

@simonnorberg: See #131. Is it an improvement or is it still 💩?

[simonnorberg]

Ah yes, the null check should be inverted. Edited my example!

[simonnorberg]

We check hasValidRedirectUrlCall() because we need to know if we should expect a callback from handleRedirectUriCallIfPresent() or not.

[simonnorberg]

For #131, I think our usage of the new getValidAccessToken would look like this: Our error handling will be the same for all callbacks that are not success.

In SignInActivity (our launch activity)

@Override
protected void onResume() {
    super.onResume();
    ConnectSdk.getValidAccessToken(new AccessTokenCallback() {
        @Override
        public void success(String accessToken) {
            goToSignedInActivity();
        }

        @Override
        public void unsuccessfulResult(Response response, boolean userWasCleared) {
            goToLogin();
        }

        @Override
        public void failure(Call<ConnectTokensTO> call, Throwable error) {
            goToLogin();
        }

        @Override
        public void noSignedInUser() {
            goToLogin();
        }
    });
}

We actually only need a simple "facade"-like method from the SDK.

public interface LoginCallback {
    void success(LoginData data);
    void error(ErrorType type);
}

ConnectSdk.login(new LoginCallback() {
    @Override
    public void success(LoginData data) {
        goToSignedInActivity();
    }

    @Override
    public void error(ErrorType type) {
        // Maybe do something for different error types.
        goToLogin();
    }
});

Ideally, the login method should do everything possible to keep the user signed in.

For example:

public void login(LoginCallback callback) {
    if (!ConnectSdk.isInitialized()) {
    // initialize sdk, wait for initialization to finish.
    }

    if (hasValidRedirectUrlCall()) {
    // handleRedirectUriCall
    }

    if (needToUpdateToken()) {
    // update tokens
    }

    if (signedIn) {
    callback.success()
    } else {
    callback.error()
    }
}

To authenticate with our backend API we need to fetch these strings in addition to the access token. ConnectSdk.getIdToken().getSerializedSignedJwt(); ConnectSdk.getIdToken().subject(); ConnectSdk.getClientId();

We fetch them before each API request.

If LoginData would contain these values then we could use ConnectSdk.login() to get these values as well. Then this method would be our only interaction point with the SDK.

public class LoginData {
    @NonNull public final String serializedSignedJwt;
    @NonNull public final String subject;
    @NonNull public final String clientId;
    @NonNull public final String accessToken;
}

What do you think?

[jorunfa]

We check hasValidRedirectUrlCall() because we need to know if we should expect a callback from handleRedirectUriCallIfPresent() or not.

See https://github.com/telenordigital/connect-android-sdk/blob/master/connect/src/com/telenor/connect/ConnectSdk.java#L533 I'm just saying handleRedirectUriCallIfPresent calls hasValidRedirectUrlCall so you don't need to do it twice.

[jorunfa]

For #131, I think our usage of the new getValidAccessToken would look like this: Our error handling will be the same for all callbacks that are not success.

In SignInActivity (our launch activity)

@Override
protected void onResume() {
    super.onResume();
    ConnectSdk.getValidAccessToken(new AccessTokenCallback() {
        @Override
        public void success(String accessToken) {
            goToSignedInActivity();
        }

        @Override
        public void unsuccessfulResult(Response response, boolean userWasCleared) {
            goToLogin();
        }

        @Override
        public void failure(Call<ConnectTokensTO> call, Throwable error) {
            goToLogin();
        }

        @Override
        public void noSignedInUser() {
            goToLogin();
        }
    });
}

The onResume method in the pull request was my attempt at a reference implementation of the new interface (don't know if you saw it or not): https://github.com/telenordigital/connect-android-sdk/blob/1c419f0a2dfbf085f745503977781c7b5bed83b4/connect-id-example/src/main/java/com/telenor/connect/connectidexample/SignedInActivity.java#L51

I guess what I'm trying to communicate is that if you get failure callback then it might just be a network problem, and you should consider trying again instead of signing out the user. But in order to know this you also need the Throwable.

We actually only need a simple "facade"-like method from the SDK.

public interface LoginCallback {
    void success(LoginData data);
    void error(ErrorType type);
}

ConnectSdk.login(new LoginCallback() {
    @Override
    public void success(LoginData data) {
        goToSignedInActivity();
    }

    @Override
    public void error(ErrorType type) {
        // Maybe do something for different error types.
        goToLogin();
    }
});

Ideally, the login method should do everything possible to keep the user signed in.

For example:

public void login(LoginCallback callback) {
    if (!ConnectSdk.isInitialized()) {
  // initialize sdk, wait for initialization to finish.
    }

    if (hasValidRedirectUrlCall()) {
  // handleRedirectUriCall
    }

    if (needToUpdateToken()) {
  // update tokens
    }

    if (signedIn) {
  callback.success()
    } else {
  callback.error()
    }
}

To authenticate with our backend API we need to fetch these strings in addition to the access token. ConnectSdk.getIdToken().getSerializedSignedJwt(); ConnectSdk.getIdToken().subject(); ConnectSdk.getClientId();

We fetch them before each API request.

If LoginData would contain these values then we could use ConnectSdk.login() to get these values as well. Then this method would be our only interaction point with the SDK.

public class LoginData {
    @NonNull public final String serializedSignedJwt;
    @NonNull public final String subject;
    @NonNull public final String clientId;
    @NonNull public final String accessToken;
}

What do you think?

If you want to add that abstraction then you could do so by doing:

public abstract class LoginCallback implements AccessTokenCallback {
    @Override
    public void success(String accessToken) {
        LoginData loginData = new LoginData();
        loginData.accessToken = ConnectSdk.getAccessToken();
        loginData.clientId = ConnectSdk.getClientId();
        loginData.serializedSignedJwt = ConnectSdk.getIdToken().getSerializedSignedJwt();
        loginData.subject = ConnectSdk.getIdToken().getSubject();
        success(loginData);
    }

    public abstract void success(LoginData loginData);

    @Override
    public void unsuccessfulResult(Response response, boolean userDataRemoved) {
        error(UN_SUCCESSFUL);
    }

    @Override
    public void failure(Call<ConnectTokensTO> call, Throwable error) {
        error(LIKELY_NETWORK_ERROR);
    }

    @Override
    public void noSignedInUser() {
        error(NO_SIGNED_IN_USER);
    }

    public abstract void error(ErrorType type);

}

enum ErrorType {
    NO_SIGNED_IN_USER, LIKELY_NETWORK_ERROR, UN_SUCCESSFUL
}

class LoginData {
    @NonNull public String serializedSignedJwt;
    @NonNull public String subject;
    @NonNull public String clientId;
    @NonNull public String accessToken;
}

And use it like so:

ConnectSdk.getValidAccessToken(new LoginCallback() {
    @Override
    public void success(LoginData loginData) {
        // so many things
    }

    @Override
    public void error(ErrorType type) {
        // check the error
    }
});

I don't think I want to add this to the SDK, because LoginData contains duplicate information (serializedSignedJwt and subject), and it would be up to you if you need that subset/structure of information.

The problem I have with ErrorType here is that it gives limited and perhaps lacking information relative to AccessTokenCallback, but I'm more open on adding that one. If it's not added you could always just copy paste the implementation in this comment. Let me know what you think.

[simonnorberg]

I don't think I want to add this to the SDK, because LoginData contains duplicate information (serializedSignedJwt and subject), and it would be up to you if you need that subset/structure of information.

Yeah, makes sense.

It would be great to have the new AccessTokenCallback in the next release. But I'm not sure it fixes the original problem that some users get a null idToken in the success callback.

I guess I can do a workaround like this:

@Override
public void success(String accessToken) {
    String clientId = ConnectSdk.getClientId();
    String jwt = null;
    String subject = null;
    IdToken idToken = ConnectSdk.getIdToken();
    if (idToken != null) {
        jwt = idToken.getSerializedSignedJwt();
        subject = idToken.getSubject();
    }
    if (accessToken != null && clientId != null && jwt != null && subject != null) {
        success(new LoginData(accessToken, clientId, jwt, subject));
    } else {
        error(NO_SIGNED_IN_USER);
    }
}

[jorunfa]

You could do that... I'm still not sure/not been able to reproduce that problem.

What you could do if you for some strange reason don't have the ID token is to use the userinfo endpoint:

1. http://docs.telenordigital.com/apis/connect/id/authentication.html#authorization-server-user-information
2. https://github.com/telenordigital/connect-android-sdk/blob/master/connect/src/com/telenor/connect/ConnectSdk.java#L481
3. https://github.com/telenordigital/connect-android-sdk/blob/master/connect/src/com/telenor/connect/id/UserInfo.java

In regards to #131, is it approved by you then, in terms of giving you what you want/need?

[simonnorberg]

What you could do if you for some strange reason don't have the ID token is to use the userinfo endpoint

Ok, I will try that.

In regards to #131, is it approved by you then, in terms of giving you what you want/need?

Yes, looks good.