-
Notifications
You must be signed in to change notification settings - Fork 97
feat: Add support for device code grant #229
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
base: master
Are you sure you want to change the base?
Conversation
chalasr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Can you rebase it? Here are some comments also
|
Hi @chalasr I'm not entirely sure I understood correctly what you wanted for the docs/device-code-grant.md improvements, but I tried my best to fit to your feedback :) PR is rebased ! |
|
Interested in this PR, but there's been no activity for a few months. Anything we can do to help? |
|
+1 interested as well ! |
ajgarlag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work here. I've managed to create a working demo. I've reviewed your code and added some suggestions.
| $this->deviceCodeManager->save($deviceCode, $newDeviceCode); | ||
| } | ||
|
|
||
| public function approveDeviceCode(string $userCode, string $userId): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether this method should be included in the bundle. I've created a working demo that doesn't use this it.
Either way, I'll move this method to the DeviceCodeManagerInterface and rename it resolveDeviceCode, adding a third required boolean parameter to indicate whether the request has been approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether this method should be included in the bundle
Oauth2-server does bring a DeviceCodeGrant::completeDeviceAuthorizationRequest method already but I'm struggling to understand how they expect it to be used. It fetches from the manager by $deviceCode where I'm pretty sure we need to use $userCode... Maybe I'm missing something.
I'll move this method to the DeviceCodeManagerInterface and rename it resolveDeviceCode, adding a third required boolean parameter to indicate whether the request has been approved.
I'm happy to move to DeviceCodeManagerInterface, however it will result in two duplicated methods with the exact same logic (for in memory and doctrine) - If that's fine by you let me know and I'll do another commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having reviewed it again, this method should be completely removed. There is a method in the league/oauth2-server code to approve (or deny) a device code authorization request: AuthorizationServer::completeDeviceAuthorizationRequest.
|
Happy to merge once Antonio's review comments are resolved. |
|
@SimonVanacco Friendly ping |
|
Hi @ajgarlag Thank you for the review, and sorry for the delay. The past few weeks have been a bit hectic :) I implemented some of your requests (new configuration options, refactoring of the repository) and left comments on the ones I won't be able to do myself without some guidance : Supporting Symfony routes for the verification_uri and removing approveDeviceCode. Let me know your thoughts on the update and I'll try to be quicker to respond now ! |
|
Thanks. Supporting route names can be done in a follow-up PR, no problem. Regarding |
| private function updateDeviceCodeModel( | ||
| DeviceCodeInterface $deviceCode, | ||
| DeviceCodeEntityInterface $deviceCodeEntity, | ||
| ): DeviceCodeInterface { | ||
| if ($deviceCodeEntity->getLastPolledAt()) { | ||
| $deviceCode->setLastPolledAt($deviceCodeEntity->getLastPolledAt()); | ||
| } | ||
|
|
||
| if ($deviceCodeEntity->getUserIdentifier()) { | ||
| $deviceCode->setUserIdentifier($deviceCodeEntity->getUserIdentifier()); | ||
| $deviceCode->setUserApproved($deviceCodeEntity->getUserApproved()); | ||
| } | ||
|
|
||
| return $deviceCode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this method mutates the received $deviceCode, I think it should not return anything.
| private function updateDeviceCodeModel( | |
| DeviceCodeInterface $deviceCode, | |
| DeviceCodeEntityInterface $deviceCodeEntity, | |
| ): DeviceCodeInterface { | |
| if ($deviceCodeEntity->getLastPolledAt()) { | |
| $deviceCode->setLastPolledAt($deviceCodeEntity->getLastPolledAt()); | |
| } | |
| if ($deviceCodeEntity->getUserIdentifier()) { | |
| $deviceCode->setUserIdentifier($deviceCodeEntity->getUserIdentifier()); | |
| $deviceCode->setUserApproved($deviceCodeEntity->getUserApproved()); | |
| } | |
| return $deviceCode; | |
| private function updateDeviceCodeModel( | |
| DeviceCodeInterface $deviceCode, | |
| DeviceCodeEntityInterface $deviceCodeEntity, | |
| ): void { | |
| if ($deviceCodeEntity->getLastPolledAt()) { | |
| $deviceCode->setLastPolledAt($deviceCodeEntity->getLastPolledAt()); | |
| } | |
| if ($deviceCodeEntity->getUserIdentifier()) { | |
| $deviceCode->setUserIdentifier($deviceCodeEntity->getUserIdentifier()); | |
| $deviceCode->setUserApproved($deviceCodeEntity->getUserApproved()); | |
| } |
| $client = $this->clientRepository->getClientEntity($deviceCode->getClient()->getIdentifier()); | ||
| if ($client) { | ||
| $deviceCodeEntity->setClient($client); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent the casting of an object to a boolean value
| $client = $this->clientRepository->getClientEntity($deviceCode->getClient()->getIdentifier()); | |
| if ($client) { | |
| $deviceCodeEntity->setClient($client); | |
| } | |
| if (null !== $client = $this->clientRepository->getClientEntity($deviceCode->getClient()->getIdentifier())) { | |
| $deviceCodeEntity->setClient($client); | |
| } |
| if ($deviceCode->getLastPolledAt()) { | ||
| $deviceCodeEntity->setLastPolledAt($deviceCode->getLastPolledAt()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent the casting of an object to a boolean value, and prevent calling the same method twice
| if ($deviceCode->getLastPolledAt()) { | |
| $deviceCodeEntity->setLastPolledAt($deviceCode->getLastPolledAt()); | |
| } | |
| if (null !== $lastPolledAt = $deviceCode->getLastPolledAt()) { | |
| $deviceCodeEntity->setLastPolledAt($lastPolledAt); | |
| } |
| if ($deviceCode->getUserIdentifier()) { | ||
| $deviceCodeEntity->setUserIdentifier($deviceCode->getUserIdentifier()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent the casting of a string to a boolean value, and prevent calling the same method twice
| if ($deviceCode->getUserIdentifier()) { | |
| $deviceCodeEntity->setUserIdentifier($deviceCode->getUserIdentifier()); | |
| } | |
| if (null !== $userIdentifier = $deviceCode->getUserIdentifier()) { | |
| $deviceCodeEntity->setUserIdentifier($userIdentifier); | |
| } |
This PR adds support for device code grants, which has been available for some time in oauth2-server
Ready to be tested, I'd love to get some feedback if you think some areas could be improved !
Design considerations