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 Client::confidential property #140

Closed
wants to merge 4 commits into from
Closed

add Client::confidential property #140

wants to merge 4 commits into from

Conversation

florianajir
Copy link

@florianajir florianajir commented Dec 7, 2019

add Client::confidential property to fit with PKCE client validation rule:

If the client is confidential (i.e. is capable of securely storing a secret) then the secret must be validated.

https://oauth2.thephpleague.com/client-repository-interface/#validateclient--bool

@codecov-io
Copy link

codecov-io commented Dec 7, 2019

Codecov Report

Merging #140 into master will increase coverage by 0.14%.
The diff coverage is 94.73%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #140      +/-   ##
============================================
+ Coverage     90.38%   90.52%   +0.14%     
- Complexity      362      364       +2     
============================================
  Files            57       57              
  Lines          1196     1214      +18     
============================================
+ Hits           1081     1099      +18     
  Misses          115      115
Impacted Files Coverage Δ Complexity Δ
League/Repository/ClientRepository.php 93.75% <0%> (ø) 14 <0> (ø) ⬇️
Model/Client.php 94.11% <100%> (+1.01%) 14 <2> (+2) ⬆️
Command/CreateClientCommand.php 100% <100%> (ø) 4 <0> (ø) ⬇️
Command/UpdateClientCommand.php 96.66% <100%> (+0.44%) 5 <0> (ø) ⬇️

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 8becc18...68c855a. Read the comment docs.

@@ -9,5 +9,6 @@
<field name="grants" type="oauth2_grant" nullable="true" />
<field name="scopes" type="oauth2_scope" nullable="true" />
<field name="active" type="boolean" />
<field name="confidential" type="boolean" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@florianajir When Laravel Passport implemented this feature they just made the secret column nullable and then isConfidential === null !== $this->secret

laravel/passport#1065

I'm wondering what are the pros and cons of both approaches?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@X-Coder264 Good point, the only pros I see is the ability to switch from public to confidential without regenerating the secret manually but on the cons the need of having a new data attribute.
It's quite the same IMO, if we go for the nullable secret then the actual code needs more changes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@florianajir I don't see the point of having a non confidential client with a secret as the Oauth specification says that non confidential / public clients are not capable of securely storing a secret. So having this extra column seems redundant.

@X-Coder264
Copy link
Collaborator

@florianajir Thanks for the PR, but it was superseded by #167

@X-Coder264 X-Coder264 closed this Feb 24, 2020
@florianajir florianajir deleted the fix/pkce-support branch February 24, 2020 10:19
@florianajir
Copy link
Author

no problem 👍

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.

4 participants