Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Spring Security 5.1's OIDC Support #9416

Merged
merged 23 commits into from Mar 16, 2019

Conversation

Projects
None yet
8 participants
@mraible
Copy link
Contributor

commented Mar 11, 2019

Contributes to fixing #9276.

  • Please make sure the below checklist is followed for Pull Requests.

  • Travis tests are green

  • Tests are added where necessary

  • Documentation is added/updated where necessary

  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

@mraible mraible referenced this pull request Mar 11, 2019

Closed

Upgrade Spring Security's OIDC Support #9276

4 of 4 tasks complete
@pvliss

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

Closing and re-opening to relaunch CI tests

@pvliss pvliss closed this Mar 11, 2019

@pvliss pvliss reopened this Mar 11, 2019

mraible added some commits Mar 12, 2019

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Closing and reopening to start new build.

@mraible mraible closed this Mar 12, 2019

@mraible mraible reopened this Mar 12, 2019

@Controller
public class AuthRedirectController {

@GetMapping("/authorize")

This comment has been minimized.

Copy link
@mraible

mraible Mar 14, 2019

Author Contributor

It'd be nice if this wasn't needed. It seems you need to request a protected resource to trigger OIDC login with Spring Security 5.1. Since it comes back to this endpoint after successful authentication, it redirects to the client app.

client:
provider:
oidc:
issuer-uri: http://localhost:9080/auth/realms/jhipster

This comment has been minimized.

Copy link
@mraible

mraible Mar 14, 2019

Author Contributor

I believe this means that Keycloak has to be running if you want integration tests to pass. This is because of OIDC discovery and how the endpoints are looked up from .well-known/openid-configuration. For example, https://dev-133320.okta.com/oauth2/default/.well-known/openid-configuration.

@mraible mraible closed this Mar 14, 2019

@mraible mraible reopened this Mar 14, 2019

@pascalgrimaud

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

We have a problem with GitHub and Travis. We can't see the Travis results in PR
The build is here: https://travis-ci.org/jhipster/generator-jhipster/builds/507021423

There is a lint error but I'm fixing it directly in your branch

After that, let's merge this !

@mraible mraible merged commit d302c3f into jhipster:master Mar 16, 2019

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Thanks @pascalgrimaud! Since all tests pass, I'm merging.

@mraible mraible deleted the mraible:spring-security-5.1 branch Mar 16, 2019

@mraible mraible referenced this pull request Mar 16, 2019

Closed

Add Resource Server by default (for mobile clients) #9424

1 of 1 task complete
@pmverma

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

@mraible I was generating an app from 5.8.2 to current master, then I got compile error for OAuth2Configuration.java.
I think this needs to be included in cleanup.js since this file has been removed with your new OIDC support.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

@pmverma pmverma referenced this pull request Mar 17, 2019

Closed

Unnecessary old OIDC files are not being cleaned up #9439

1 of 1 task complete
@DanielFran

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

@mraible dependency org.springframework.security.oauth.boot:spring-security-oauth2-autoconfigure was used by previous implementation only for OAUTH2

With your changes, it is now defined to be used by UAA. Is there any reason?

prior:
https://github.com/jhipster/generator-jhipster/blob/v5.x_maintenance/generators/server/templates/pom.xml.ejs#L585

after:
https://github.com/jhipster/generator-jhipster/blob/master/generators/server/templates/pom.xml.ejs#L609

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@DanielFran I tried not to modify UAA in any way since I don't know much about it. If UAA doesn't need it, we should remove it.

@jdubois

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

I also have integration tests errors with this new code...
Here is my .yo-rc.json:


