[WIP] Add documentation for authorization code grant #177
Conversation
Codecov Report
@@ Coverage Diff @@
## v3.x #177 +/- ##
============================================
+ Coverage 90.89% 91.48% +0.59%
- Complexity 367 454 +87
============================================
Files 56 66 +10
Lines 1208 1621 +413
============================================
+ Hits 1098 1483 +385
- Misses 110 138 +28
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@v-m-i
Thanks for the great documentation.
I didn't understand the following two points, so the documentation was very helpful.
- How to change the path of the endpoint
- How to handle redirect before authentication
I commented on some interesting points.
I have implemented my own authorization confirmation page.
- login page
- authorization confirmation page(ex. Do you allow read permission for this application?)
- redirect with authorization code
I would like to know if there is a best practice on how to set the authorization confirmation page.
Here is my code implemented without documentation.
https://github.com/okazy/ec-cube/compare/f604226b42131689f763a2d87af0994173c87390...okazy:d981ba70858282c1353f36367299db04053d03ff?expand=1
Co-Authored-By: Hideki Okajima <hideki518c@gmail.com>
|
@okazy Regarding authorization confirmation page, I don't know best practice for creating it. @HypeMC |
tdutrion
left a comment
tdutrion
left a comment
There was a problem hiding this comment.
Hi! Thanks for this PR, it has been very useful to me!
Just a couple of changes that might be interesting here :)
|
|
||
| ## Requirements | ||
|
|
||
| To use authorization code grant `enable_auth_code_grant` parameter inside `authorization_server` must be set to `true` (it is set to `true` by default). |
There was a problem hiding this comment.
[info] User Deprecated: "trikoder_oauth2.authorization_server.enable_auth_code_grant" is deprecated, use "trikoder_oauth2.authorization_server.grant_types.authorization_code.enable" instead.
This should be updated to use the new parameters.
There was a problem hiding this comment.
@tdutrion
Thank you for noticing this, I have updated the documentation.
|
|
||
| public function onAuthorizationRequestResolve(AuthorizationRequestResolveEvent $event) | ||
| { | ||
| if (null !== $event->getUser()) { |
There was a problem hiding this comment.
You could reverse the condition (null === $event->getUser()) and just return after setting the response:
public function onAuthorizationRequestResolve(AuthorizationRequestResolveEvent $event): void
{
if (null === $event->getUser()) {
$event->setResponse(new Response(302, [
'Location' => $this->urlGenerator->generate('login', [
'returnUrl' => $this->requestStack->getMasterRequest()->getUri(),
]),
]));
return;
}
$event->resolveAuthorization(AuthorizationRequestResolveEvent::AUTHORIZATION_APPROVED);
}There was a problem hiding this comment.
@tdutrion
I agree, simplifying if statements using early returns is (usually) good practice. I have updated the documentation with your example.
|
@trikoder maybe it's time to accept this PR? |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.x #177 +/- ##
============================================
+ Coverage 90.89% 91.48% +0.59%
- Complexity 367 454 +87
============================================
Files 56 66 +10
Lines 1208 1621 +413
============================================
+ Hits 1098 1483 +385
- Misses 110 138 +28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6 participants
Closes #160