-
Notifications
You must be signed in to change notification settings - Fork 114
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
base: v3.x
Are you sure you want to change the base?
Conversation
781f307
to
6e909dc
Compare
6e909dc
to
04775b5
Compare
[] | ||
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] |
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.
@v-m-i I have several issues with this approach:
- this hole
[0]
approach seems kind of hacky, why not just leave[]
as the default & useempty
:
- what if someone runs it like this:
bin/console trikoder:oauth2:update-client 1 --redirect-uri=url --redirect-uri
, they would get:
I'm thinking maybe this would be a better approach:
- leave
InputOption::VALUE_REQUIRED
&[]
as the default - 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?
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.
Hi @HypeMC
-
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. -
This comand sets attributes it has recieved, meaning that after running
trikoder:oauth2:update-client 1 --scope=s3
client1
would have onlys3
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 Report
@@ 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
Continue to review full report at Codecov.
|
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
, addedactive
option.