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

Fix sso oic OpenIdConnectAuthenticator #2845

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

hi-yamap
Copy link
Contributor

Fixed issues that occurred when using single sign-on with AWS Cognito.

  • Fixed an issue where users could not re-login.
  • Fixed an issue where strings were not parsed correctly when jwtClaim contained an array type element.
  • Improved to process all elements included in jwtClaim.

https://discuss.codelibs.org/t/openid-sso/2467

@marevol marevol merged commit 28bd300 into codelibs:master Oct 3, 2024
4 checks passed
marevol pushed a commit that referenced this pull request Oct 3, 2024
@marevol marevol added this to the 14.18.0 milestone Oct 3, 2024
@anguswilliams
Copy link

anguswilliams commented Oct 7, 2024

Hi @marevol and @hi-yamap, thanks for this MR, I've tested it out and it so far it looks like it's resolved my OIDC SSO issues.

It looks like this has introduced a regression if groups are included in the JWT claim. I'm testing against the latest snapshot docker image.

java.lang.ClassCastException: class java.util.ArrayList cannot be cast to class [Ljava.lang.String; (java.util.ArrayList and [Ljava.lang.String; are in module java.base of loader 'bootstrap')
	at org.codelibs.fess.app.web.base.login.OpenIdConnectCredential.getUserGroups(OpenIdConnectCredential.java:50)
	at org.codelibs.fess.app.web.base.login.OpenIdConnectCredential.getUser(OpenIdConnectCredential.java:58)
	at org.codelibs.fess.sso.oic.OpenIdConnectAuthenticator.lambda$resolveCredential$2(OpenIdConnectAuthenticator.java:276)
	at org.codelibs.fess.app.web.base.login.FessLoginAssist$LoginCredentialResolver.lambda$resolve$0(FessLoginAssist.java:181)
	at org.lastaflute.web.login.TypicalLoginAssist$CredentialResolver.resolve(TypicalLoginAssist.java:193)
	at org.codelibs.fess.app.web.base.login.FessLoginAssist$LoginCredentialResolver.resolve(FessLoginAssist.java:181)
	at org.codelibs.fess.sso.oic.OpenIdConnectAuthenticator.resolveCredential(OpenIdConnectAuthenticator.java:276)
	at org.codelibs.fess.app.web.base.login.FessLoginAssist.resolveCredential(FessLoginAssist.java:168)
	at org.lastaflute.web.login.TypicalLoginAssist.findLoginUser(TypicalLoginAssist.java:122)
	at org.lastaflute.web.login.TypicalLoginAssist.doLogin(TypicalLoginAssist.java:283)
	at org.lastaflute.web.login.TypicalLoginAssist.loginRedirect(TypicalLoginAssist.java:252)
	at org.codelibs.fess.app.web.sso.SsoAction.index(SsoAction.java:64)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
	at java.base/java.lang.reflect.Method.invoke(Unknown Source)
	at org.lastaflute.web.ruts.GodHandableAction.invokeExecuteMethod(GodHandableAction.java:358)
	at org.lastaflute.web.ruts.GodHandableAction.actuallyExecute(GodHandableAction.java:329)
	at org.lastaflute.web.ruts.GodHandableAction.doExecute(GodHandableAction.java:152)
	at org.lastaflute.web.ruts.GodHandableAction.lambda$transactionalExecute$0(GodHandableAction.java:143)
	at org.lastaflute.db.jta.stage.JTATransactionStage.performTx(JTATransactionStage.java:102)
	at org.lastaflute.db.jta.stage.JTATransactionStage.lambda$requiresNew$1(JTATransactionStage.java:59)
	at org.lastaflute.di.tx.adapter.JTATransactionManagerAdapter.requiresNew(JTATransactionManagerAdapter.java:73)
	at org.lastaflute.db.jta.stage.JTATransactionStage.requiresNew(JTATransactionStage.java:58)
	at org.lastaflute.db.jta.stage.JTATransactionStage.selectable(JTATransactionStage.java:84)
	at org.lastaflute.web.ruts.GodHandableAction.transactionalExecute(GodHandableAction.java:142)
	at org.lastaflute.web.ruts.GodHandableAction.execute(GodHandableAction.java:117)
	at org.lastaflute.web.ruts.ActionRequestProcessor.performAction(ActionRequestProcessor.java:253)
	at org.lastaflute.web.ruts.ActionRequestProcessor.fire(ActionRequestProcessor.java:182)
	at org.lastaflute.web.ruts.ActionRequestProcessor.process(ActionRequestProcessor.java:114)
	at org.lastaflute.web.servlet.filter.RequestRoutingFilter.processAction(RequestRoutingFilter.java:289)
	at org.lastaflute.web.servlet.filter.RequestRoutingFilter.routingToAction(RequestRoutingFilter.java:237)
	at org.lastaflute.web.servlet.filter.RequestRoutingFilter.lambda$createActionFoundPathHandler$0(RequestRoutingFilter.java:201)
	at org.lastaflute.web.path.ActionPathResolver.executeHandlerIfFound(ActionPathResolver.java:324)
	at org.lastaflute.web.path.ActionPathResolver.mappingActionPath(ActionPathResolver.java:190)
	at org.lastaflute.web.path.ActionPathResolver.handleActionPath(ActionPathResolver.java:114)
	at org.lastaflute.web.servlet.filter.RequestRoutingFilter.doFilter(RequestRoutingFilter.java:132)
	at org.lastaflute.web.servlet.filter.LastaToActionFilter.viaEmbeddedFilter(LastaToActionFilter.java:153)
	at org.lastaflute.web.servlet.filter.LastaToActionFilter.viaInsideHookDeque(LastaToActionFilter.java:144)
	at org.lastaflute.web.servlet.filter.LastaToActionFilter.viaInsideHook(LastaToActionFilter.java:128)
	at org.lastaflute.web.servlet.filter.LastaToActionFilter.doFilter(LastaToActionFilter.java:120)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:168)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:144)
	at org.lastaflute.web.servlet.filter.LastaShowbaseFilter.toNextChain(LastaShowbaseFilter.java:171)
	at org.lastaflute.web.servlet.filter.LastaShowbaseFilter.lambda$viaEmbeddedFilter$3(LastaShowbaseFilter.java:150)
	at org.lastaflute.web.servlet.filter.RequestLoggingFilter.actuallyFilter(RequestLoggingFilter.java:237)
	at org.lastaflute.web.servlet.filter.RequestLoggingFilter.doFilter(RequestLoggingFilter.java:209)
	at org.lastaflute.web.servlet.filter.LastaShowbaseFilter.viaEmbeddedFilter(LastaShowbaseFilter.java:148)
	at org.lastaflute.web.servlet.filter.LastaShowbaseFilter.viaOutsideHookDeque(LastaShowbaseFilter.java:139)
	at org.lastaflute.web.servlet.filter.LastaShowbaseFilter.viaOutsideHook(LastaShowbaseFilter.java:123)
	at org.lastaflute.web.servlet.filter.LastaShowbaseFilter.doFilter(LastaShowbaseFilter.java:115)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:168)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:144)
	at org.codelibs.fess.filter.WebApiFilter.doFilter(WebApiFilter.java:51)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:168)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:144)
	at org.codelibs.fess.filter.CorsFilter.doFilter(CorsFilter.java:65)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:168)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:144)
	at org.lastaflute.web.servlet.filter.LastaPrepareFilter.toNextFilter(LastaPrepareFilter.java:280)
	at org.lastaflute.web.servlet.filter.LastaPrepareFilter.viaHotdeploy(LastaPrepareFilter.java:243)
	at org.lastaflute.web.servlet.filter.LastaPrepareFilter.viaLastaDiContext(LastaPrepareFilter.java:230)
	at org.lastaflute.web.servlet.filter.LastaPrepareFilter.doFilter(LastaPrepareFilter.java:203)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:168)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:144)
	at org.codelibs.fess.filter.EncodingFilter.doFilter(EncodingFilter.java:118)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:168)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:144)
	at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:168)
	at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:90)
	at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:482)
	at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:130)
	at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:93)
	at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74)
	at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:346)
	at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:383)
	at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:63)
	at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:937)
	at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1791)
	at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:52)
	at org.apache.tomcat.util.threads.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1190)
	at org.apache.tomcat.util.threads.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:659)
	at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:63)
	at java.base/java.lang.Thread.run(Unknown Source)

