Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add event in firewall / guard #251

Draft
wants to merge 49 commits into
base: v3.x
Choose a base branch
from

Conversation

froozeify
Copy link

@froozeify froozeify commented Dec 6, 2020

This PR aim to add support of Event inside the Firewall and Guard authenticator.

More details about the starting discussion here

What's this PR changes

  • Add event inside the security Firewall
    • Add events
    • Create doc
  • Add event inside the security Guard
    • Add events
    • Create doc
  • Add the possibility to change the format of the error response (currently was no formatting).
    • Return a json

@codecov-io
Copy link

codecov-io commented Dec 6, 2020

Codecov Report

Merging #251 (95a690d) into v3.x (37571a1) will decrease coverage by 0.44%.
The diff coverage is 86.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##               v3.x     #251      +/-   ##
============================================
- Coverage     92.50%   92.05%   -0.45%     
- Complexity      442      456      +14     
============================================
  Files            66       69       +3     
  Lines          1600     1648      +48     
============================================
+ Hits           1480     1517      +37     
- Misses          120      131      +11     
Impacted Files Coverage Δ Complexity Δ
DependencyInjection/Configuration.php 100.00% <ø> (ø) 14.00 <0.00> (ø)
DependencyInjection/TrikoderOAuth2Extension.php 91.66% <ø> (-0.34%) 27.00 <0.00> (ø)
Event/AuthenticationFailureEvent.php 55.55% <55.55%> (ø) 4.00 <4.00> (?)
Event/AuthenticationScopeFailureEvent.php 60.00% <60.00%> (ø) 2.00 <2.00> (?)
.../Exception/MissingAuthorizationHeaderException.php 66.66% <66.66%> (ø) 5.00 <5.00> (?)
.../Exception/OAuth2AuthenticationFailedException.php 66.66% <66.66%> (ø) 5.00 <5.00> (?)
Response/ErrorJsonResponse.php 100.00% <100.00%> (ø) 1.00 <1.00> (?)
Security/Exception/InsufficientScopesException.php 100.00% <100.00%> (ø) 1.00 <1.00> (ø)
Security/Firewall/OAuth2Listener.php 100.00% <100.00%> (ø) 7.00 <0.00> (ø)
...curity/Guard/Authenticator/OAuth2Authenticator.php 96.55% <100.00%> (+1.55%) 20.00 <1.00> (+1.00)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37571a1...95a690d. Read the comment docs.

@froozeify
Copy link
Author

froozeify commented Dec 26, 2020

This PR is ready to be merge, for the moment I've created a response_formatter but I think the main goal would be to remove it and only use JsonResponse for a better consistency. (maybe for the next major release ?)

I've created this formatter to avoid break current user implementation on error handling (currently the error is return directly in the body with no formatting).

@X-Coder264
Copy link
Collaborator

@froozeify I don't have the time atm to fully review this PR. But just taking a quick look I can't help but notice the removal of the exception_event_listener_priority config option and the exception listener. That is a big breaking change or am I missing something? The way I see these events as extension points is that they are being dispatched and if no response is being set on them the code fallbacks to the same behavior of what the current v3.x code does (throwing an exception and having an exception listener handle it). Otherwise I don't see how this could land on a minor v3.x release.

Also I'd prefer the response_formatter stuff to be a separate PR after the events stuff gets merged as this is otherwise quite a big change which makes it hard to review (and the events stuff doesn't need the response formatting stuff in order to be merged).

@froozeify
Copy link
Author

froozeify commented Dec 29, 2020

@X-Coder264 Yes the removal of exception_event_listener_priority was one point that makes me doubting.
But the problem that I'm having here is that the implementation of event and exception_event_listener_priority stuff can't be together (so no soft adding one and keeping the other side by side with @deprecation warning on the old method).

When I start developing the Event feature I was more thinking to add that in a major update but didn't find a v4.x (I should have asked...). And since it was my first PR on a public project forgot about it... (more used to work on company project with a continuously app evolution, so not respecting the Semantic Versioning don't have a major impact...)

What I could propose, so the Event is in the v3.x code is:

  • Add the event logic (modify with event having nullable response format) Impossible, otherwise event in v3.x will be meaningless, user will have to add response for every event (very tedious I think). So won't be used, and adding @deprecation in the old method will only spam user logs...
  • Add deprecation notification inside ConvertExceptionToResponseListener class Also not a good idea since the deprecation will always be throw (the converter is always called by default)

tl;dr: I think the event system is awesome 😉 but it can only be added in the next major release and by removing at the same time the old exception_event_listener_priority stuff since both can't live together (unless someone has an idea, but don't find one yet)


For the response_formatter stuff, it was an easy method I had found to make the transition to a more consistent app response format (currently almost all are formatted as JsonResponse and only the error are in Response format).

For that part, I will remove it from this branch.
But my main question is that with the new Event (that will be added in v4.x ??) maybe we could also format by default the errors as JsonResponse, making the need of that formatter unnecessary.

If you still want it, I can move that code in a separate issue and PR so that custom formatting could be added in a minor v3.x release. (making some user like me happy, because I'd prefer having my frontend always getting JsonResponse, and not having to guess what the response format could be).

More reasonably, I think we should not add it anymore.
Since we could convert the few response that are wrongly formatted as Response seamlessly with the new Event logic. Making after all the response consistent (always formatted in Json).

@froozeify froozeify marked this pull request as draft March 17, 2021 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants