InseeFr / Keycloak-FranceConnect

Extension Keycloak facilitant l'utilisation de FranceConnect
MIT License
87 stars 31 forks source link

Environment management & unit tests #62

Closed mboisnard closed 2 years ago

mboisnard commented 2 years ago
mboisnard commented 2 years ago

Questions:

micedre commented 2 years ago

Questions:

* Why override the `keycloakInitiatedBrowserLogout` method in AbstractBaseIdentityProvider? It seems that OIDCIdentityProvider.keycloakInitiatedBrowserLogout do the same thing with token expiration management (in OIDCIdentityProvider.getIDTokenForLogout method) I think we can override the method only in FCIdentityProvider to fix [[BUG] Manage logout for FranceConnect+ #60](https://github.com/InseeFr/Keycloak-FranceConnect/issues/60).

There was an issue in Keycloak https://issues.redhat.com/browse/KEYCLOAK-7209, which caused some errors with France Connect. There was also a bug with FranceConnect (with state) , and this override was to mitigate this. https://github.com/InseeFr/Keycloak-FranceConnect/commit/a2513d92c1600498dec0a7d06db8a208835bdc15

That being said, I'm not sure it is still necessary.

On the logout, I don't think there is a compatibility between FCv1 and FCv2 but I didn't really check it yet.

* Currently FranceConnectIntegrationV1 & FranceConnectProductionV1 only work with EIDAS 1 level. If you try to use another level it will raise an exception because there is no issuer url configured. Is it good for you? Should we prevent this case?

Yes, we should, good catch.

mboisnard commented 2 years ago

That being said, I'm not sure it is still necessary.

Ok it will be interesting to check this point with fix #60

Yes, we should, good catch.

With our code, I think we can add these checks:

` public FranceConnectIdentityProvider(KeycloakSession session, FranceConnectIdentityProviderConfig config) { super(session, config, useJwks(config) ? Utils.getJsonWebKeySetFrom(config.getJwksUrl(), session) : null); }

public static boolean useJwks(FranceConnectIdentityProviderConfig config) { return config.isUseJwksUrl() && config.getJwksUrl() != null; }

... else if (APPLICATION_JWT_TYPE.isCompatible(contentMediaType) && useJwks(getConfig())) { ... var eidasLevel = getConfig().getEidasLevel(); var jwt = EIDAS1.equals(eidasLevel) ? response.asString() : decryptJWE(response.asString()); } ` Public keys are stored in AbstractBaseIdentityProvider so we can't verify userinfo JWT if there is no jwks url. Edit: It's possible to have a signed UserInfo without public key only if JWT is signed in HS256, so idk ... ^^

If it's good for you, I can add this fix in this PR.

micedre commented 2 years ago

Yes you can add it as is. This is not strictly OIDC compliant but we only want to be compatible with France Connect.

Could you squash some of your commit together ?

mboisnard commented 2 years ago

Done, I added the isJWETokenFormatRequired method to avoid the issue and left only main commits.

micedre commented 2 years ago

thanks !