Allow to create AuthenticationFailedException with attributes that puts the failure in a better context

[michalvavrik]

• this is required for the Authentication failure exceptions and events should provide additional request specific information quarkus#45207

[sberyozkin]

May be it is worth to have a package private AbstractAuthenticationException extends SecurityException implements AuthenticationException that all quarkus-security exceptions will extend ? Though we can do it later for sure, please don't spend time on it for now

[michalvavrik]

May be it is worth to have a package private AbstractAuthenticationException extends SecurityException implements AuthenticationException that all quarkus-security exceptions will extend ? Though we can do it later for sure, please don't spend time on it for now

I thought about it, it is not about time, I am in no hurry. It's more about - does it make sense to have such attributes for other exceptions? If you have examples in mind, then I can do it. In general, I think we should only add there what is necessary and hard to access from elsewhere. If you can use a SecurityEvent and get routingcontext from there and the failure message, what else do you need?

[michalvavrik]

We have time, but if @FroMage is busy, I think any of @gsmet or @cescoffier could review as well. It is fairly small change and we need Quarkus Security API release before I can finish this in Quarkus main project.

[michalvavrik]

I realized this is related to quarkusio/quarkus#44993 so if @FroMage finds time next week, great. I'll add Clement and Guillaume as IMO we just need one more reviewer to go.

[michalvavrik]

The change is quite trivial, I think if each review in this repo continues to take a long time, it won't motivate us to actually put stuff here and we will probably prefer Quarkus Security runtime SPI that is in the main repo. (I am speaking for myself, not anyone else)

[gsmet]

No need to change it but if I had a recommendation, in this case, it would be easier to initialize the attributes to an empty Map.of() instead of null when no attributes.

It won't require any additional memory and it would simplify a few things.

[michalvavrik]

No need to change it but if I had a recommendation, in this case, it would be easier to initialize the attributes to an empty Map.of() instead of null when no attributes.

It won't require any additional memory and it would simplify a few things.

thanks @gsmet , I'll change it when I am at my laptop later today.

[michalvavrik]

Fixed, thanks for the suggestion.

[gsmet]

@michalvavrik just a warning: we need to make sure that no security-sensitive attributes end up in a generated message.

[michalvavrik]

@michalvavrik just a warning: we need to make sure that no security-sensitive attributes end up in a generated message.

Understood, I'll keep it in mind. There will be discussion what should end-up there anyway as I have tried to implement quarkusio/quarkus#45207 a week ago or so and I found only 2 situations, only one I found a test for. I'll open discussion in the issue to clarify when and what should be there.