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

Add global logout for OIDC authentication #8757

Merged
merged 22 commits into from Jan 23, 2019

Conversation

Projects
None yet
8 participants
@mraible
Copy link
Contributor

commented Nov 5, 2018

  • 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

Add global location for OIDC authentication. This PR covers monoliths, gateways, Angular, and React.

const data = response.body;
let logoutUrl = data.logoutUrl;
// keycloak
if (logoutUrl.indexOf('localhost') > -1) {

This comment has been minimized.

Copy link
@antarus

antarus Nov 6, 2018

Contributor

This test seems odd to me, Keycloak is not necessarily exposed on localhost, no?

This comment has been minimized.

Copy link
@mraible

mraible Nov 6, 2018

Author Contributor

You are correct, and Okta is not necessarily exposed on oktapreview.com. I'm open to suggestions for better logic.

This comment has been minimized.

Copy link
@antarus

antarus Nov 8, 2018

Contributor

can be add a variable in webpack?
we configure SERVER_API_URL so why not add a variable to find out if the OIDC is keycloak or okta.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

@deepu105 and @sendilkumarn: do you know how I might implement the redirect-after-logout logic for React?

@deepu105

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

@mraible I need to take a look, you can always use the location object to do that, did you check react router to see if there is a better way?

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

Yeah, once I have the data it shouldn’t be hard. I’m trying to figure out how to get the logoutUrl and idToken from the POST to /api/logout.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

@ZiyaErcanErdem

This comment has been minimized.

Copy link

commented Nov 9, 2018

Enhancing existing AjaxLogoutSuccessHandler and logging out identity on gateway may be another option like below;

public class KeycloakLogoutSuccessHandler extends AjaxLogoutSuccessHandler {
	
    private final KeycloakIdentityManager keycloakIdentityManager;
    
    @Value("${security.oauth2.client.access-token-uri:#{null}}")
    private String accessTokenUri;
    
	private String logoutURI;
	
	public KeycloakLogoutSuccessHandler(
			@Value("${security.oauth2.client.access-token-uri:#{null}}") final String accessTokenUri,
			KeycloakIdentityManager keycloakIdentityManager) {
		
		this.keycloakIdentityManager = keycloakIdentityManager;
		
		if (StringUtils.isNotBlank(accessTokenUri)) {
			this.logoutURI = accessTokenUri.replace("token", "logout");
        }
		
	}
	
    @Override
    public void onLogoutSuccess(HttpServletRequest request, HttpServletResponse response, Authentication authentication) throws IOException, ServletException {
        super.onLogoutSuccess(request, response, authentication);

        boolean logoutSuccess = false;
        String userName = this.extractAuthenticatedUserName(authentication);
        if(StringUtils.isNotBlank(userName)) {
        	logoutSuccess = this.keycloakIdentityManager.logoutUserByUsername(userName);
        }
        if (!logoutSuccess) {
            response.addHeader("SSO-Logout-URL", logoutURI);
        }
    }
    
    private String extractAuthenticatedUserName(Authentication authentication) {
    	if(null == authentication || !(authentication.getPrincipal() instanceof User)) {
    		return null;
    	}        	
    	User authenticatedUser = (User) authentication.getPrincipal();
    	String userName = authenticatedUser.getUsername();
    	return userName;
    }
}

@EnableOAuth2Sso
@Configuration
public class OAuth2SsoConfiguration extends WebSecurityConfigurerAdapter {

    
    private final CorsFilter corsFilter;
    private final KeycloakLogoutSuccessHandler keycloakLogoutSuccessHandler;

    public OAuth2SsoConfiguration(CorsFilter corsFilter, KeycloakLogoutSuccessHandler keycloakLogoutSuccessHandler) {
        this.corsFilter = corsFilter;
        this.keycloakLogoutSuccessHandler = keycloakLogoutSuccessHandler;
    }

 ...

   @Override
    protected void configure(HttpSecurity http) throws Exception {
        http
            .csrf()
                .csrfTokenRepository(CookieCsrfTokenRepository.withHttpOnlyFalse())
            .and()
            .addFilterBefore(corsFilter, CsrfFilter.class)
            .headers()
            .frameOptions()
            .disable()
        .and()
            .logout()
            .logoutUrl("/api/logout")
            .logoutSuccessHandler(this.keycloakLogoutSuccessHandler)
        .and()
            .authorizeRequests()
            .antMatchers("/login").permitAll()
            .antMatchers("/api/profile-info").permitAll()
            .antMatchers("/api/**").authenticated()
            .antMatchers("/management/health").permitAll()
            .antMatchers("/management/**").hasAuthority(AuthoritiesConstants.ADMIN)
            .anyRequest().permitAll();
    }

...
}
@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

mraible added some commits Nov 12, 2018

mraible added some commits Nov 12, 2018

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2018

The ngx-mariadb-oauth2-sass-infinispan and ms-ngx-gateway-eureka-oauth2 example apps are failing because of Protractor tests. I've found that adding browser.sleep(500) in some of the administration.spec.ts tests solves the problem. I'll include this small change in this PR. @jhipster/developers Thoughts?

diff --git a/src/test/javascript/e2e/admin/administration.spec.ts b/src/test/javascript/e2e/admin/administration.spec.ts
index c39c6ea..ec5761a 100644
--- a/src/test/javascript/e2e/admin/administration.spec.ts
+++ b/src/test/javascript/e2e/admin/administration.spec.ts
@@ -35,6 +35,7 @@ describe('administration', () => {
 
     it('should load configuration', async () => {
         await navBarPage.clickOnAdmin('my-prefix-configuration');
+        await browser.sleep(500);
         const expect1 = 'Configuration';
         const value1 = await element(by.id('configuration-page-heading')).getText();
         expect(value1).to.eq(expect1);
@@ -42,6 +43,7 @@ describe('administration', () => {
 
     it('should load audits', async () => {
         await navBarPage.clickOnAdmin('audits');
+        await browser.sleep(500);
         const expect1 = 'Audits';
         const value1 = await element(by.id('audits-page-heading')).getText();
         expect(value1).to.eq(expect1);
@@ -49,6 +51,7 @@ describe('administration', () => {
 
     it('should load logs', async () => {
         await navBarPage.clickOnAdmin('logs');
+        await browser.sleep(500);
         const expect1 = 'Logs';
         const value1 = await element(by.id('logs-page-heading')).getText();
         expect(value1).to.eq(expect1);

mraible added some commits Nov 12, 2018

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2018

I'm trying to reproduce the issues with the ms-ngx-gateway-eureka-oauth2 sample. Running it locally, the app is so slow it's unbearable. It seems this might have to do with the 20MB main.bundle.js file. According to Protractor, it takes over 16 seconds to just log in!

  account
    ✓ should fail to login with bad password (1749ms)
    ✓ should login successfully with admin account (16558ms)

screen shot 2018-11-12 at 10 03 29 am

This particular example has a whole lot of entities. Shouldn't entities be lazy-loaded so the main bundle isn't so huge?

entity-with-dto
entity-with-pagination
entity-with-pagination-and-dto
entity-with-service-class
entity-with-service-class-and-dto
entity-with-service-class-and-pagination
entity-with-service-class-pagination-and-dto
entity-with-service-impl
entity-with-service-impl-and-dto
entity-with-service-impl-and-pagination
entity-with-service-impl-pagination-and-dto
entity.module.ts
field-test-entity
field-test-infinite-scroll-entity
field-test-mapstruct-entity
field-test-pager-entity
field-test-pagination-entity
field-test-service-class-entity
field-test-service-impl-entity
test-root
@jdubois

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

Yes I'm guessing this is because of the entities, because I did some tests recently with an empty project (no entities) and it was really good.

So the solution is indeed to lazy load them - @wmarques is our expert here, and I remember this is very buggy and complicated (don't believe the Angular evangelists, ever!!!)

@Akuka

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2018

🙄
Will it be part of the next release?

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

@Akuka This issue depends on #8925. Without lazy-loaded entities, the Travis tests don't pass. 😕

@ndywicki

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

Hi guys,

For information,
At my work for production environment the OIDC Server is not Keycloak but another OIDC enterprise server implementation.

The logout (end-session) uri must contain the idToken like :
https://oidc-server/oauth2/connect/endSession?realm=myrealm&id_token_hint={id_token}

So for this case I use generic logout handler with oAuth2RestTemplate and the logout URI set in the application.yml :

@Component
public class Oauth2SecurityContextLogoutHandler extends SecurityContextLogoutHandler {

    private final Logger log = LoggerFactory.getLogger(Oauth2SecurityContextLogoutHandler.class);

    private final UserInfoRestTemplateFactory userInfoRestTemplateFactory;

    private final ApplicationProperties applicationProperties;

    public Oauth2SecurityContextLogoutHandler(UserInfoRestTemplateFactory userInfoRestTemplateFactory, ApplicationProperties applicationProperties) {
        this.userInfoRestTemplateFactory = userInfoRestTemplateFactory;
        this.applicationProperties = applicationProperties;
    }

    @Override
    public void logout(HttpServletRequest request, HttpServletResponse response,
                       Authentication authentication) {

        OAuth2RestTemplate oAuth2RestTemplate = userInfoRestTemplateFactory.getUserInfoRestTemplate();
        if (oAuth2RestTemplate.getAccessToken() != null && oAuth2RestTemplate.getAccessToken().getAdditionalInformation() != null) {
            // Retrieved id_token
            String idToken = (String) oAuth2RestTemplate.getAccessToken().getAdditionalInformation().get("id_token");
            // Call end-session endpoint
            oAuth2RestTemplate.getForObject(applicationProperties.getOauth2().getClient().getLogoutUri(), String.class, idToken);
        }
        super.logout(request, response, authentication);
    }
}

Regards,
Nicolas

@DanielFran DanielFran closed this Jan 12, 2019

@DanielFran DanielFran reopened this Jan 12, 2019

@DanielFran

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

Close/Reopen to launch tests

@DanielFran DanielFran closed this Jan 12, 2019

@DanielFran DanielFran reopened this Jan 12, 2019

@mraible mraible closed this Jan 22, 2019

@mraible mraible reopened this Jan 22, 2019

mraible added some commits Jan 22, 2019

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

I believe this is ready to merge. All CI tests pass. The two samples that seemed to test this most thoroughly are:

  • ngx-mariadb-oauth2-sass-infinispan
  • ms-ngx-gateway-eureka-oauth2

They're tested by CI, but I'm not sure their equivalent React flavors are. I'll test them locally and confirm (or make changes until) all e2e tests pass.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

I found issues with React + the entity generator and fixed them. I plan on merging this once the tests pass.

@mraible mraible merged commit e1e4ca8 into jhipster:master Jan 23, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
jhipster.generator-jhipster Build #20190122.12 succeeded
Details

@mraible mraible deleted the mraible:global-oidc-logout branch Jan 23, 2019

@jdubois jdubois added this to the 5.8.0 milestone Jan 25, 2019

@PierreBesson PierreBesson referenced this pull request Jan 26, 2019

Closed

Using Keycloak/OKTA with jwt instead of sessions #9120

1 of 1 task complete

@mraible mraible referenced this pull request Jan 30, 2019

Closed

Add global logout for OIDC authentication #256

1 of 1 task complete

@deshetti deshetti referenced this pull request Mar 3, 2019

Closed

Global logout for OIDC authentication #61

0 of 9 tasks complete
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.