davido / gerrit-oauth-provider

OAuth2 authentication provider for Gerrit Code Review. Please upload changes for review to: https://gerrit-review.googlesource.com/#/admin/projects/plugins/oauth
Apache License 2.0
141 stars 84 forks source link

Error extracting token from Keycloak response #139

Closed ctlajoie closed 4 years ago

ctlajoie commented 4 years ago

I am getting this error in Gerrit after keycloak redirects back to Gerrit. There are no errors on the keycloak side - as far as it is concerned, nothing went wrong. It looks like it's trying to parse form parameters from a JSON response.

[2020-03-04 20:07:15,942] [HTTP-106] WARN  org.eclipse.jetty.server.HttpChannel : /oauth
com.github.scribejava.core.exceptions.OAuthException: Response body is incorrect. Can't extract a 'access_token=([^&]+)' from this: '{"access_token":"eyJhbGciOiJSUzI1NiIsInR.(trimmed)","expires_in":300,"refresh_expires_in":115200,"refresh_token":"eyJhbGciOiJIUzI1N.(trimmed)","not-before-policy":0,"session_state":"f54ae4a2-a675-419b-8765-44e9e57ff7fd","scope":"openid profile email"}'
    at com.github.scribejava.core.extractors.OAuth2AccessTokenExtractor.extractParameter(OAuth2AccessTokenExtractor.java:70)
    at com.github.scribejava.core.extractors.OAuth2AccessTokenExtractor.extract(OAuth2AccessTokenExtractor.java:48)
    at com.github.scribejava.core.extractors.OAuth2AccessTokenExtractor.extract(OAuth2AccessTokenExtractor.java:16)
    at com.github.scribejava.core.oauth.OAuth20Service.sendAccessTokenRequestSync(OAuth20Service.java:53)
    at com.github.scribejava.core.oauth.OAuth20Service.getAccessToken(OAuth20Service.java:97)
    at com.github.scribejava.core.oauth.OAuth20Service.getAccessToken(OAuth20Service.java:92)
    at com.googlesource.gerrit.plugins.oauth.KeycloakOAuthService.getAccessToken(KeycloakOAuthService.java:123)
    at com.google.gerrit.httpd.auth.oauth.OAuthSession.login(OAuthSession.java:104)
    at com.google.gerrit.httpd.auth.oauth.OAuthWebFilter.doFilter(OAuthWebFilter.java:105)
    at com.google.gerrit.httpd.RequireSslFilter.doFilter(RequireSslFilter.java:72)
    at com.google.gerrit.httpd.RunAsFilter.doFilter(RunAsFilter.java:120)
    at com.google.gerrit.httpd.AllRequestFilter$FilterProxy$1.doFilter(AllRequestFilter.java:133)
    at com.google.gerrit.httpd.AllRequestFilter$FilterProxy.doFilter(AllRequestFilter.java:135)
    at com.google.gerrit.httpd.RequestMetricsFilter.doFilter(RequestMetricsFilter.java:57)
    at com.google.gerrit.httpd.RequestContextFilter.doFilter(RequestContextFilter.java:69)
    at com.google.inject.servlet.ManagedFilterPipeline.dispatch(ManagedFilterPipeline.java:121)
    at com.google.inject.servlet.GuiceFilter.doFilter(GuiceFilter.java:133)
    at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1610)
    at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:540)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:255)
    at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1700)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:255)
    at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1345)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:203)
    at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:480)
    at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1667)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:201)
    at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1247)
    at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:144)
    at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:132)
    at org.eclipse.jetty.server.Server.handle(Server.java:505)
    at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:370)
    at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:267)
    at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:305)
    at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:103)
    at org.eclipse.jetty.io.ChannelEndPoint$2.run(ChannelEndPoint.java:117)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:333)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:310)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:168)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:126)
    at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:366)
    at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:698)
    at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:804)
    at java.lang.Thread.run(Thread.java:748)
davido commented 4 years ago

This is a regression from scribejava upgrade: [1].

I think the solution would be to drop the override, so this method should be removed, because the default is already Json extractor: [2].

[1] https://gerrit-review.googlesource.com/c/plugins/oauth/+/253929/2/src/main/java/com/googlesource/gerrit/plugins/oauth/KeycloakApi.java#48 [2] https://github.com/scribejava/scribejava/blob/master/scribejava-core/src/main/java/com/github/scribejava/core/builder/api/DefaultApi20.java#L41

ctlajoie commented 4 years ago

I tested that solution (removing the override for getAccessTokenExtractor()) and that works fine.

davido commented 4 years ago

@ctlajoie Sorry for the troubles and thanks for confirming!

I will add more unit tests in this code area to avoid future regressions and will coduct a new release.

davido commented 4 years ago

I have sent a fix for review: [1].

[1] https://gerrit-review.googlesource.com/c/plugins/oauth/+/258255

davido commented 4 years ago

The mentioned CL was submitted. I will merge the fix up to master and conduct a new release.