Add notice to the login screen to tell the user that an application w… #59
Conversation
…ants to access - fixes #43
DeepDiver1975
requested review from
SamuAlfageme,
joneug,
lukebier,
ogoffart and
tomneedham
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
============================================
- Coverage 91.85% 88.92% -2.93%
- Complexity 166 171 +5
============================================
Files 20 20
Lines 577 596 +19
============================================
Hits 530 530
- Misses 47 66 +19
Continue to review full report at Codecov.
|
ogoffart
left a comment
ogoffart
left a comment
There was a problem hiding this comment.
The screenshot looks good, and it seems that's what we want.
But i'm not qualified to review PHP code.
| var $loginMessage = $('#body-login').find('#message'); | ||
| if ($loginMessage.length) { | ||
| var client = $("data[key='oauth2']").attr('value'); | ||
| var msg = t('oauth2', 'The application "{app}" is requesting access to your account. To authorize it, please log in first.', {app : client}); |
There was a problem hiding this comment.
Forgive my ignorance, as i don't really know the frameworks involved... But is 'client' getting html-escaped somewhere?
There was a problem hiding this comment.
afaik t() is doing this properly .... will have a second look ....
There was a problem hiding this comment.
Just tested - it is escaped - thx for the hint
| }); | ||
| } | ||
|
|
||
| public function boot() { |
There was a problem hiding this comment.
So if I understand correctly, this function is called for the login screen (Is it the login screen only or all the screens?) and injects the javascript that will change the login message.
There was a problem hiding this comment.
it will be called for any request-
There was a problem hiding this comment.
Should you not check that it is only for the login screen?
There was a problem hiding this comment.
the js code is doing this - not perfect but enough
| } | ||
| $params = []; | ||
| parse_str($urlParts['query'], $params); | ||
| if (!isset($params['client_id'])) { |
There was a problem hiding this comment.
You are checking for redirect_uri and client_id but in the controller function there are some more checks.
There was a problem hiding this comment.
Besides that: looks good. 👍
There was a problem hiding this comment.
basically 'just because' I don't need the other parameter at this point.
There was a problem hiding this comment.
Ok. I just thought that a request with missing parameters at the login stage will eventually fail at the authorize stage...
…ants to access - fixes #43