  "generator-jhipster": {
    "promptValues": {
      "packageName": "com.mycompany.myapp"
    },
    "jhipsterVersion": "5.8.2",
    "applicationType": "microservice",
    "baseName": "micro",
    "packageName": "com.mycompany.myapp",
    "packageFolder": "com/mycompany/myapp",
    "serverPort": "8081",
    "authenticationType": "oauth2",
    "cacheProvider": "no",
    "enableHibernateCache": false,
    "websocket": false,
    "databaseType": "mongodb",
    "devDatabaseType": "mongodb",
    "prodDatabaseType": "mongodb",
    "searchEngine": false,
    "messageBroker": false,
    "serviceDiscoveryType": "eureka",
    "buildTool": "maven",
    "enableSwaggerCodegen": false,
    "jwtSecretKey": "ZjAxMTZhN2MyNWMwYzAzOWVlMTY1ODY5M2Y1OGQzNGI5ZGYxYjQ0ZDgyYmExN2JjN2NiMjhmMTA5YWQ3ZmJiNzU2YjJmYzhjZmU2MWQ1MWQ2N2Q2N2JiMmZmNDNmODhhNWM2NDNkOWQ3Y2IwYzRhYzRhYTE0Njk0YjA5YTZiZDQ=",
    "testFrameworks": [],
    "jhiPrefix": "jhi",
    "entitySuffix": "",
    "dtoSuffix": "DTO",
    "otherModules": [],
    "enableTranslation": false,
    "clientPackageManager": "npm",
    "skipClient": true,
    "skipUserManagement": true
  }
}

Running ./mvnw package -Pprod verify jib:dockerBuild fail during integration tests, with message:

Caused by: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'org.springframework.security.config.annotation.web.configuration.OAuth2ClientConfiguration$OAuth2ClientWebMvcSecurityConfiguration': Unsatisfied dependency expressed through method 'setClientRegistrationRepository' parameter 0; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'clientRegistrationRepository' defined in class path resource [org/springframework/boot/autoconfigure/security/oauth2/client/servlet/OAuth2ClientRegistrationRepositoryConfiguration.class]: Bean instantiation via factory method failed; nested exception is org.springframework.beans.BeanInstantiationException: Failed to instantiate [org.springframework.security.oauth2.client.registration.InMemoryClientRegistrationRepository]: Factory method 'clientRegistrationRepository' threw exception; nested exception is java.^Clang.IllegalArgumentException: Unable to resolve the OpenID Configuration with the provided Issuer of "http://localhost:9080/auth/realms/jhipster"
@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@jdubois I'll see if I can reproduce. Related: why do you have a jwtSecretKey in an app definition for oauth2?

@jdubois

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

@mraible I don't know why it's there: it's probably always generated when using the CLI, even if it's not used.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@jdubois The more complete error you're seeing is:

Factory method 'clientRegistrationRepository' threw exception; nested exception is 
java.lang.IllegalArgumentException: Unable to resolve the OpenID Configuration 
with the provided Issuer of "http://localhost:9080/auth/realms/jhipster"

Integration tests now require Keycloak to be running because all the endpoints are looked up on-the-fly (an OIDC feature). We could mock out the http://localhost:9080/auth/realms/jhipster/.well-known/openid-configuration endpoint with Wiremock or something, but these are integration tests and it seems like the integrated services should be running.

@jdubois

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Oh sorry @mraible !!! Indeed I remember seeing this... This is quite annoying, at least can you wrap the error in the test with a try/catch block, to provide a meaningful error message and explain people what they need to do?

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

I'm not sure how to try/catch a dependency injection issue in Spring.

@jgrandja Do you have any suggestions on how we might go about having better error messages (in tests) when the IdP's well-known endpoint is not available?

Here's what we see when running an integration tests and Keycloak is not running:

Caused by: org.springframework.beans.BeanInstantiationException: Failed to instantiate 
[org.springframework.security.oauth2.client.registration.InMemoryClientRegistrationRepository]: 
Factory method 'clientRegistrationRepository' threw exception; nested exception is 
java.lang.IllegalArgumentException: Unable to resolve the OpenID Configuration with the 
provided Issuer of "http://localhost:9080/auth/realms/jhipster"
@jgrandja

This comment has been minimized.

Copy link

commented Mar 26, 2019

@mraible I don't think it makes sense to catch an exception when the ApplicationContext does not initialize and startup properly - it should proceed to fail since it would be in an inconsistent state.

