klausbetz / apple-identity-provider-keycloak

An extension for Keycloak, that enables web-based sign in with Apple and token exchange
Apache License 2.0
193 stars 27 forks source link

NullPointerException when user refuses consent #31

Closed daviddelannoy closed 1 year ago

daviddelannoy commented 1 year ago

Hi,

Version used : 1.2.0 (with KC 19.0.3, wildfly legacy)

When a user refuses Apple Consent screen (to share its email) by clicking the cancel button :

2023-02-15_17-10

Then we got this NPE :

{
  "class": "org.keycloak.services.managers.AuthenticationSessionManager",
  "method": "lambda$getUserSession$7",
  "line": 220
},
{
  "class": "org.keycloak.utils.LockObjectsForModification",
  "method": "lockObjectsForModification",
  "line": 75
},
{
  "class": "org.keycloak.utils.LockObjectsForModification",
  "method": "lockUserSessionsForModification",
  "line": 65
},
{
  "class": "org.keycloak.services.managers.AuthenticationSessionManager",
  "method": "getUserSession",
  "line": 220
},
{
  "class": "org.keycloak.services.resources.IdentityBrokerService",
  "method": "checkAccountManagementFailedLinking",
  "line": 1072
},
{
  "class": "org.keycloak.services.resources.IdentityBrokerService",
  "method": "error",
  "line": 860
},
{
  "class": "at.klausbetz.provider.AppleIdentityProviderEndpoint",
  "method": "authResponse",
  "line": 81
},
{
  "class": "jdk.internal.reflect.NativeMethodAccessorImpl",
  "method": "invoke0"
},
[...]

I think that this functional case should be handled like ACCESS_DENIED. But in both callback.cancelled() or callback.error() authenticationSession is required in checkAccountManagementFailedLinking method.

Before submitting a PR, what do you think about fetching authenticationSession here before this line :

https://github.com/klausbetz/apple-identity-provider-keycloak/blob/09a2721e425bcfe54eba298cb2925c857608fd1c/src/main/java/at/klausbetz/provider/AppleIdentityProviderEndpoint.java#L65

    @POST
    public Response authResponse(@FormParam(OAUTH2_PARAMETER_STATE) String state, @FormParam(OAUTH2_PARAMETER_CODE) String authorizationCode, @FormParam(OAUTH2_PARAMETER_USER) String user, @FormParam(OAuth2Constants.ERROR) String error) {
        if (state == null) {
            event.event(EventType.IDENTITY_PROVIDER_LOGIN);
            event.error(Errors.IDENTITY_PROVIDER_LOGIN_FAILURE);
            return ErrorPage.error(session, null, Response.Status.BAD_GATEWAY, Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR);
        }

        IdentityBrokerState idpState = IdentityBrokerState.encoded(state);
        String clientId = idpState.getClientId();
        String tabId = idpState.getTabId();

        AuthenticationSessionModel authSession = this.callback.getAndVerifyAuthenticationSession(state);
        session.getContext().setAuthenticationSession(authSession);

[...]

just like it is done in AbstractOAuth2IdentityProvider#authResponse() ?

It fixes the issue. I think this code can be removed too

What do you think @klausbetz ?

klausbetz commented 1 year ago

Hi @daviddelannoy πŸ˜„

It's great to see you digging into this one!
This bug indeed should be fixed. Thx for submitting a PR. I'll give it a test and closer look.

daviddelannoy commented 1 year ago

You're welcome πŸ˜‰

Thanks for maintaining this provider! πŸ‘