Skip to content
This repository was archived by the owner on Dec 12, 2018. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@
*
* @since 1.1.0
*/
public interface GoogleAuthenticatorChallenge extends Challenge<GoogleAuthenticatorFactor,GoogleAuthenticatorChallengeStatus>{
public interface GoogleAuthenticatorChallenge extends Challenge<GoogleAuthenticatorFactor, GoogleAuthenticatorChallengeStatus> {

/**
* A GoogleAuthenticatorChallenge can be validated at the same time the challenge is created by setting the code on the challenge.
*
* @param code the code the validated at the same time the GoogleAuthenticatorChallenge is created
* @since 1.4.0
*/
void setCode(String code);
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public final class GoogleAuthenticatorChallenges extends Challenges {
}

private static final Class<CreateChallengeRequestBuilder> BUILDER_CLASS =
Classes.forName("com.stormpath.sdk.impl.challenge.DefaultCreateChallengeRequestBuilder");
Classes.forName("com.stormpath.sdk.impl.challenge.google.DefaultGoogleAuthenticatorCreateChallengeRequestBuilder");

//Prevent instantiation outside of outer class.
//Use getInstance() to retrieve the singleton instance.
Expand Down Expand Up @@ -152,9 +152,9 @@ public static EqualsExpressionFactory status() {
*
* @since 1.1.0
*/
public static CreateChallengeRequestBuilder<GoogleAuthenticatorChallenge> newCreateRequestFor(GoogleAuthenticatorChallenge challenge) {
Constructor ctor = Classes.getConstructor(BUILDER_CLASS, Challenge.class);
return (CreateChallengeRequestBuilder<GoogleAuthenticatorChallenge>) Classes.instantiate(ctor, challenge);
public static GoogleAuthenticatorCreateChallengeRequestBuilder newCreateRequestFor(GoogleAuthenticatorChallenge challenge) {
Constructor ctor = Classes.getConstructor(BUILDER_CLASS, GoogleAuthenticatorChallenge.class);
return (GoogleAuthenticatorCreateChallengeRequestBuilder) Classes.instantiate(ctor, challenge);
}

private static EqualsExpressionFactory newEqualsExpressionFactory(String propName) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright 2017 Stormpath, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.stormpath.sdk.challenge.google;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing license header

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed.


import com.stormpath.sdk.challenge.CreateChallengeRequest;

/**
* A Google Authenticator specific create challenge request
*
* Google Authenticator challenges can validate a code at the same time that the challenge is created.
*
* @since 1.4.0
*/
public interface GoogleAuthenticatorCreateChallengeRequest extends CreateChallengeRequest {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright 2017 Stormpath, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.stormpath.sdk.challenge.google;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing license

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed


import com.stormpath.sdk.challenge.CreateChallengeRequestBuilder;

/**
* A builder to construct {@link GoogleAuthenticatorCreateChallengeRequest}s Google Authenticator specific create challenge requests.
*
* Google Authenticator can both create the challenge and set the code in one API call.
* See {@link com.stormpath.sdk.factor.google.GoogleAuthenticatorFactor}
*
* @since 1.4.0
*/
public interface GoogleAuthenticatorCreateChallengeRequestBuilder extends CreateChallengeRequestBuilder<GoogleAuthenticatorChallenge> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this class still needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but, maybe not - looking


/**
* @param code to be used to validate the challenge for the {@link com.stormpath.sdk.factor.google.GoogleAuthenticatorFactor}
* @return GoogleAuthenticatorCreateChallengeRequestBuilder for method chaining with the builder pattern
*/
GoogleAuthenticatorCreateChallengeRequestBuilder withCode(String code);
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,16 @@
* A {@code GoogleAuthenticatorFactor} is a Factor that represents a two-step verification services using
* the Time-based One-time Password Algorithm (TOTP) and HMAC-based One-time Password Algorithm (HOTP),
* for authenticating users of mobile applications by Google.
* When issuing a challenge via an GoogleAuthenticatorFactor, a code is sent to the Google Authenticator app,
* and the user can enter the received code back into the system to verify/complete the challenge.
* <p/>
* The TOTP algorithm determines the next valid code without the requirement for any communication between server and
* TOTP client.
* <p/>
* As such, the challenge can be created with the code at the same time:
* <p/>
*
* <pre>
* GoogleAuthenticatorChallenge challenge = googleAuthenticatorFactor.createChallenge(code);
* </pre>
*
* @since 1.1.0
*/
Expand Down Expand Up @@ -80,4 +88,13 @@ public interface GoogleAuthenticatorFactor<T extends GoogleAuthenticatorChalleng
* @return the base64QRImage.
*/
String getBase64QrImage();

/**
*
* @param code The code to validate the challenge that is created. With Google Authenticator, you can create a
* challenge and validate it all in one call.
* @return the {@link GoogleAuthenticatorChallenge}
* @since 1.4.0
*/
GoogleAuthenticatorChallenge createChallenge(String code);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import static org.testng.Assert.assertNotNull
class GoogleAuthenticatorChallengeIT extends AbstractMultiFactorIT {

@Test
void testSuccessfulGoogleAuthenticatorChallenge() {
void testSuccessfulGoogleAuthenticatorChallengeWithCode() {
Directory dir = client.instantiate(Directory)
dir.name = uniquify("Java SDK: ${this.getClass().getSimpleName()}.${new Object(){}.getClass().getEnclosingMethod().getName()}")
dir = client.currentTenant.createDirectory(dir);
Expand All @@ -60,6 +60,24 @@ class GoogleAuthenticatorChallengeIT extends AbstractMultiFactorIT {
assertGoogleAuthenticatorChallengeResponse(factor, getCurrentValidCode(factor), 'SUCCESS')
}

@Test
void testSuccessfulGoogleAuthenticatorChallengeTwoStep() {
Directory dir = client.instantiate(Directory)
dir.name = uniquify("Java SDK: ${this.getClass().getSimpleName()}.${new Object(){}.getClass().getEnclosingMethod().getName()}")
dir = client.currentTenant.createDirectory(dir);
deleteOnTeardown(dir)
Account account = createTempAccountInDir(dir)

def randomAccountName = uniquify("Random Account Name")
def randomIssuer = uniquify("Random Issuer")
def factor = createGoogleAuthenticatorFactor(account, randomIssuer, randomAccountName)
assertGoogleAuthenticatorFactorFields(factor, randomIssuer, randomAccountName)

sleepToAvoidCrossingThirtySecondMark()

GoogleAuthenticatorChallenge challenge = createChallengeWithCode(factor)
assertTrue challenge.validate(getCurrentValidCode(factor))
}

@Test
void testGoogleAuthenticatorChallengeWithGarbageCode() {
Expand All @@ -75,7 +93,7 @@ class GoogleAuthenticatorChallengeIT extends AbstractMultiFactorIT {

Throwable e = null
try {
createChallenge(factor, "bogus")
createChallengeWithCode(factor, "bogus")
} catch (ResourceException re) {
e = re
assertEquals(re.status, 400)
Expand All @@ -98,7 +116,7 @@ class GoogleAuthenticatorChallengeIT extends AbstractMultiFactorIT {

Throwable e = null
try {
createChallenge(factor, "123456")
createChallengeWithCode(factor, "123456")
} catch (ResourceException re) {
e = re
assertEquals(re.status, 400)
Expand Down Expand Up @@ -127,7 +145,7 @@ class GoogleAuthenticatorChallengeIT extends AbstractMultiFactorIT {

private void assertGoogleAuthenticatorChallengeResponse(GoogleAuthenticatorFactor factor, String code, String status, String verificationStatus = 'VERIFIED') {
String factorHref = factor.href
GoogleAuthenticatorChallenge challenge = createChallenge(factor, code)
GoogleAuthenticatorChallenge challenge = createChallengeWithCode(factor, code)
assertInitialChallengeFields(challenge, status, false)

FactorOptions options = Factors.options().withChallenges().withMostRecentChallenge()
Expand Down Expand Up @@ -182,13 +200,19 @@ class GoogleAuthenticatorChallengeIT extends AbstractMultiFactorIT {
sleep(secondsToWait * 1000)
}

private GoogleAuthenticatorChallenge createChallenge(GoogleAuthenticatorFactor factor, String code = null) {
private GoogleAuthenticatorChallenge createChallengeWithCode(GoogleAuthenticatorFactor factor, String code = null) {
def challenge = client.instantiate(GoogleAuthenticatorChallenge.class)
challenge.setCode(code)
ChallengeOptions options = Challenges.GOOGLE_AUTHENTICATOR.options().withFactor()
return factor.createChallenge(Challenges.GOOGLE_AUTHENTICATOR.newCreateRequestFor(challenge).withResponseOptions(options).build())
}

private GoogleAuthenticatorChallenge createChallengeWithoutCode(GoogleAuthenticatorFactor factor) {
def challenge = client.instantiate(GoogleAuthenticatorChallenge.class)
ChallengeOptions options = Challenges.GOOGLE_AUTHENTICATOR.options().withFactor()
return factor.createChallenge(Challenges.GOOGLE_AUTHENTICATOR.newCreateRequestFor(challenge).withResponseOptions(options).build())
}

private String getCurrentValidCode(GoogleAuthenticatorFactor factor) {
return getCurrentCode(factor, true)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
public abstract class AbstractChallenge<T extends Factor, R extends Enum> extends AbstractInstanceResource implements Challenge<T,R> {

public static final EnumProperty<Enum> STATUS = new EnumProperty<>("status", Enum.class);
static final StringProperty CODE = new StringProperty("code");
public static final StringProperty CODE = new StringProperty("code");
static final ResourceReference<Account> ACCOUNT = new ResourceReference<>("account", Account.class);
static final ResourceReference<? extends Factor> FACTOR = new ResourceReference<>("factor", Factor.class);
public static final DateProperty CREATED_AT = new DateProperty("createdAt");
Expand Down Expand Up @@ -92,16 +92,11 @@ public Challenge setFactor(T factor) {
@Override
public boolean validate(String code) {
Assert.notNull(code, "code can not be null.");
setCode(code);
setProperty(CODE, code);
Challenge returnedChallenge = getDataStore().create(getHref(), this);
if ((returnedChallenge.getStatus()).name().equals("SUCCESS")) {
return true;
}
return false;
}

protected Challenge setCode(String code) {
setProperty(CODE, code);
return this;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
/**
* @since 1.1.0
*/
public class DefaultCreateChallengeRequestBuilder<T extends Challenge> implements CreateChallengeRequestBuilder {
public class DefaultCreateChallengeRequestBuilder<T extends Challenge> implements CreateChallengeRequestBuilder<T> {

private final T challenge;
private ChallengeOptions options;
protected final T challenge;
protected ChallengeOptions options;

public DefaultCreateChallengeRequestBuilder(T challenge) {
Assert.notNull(challenge, "Challenge can't be null.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
/**
* @since 1.1.0
*/
public class DefaultGoogleAuthenticatorChallenge extends AbstractChallenge<GoogleAuthenticatorFactor, GoogleAuthenticatorChallengeStatus> implements GoogleAuthenticatorChallenge{
public class DefaultGoogleAuthenticatorChallenge extends AbstractChallenge<GoogleAuthenticatorFactor, GoogleAuthenticatorChallengeStatus> implements GoogleAuthenticatorChallenge {

static final Map<String, Property> PROPERTY_DESCRIPTORS = AbstractChallenge.PROPERTY_DESCRIPTORS;

Expand All @@ -53,4 +53,9 @@ public GoogleAuthenticatorChallengeStatus getStatus() {
}
return GoogleAuthenticatorChallengeStatus.valueOf(value.toUpperCase());
}

@Override
public void setCode(String code) {
setProperty(CODE, code);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@mrafiei mrafiei Jan 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright 2017 Stormpath, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.stormpath.sdk.impl.challenge.google;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing license

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed


import com.stormpath.sdk.challenge.Challenge;
import com.stormpath.sdk.challenge.ChallengeOptions;
import com.stormpath.sdk.challenge.google.GoogleAuthenticatorCreateChallengeRequest;
import com.stormpath.sdk.impl.challenge.DefaultCreateChallengeRequest;

/**
* @since 1.4.0
*/
public class DefaultGoogleAuthenticatorCreateChallengeRequest extends DefaultCreateChallengeRequest implements GoogleAuthenticatorCreateChallengeRequest {

public DefaultGoogleAuthenticatorCreateChallengeRequest(Challenge challenge, ChallengeOptions options) {
super(challenge, options);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Copyright 2017 Stormpath, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.stormpath.sdk.impl.challenge.google;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing license

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adressed


import com.stormpath.sdk.challenge.google.GoogleAuthenticatorCreateChallengeRequest;
import com.stormpath.sdk.challenge.google.GoogleAuthenticatorChallenge;
import com.stormpath.sdk.challenge.google.GoogleAuthenticatorCreateChallengeRequestBuilder;
import com.stormpath.sdk.impl.challenge.DefaultCreateChallengeRequestBuilder;

/**
* @since 1.4.0
*/
public class DefaultGoogleAuthenticatorCreateChallengeRequestBuilder extends DefaultCreateChallengeRequestBuilder<GoogleAuthenticatorChallenge> implements GoogleAuthenticatorCreateChallengeRequestBuilder {

public DefaultGoogleAuthenticatorCreateChallengeRequestBuilder(GoogleAuthenticatorChallenge challenge) {
super(challenge);
}

@Override
public GoogleAuthenticatorCreateChallengeRequestBuilder withCode(String code) {
challenge.setCode(code);
return this;
}

@Override
public GoogleAuthenticatorCreateChallengeRequest build() {
return new DefaultGoogleAuthenticatorCreateChallengeRequest(challenge, options);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.stormpath.sdk.challenge.google.GoogleAuthenticatorChallenge;
import com.stormpath.sdk.factor.FactorType;
import com.stormpath.sdk.factor.google.GoogleAuthenticatorFactor;
import com.stormpath.sdk.impl.challenge.google.DefaultGoogleAuthenticatorChallenge;
import com.stormpath.sdk.impl.ds.InternalDataStore;
import com.stormpath.sdk.impl.factor.AbstractFactor;
import com.stormpath.sdk.impl.resource.Property;
Expand Down Expand Up @@ -89,6 +90,13 @@ public String getBase64QrImage() {
return getString(BASE64_QR_IMAGE);
}

@Override
public GoogleAuthenticatorChallenge createChallenge(String code) {
GoogleAuthenticatorChallenge challenge = new DefaultGoogleAuthenticatorChallenge(getDataStore());
challenge.setCode(code);
return createChallenge(challenge);
}

protected FactorType getConcreteFactorType() {
return FactorType.GOOGLE_AUTHENTICATOR;
}
Expand Down