Refactor GoogleAuthenticatorChallenge to allow for setting code along with challenge. #1244

dogeared · 2017-01-17T23:45:06Z

Resolves #1243

… with challenge.

dogeared · 2017-01-17T23:48:47Z

This enables creating a challenge including the code at the same time for GoogleAuthenticator:

        GoogleAuthenticatorChallenge challenge = client.instantiate(GoogleAuthenticatorChallenge.class);
        challenge = (GoogleAuthenticatorChallenge) factor.createChallenge(
            Challenges.GOOGLE_AUTHENTICATOR
                .newCreateRequestFor(challenge)
                .withCode(code)
                .build()
        );

NOTE: I will add the above to the javadocs before deploying. Just wanted to get it built on CI first.

mrafiei · 2017-01-18T00:18:21Z

impl/src/main/java/com/stormpath/sdk/impl/challenge/AbstractChallenge.java

-        setProperty(CODE, code);
-        return this;
-    }
 }


Why did we remove the setCode() method and made the property public instead?

Before this PR, that method was only used in one place in the validate method, and in two places in tests which should not have had access to that protected method anyway. So there was no need for it.
The property, on the other hand, is used both in the validate method and now also when creating a Google Authenticator challenge and providing the code at the same time (which is not allowed in the SMS case).

mrafiei · 2017-01-18T00:28:25Z

...c/main/java/com/stormpath/sdk/impl/challenge/google/DefaultGoogleAuthenticatorChallenge.java

+    @Override
+    public void setCode(String code) {
+        setProperty(CODE, code);
+    }


This method could have stayed in the Abstract Class, no? SmsChallenge also needs to be able to set the code.
Looking at the following snippet in IAM for SmsChallenge
JsonPath challengeResponse = postRestCall(challenge.href + "?expand=factor", [code: code])

See above. There is no reason for a code to be set on an SMS challenge outside of the validate method.

But doesn't the snippet above post to the sms challenge with a code in the body? That means it is supported on the API.

@mrafiei - Previously, the interface didn't support calling setCode for either SMS or GA as it was not public. Based on the way that GA works, it makes sense to set the code at the same time you create the challenge. It doesn't make the same sense for SMS, which is why we moved it into the more specific GA side of things.

What I am saying is that with GA, you can algorithmically know the code, which is how the TOTP app gives it to you, so it makes sense to be able to create the challenge and submit the code all in one shot.

With SMS, you can't know the code (from an end-user perspective) until you've received the text. I will dig into the spec in Jira/Confluence some more, but am I missing something here?

@mrafiei POSTing to a challenge href with a code is indeed supported by the REST API, and that is what is mirrored by the validate method on AbstractChallenge.

@dogeared Its correct what you stated above, my only thing is since it is supported on the REST API we should also support it for Sms challenges on SDK side to be consistent.

mrioan · 2017-01-18T01:42:15Z

.../main/java/com/stormpath/sdk/challenge/google/GoogleAuthenticatorCreateChallengeRequest.java

@@ -0,0 +1,5 @@
+package com.stormpath.sdk.challenge.google;


Missing license header

mrioan · 2017-01-18T01:42:59Z

.../main/java/com/stormpath/sdk/challenge/google/GoogleAuthenticatorCreateChallengeRequest.java

+
+import com.stormpath.sdk.challenge.CreateChallengeRequest;
+
+public interface GoogleAuthenticatorCreateChallengeRequest extends CreateChallengeRequest {}


Missing Javadoc. In particular both a short description and the version are important since it is an interface in the api module

mrioan · 2017-01-18T01:43:07Z

...ava/com/stormpath/sdk/challenge/google/GoogleAuthenticatorCreateChallengeRequestBuilder.java

@@ -0,0 +1,8 @@
+package com.stormpath.sdk.challenge.google;


Missing license

mrioan · 2017-01-18T01:43:58Z

...ava/com/stormpath/sdk/challenge/google/GoogleAuthenticatorCreateChallengeRequestBuilder.java

+
+import com.stormpath.sdk.challenge.CreateChallengeRequestBuilder;
+
+public interface GoogleAuthenticatorCreateChallengeRequestBuilder  extends CreateChallengeRequestBuilder<GoogleAuthenticatorChallenge> {


Missing Javadoc in the class and in the method. Both a short description and the version are important since it is an interface in the api module

mrioan · 2017-01-18T01:48:07Z

...om/stormpath/sdk/impl/challenge/google/DefaultGoogleAuthenticatorCreateChallengeRequest.java

@@ -0,0 +1,13 @@
+package com.stormpath.sdk.impl.challenge.google;


Missing license

mrioan · 2017-01-18T01:48:13Z

...mpath/sdk/impl/challenge/google/DefaultGoogleAuthenticatorCreateChallengeRequestBuilder.java

@@ -0,0 +1,24 @@
+package com.stormpath.sdk.impl.challenge.google;


Missing license

mrioan · 2017-01-18T12:51:21Z

@dogeared now that I see that a developer would do:

GoogleAuthenticatorChallenge challenge = client.instantiate(GoogleAuthenticatorChallenge.class);
        challenge = (GoogleAuthenticatorChallenge) factor.createChallenge(
            Challenges.GOOGLE_AUTHENTICATOR
                .newCreateRequestFor(challenge)
                .withCode(code)
                .build()
        );

I does not look good to me. You are creating a challenge and then creating another one passing that challenge. I think the proper sentence should be like this:

GoogleAuthenticatorChallenge challenge = (GoogleAuthenticatorChallenge) factor.createChallenge(
            Challenges.GOOGLE_AUTHENTICATOR
                .withCode(code)
                .build()
        );

See that the operation createChallenge already knows that it will create a challenge and it is receiving a google code as a parameter.

dogeared · 2017-01-18T14:00:34Z

@mrioan - I agree. However, this pattern is all throughout MFA code and will require some more refactoring to have the change be more systemic. I will endeavor to update the code you pointed out as the model for other changes and I'll put in issues for those other changes in upcoming releases.

mrioan

@dogeared Two minor comments. When they are addressed you can consider this PR approved

mrioan · 2017-01-18T16:26:08Z

...ava/com/stormpath/sdk/challenge/google/GoogleAuthenticatorCreateChallengeRequestBuilder.java

+ *
+ * @since 1.4.0
+ */
+public interface GoogleAuthenticatorCreateChallengeRequestBuilder  extends CreateChallengeRequestBuilder<GoogleAuthenticatorChallenge> {


is this class still needed?

@mrioan - I think so. There are two supported ways to validate a challenge on the backend - by hitting the .../factors... collection endpoint and by hitting a specific challenge endpoint, so I think we need to support that in the SDK as well.

but, maybe not - looking

mrioan · 2017-01-18T16:26:40Z

api/src/main/java/com/stormpath/sdk/factor/google/GoogleAuthenticatorFactor.java

+     *             challenge and validate it all in one call.
+     * @return the {@link GoogleAuthenticatorChallenge}
+     */
+    GoogleAuthenticatorChallenge createChallenge(String code);


Refactor GoogleAuthenticatorChallenge to allow for setting code along…

e51d1ef

… with challenge.

dogeared added this to the 1.4.0 milestone Jan 17, 2017

dogeared added the in progress label Jan 17, 2017

mrafiei reviewed Jan 18, 2017

View reviewed changes

mrioan suggested changes Jan 18, 2017

View reviewed changes

Richard Blaylock and others added 2 commits January 17, 2017 18:00

Add missing license headers.

9fd6639

Updated javadocs and licenses per @mrioan

499553e

mrafiei approved these changes Jan 18, 2017

View reviewed changes

Added convenience method to create challenge for Google Authenticator.

4f8b706

mrioan suggested changes Jan 18, 2017

View reviewed changes

updated 'since' per @mrioan

5e88bfe

dogeared merged commit a239274 into master Jan 18, 2017

dogeared deleted the GA_Refactor branch January 18, 2017 17:25

dogeared removed the in progress label Jan 18, 2017


		import com.stormpath.sdk.challenge.CreateChallengeRequest;

		public interface GoogleAuthenticatorCreateChallengeRequest extends CreateChallengeRequest {}

		@@ -0,0 +1,13 @@
		package com.stormpath.sdk.impl.challenge.google;

		@@ -0,0 +1,24 @@
		package com.stormpath.sdk.impl.challenge.google;

Refactor GoogleAuthenticatorChallenge to allow for setting code along with challenge. #1244

Refactor GoogleAuthenticatorChallenge to allow for setting code along with challenge. #1244

Uh oh!

Conversation

dogeared commented Jan 17, 2017

Uh oh!

dogeared commented Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrafiei Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrioan commented Jan 18, 2017

Uh oh!

dogeared commented Jan 18, 2017

Uh oh!

mrioan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dogeared commented Jan 17, 2017 •

edited

Loading

mrafiei Jan 18, 2017 •

edited

Loading