-
Notifications
You must be signed in to change notification settings - Fork 13
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
Allow user to cherry pick scopes. #28
Conversation
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.
It might be too much, but in my testing, when I used a different userURL, the form seems to accept it. Of course, the password will not match etc, so there will be no signed code, but maybe we can display the username, or, since we only support one, check for a valid one before showing the form?
Bit unrelated to this PR maybe, I can open a new issue.
edit: moving this to it's own issue
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.
Tested a bit with all kinds of scopes, looks good to me.
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.
DO NOT MERGE
@@ -270,6 +274,18 @@ function get_q_value($mime, $accept) | |||
error_page('Login Failed', 'Invalid username or password.'); | |||
} | |||
|
|||
$scope = filter_input_regexp(INPUT_POST, 'scopes', '@^[\x21\x23-\x5B\x5D-\x7E]+$@', FILTER_REQUIRE_ARRAY); |
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 had an unexpected output trying to use an emoji as scope. Suggesting this does not return false
when one of the scopes is wrong, but will update the scopes
array to contain false
. This means the following check against false
does nothing and the implode()
will actually add extra spaces.
Needs some more testing before merging!
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.
This is addressed by 17bdeea.
I originally misread what FILTER_REQUIRE_ARRAY did. It does not return false if a single item in the array does not match, but sets that array item to false instead. This adds a check and errors out if a scope is set to false instead of going ahead and imploding the array.
If you have time, @sebsel, please rerun your scope tests to make sure my new commit did not break anything. |
Would be nice to have some automation in these tests. I don't know exactly what I did last time 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.
reviewed with @Zegnat at IWC Berlin, LGTM
This presents the requested scopes in a list with checkboxes so the user can uncheck any they do not wish to grant. The default is to have all the requested scopes checked.
First step towards #26.