Show error in case of an unkown essential acr claim. Make sure correc…#10088
Show error in case of an unkown essential acr claim. Make sure correc…#10088mposolda merged 3 commits intokeycloak:mainfrom
Conversation
…t acr is set after authentication flow during step-up authentication Closes keycloak#8724 Co-authored-by: Cornelia Lahnsteiner <[email protected]>
|
@CorneliaLahnsteiner @romgeFYI. I sent discussion with some ideas for improvements / changes to step-up authentication. It is here #10120 . Your feedback is welcome! (I am adding it here even if this discussion is not strictly related to this PR. For future feedback regarding the discussion, please comment in the discussion.) |
|
@mposolda Thanks for the information. I will definitely look at the ideas for improvements/changes next week (quite a lot going on this week unfortunately)! I already have a few on my list to add. |
| private final AuthenticationProcessor processor; | ||
| private final AuthenticationFlowModel flow; | ||
| private boolean successful; | ||
| private boolean successful = false; |
There was a problem hiding this comment.
May I suggest Boolean.FALSE?
There was a problem hiding this comment.
Hm... I am not sure if it is better to use Boolean.FALSE ? This would mean one more necessity for auto-boxing from Boolean object (Boolean.FALSE) to primitive value false. And the successful is a primitive variable, which is not sent anywhere and not parsed from/to String etc and hence is completely fine with stick only to primitive value (true/false) IMO without need of any boxing/unboxing from/to Java object of type Boolean? IMO best for performance and also readability.
Or do you have some article or resource, which recommends to not use primitive for such use-case?
There was a problem hiding this comment.
It seems fine to me, just following some guides to avoid constants. But not any problem or technical need.
There was a problem hiding this comment.
Thanks for clarification. I consider constants makes sense especially for strings and numbers. Also for example when I want to specify for example default value or when I want to search for the usage. Pretty much most of the use-cases like used for example in class org.keycloak.models.Constants . For this particular use-case, I don't see much value of using Boolean.FALSE instead of false . But thanks for bringing this.
|
@mabartos @marcelomrwin Thanks for the review and feedback on this! |
…t acr is set after authentication flow during step-up authentication
Closes #8724
Co-authored-by: Cornelia Lahnsteiner [email protected]