Fix issue #29: Add verification for eidas level

[bntan]

Fix issue #29: Add verification for eidas level See: https://github.com/InseeFr/Keycloak-FranceConnect/issues/29

[micedre]

Could you rebase on last master? we had a lot of modification recently, thanks :)

[bntan]

@micedre, rebase on the last master done!

[mboisnard]

Hello, do you really need a pattern matcher for this verification? I think we can check eidas level using ordinal method on EidasLevel enum inside the getFederatedIdentity method. This method is used by the default authResponse implementation (and it can avoid code duplication anyway)

[bntan]

Hello @mboisnard,

For the pattern matcher, it is because my code was based on the master before your yesterday commits where all acr values were strings (this.config.getAcrValues()). Let's do it in the opposite side: converting the returned string acr values (in the id_token) to EidasLevel and use the ordinal method.

I haven't used overwritted getFederatedIdentity method (or extractIdentity method) because I wanted to have a specific error in the case the acr returned by FC is less than the acr requested i.e. having return ErrorPage.error(session, null, Response.Status.BAD_GATEWAY, ACR_INSUFFICIENT_MESSAGE); instead of falling into the unexpected error ErrorPage.error(session, null, Response.Status.BAD_GATEWAY, Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR), so we can display a better explicit message to the final user or do other things in this particular case. What do you think?

Regards,

[micedre]

Hello,

Ok for using the enum, instead of pattern matching.

I think it is best to avoid overriding more methods from keycloak, to avoid complication on update. I think we could try to validate the eidas level in getFederatedIdentity() and log the exact error instead of returning it to the end user (and maybe do an issue with keycloak to be able to add our own message to the end user in this case).

[bntan]

Thanks @micedre and @mboisnard for your inputs. I will do the change according to what we had said and keep you posted. Regards,

[bntan]

@micedre, @mboisnard, discussed item commited:

• Use enum instead of pattern matching
• Use unexpected error + logs instead of specific error: Regards,

[bntan]

@mboisnard, new code commited:

• Remove unnecessary null check
• Remove issue URL
• Rename acr to eidasLevel
• Consider no matching return acr as null
• Remove eof empty line

Other comments?

[bntan]

@mboisnard, done! And to even beautify the code, I have also used an existing constant when adding acr_values in createAuthorizationUrl method. Thanks for the great comments :)

[mboisnard]

@micedre is it good for you?

[micedre]

yes seems fine to me, thank you both for all that.