A sanitised version of JWT claim looks like this (using okta)

{
    "sub":"xxx",
    "name":"X X",
    "email":"[email protected]",
    "ver":1,
    "iss":"https://xxx.okta.com",
    "aud":"xxx",
    ...
    "at_hash":"xxx",
    "groups":["group1","group2","group3","group4"]
}

You can see the groups claim is an array of groups as you'd expect, but it looks like the logic to build a string array was removed in this MR. Currently my workaround is to ensure the groups key is not present in the claim, which works but means I'm unable to map permissions.

@hi-yamap
Copy link
Contributor Author

hi-yamap commented Oct 7, 2024

The implementation was modified to enable parsing of mixed-type arrays, which changed the type of attributes["groups"].
In the previous code, attributes["groups"] was a String[], but it was changed to a List<Object>. As a result, the cast now fails.
This might not be the ideal solution, but if fixed like this, it seems to work.

private Object parseArray(JsonParser jsonParser) throws IOException {
    List<Object> list = new ArrayList<>();
    while (jsonParser.nextToken() != JsonToken.END_ARRAY) {
        if (jsonParser.getCurrentToken() == JsonToken.START_OBJECT) {
            list.add(parseObject(jsonParser));
        } else if (jsonParser.getCurrentToken() == JsonToken.START_ARRAY) {
            list.add(parseArray(jsonParser)); // Nested array
        } else {
            list.add(parsePrimitive(jsonParser));
        }
    }

    if (list.stream().allMatch(String.class::isInstance)) {
        return list.toArray(new String[list.size()]);
    }

    return list;
}

@anguswilliams
Copy link

Hey @hi-yamap, are you happy to put through a pull request for this? I can if not, would be good to fix this, as a list of groups seems pretty common.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants