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

Fix UpdateClientCommand options #192

Open
wants to merge 1 commit into
base: v3.x
Choose a base branch
from

Conversation

v-m-i
Copy link

@v-m-i v-m-i commented Mar 30, 2020

Closes #191

Array options won't be deleted if they were not sent.
Client won't be activated if deactivated flag was not sent.
Removed deactivated, added active option.

[]
InputOption::VALUE_OPTIONAL | InputOption::VALUE_IS_ARRAY,
'Sets redirect uri for client. Use this option multiple times to set multiple redirect URIs. Use it without value to remove existing values.',
[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

@v-m-i I have several issues with this approach:

  1. this hole [0] approach seems kind of hacky, why not just leave [] as the default & use empty:
    image
  2. what if someone runs it like this: bin/console trikoder:oauth2:update-client 1 --redirect-uri=url --redirect-uri, they would get:
    image

I'm thinking maybe this would be a better approach:

  1. leave InputOption::VALUE_REQUIRED & [] as the default
  2. add --remove-* equivalents, eg --remove-grant-type

Then if someone has a client with the scopes s1 & s2, they could do: trikoder:oauth2:update-client 1 --scope=s3 --remove-scope=s1 & would end up with a client that has the scopes s2 & s3. Hint: you could use array_merge & array_diff.

@spideyfusion @X-Coder264 Any thoughts on this one?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @HypeMC

  1. I agree it is hacky, you are right, there is no reason to use [0] when I could have used [], I will change it if we chose this "set" approach.

  2. This comand sets attributes it has recieved, meaning that after running trikoder:oauth2:update-client 1 --scope=s3 client 1 would have only s3 scope, regardless of how many scopes it had before.

We have two requirements to satisfy here:
1. User can remove all scopes from client
2. User can set scopes for client

I think that in order to satisfy first requirement our InputOption must be VALUE_OPTIONAL because trikoder:oauth2:update-client 1 --scope is the only way to remove all scopes with "set" approach?

Add/Remove approach you have mentioned is something I would like more than this "set" approach. If we would decide to go with that approach, I would suggest that --scope, and other methods are prefixed with add, like this: trikoder:oauth2:update-client 1 --add-scope=s3 --remove-scope=s1

@codecov-io
Copy link

Codecov Report

Merging #192 into v3.x will increase coverage by 0.06%.
The diff coverage is 97.82%.

Impacted file tree graph

@@             Coverage Diff              @@
##               v3.x     #192      +/-   ##
============================================
+ Coverage     92.05%   92.12%   +0.06%     
- Complexity      381      391      +10     
============================================
  Files            56       56              
  Lines          1272     1296      +24     
============================================
+ Hits           1171     1194      +23     
- Misses          101      102       +1     
Impacted Files Coverage Δ Complexity Δ
Command/UpdateClientCommand.php 96.10% <97.82%> (-0.13%) 15.00 <6.00> (+10.00) ⬇️

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 6d65806...04775b5. Read the comment docs.

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