initial merge into master #2
Conversation
- Added Shiro + Dropwizard example to help stub out code - NOTE: this branch depends on okta/okta-sdk-java branch `expose-for-authn` - Still more work to be done, currently there are no tests, and I'm not happy with usage of the StateHandler
NOTE: this branch requires the okta/okta-sdk-java branch: expose-for-authn
Renamed OktaAuthenticationStateHandler to ExampleAuthenticationStateHandler (to better show that part of the example)
Added tests too the `-Pci` profile now passes!
the `ci` profile now passes
bdemers
requested review from
robertjd,
bretterer,
jmelberg-okta,
vijetmahabaleshwar-okta and
laura-rodriguez
…casting) This required some groovy test cleanup `object.field` needed to be `object.getField()` as the objects are proper maps, and field accessors do NOT work when objects are maps
fixed references to okta-sdk-root / okta-sdk-java
|
|
||
| The Okta Authentication SDK is a convenience wrapper around [Okta's Authentication API](https://developer.okta.com/docs/api/resources/authn.html). | ||
|
|
||
| **NOTE:** Using an OAuth 2.0 or OIDC to integrate your application instead of this library will require much less work, and has a smaller risk profile. Please see [this guide](https://developer.okta.com/use_cases/authentication/) to see if using this API is right for your use case. You could also use our [Spring Boot Integration](https://github.com/okta/okta-spring-boot), or [Spring Security](https://developer.okta.com/blog/2017/12/18/spring-security-5-oidc) out of the box. |
There was a problem hiding this comment.
Elaborate on risk profile? Sounds kind of scary, what am I getting myself into if I use this SDK?
There was a problem hiding this comment.
That was kind of the goal, we need some canned text around this. @nbarbettini ?
Proxying user passwords, managing those passwords safely etc (which is outside the scope of this library, and directly falls to the developers responsibility to make sure that those requests are NOT logged, and memory is cleaned up after)
|
|
||
| @Override | ||
| public void handleUnknown(AuthenticationResponse unknownResponse) { | ||
| // redirect to "/error" |
There was a problem hiding this comment.
What happens if I don't provide handleUnknown ?
| * @throws AuthenticationException any other authentication related error | ||
| */ | ||
| @ApiReference(path = "/api/v1/authn/credentials/reset_password", href = "https://developer.okta.com/docs/api/resources/authn.html#reset-password") | ||
| AuthenticationResponse resetPassword(char[] newPassword, String stateToken, AuthenticationStateHandler stateHandler) throws AuthenticationException; |
There was a problem hiding this comment.
Expecting to also see resetPassword(ResetPasswordRequest request, ...)
| return doPost("/api/v1/authn/factors/" + factorId + "/lifecycle/resend", toRequest(stateToken), stateHandler); | ||
| } | ||
|
|
||
| @Override |
…nticationStateHandler) Based on review feedback, this keeps the interface consistent with the other non-empty bodied requests
|
|
||
| import com.okta.sdk.resource.Resource; | ||
|
|
||
| public interface VerifyFactorRequest extends Resource { |
There was a problem hiding this comment.
Per conversation earlier, have these extend BaseResource so that I can set arbitrary properties on request bodies (for situations where we extend the API but haven't yet updated the explicit type in this library)
BaseResource was moved to okta-sdk-java as ExtensibleResource
|
|
||
| ## Javadocs | ||
|
|
||
| You can see this project's Javadocs at https://developer.okta.com/okta-authn-sdk-java/ |
There was a problem hiding this comment.
Can you create a JIRA to add this path as a behavior in Cloudfront?
There was a problem hiding this comment.
+1! as soon as the repo is public
There was a problem hiding this comment.
Also - is there a reason we aren't using the path okta-auth-java?
README.md
Outdated
|
|
||
| ## Client configuration | ||
|
|
||
| There are a few ways to configure the client, but the easiest way is to create a `~/.okta/okta.yaml`file and `orgUrl` value: |
There was a problem hiding this comment.
Space between ~/.okta/okta.yaml and file
| ``` yaml | ||
| okta: | ||
| client: | ||
| orgUrl: https://dev-123456.oktapreview.com |
There was a problem hiding this comment.
nit: Lets use our current standard for displaying orgs:
okta:
client:
orgUrl: https://{yourOktaDomain}There was a problem hiding this comment.
I don't think we have a standard for README's yet, all of the SDKs are slightly different currently
There was a problem hiding this comment.
We are actually starting to roll this out. Alex D is wanting us to use the same schema across all of our documentation. Changes into D.O.C (#1997) will be merged this week, followed by updating our SDKs, libs, and sample README documentation.
I'm sure we're ok holding off for now, but seems like we could get a head start now. 👍
There was a problem hiding this comment.
We should talk about this one a big more, as we are NOT replacing these strings like we do on d.o.c. (but either way, we can update the readme once it gets spec'd out, as the release isn't effected by the readme contents)
|
|
||
| @Override | ||
| public void handleSuccess(AuthenticationResponse successResponse) { | ||
|
|
| String dest = relayState != null ? relayState : "/"; | ||
| // redirect to dest | ||
| } | ||
| // other state transition successful |
There was a problem hiding this comment.
I'm a little confused by this comment. In the above code block, it implies that we're checking for a sessionToken and redirecting if it exists. This will not cause the below to execute.
I'd expect to see an else here to segue into another flow if we don't have a sessionToken.
There was a problem hiding this comment.
A redirect typically does not block the execution path (depending on how it is implemented). But it is very dependent on the use case. The "other state transition successful" portion could be a few different states. so I don't want to generalize what to do there. (I do want to comment on when a user should be considered logged in though)
|
|
||
| /** | ||
| * Base Authentication Exception. You can catch this exception or handle a more specific child exception. This exception | ||
| * was thrown as a result of a contains error information returned {@code 4xx} status, related error messages are |
There was a problem hiding this comment.
I think we're missing some text when describing result of a in the comment here.
This exception was thrown as a result of ______ and contains error information ...
| * @return the code of the error | ||
| */ | ||
| @Override | ||
| public String getCode() { |
There was a problem hiding this comment.
I'm unsure if providing the Okta error code is useful for the developer. Just out of curiosity, how were you expecting this to be used?
There was a problem hiding this comment.
Troubleshooting and logging
There was a problem hiding this comment.
@jmelberg-okta we should never "hide" anything that we get back from the API from the end user. they can decide if they want to use it or not
| AuthenticationResponse cancel(String stateToken); | ||
|
|
||
| /** | ||
| * The sms,call and token:software:totp factor types require activation to complete the enrollment process. |
|
|
||
| /** | ||
| * Sets the base URL of the Okta REST API to use. If unspecified, this value defaults to | ||
| * {@code https://api.okta.com/v1} - the most common use case for Okta's public SaaS cloud. |
There was a problem hiding this comment.
Shouldn't this value default to their own org + api/v1?
| * {@code https://api.okta.com/v1} - the most common use case for Okta's public SaaS cloud. | ||
| * | ||
| * <p>Customers using Okta's Enterprise HA cloud might need to configure this to be | ||
| * {@code https://enterprise.okta.io/v1} for example.</p> |
There was a problem hiding this comment.
Yeah, i need to update the java sdk too :D hold over from the SP code
| // do nothing this will be handled by the caller | ||
| String factorType = mfaChallengeResponse.getFactors().get(0).getType().name().toLowerCase(Locale.ENGLISH); | ||
| redirect("/login/mfa/verify/"+ factorType, mfaChallengeResponse); | ||
| // log(mfaChallengeResponse); |
|
|
||
| @Override | ||
| public AuthenticationResponse changePassword(char[] oldPassword, char[] newPassword, String stateToken, AuthenticationStateHandler stateHandler) throws AuthenticationException { | ||
| // TODO: validate params? state is required, old and new will be validated on the server? |
There was a problem hiding this comment.
+1 I'm curious if this is dependent on the password policy. For example, if my policy doesn't have a "cannot be last X passwords" rule, would this work?
There was a problem hiding this comment.
A CredentialsException would likely be thrown (assuming the server returned a E0000014)
|
|
||
| String errorCode = resourceException.getCode(); | ||
|
|
||
| switch (errorCode) { |
There was a problem hiding this comment.
Ah. Now I see why getCode was implemented.
| * <li>~/.okta/okta.json</li> | ||
| * <li>~/.okta/okta.yaml</li> | ||
| * <li>~/okta.properties</li> | ||
| * <li>~/okta.json</li> |
There was a problem hiding this comment.
Double checking - do you support all of these locations? If so, we should make sure we add a test for json/yaml parsing
to help with CI debugging without requiring another commit
| * @return the code of the error | ||
| */ | ||
| @Override | ||
| public String getCode() { |
There was a problem hiding this comment.
@jmelberg-okta we should never "hide" anything that we get back from the API from the end user. they can decide if they want to use it or not
| */ | ||
| public class AuthenticationFailureException extends AuthenticationException { | ||
|
|
||
| public static final String ERROR_CODE = "E0000004"; |
There was a problem hiding this comment.
Where is this error code coming from and why are you setting it in the code?
There was a problem hiding this comment.
Couple places: https://developer.okta.com/reference/error_codes/
But mostly they are inline in the doc: https://developer.okta.com/docs/api/resources/authn#response-parameters
I'm using the error code to trigger a more correctly typed exception But if the developer only wants to worry about dealing with a single exception they can just catch AuthenticationException
There was a problem hiding this comment.
Sounds good! I just wasn't sure of why you were coding this instead of using the error that comes back from the API
| */ | ||
| public class CredentialsException extends AuthenticationException { | ||
|
|
||
| public static final String ERROR_CODE = "E0000014"; |
There was a problem hiding this comment.
Again, Are these errors at least listed somewhere?
There was a problem hiding this comment.
Hi @bretterer! https://developer.okta.com/reference/error_codes/
edit: (missed @bdemers reference above)
There was a problem hiding this comment.
Thanks Len! Good hearing from you!
| */ | ||
| public class FactorValidationException extends AuthenticationException { | ||
|
|
||
| public static final String ERROR_CODE = "E0000068"; |
There was a problem hiding this comment.
Last time Ill bring this one up.
| * @see <a href="https://developer.okta.com/docs/api/resources/authn.html">Okta Authentication API documentation</a> | ||
| * @since 0.1.0 | ||
| */ | ||
| public interface AuthenticationClient { |
There was a problem hiding this comment.
Why are all of the methods on this class duplicated? I know I am not a Java dev, but I would rather see just the ones with *Request as the first param passed.
There was a problem hiding this comment.
I went back and forth with this. Personally I'd want to use the simple non-*Request methods (so I wouldn't need to deal with creating the Request object). But... if something is missing in the non-request signature, the developer can take the more advanced path and use the *Request method signatures.
Likely show the simple cases most of the user doc, with a note about the others
Codecov Report
@@ Coverage Diff @@
## master #2 +/- ##
=========================================
Coverage ? 89.14%
Complexity ? 335
=========================================
Files ? 41
Lines ? 682
Branches ? 23
=========================================
Hits ? 608
Misses ? 63
Partials ? 11
Continue to review full report at Codecov.
|
Depends on: