Code flow with discovery + hosted login page example by bdemers · Pull Request #7 · okta/okta-spring-boot

bdemers · 2017-09-20T19:24:48Z

Lots of stuff in here, any feedback is great high level or low.

There are a couple of important changes in this one:
1.) Add call discovery endpoint to simplify spring security oauth config
2.) Support custom 'authorities' claim in ID token
3.) Add support for a customizable hosted login page (using the Okta widget), works like any other Spring redirect.
4.) Renamed the modules, so we can support having a singular Spring Boot Starter for all of your Okta needs (Spring Sec, Spring Sec + Thymeleaf, and non Spring Sec in the future)

Still a bunch of things to do before merging, lots of cleaning (and tests) But it is at a point where we can chat more about this approach.

Still need to think about how the mvc/thymeleaf module is structured, the current 'optional' dependency is not great

Ran PMD and Findbugs, fixed minor issues

dogeared · 2017-09-20T22:05:14Z

.../hosted-login-code-flow/src/main/java/com/okta/spring/example/resources/WelcomeResource.java

+     */
+    @GetMapping("/")
+    @PreAuthorize("#oauth2.clientHasRole('Everyone') || #oauth2.hasScope('email')")
+//    @PreAuthorize("#oauth2.clientHasRole('Everyone') || #oauth2.hasScope('email')")


looks like delete this line

dogeared · 2017-09-20T22:06:04Z

.../hosted-login-code-flow/src/main/java/com/okta/spring/example/resources/WelcomeResource.java

+import java.security.Principal;
+
+@RestController
+public class WelcomeResource {


Should this be: WelcomeController or WelcomeResourceController?

dogeared · 2017-09-20T22:07:11Z

.../hosted-login-code-flow/src/main/java/com/okta/spring/example/resources/WelcomeResource.java

+        return new Welcome("The message of the day is boring.", principal.getName());
+    }
+
+    @XmlRootElement


ew - why XML? ;)

Ha! that works for both JSON and XML IIRC, but let me see if that is still needed

i'm wrong... I swear it was needed at one point...

dogeared · 2017-09-20T22:08:43Z

...ples/redirect-code-flow/src/main/java/com/okta/spring/example/resources/WelcomeResource.java

+import java.security.Principal;
+
+@RestController
+public class WelcomeResource {


WelcomeResourceController or WelcomeController

dogeared · 2017-09-20T22:10:00Z

oauth2/pom.xml

+    <name>Okta Spring Boot :: OAuth2</name>
+
+    <properties>
+        <spring-boot.version>1.5.4.RELEASE</spring-boot.version>


1.5.7 is latest release

dogeared · 2017-09-20T22:22:00Z

oauth2/src/main/java/com/okta/spring/oauth/OktaOAuthProperties.java

+import java.util.Map;
+
+@ConfigurationProperties("okta")
+public class OktaOAuthProperties {


I'm a little confused on the difference between OkatOAuthProperties and OAuthProperties and why OAuthProperties is embedded in OktaOAuthProperties.

This brings up a few questions, i'm not sure the best way to handle the properties object.
But your point is valid, the top level should be renamed to OktaProperties (on it)

The odd nested OAuthProperties getOauth2()/oauth2 to make Spring's ConfigurationProperties work out of the box.

it might be worth breaking this up into multiple classes, one for each okta.* so one for okta.client, okta.oauth2 and another for okta.web (this last one is already separate)

The okta.client properties are defined by the the SDK's spec so they are reused where possible

dogeared · 2017-09-20T22:23:03Z

oauth2/src/main/java/com/okta/spring/oauth/OktaOAuthProperties.java

+    }
+
+
+    public OAuthProperties getOauth2() {


seems like this should be: getOAuthProperties() or getOAuth2Properties (which also means that the variable should probs be renamed too)

See above, that might help clear up the confusion (instead of forcing them all in a single class)?

dogeared · 2017-09-20T22:25:11Z

oauth2/src/test/groovy/com/okta/spring/oauth/code/ClaimsAuthoritiesExtractorTest.groovy

+import org.testng.annotations.Test
+
+import static org.hamcrest.MatcherAssert.assertThat
+import static org.hamcrest.Matchers.*


not as critical for tests, but we've had a standard in the past of making imports explicit

yeah, just too many damn static methods in hamcrest, easy enough to clean up the imports though

dogeared · 2017-09-20T22:26:07Z

oauth2/src/test/groovy/com/okta/spring/oauth/code/OktaOAuthConfigTest.groovy

+import org.testng.annotations.Test
+
+import static org.hamcrest.MatcherAssert.assertThat
+import static org.hamcrest.Matchers.*


same note on expanding imports

dogeared · 2017-09-20T22:28:43Z

thymeleaf/src/main/java/com/okta/spring/oauth/code/mvc/TemplateLayoutInterceptor.java

+    protected boolean shouldExecute(HttpServletRequest request, HttpServletResponse response,
+                                    Object handler, ModelAndView modelAndView) {
+
+        if (modelAndView == null || !modelAndView.isReference() /*|| STORMPATH_JSON_VIEW_NAME.equals(modelAndView.getViewName()) */) {


time to let go of STORMPATH comments ;)

bdemers · 2017-09-22T01:22:33Z

@dogeared all clear, but, take a look at this comment if you missed it in the scroll back:
https://github.com/okta/okta-spring-security/pull/7/files/fac06be4678b12312f8a730e828335cdd74f4735#r140393885

Thoughts on how these properties should be structured ?

mraible

Minor code formatting comments. Didn't try the examples. Does Travis run integration tests to prove they work?

mraible · 2017-09-22T20:01:00Z

...n-code-flow/src/main/java/com/okta/spring/example/HostedLoginCodeFlowExampleApplication.java

+import org.springframework.security.config.annotation.method.configuration.GlobalMethodSecurityConfiguration;
+import org.springframework.security.oauth2.config.annotation.web.configuration.EnableOAuth2Client;
+import org.springframework.security.oauth2.provider.expression.OAuth2MethodSecurityExpressionHandler;
+


Remove extra line break.

mraible · 2017-09-22T20:02:16Z

examples/hosted-login-code-flow/src/main/resources/application.yml

+    customLoginRoute: /login
+
+    # default is '/login'
+    redirectUri: /login/okta


Why do the defaults need to be changed? Does it work when these aren't specified, or are these settings specific to this example?

Actually, I want to pick your brain about this. I'm not super happy with the way this works at the moment. (this is also related to your next comment).
I'm trying to keep the the hosted page stuff optional, and default to a standard redirect (takes less config, easier to debug, etc).

This project still redirects, it just redirects to the oauth2 filter at: /login/okta which grabs the state and then renders the widget template login.html

Longer term it would be nice to handle this all in the same request, and just forward to the login.html template (to save a hop, and configuration)

mraible · 2017-09-22T20:02:41Z

examples/hosted-login-code-flow/src/main/resources/application.yml

+  oauth2:
+    client:
+      # Need to figure out how to set this, this needs to be an absolute URI
+      userAuthorizationUri: http://localhost:8080/login


This is a Spring Security setting, or ours?

Spring, it needs an absolute URI. (Ideally we need a better solution for this)

mraible · 2017-09-22T20:03:28Z

examples/redirect-code-flow/pom.xml

+    </dependencies>
+
+    <build>
+        <defaultGoal>spring-boot:run</defaultGoal>


My fave! I tried to get the Spring Boot guys to do this, but they refused.

mraible · 2017-09-22T20:03:43Z

...es/redirect-code-flow/src/main/java/com/okta/spring/example/RedirectCodeFlowApplication.java

+        }
+    }
+
+


Remove extra line break.

mraible · 2017-09-22T20:05:59Z

oauth2/src/main/java/com/okta/spring/oauth/code/OktaOAuthConfig.java

+    protected void init() {
+
+        // update properties based on discovery if needed
+


Unnecessary line break.

mraible · 2017-09-22T20:06:37Z

oauth2/src/main/java/com/okta/spring/oauth/code/OktaOAuthConfig.java

+    protected void configure(HttpSecurity http) throws Exception {
+
+        // FIXME: MVC bleed
+        http.authorizeRequests().antMatchers("/okta/*.css").permitAll();


Spring's fault or ours?

Ours, I ran into an ordering issue (?) where a WebSecurityConfigurerAdapter from the thymeleaf module was not getting applied correctly (and blocking the css). A little googling showed that I should apply() other WebSecurityConfigurerAdapter. Thoughts?

mraible · 2017-09-22T20:07:56Z

oauth2/src/main/java/com/okta/spring/oauth/implicit/ResourceServerConfig.java

+        return new ResourceServerConfigurerAdapter() {
+            @Override
+            public void configure(final ResourceServerSecurityConfigurer config) {
+                config.resourceId(OAuthProperties.getOauth2().getAudience()); // set audience


Comment doesn't match - remove.

mraible · 2017-09-22T20:08:39Z

oauth2/src/test/groovy/com/okta/spring/oauth/discovery/DiscoveryMetadataTest.groovy

+    @Test
+    void basicParseTest() {
+
+


too much space.

mraible · 2017-09-22T20:09:38Z

pom.xml

+            <dependency>
+                <groupId>com.okta.spring</groupId>
+                <artifactId>okta-spring-boot-starter</artifactId>
+                <version>0.2.0-SNAPSHOT</version>


Should this be ${project.version} to simplify things?

Nope, ${project.version} is not recommended for dependencies. There are cases where if you use a parent from this project in a separately versioned project (other git repo), then then ${project.version} will not resolve correctly. (there is another edge case too, but I forget).

Older versions of the release plugin did update these correctly as well, but that has been fixed.

bdemers · 2017-09-25T14:56:10Z

Thanks for the reviews so far guys! After I get a green merge light I'd like to continue using this as a feature branch and NOT merge to master until the following issues are resolved (hopefully in more targeted PRs):

1.) Extra config when using a hosted login page
2.) Extra redirect when using a hosted login page
3.) Organize the configuration OktaProperties better?
4.) Fix okta.css bleed over in OktaOAuthConfig

If you agree, i'll create issues for each of these and we can merge this to master once they are resolved

Attempted to use the built in Spring OIDC discovery, but it didn't support all of the fields Okta has Updated current code to reflect that pattern though

…ientId'

Set default Order(80) on OktaHttpSecurityConfigurationAdapter to make one less line of code to explain

Realize there is too much code here...

bdemers · 2017-09-28T23:43:04Z

Way to much code in here, both for a pull request and in general.
i'm rewriting this in WAY less code, and working better within the bounds of spring boot.
I think i was missing fundamental when i started this PR (and was focused on a hosted page, which is no longer the goal)

bdemers added 9 commits September 20, 2017 15:15

Add Code Flow with discovery into the mix

c8c3ae7

Still a bunch of things to do before merging, lots of cleaning (and tests) But it is at a point where we can chat more about this approach.

A little more hacking to get features into the hosted login page

3b2f788

hacking on conditionals

e170755

Merge modules together

3592b08

Still need to think about how the mvc/thymeleaf module is structured, the current 'optional' dependency is not great

renaming example to ImplicitFlowExample

c6cd0cd

Add missing license headers

949037a

simplify login template

efec759

Added tests for new code flow related bits

fac06be

Ran PMD and Findbugs, fixed minor issues

Updating version to 0.2.0

8e08ad9

bdemers requested review from dogeared and mraible September 20, 2017 19:24

dogeared suggested changes Sep 20, 2017

View reviewed changes

bdemers added 3 commits September 21, 2017 21:10

Changes based on code review feedback

d124bd2

organize test imports

cb3779b

minor pom cleanup

4c17ba9

mraible reviewed Sep 22, 2017

View reviewed changes

bdemers added 6 commits September 25, 2017 11:00

Minor formatting feedback from review

4ec7da2

Moving hosted page logic to example

1f67b1f

Moving config logic to separate module

33ced48

Hacking around an odd security.oauth2.client.clientId problem

bafe60a

Improved OIDC discovery based on Spring impl

aefe85e

Attempted to use the built in Spring OIDC discovery, but it didn't support all of the fields Okta has Updated current code to reflect that pattern though

Fix for spring boot conditional on missing 'security.oauth2.client.cl…

ef94327

…ientId'

bdemers force-pushed the code-flow branch from 4574806 to ef94327 Compare September 27, 2017 19:36

bdemers added 2 commits September 27, 2017 15:50

Removing field injection in favor of constructor injection

5e3ffbd

Update Readme for code flow

74b8ce1

Set default Order(80) on OktaHttpSecurityConfigurationAdapter to make one less line of code to explain

bdemers changed the title ~~Code flow with discovery & hosted login page~~ Code flow with discovery + hosted login page example Sep 27, 2017

Switch example annotation to @EnableOAuth2Sso

1ff2e24

Realize there is too much code here...

bdemers closed this Sep 28, 2017

bdemers deleted the code-flow branch October 5, 2017 15:31

		protected void init() {

		// update properties based on discovery if needed

Conversation

bdemers commented Sep 20, 2017

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!

bdemers commented Sep 22, 2017

Uh oh!

mraible 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!

bdemers commented Sep 25, 2017

Uh oh!

bdemers commented Sep 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants