oauth2-bundle
oauth2-bundle copied to clipboard
Add event in firewall / guard
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
- [x] Add event inside the security Firewall
- [x] Add events
- [x] Create doc
- [x] Add event inside the security Guard
- [x] Add events
- [x] Create doc
- [x] Add the possibility to change the format of the error response (currently was no formatting).
- [x] Return a json
Codecov Report
Merging #251 (95a690d) into v3.x (37571a1) will decrease coverage by
0.44%
. The diff coverage is86.66%
.
@@ 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.
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).
@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).
@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).