Store RegistrationResponse in AuthState.

[zamzterz]

Make use of the stored RegistrationResponse when constructing token requests in the TokenActivity of the demo app.

---

This change is

[iainmcgin]

Review status: 0 of 6 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

app/src/net/openid/appauthdemo/MainActivity.java, line 89 [r1] (raw file):
Pass in new AuthState() here instead of null, which should simplify the later handling of updating the AuthState in TokenActivity.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 120 [r1] (raw file):
this conditional should disappear after you ensure an AuthState instance is always available from the intent.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 275 [r1] (raw file):
We should generalize this by adding a new method to AuthState that can provide a ClientAuthentication instance based on the properties found in the registration request and response. Something like:

public ClientAuthentication getClientAuthentication() {
    if (mLastRegistrationResponse == null) {
        return NoClientAuthentication.INSTANCE;
    }

    switch (mLastRegistrationResponse.request.tokenEndpointAuthenticationMethod) {
        case ClientSecretBasic.NAME: 
            return new ClientSecretBasic(mLastRegistrationResponse.clientSecret);
        case ClientSecretPost.NAME: 
            return new ClientSecretPost(mLastRegistrationResponse.clientSecret);
        default: 
            return NoClientAuthentication.INSTANCE;
    }
}

---

library/java/net/openid/appauth/AuthState.java, line 419 [r1] (raw file):
Should a new registration response invalidate (clear) any existing authorization response / token response? This seems likely, as the client ID would change?

---

library/javatests/net/openid/appauth/AuthStateTest.java, line 1 [r1] (raw file):
the check task in the build is complaining you have some unused imports in this file.

---

Comments from Reviewable

[zamzterz]

Review status: 0 of 9 files reviewed at latest revision, 5 unresolved discussions.

---

app/src/net/openid/appauthdemo/MainActivity.java, line 89 [r1] (raw file):
Done.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 120 [r1] (raw file):
Done.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 275 [r1] (raw file):
This is much nicer!

I implemented your suggestion with some modifications:

1. We can't look at any parameters in the registration request since the identity provider is free to "reject or replace any of the Client's requested field values and substitute them with suitable values". The OP returns all registered metadata for the client, so we can look straight at the registration response (mLastRegistrationResponse.tokenEndpointAuthenticationMethod).
2. Explicitly handle the case "none".
3. Let the default case throw an exception since the library can't gracefully handle an unknown client authentication method. NoClientAuthentication will probably not work (since the case "none" is explicitly handled), so we can save the network round-trip by reporting this and let an implementer deal with any custom authentication methods.

Done.

---

library/java/net/openid/appauth/AuthState.java, line 419 [r1] (raw file):
We touched on this discussion previously, and I'm a bit unsure of the full extent of a new registration. There are some different parts:

1. If the access token is a Bearer token it could technically still be used (since anyone in possession of it can use it, the new client id won't matter).
2. The refresh token on the other hand can't be used with an OP which enforces client authentication at the token endpoint (the authentication with a new client id/client secret won't match the client id bound to the refresh token).
3. The last authorization and token response is definitely invalid, since they represent a session for the user with another client (another client id).

I've updated the change to invalidate the entire AuthState (mRefreshToken, mScope, mLastAuthorizationResponse, mLastTokenResponse, and mAuthorizationException). Maybe this can be improved in a future implementation, but it should be sufficient for now.

Done.

---

library/javatests/net/openid/appauth/AuthStateTest.java, line 1 [r1] (raw file):
Sorry about that!
Done.

---

Comments from Reviewable

[iainmcgin]

Review status: 0 of 9 files reviewed at latest revision, 4 unresolved discussions.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 369 [r2] (raw file):
This should be @NonNull now, I think.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 376 [r2] (raw file):
No need for the conditional here if we require that authState is non-null.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 395 [r2] (raw file):
This should actually be an error condition (throw unchecked exception), as an AuthState instance is now required.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 401 [r2] (raw file):
Similarly, I think you should throw an exception here, as without an AuthState instance the TokenActivity cannot function.

---

Comments from Reviewable

[zamzterz]

Review status: 0 of 9 files reviewed at latest revision, 4 unresolved discussions.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 369 [r2] (raw file):
Done.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 376 [r2] (raw file):
Done.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 395 [r2] (raw file):
Done.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 401 [r2] (raw file):
Done.

---

Comments from Reviewable

[iainmcgin]

Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion.

---

app/src/net/openid/appauthdemo/TokenActivity.java, line 395 [r2] (raw file):
IllegalArgumentException is preferable to RuntimeException.

---

Comments from Reviewable

[zamzterz]

app/src/net/openid/appauthdemo/TokenActivity.java, line 395 [r2] (raw file):
Of course (I at least got it right in the other place :)).
Done.

---

Comments from Reviewable

[iainmcgin]

---

Review status: 0 of 9 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable