AGDROID-686 Token integrity check #21

aidenkeating · 2018-01-31T10:41:43Z

Motivation

JIRA: https://issues.jboss.org/browse/AGDROID-686

Description

Function to Verify JWT Tokens (Access, Identity, Refresh) using a Keycloak Realm Public Key before they are used as part of client side access control decisions.

Provide the configuration from the JSON mobile configuration to the OIDCCredentials.

Tested successfully with both valid & tampered Access/Identity tokens. Identity Brokering originated tokens can also be validated.

Progress

Get the Public Key from the Mobile Core Config (when available)
Get the Audience from the Mobile Core Config (when available)
Construct the integrity parameters (build issuer etc.) from the core config.
Add Unit tests - Test Wrong issues, audiences, tampered signature, payload, algorithm info sections etc

aidenkeating · 2018-01-31T11:10:32Z

@ziccardi @secondsun Would you mind taking a look?

This adds IntegrityCheckParameters that are part of the OIDCCredentials containing the issuer, audience and publicKey from the mobile JSON config.

At the moment it looks like the SDK user would provide the credential at login. In this case I'm kind of confused as at the moment Credential seems to be used as a container for the tokens when a user has already authenticated. I don't understand why/how a user would provide that information at login.

Something tells me OIDCCredential has become more like AuthState and OIDCCredentials should be refactored to OIDCAuthState. Something that is created once a user is logged in instead of something the SDK user provides.

A call may be better to explain this.

ziccardi · 2018-01-31T11:23:26Z

@aidenkeating The authentication is performed through the usage of an authentication chain that can be configured at runtime.
Some of the rings of the authentication chain require a credential to be supplied (for example, to check that the token is still valid), some of them do not require the credentials (for example the browser authentication).
In both cases, after the user has been authenticated, an UserPrincipal object is returned containing the UserCredentials (in case of OIDC, it will contain the auth tokens).

ziccardi · 2018-01-31T11:24:12Z

auth/src/main/java/org/aerogear/auth/AbstractAuthenticator.java

-    public AbstractAuthenticator(final AuthServiceConfig config) {
-        this.config = config;
+
+    public AbstractAuthenticator(ServiceConfiguration serviceConfig) {


Make the parameter final

ziccardi · 2018-01-31T11:25:20Z

auth/src/main/java/org/aerogear/auth/AuthService.java

-    private AuthService(final AuthServiceConfig config) {
+    public AuthService() {}
+
+    public void bootstrap(MobileCore core, ServiceConfiguration serviceConfig) {


Make the params final

ziccardi · 2018-01-31T11:26:50Z

auth/src/main/java/org/aerogear/auth/credentials/IntegrityCheckParameters.java

+            .toString();
+    }
+
+    public static IntegrityCheckParameters deserialize(String serializedParams) throws JSONException {


Make the param final

ziccardi · 2018-01-31T11:27:34Z

auth/src/main/java/org/aerogear/auth/credentials/IntegrityCheckParameters.java

+    private String audience;
+    private String publicKey;
+
+    public IntegrityCheckParameters(String audience, String issuer, String publicKey) {


Make the params final

ziccardi · 2018-01-31T11:28:42Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+     * @param jwtToken The JWT token to verify.
+     * @return <code>true</code> if the token integrity is good.
+     */
+    public boolean verifyToken(String jwtToken) {


Make the param final

ziccardi · 2018-01-31T11:29:18Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+     * @param audience - The expected Audience of the JWT
+     * @return <code>true</code> if the token integrity is good.
+     */
+    public boolean verifyToken(String jwtToken, String publicKey, String issuer, String audience) {


See previous comments

ziccardi · 2018-01-31T11:31:07Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+            //  Validate the JWT and process it to the Claims
+            JwtClaims jwtClaims = jwtConsumer.processToClaims(jwtToken);
+            verified = true;
+            System.out.println("JWT Verified Successfully.");


Don't use System.out

ziccardi · 2018-01-31T11:31:17Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+        catch (InvalidJwtException e)
+        {
+            // InvalidJwtException will be thrown, if the JWT failed processing or validation in anyway.
+            System.out.println("JWT Validation Failed. " + e.getLocalizedMessage());


See previous comment

ziccardi · 2018-01-31T11:32:08Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+            .toString();
+    }
+
+    public static OIDCCredentials deserialize(String serializedCredential) throws JSONException {


Make parameter final

ziccardi · 2018-01-31T11:33:59Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+    public String serialize() throws JSONException {
+        return new JSONObject()
+            .put("authState", this.authState.jsonSerializeString())
+            .put("integrityCheck", this.integrityCheckParameters.serialize())


integrityCheckParameters could be null

secondsun · 2018-01-31T14:23:07Z

@aidenkeating Do you have a demo project that shows the code being used somewhere?

aidenkeating · 2018-01-31T14:28:19Z

@secondsun Not yet. I can implement it in https://github.com/feedhenry/mobile-security-android-template

secondsun

@aidenkeating Mostly looks good, added a few changes. The most important one I think is changing the JSON exception to a more descriptive run time exception.

The general argument is that we aren't working on JSON at a conceptual level. We are using OIDC tokens and configurations instead. This means that JSONException is leaking internal information that is not relevant to the user's tasks. Because this information isn't relevant we end up with exceptions that aren't solvable by the user (see here throws JSONException being leaked everywhere)

secondsun · 2018-01-31T14:27:54Z

auth/build.gradle

Remove these two lines, they are already set above to 1.8

secondsun · 2018-01-31T14:28:42Z

auth/src/main/java/org/aerogear/auth/AbstractAuthenticator.java

@@ -1,6 +1,7 @@
 package org.aerogear.auth;


packages need to be renamed to conform to https://github.com/aerogear/proposals/blob/master/sdks/SDK-naming.md

@secondsun : just to be sure, should we rename it to org.aerogear.android.ags.auth?

@ziccardi Yes

secondsun · 2018-01-31T14:31:15Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+    public boolean verifyToken(final String jwtToken, final String publicKey, final String issuer, final String audience) {
+        // add the Begin/End tags to the public key generated from Keycloak
+        String beginPublicKey = "-----BEGIN PUBLIC KEY-----";
+        String endPublicKey = "-----END PUBLIC KEY-----";


these should be private static final constant fields instead of local variables.

secondsun · 2018-01-31T14:31:47Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+            JwtClaims jwtClaims = jwtConsumer.processToClaims(jwtToken);
+            return true;
+        } catch (InvalidJwtException e) {
+            e.printStackTrace();


For now use Android's Log object instead of printing the stack trace.

secondsun · 2018-01-31T14:33:50Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

@@ -67,8 +145,21 @@ public boolean isAuthorized() {
     * Returns stringified JSON for the OIDCCredential.
     * @return Stringified JSON OIDCCredential
     */
-    public String serialise() {
-        return this.authState.jsonSerializeString();
+    public String serialize() throws JSONException {


How is serialize used?

Personally I would remove the JSONException from the method declaration and rethrow it as a runtime exception and add that clue to the documentation.

@secondsun serialize is used when persisting the auth state in the AuthStateManager.

secondsun · 2018-01-31T14:35:18Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+        return jsonCredential.toString();
+    }
+
+    public static OIDCCredentials deserialize(final String serializedCredential) throws JSONException {


See above comments about JSONException.

secondsun · 2018-01-31T14:45:16Z

auth/src/main/java/org/aerogear/auth/utils/AuthStateManager.java

@@ -39,11 +39,11 @@ public OIDCCredentials read() {
     * Saves a token
     * @param authState token to be saved
     */
-    public synchronized void write(final OIDCCredentials authState) {
+    public synchronized void write(final OIDCCredentials authState) throws JSONException {


I don't like the name "write". I think save is more descriptive. Also if we wrap the JSONExceptions above we aren't "leaking" it here.

secondsun · 2018-01-31T14:46:17Z

auth/src/test/java/org/aerogear/auth/credentials/OIDCCredentialsTest.java

+        OIDCCredentials oidcCredentials = new OIDCCredentials();
+        assertFalse("Expect Token Validation with Tampered Refresh Token to Fail", oidcCredentials.verifyToken(TAMPERED_REFRESH_TOKEN, VALID_PUBLIC_KEY, VALID_ISSUER, VALID_AUDIENCE));
+    }
+


Could the repeated "OIDCCredentials oidcCredentials = new OIDCCredentials();" be in the setup block?

ziccardi · 2018-01-31T15:26:24Z

auth/src/main/java/org/aerogear/auth/AuthService.java

-    private AuthService(final AuthServiceConfig config) {
+    public AuthService() {}
+
+    public void bootstrap(final MobileCore core, final ServiceConfiguration serviceConfig) {


When is the bootstrap called?
If we don't initialise the object inside the constructor, we should than throw an IllegalStateException if other methods are called before the bootstrap one.

@secondsun Am I correct in saying the core will initialize and then call bootstrap on a ServiceModule?

@aidenkeating Yes. https://github.com/aerogear/proposals/blob/master/android-sdk/README.md#starting-up-mobile-core

ziccardi · 2018-01-31T15:28:19Z

auth/src/main/java/org/aerogear/auth/credentials/IntegrityCheckParameters.java

+
+    public IntegrityCheckParameters() {}
+
+    public String getIssuer() {


Add some documentation here. What is the issuer?

ziccardi · 2018-01-31T15:28:48Z

auth/src/main/java/org/aerogear/auth/credentials/IntegrityCheckParameters.java

+        return this.audience;
+    }
+
+    public String getPublicKey() { return this.publicKey; }


What is this public key? What is the format it is returned? It is a DSA? RSA? or what?

ziccardi · 2018-01-31T15:29:44Z

auth/src/main/java/org/aerogear/auth/credentials/IntegrityCheckParameters.java

+            .toString();
+    }
+
+    public static IntegrityCheckParameters deserialize(final String serializedParams) throws JSONException {


Is this object supposed to be constructed only through this method? If yes, the constructor should be private

No this will need to be constructed through the constructor when initially created at authentication time.

ziccardi · 2018-01-31T15:31:18Z

auth/src/main/java/org/aerogear/auth/credentials/IntegrityCheckParameters.java

+            .toString();
+    }
+
+    public static IntegrityCheckParameters deserialize(final String serializedParams) throws JSONException {


A null check should be added here

ziccardi · 2018-01-31T15:43:00Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+                .setRequireSubject() // require the subject claim
+                .setExpectedIssuer(issuer) // whom the JWT needs to have been issued by
+                .setExpectedAudience(audience) // to whom the JWT is intended for
+                .setVerificationKey(jwtPublicKey) // verify the signature with the public key


is it ok to have a null jwtPublicKey here? If it is not, make it final and do not assign null to it.

final PublicKey jwtPublicKey; try { jwtPublicKey = utils.fromPemEncoded(constructedPublicKey); } catch (JoseException e) {

ziccardi · 2018-01-31T15:44:04Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+            JwtClaims jwtClaims = jwtConsumer.processToClaims(jwtToken);
+            return true;
+        } catch (InvalidJwtException e) {
+            e.printStackTrace();


If we can safely return false here, it would be better to use logger to log exceptions.

ziccardi · 2018-01-31T15:46:05Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

-    public String serialise() {
-        return this.authState.jsonSerializeString();
+    public String serialize() throws JSONException {
+        JSONObject jsonCredential = new JSONObject()


Every time a variable is not supposed to be assigned again, it should be declared as final.

ziccardi · 2018-01-31T15:47:26Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java

+        return jsonCredential.toString();
+    }
+
+    public static OIDCCredentials deserialize(final String serializedCredential) throws JSONException {


If this object is supposed to be created only through this method, the constructor should be private.

ziccardi · 2018-01-31T15:48:31Z

auth/src/main/java/org/aerogear/auth/utils/AuthStateManager.java

        if (authState == null) {
            clear();
        } else {
-            if(!prefs.edit().putString(KEY_STATE, authState.serialise()).commit()) {
+            if(!prefs.edit().putString(KEY_STATE, authState.serialize()).commit()) {
                throw new IllegalStateException("Failed to update state from shared preferences");


Is it correct to throw an unchecked exception here?

@ziccardi Yes.

However, we should add @throws to the javadocs

secondsun · 2018-01-31T20:34:12Z

auth/build.gradle

    testImplementation 'org.mockito:mockito-core:2.10.0'
    testImplementation 'junit:junit:4.12'
+    testImplementation "org.robolectric:robolectric:3.6.1"
+    compile 'org.bitbucket.b_c:jose4j:0.6.3'


compile is deprecated. replace with implementation and move above the test dependencies. Also can you include the dependency in the android bom https://github.com/aerogear/aerogear-parent/blob/master/aerogear-android-sdk-bom/pom.xml, passos is working on moving this to use a more managed system.

secondsun · 2018-01-31T20:34:44Z

auth/src/main/java/org/aerogear/auth/AbstractAuthenticator.java

@@ -1,6 +1,7 @@
 package org.aerogear.auth;


@ziccardi Yes

secondsun · 2018-01-31T20:35:36Z

auth/src/main/java/org/aerogear/auth/credentials/IIntegrityCheckParameters.java

+    String getIssuer();
+    String getPublicKey();
+    boolean isValid();
+    String serialize() throws JSONException;


nix the exception

secondsun · 2018-01-31T20:36:12Z

auth/src/main/java/org/aerogear/auth/credentials/IntegrityCheckParameters.java

+     */
+    public String getPublicKey() { return this.publicKey; }
+
+    public String serialize() throws JSONException {


This code will never throw a JSON exception, best not to declare it.

secondsun · 2018-01-31T20:39:39Z

auth/src/main/java/org/aerogear/auth/credentials/IntegrityCheckParameters.java

+     * Return json representation of the parameters
+     * @return json string representation of parameters
+     */
+    public static IntegrityCheckParameters deserialize(final String serializedParams) throws JSONException {


Kill the exception.

If you have to throw a checked exception (pro tip, you don't), make it more specific to the task at hand. For instance if there is no "issuer" property on the object then throwing as JSON exception is less useful than throwing a new IllegalArgumentException with a useful message.

Also, add that the object needs to be a json object with those parameters in the header.

Also also, if all we are doing is storing this in shared preferences can we make it a parcelable instead?

@secondsun I'm not sure I understand how parcelable would help here?

secondsun · 2018-01-31T20:40:07Z

auth/src/main/java/org/aerogear/auth/credentials/OIDCCredentials.java


-    public OIDCCredentials() {
-        this.authState = new AuthState();
+    public OIDCCredentials(final String serialisedCredential, final IIntegrityCheckParameters integrityCheckParameters) throws JSONException {


Kill the JSONException. Add javadocs.

secondsun · 2018-01-31T20:42:18Z

auth/src/main/java/org/aerogear/auth/utils/AuthStateManager.java

        if (authState == null) {
            clear();
        } else {
-            if(!prefs.edit().putString(KEY_STATE, authState.serialise()).commit()) {
+            if(!prefs.edit().putString(KEY_STATE, authState.serialize()).commit()) {
                throw new IllegalStateException("Failed to update state from shared preferences");


However, we should add @throws to the javadocs

aidenkeating · 2018-02-01T13:08:09Z

@ziccardi @secondsun Mind taking another look?

ziccardi · 2018-02-05T09:45:16Z

auth/src/main/java/org/aerogear/android/ags/auth/AbstractAuthenticator.java

-    protected AuthServiceConfig getConfig() {
-        return config;
-    }
+    public ServiceConfiguration getServiceConfig() { return this.serviceConfig; }


Is ServiceConfiguration an immutable object?
If it is not, different threads could change the content of the ServiceConfiguration object, thus changing the internal state of the Authenticator without any control by the latter, making it crash prone.

Not too familiar with any core stuff. @secondsun would you have an answer to this?

ServiceConfiguration is meant to be immutable. If it isn't then feel free to make it so.

Have made this change in a separate PR. Don't want this one to bloat much more. https://github.com/aerogear/aerogear-android-sdk/pull/24/files

ziccardi · 2018-02-05T09:47:52Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/IntegrityCheckParameters.java

+
+public class IntegrityCheckParameters implements IIntegrityCheckParameters {
+
+    private String issuer;


Make these final

ziccardi · 2018-02-05T09:49:35Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/OIDCCredentials.java

+     * @return <code>true</code> if the token integrity is good.
+     */
+    public boolean verifyToken(final String jwtToken) {
+        final String issuer = integrityCheckParameters.getIssuer();


There is no added value in using variables to store the values here.

ziccardi · 2018-02-05T09:50:50Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/OIDCCredentials.java

+            jwtPublicKey = utils.fromPemEncoded(constructedPublicKey);
+        } catch (JoseException e) {
+            Log.e(TAG, e.getMessage(), e);
+            throw new RuntimeException(e);


Please use a more specific exception. In this case, if the parameters are not correct, an IllegalArgumentException could be the right one.

ziccardi · 2018-02-05T09:51:46Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/OIDCCredentials.java

+            }
+            return jsonCredential.toString();
+        } catch(JSONException e) {
+            throw new RuntimeException(e);


See previous comment

ziccardi · 2018-02-05T09:51:56Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/OIDCCredentials.java

+            final IntegrityCheckParameters icParams = IntegrityCheckParameters.deserialize(serializedIntegrityChecks);
+            return new OIDCCredentials(serializedAuthState, icParams);
+        } catch(JSONException e) {
+            throw new RuntimeException(e);


See previous comment

secondsun

Looking better, I found a stub method and most of my comments are minor doc and annotation tweaks.

secondsun · 2018-02-05T15:51:21Z

auth/build.gradle

    testImplementation 'org.mockito:mockito-core:2.10.0'
    testImplementation 'junit:junit:4.12'
+    testImplementation "org.robolectric:robolectric:3.6.1"


You'll need to rebase on master and add these dependencies to the aerogear-android-sdk-bom. @danielpassos Can show you how.

secondsun · 2018-02-05T15:52:31Z

auth/src/main/java/org/aerogear/android/ags/auth/AbstractAuthenticator.java

-    protected AuthServiceConfig getConfig() {
-        return config;
-    }
+    public ServiceConfiguration getServiceConfig() { return this.serviceConfig; }


ServiceConfiguration is meant to be immutable. If it isn't then feel free to make it so.

secondsun · 2018-02-05T15:53:52Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/IIntegrityCheckParameters.java

+    String getIssuer();
+    String getPublicKey();
+    boolean isValid();
+    String serialize();


Do you think JavaDocs are needed? The properties and things look self explanatory to me, but if there are any weirdnesses or edge cases to be aware of (like what does isValid check to validate) then I would want them added.

I think you're right about isValid requiring some docs here. For the others I don't think so, they're self explanatory I think.

secondsun · 2018-02-05T15:54:36Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/IntegrityCheckParameters.java

+import org.json.JSONException;
+import org.json.JSONObject;
+
+public class IntegrityCheckParameters implements IIntegrityCheckParameters {


For the methods you implement you need to add the @Override annotation.

secondsun · 2018-02-05T15:55:00Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/IntegrityCheckParameters.java

+
+    /**
+     * Return json representation of the parameters
+     * @return json string representation of parameters


Add @throws to describe why the runtime exception may be thrown.

secondsun · 2018-02-05T15:55:22Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/OIDCCredentials.java

+/**
+ * Credentials for OIDC based authentication
+ */
+public class OIDCCredentials implements ICredential {


Add @Override where appropriate.

ICredential is just empty at the moment.

secondsun · 2018-02-05T15:55:49Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/OIDCCredentials.java

+     * @param serialisedCredential JSON string representation of the authState field produced by
+     *                             {@link #deserialize(String)}.
+     * @param integrityCheckParameters Integrity check parameters for the token.
+     */


Add @throws to describe the runtime exception to the javadoc

secondsun · 2018-02-05T15:56:14Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/OIDCCredentials.java

+        }
+    }
+
+    public static OIDCCredentials deserialize(final String serializedCredential) {


secondsun · 2018-02-05T15:56:32Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/OIDCCredentials.java

+     * @throws AuthenticationException
+     */
+    public boolean renew() throws AuthenticationException {
+        throw new IllegalStateException("Not yet implemented");


Need implementation :)

Not sure why this is showing up as changed, but the renew was there before this PR and I think it's outside the scope of this PR. I think its implementation is kind of dependent on knowing how login will work.

secondsun · 2018-02-05T15:57:26Z

auth/src/main/java/org/aerogear/android/ags/auth/utils/AuthStateManager.java

    }

    /**
     * Saves a token
     * @param authState token to be saved
+     * @throws IllegalArgumentException


Shouldn't this be illegal state exception?

aidenkeating · 2018-02-06T11:46:35Z

@secondsun Dependencies were added in this PR, waiting on a new release now. Have made a temporary commit that will fail so I don't forget

rachael-oregan · 2018-02-06T13:34:45Z

auth/src/main/java/org/aerogear/android/ags/auth/utils/AuthStateManager.java

    private static final String STORE_NAME = "org.aerogear.android.auth.AuthState";
    private static final String KEY_STATE = "state";

    private final SharedPreferences prefs;

-    public AuthStateManager(final Context context) {
+    private AuthStateManager(final Context context) {


I'm not sure if Singleton's are suppose to have parameters? This way couldn't the singleton be instantiated multiple times which defeats the purpose?

Private constructor and prefs is a final variable. IN this case it isn't bad because the references set up aren't mutable. However I am generally -1 to static singletons because they have annoying behaviors when you are testing that you have to code around.

aidenkeating · 2018-02-06T14:25:12Z

auth/src/main/java/org/aerogear/android/ags/auth/utils/AuthStateManager.java

     */
    public synchronized void clear() {
        if (!prefs.edit().remove(KEY_STATE).commit()) {
            throw new IllegalStateException("Failed to clear state from shared preferences");
        }
    }
+
+    public static AuthStateManager getInstance(final Context context) {


@rachael-oregan To respond to this comment 260a5b3#r166298337 yeah I'm not sure the best way to approach this. I don't think providing a 'setter' of any kind is a good idea as then context could change if the setter is called multiple times. This can't be initialized multiple times but I can see the awkwardness of it.

This way context is bound to whatever is provided first, instead of being able to be set multiple times.

There's also the option of providing context on each function call I guess.

Perhaps @ziccardi or @secondsun have a more informed view on how to approach this.

@aidenkeating Just a proposal: we could add a bootstrap method with package level visibility. It could then be called from the bootstrap method of the AuthService. This way other classes will see only a getInstance with no parameters. WDYT?

secondsun · 2018-02-06T15:04:43Z

auth/src/main/java/org/aerogear/android/ags/auth/AuthService.java

@@ -75,7 +79,7 @@ public void setAuthenticatorChain(AuthenticationChain newChain) {
    public static synchronized AuthService getInstance() {
        if (INSTANCE == null) {
            // FIXME: load the configurations from core and pass it here
-            INSTANCE = new AuthService(null);
+            INSTANCE = new AuthService();


We should NOT be using singletons. References to services should be fetch through the mobile core and not plucked from the ether.

See https://github.com/aerogear/proposals/blob/master/android-sdk/README.md#referencing-a-service

Yeah I don't think this is used anyway, doesn't seem to be referenced anywhere. Must have been left over. Will remove

aidenkeating · 2018-02-06T16:08:06Z

@secondsun @ziccardi Removed unused singleton stuff and updated the android sdk bom stuff. Tests have started to pass again.

aidenkeating · 2018-02-07T15:57:58Z

@secondsun Would you mind taking a look? One question, we currently have the auth module typed as keycloak because it matches up with the actual keycloak service. We also have a keycloak module in this SDK. I'm guessing them both being typed as keycloak is a bad thing? Should we modify the keycloak module to something else or remove it altogether as it will be replaced with the auth service?

danielpassos · 2018-02-07T16:42:47Z

@aidenkeating Keycloak Service is a temporary one, It was to help us in the core PoC and going to be removed

aidenkeating · 2018-02-08T08:55:42Z

@danielpassos @ziccardi @secondsun Would you mind taking a look? I think this is ready to merge

ziccardi

Just a couple of comments

ziccardi · 2018-02-08T09:20:23Z

auth/src/main/java/org/aerogear/android/ags/auth/credentials/OIDCCredentials.java

+     * @return <code>true</code> if user is authorized and token is not expired.
+     */
+    public boolean checkValidAuth() {
+        return isAuthorized() && getNeedsRenewal();


Shouldn't be return isAuthorized() && !getNeedsRenewal(); ?

ziccardi · 2018-02-08T09:23:51Z

auth/src/main/java/org/aerogear/android/ags/auth/utils/AuthStateManager.java

     */
    public synchronized void clear() {
        if (!prefs.edit().remove(KEY_STATE).commit()) {
            throw new IllegalStateException("Failed to clear state from shared preferences");
        }
    }
+
+    public static AuthStateManager getInstance(final Context context) {


@aidenkeating Just a proposal: we could add a bootstrap method with package level visibility. It could then be called from the bootstrap method of the AuthService. This way other classes will see only a getInstance with no parameters. WDYT?

aidenkeating · 2018-02-08T11:54:47Z

@ziccardi @danielpassos Mind taking a look? Have move the AuthStateManager out to the auth package. Have also included an init(Context) function in the AuthService to allow the user to provide the context. This is following the example used in this PR https://github.com/aerogear/aerogear-android-sdk/pull/27/files#diff-7037942ee1f06907ac7d09f6d65ccd6dR88

rachael-oregan · 2018-02-08T13:58:53Z

auth/src/main/java/org/aerogear/android/ags/auth/AuthService.java

-     * Authentication service singleton.
-     */
-    private static AuthService INSTANCE;
+public class AuthService implements ServiceModule {


I presume this won't compile because its not implementing the methods from ServiceModule right?

Also how is the AuthService object got now?

It is. Well CircleCI tests are passing. Jenkins is failing because the gradle daemon decided to go ahead and stop.

The ServiceModule interface has changed in this PR #22

Ah right cool, thanks for explaining

aidenkeating · 2018-02-08T16:52:31Z

@wtrocki Would you mind taking a look? I think this is ready to merge. Don't want this PR to bloat too much more. Jenkins appears to be failing because of an internal issue

Changes were applied

wtrocki

LGTM. Adjustments can be done later stage

wtrocki · 2018-02-08T18:00:19Z

I have spent some time checking changes on local machine.
PR recieved pretty good feedback. Considering that this changes cannot be properly tested until we get login implementation I think it's best to get that into master and follow up with other PR's.

Really good job @aidenkeating

danielpassos · 2018-02-09T01:00:14Z

I didn't have time to review it, I was about to start now but it's already merged. I didn't test it but I have 2 comments about this PR:

Correct or not all other modules are using org.aerogear.mobile.[module-name] as the package
I think all method who going to throw the exception needs to have that explicit in the method not only in the Javadoc.

aidenkeating · 2018-02-09T08:43:03Z

@danielpassos The package name was to address this comment #21 (comment) and this proposal https://github.com/aerogear/proposals/blob/master/sdks/SDK-naming.md#proposed-solution

ziccardi suggested changes Jan 31, 2018

View reviewed changes

secondsun suggested changes Jan 31, 2018

View reviewed changes

ziccardi suggested changes Jan 31, 2018

View reviewed changes

secondsun suggested changes Jan 31, 2018

View reviewed changes

ziccardi suggested changes Feb 5, 2018

View reviewed changes

aidenkeating force-pushed the AGDROID-686 branch from cfd6483 to a8ea23c Compare February 5, 2018 11:53

secondsun suggested changes Feb 5, 2018

View reviewed changes

rachael-oregan mentioned this pull request Feb 6, 2018

[WIP] AGDROID-681 - logout implementation #25

Merged

3 tasks

aidenkeating force-pushed the AGDROID-686 branch from 7bae221 to b22559e Compare February 6, 2018 11:25

aidenkeating force-pushed the AGDROID-686 branch from 42a2d13 to 33d9f97 Compare February 6, 2018 11:56

rachael-oregan reviewed Feb 6, 2018

View reviewed changes

aidenkeating force-pushed the AGDROID-686 branch from 33d9f97 to 3abae5d Compare February 6, 2018 14:16

aidenkeating commented Feb 6, 2018

View reviewed changes

secondsun previously requested changes Feb 6, 2018

View reviewed changes

aidenkeating force-pushed the AGDROID-686 branch from 08467bd to 8456248 Compare February 6, 2018 15:48

aidenkeating changed the title ~~[WIP] AGDROID-686 Token integrity check~~ AGDROID-686 Token integrity check Feb 6, 2018

wei-lee mentioned this pull request Feb 7, 2018

AGDROID-686 - Add Token Verification [Do No Merge] #12

Closed

4 tasks

aidenkeating force-pushed the AGDROID-686 branch 2 times, most recently from a67a626 to 252808c Compare February 7, 2018 15:53

AGDROID-711 Pass integrity check params to credentials

8329540

tomjackman and others added 14 commits February 8, 2018 08:56

add token verification starter code

88a98fd

improve log messages

f5008ca

add unit tests

ef67979

Use integrity parameters for integrity check

9eeba6a

AGDROID-686 Make parameters final

1ce22ca

AGDROID-686 Dont leak JSONExceptions

5c98652

AGDROID-686 Move to org.aerogear.android.ags.auth package

c1364a7

AGDROID-686 Use appropriate exceptions

ccf8a60

AGDROID-686 Make AuthStateManager a singleton

0ab3283

AGDROID-686 Update docs

fa37ada

AGDROID-686 Document IIntegrityCheckParameters isValid

81f922b

AGDROID-686 Update android sdk bom version

6f82033

AGDROID-686 Remove unused singleton stuff

5510340

AGDROID-686 Address core changes

63ad9bb

aidenkeating force-pushed the AGDROID-686 branch from 252808c to 63ad9bb Compare February 8, 2018 08:57

ziccardi previously requested changes Feb 8, 2018

View reviewed changes

Aiden Keating added 2 commits February 8, 2018 09:32

AGDROID-686 Fix checkAuthStatus

ee5ad56

AGDROID-686 Retrieve context using an init method

9afbbeb

rachael-oregan reviewed Feb 8, 2018

View reviewed changes

wtrocki requested a review from JameelB February 8, 2018 16:57

wtrocki approved these changes Feb 8, 2018

View reviewed changes

wtrocki merged commit b71d208 into aerogear:master Feb 8, 2018


		public IntegrityCheckParameters() {}

		public String getIssuer() {


		public class IntegrityCheckParameters implements IIntegrityCheckParameters {

		private String issuer;

AGDROID-686 Token integrity check #21

AGDROID-686 Token integrity check #21

Conversation

aidenkeating commented Jan 31, 2018 • edited

Motivation

Description

Progress

aidenkeating commented Jan 31, 2018

ziccardi commented Jan 31, 2018

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

secondsun commented Jan 31, 2018

aidenkeating commented Jan 31, 2018

secondsun left a 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

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

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

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

aidenkeating commented Feb 1, 2018 • edited

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

secondsun left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aidenkeating commented Jan 31, 2018 •

edited

aidenkeating commented Feb 1, 2018 •

edited