Could you not start Keycloak in @BeforeClass in the integration test class to ensure it's up before the tests run?

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@jgrandja I figured out a solution thanks to some code and tests I found in Spring Security repositories. I created a new @TestConfiguration class that defines the ClientRegistrationRepository and JwtDecoder beans.

import org.springframework.boot.test.context.TestConfiguration;
import org.springframework.context.annotation.Bean;
import org.springframework.security.oauth2.client.registration.ClientRegistration;
import org.springframework.security.oauth2.client.registration.ClientRegistrationRepository;
import org.springframework.security.oauth2.client.registration.InMemoryClientRegistrationRepository;
import org.springframework.security.oauth2.core.AuthorizationGrantType;
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
import org.springframework.security.oauth2.jwt.JwtDecoder;

import static org.mockito.Mockito.mock;

/**
 * This class allows you to run unit and integration tests without an IdP.
 */
@TestConfiguration
public class TestSecurityConfiguration {
    private ClientRegistrationRepository clientRegistrationRepository;

    public TestSecurityConfiguration() {
        ClientRegistration mockRegistration = clientRegistration().build();
        this.clientRegistrationRepository = new InMemoryClientRegistrationRepository(mockRegistration);
    }

    @Bean
    ClientRegistrationRepository clientRegistrationRepository() {
        return this.clientRegistrationRepository;
    }

    private ClientRegistration.Builder clientRegistration() {
        return ClientRegistration.withRegistrationId("oidc")
            .redirectUriTemplate("{baseUrl}/{action}/oauth2/code/{registrationId}")
            .clientAuthenticationMethod(ClientAuthenticationMethod.BASIC)
            .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
            .scope("read:user")
            .authorizationUri("https://jhipster.org/login/oauth/authorize")
            .tokenUri("https://jhipster.org/login/oauth/access_token")
            .jwkSetUri("https://jhipster.org/oauth2/jwk")
            .userInfoUri("https://api.jhipster.org/user")
            .userNameAttributeName("id")
            .clientName("Client Name")
            .clientId("client-id")
            .clientSecret("client-secret");
    }

    @Bean
    JwtDecoder jwtDecoder() {
        return mock(JwtDecoder.class);
    }
}

Then I import this class in @SpringBootTest.

@SpringBootTest(classes = {MicroApp.class, TestSecurityConfiguration.class})

It'd be cool if it the TestSecurityConfiguration could be auto-detected, but I think this is "good enough".

@jgrandja

This comment has been minimized.

Copy link

commented Mar 27, 2019

@mraible

It'd be cool if it the TestSecurityConfiguration could be auto-detected, but I think this is "good enough".

If you make the class static, it should auto-detect. Try this:

@TestConfiguration
static class TestSecurityConfiguration {
...

@mraible mraible referenced this pull request Mar 27, 2019

Merged

Add ability to run tests with no OIDC Provider running #9484

4 of 4 tasks complete
@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@jgrandja Nope - static isn't allowed in a top level class.

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:testCompile (default-testCompile) on project micro: Compilation failure: Compilation failure: 
[ERROR] /Users/mraible/oauth2/src/test/java/com/mycompany/myapp/config/TestSecurityConfiguration.java:[18,8] modifier static not allowed here

FWIW, I just created a PR at #9484.

@jgrandja

This comment has been minimized.

Copy link

commented Mar 27, 2019

@mraible I totally missed that :( What I meant to say is that a static inner class should be auto-detected. Not sure if making TestSecurityConfiguration as an static inner class in the test class makes sense? In general, static inner class @Configuration should get auto-detected.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@jgrandja That would work for just one test, but there are around 8 integration tests in JHipster that need this OIDC mocked. Having one config class and imports might be the easiest to understand.

@jgrandja

This comment has been minimized.

Copy link

commented Mar 27, 2019

@mraible Yeah, that does make sense. Forget anything I just said :)

@avdev4j

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

Oh @mraible
This PR is very welcome.
I tried oauth2 with openAM on both 5.8.2 and Master.

Well this new way is highly better and easier to configure and use.

@jdubois jdubois added this to the 6.0.0-beta.0 milestone Apr 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.