1211 Support stormpath_factor_challenge grant type; expose more info … #1214

palimpsestor · 2017-01-11T02:17:38Z

resolves #1211

…from OAuthExceptions.

dogeared · 2017-01-13T04:19:25Z

api/src/main/java/com/stormpath/sdk/oauth/Authenticators.java

+    /**
+     * Constructs {@link OAuthStormpathFactorChallengeGrantRequestAuthenticator}s.
+     *
+     * @since 1.1.0


dogeared · 2017-01-13T04:20:23Z

seems like a few new classes/interfaces in here. Add some new tests?

…ing, too.

mrioan

@palimpsestor I provided a few comments. Back to you

mrioan · 2017-01-14T00:36:07Z

extensions/httpclient/src/test/groovy/com/stormpath/sdk/impl/application/ApplicationIT.groovy

+        catch (ResourceException re) {
+            assertEquals(re.getStatus(), 400)
+            assertEquals(re.getCode(), 13104)
+            assertEquals("The code submitted is not valid.", re.getDeveloperMessage())


@palimpsestor We do not assert messages any longer, doing that has caused tests to begin to fail out of the blue since the the backend sometimes change them. Code and Status must be asserted since they are part of the "contract"

Removed that assertion.

mrioan · 2017-01-14T00:40:57Z

extensions/httpclient/src/test/groovy/com/stormpath/sdk/impl/application/ApplicationIT.groovy

+    private static final int KEY_MODULUS = (int) Math.pow(10, CODE_DIGITS);
+    private static final int CODE_DIGITS = 6;
+
+    private static int calculateCode(byte[] key, long tm) {


@palimpsestor Could you please add a comment here explaining what this function does? It seems that eventually we might need to reduce this logic but it will be difficult to achieve that without first understanding what this does. Thanks!

Added a comment, and went ahead and simplified this method a bit. This is roughly identical to what we use on the IAM side when verifying a challenge to a Google Authenticator factor.

mrioan · 2017-01-14T00:44:08Z

extensions/servlet/src/main/java/com/stormpath/sdk/servlet/filter/oauth/OAuthException.java

+    private static final ObjectMapper objectMapper;
+
+    static {
+        objectMapper = new ObjectMapper();


Can't this be initialized in line 30?

mrioan · 2017-01-14T00:46:44Z

extensions/servlet/src/main/java/com/stormpath/sdk/servlet/filter/oauth/OAuthException.java

+    }

-        json += "}";
+    public OAuthException(OAuthErrorCode code, Map<String, Object> error, String message) {


Could you please move this constructor to line 59 so all the constructors are together?

mrioan · 2017-01-14T00:55:32Z

extensions/servlet/src/main/java/com/stormpath/sdk/servlet/mvc/AccessTokenController.java

+            if (action instanceof String) {
+                // get action map from error based on the action
+                Map<String, Object> errorMap = new LinkedHashMap<>();
+                errorMap.put("error_description", defaultError.getProperty("error_description"));


Can't all this hardcoded properties being exposed in a single place as constants in order to be referenced here? This will help both avoid typos and avoid issues if any property is changed in the future

mrioan · 2017-01-14T01:04:05Z

...in/java/com/stormpath/sdk/oauth/OAuthStormpathFactorChallengeGrantRequestAuthentication.java

@@ -0,0 +1,14 @@
+package com.stormpath.sdk.oauth;


Missing license header

mrioan · 2017-01-14T01:04:19Z

...ain/java/com/stormpath/sdk/oauth/OAuthStormpathFactorChallengeGrantRequestAuthenticator.java

@@ -0,0 +1,9 @@
+package com.stormpath.sdk.oauth;


Missing license header

mrioan · 2017-01-14T01:04:38Z

...ain/java/com/stormpath/sdk/oauth/OAuthStormpathFactorChallengeGrantRequestAuthenticator.java

+/**
+ * @since 1.3.1
+ */
+public interface OAuthStormpathFactorChallengeGrantRequestAuthenticator extends OAuthRequestAuthenticator<OAuthGrantRequestAuthenticationResult> {


Please add a short description to this interface, developers will be seeing this interface

mrioan · 2017-01-14T01:04:53Z

...a/com/stormpath/sdk/oauth/OAuthStormpathFactorChallengeGrantRequestAuthenticatorFactory.java

@@ -0,0 +1,7 @@
+package com.stormpath.sdk.oauth;


Missing license header

mrioan · 2017-01-14T01:04:59Z

...a/com/stormpath/sdk/oauth/OAuthStormpathFactorChallengeGrantRequestAuthenticatorFactory.java

+/**
+ * @since 1.3.1
+ */
+public interface OAuthStormpathFactorChallengeGrantRequestAuthenticatorFactory extends OAuthRequestAuthenticatorFactory<OAuthStormpathFactorChallengeGrantRequestAuthenticator> {


Please add a short description to this interface, developers will be seeing this interface

…expiring code.

mrioan · 2017-01-14T05:02:15Z

@palimpsestor Travis is failing with this error: NoClassDefFoundError: Could not initialize class com.stormpath.sdk.oauth.Authenticators. It does not look like a legit problem because that class really exists. I will re-execute that job now.

mrioan · 2017-01-14T09:58:15Z

@palimpsestor. Approved. Merging now

1211 Support stormpath_factor_challenge grant type; expose more info …

f78b279

…from OAuthExceptions.

palimpsestor added the in progress label Jan 11, 2017

dogeared force-pushed the master branch from 49948b6 to 57f4fac Compare January 12, 2017 05:39

dogeared added this to the 1.3.1 milestone Jan 12, 2017

dogeared changed the base branch from master to 1.3.x January 12, 2017 21:08

dogeared force-pushed the 1.3.x branch from f694159 to 918f9ec Compare January 12, 2017 21:35

dogeared reviewed Jan 13, 2017

View reviewed changes

Richard Blaylock added 2 commits January 13, 2017 13:33

1211 Add tests for stormpath_factor_challenge grant type, and good th…

0c08883

…ing, too.

Merge branch '1.3.x' into Issue-1211-mfa-policy-support

02f4185

mrioan self-assigned this Jan 14, 2017

1211 Correct @SInCE tag.

c93dc37

mrioan suggested changes Jan 14, 2017

View reviewed changes

mrioan assigned palimpsestor and unassigned mrioan Jan 14, 2017

Richard Blaylock added 12 commits January 13, 2017 17:33

1211 Add (potential) sleep to avoid intermittent test failure due to …

3cfef09

…expiring code.

1211 Clarify names of Google Authentictor-specific tests.

07c9e0d

1211 Avoid asserting error messages.

3721a33

1211 Simplify and clarify TOTP generation method.

872f82b

1211 Combine object mapper declaration and initialization.

e880ae8

1211 Move constructor to live with other constructors.

d8550f3

1211 Make string constants constants.

948d790

1211 Add description for the convertToOAuthException method.

4d17980

1211 Add missing license headers.

44a3f3e

1211 Add description to public interface.

5e1c50f

1211 Add more missing license headers.

5c14a65

1211 Add descriptions to public interfaces.

3530514

1211 Adjust for changed class name.

c8333b1

mrioan approved these changes Jan 14, 2017

View reviewed changes

mrioan merged commit 8c11022 into 1.3.x Jan 14, 2017

mrioan removed the in progress label Jan 14, 2017

mrioan deleted the Issue-1211-mfa-policy-support branch January 14, 2017 10:00

1211 Support stormpath_factor_challenge grant type; expose more info … #1214

1211 Support stormpath_factor_challenge grant type; expose more info … #1214

Uh oh!

Conversation

palimpsestor commented Jan 11, 2017 • edited by mrioan 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!

dogeared commented Jan 13, 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!

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!

Choose a reason for hiding this comment

Uh oh!

mrioan commented Jan 14, 2017

Uh oh!

mrioan commented Jan 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

palimpsestor commented Jan 11, 2017 •

edited by mrioan

Loading