Added redirection check for administrator and site owner roles. #351
Conversation
| // Redirecting user login page. | ||
| $userRolesArray = [ | ||
| 'administrator', | ||
| 'site_owner', |
There was a problem hiding this comment.
@kairamkondarajesh site_owner is a Y Cloud specific role and should not be here. You'll need to provide a way for this array to be supplemented by other code, or provide a comment that suggests that it be patched in case new roles need to be added.
| // Redirecting user login page. | ||
| foreach ($userRolesArray as $role) { | ||
| if ($account->hasRole($role)) { | ||
| $response = new RedirectResponse('/user/login', 301); |
There was a problem hiding this comment.
Please, add this code above:
$loginUrl = Url::fromRoute('user.login');
and replace this line:
| $response = new RedirectResponse('/user/login', 301); | |
| $response = new RedirectResponse($loginUrl, 301); |
It will be a more stable approach. I had many projects in the past when we were renaming user login/register routes to avoid spam bots attacks. In your case, it is hard-coded.
There was a problem hiding this comment.
@anpolimus I have updated the code as per you suggestion, Can you please review.
| foreach ($userRolesArray as $role) { | ||
| if ($account->hasRole($role)) { | ||
| $response = new RedirectResponse('/user/login', 301); | ||
| $response->send(); |
There was a problem hiding this comment.
It is better to add Drupal message which is saying that you have to login as real user, sin you are an administrator.
There was a problem hiding this comment.
@anpolimus I have added Drupal message, Can you please review.
|
Build 3293 |
|
Build 3293 |
|
Build 3294 |
|
Build 3294 |
|
Build 3295 |
|
Build 3295 |
| foreach ($userRolesArray as $role) { | ||
| if ($account->hasRole($role)) { | ||
| $loginUrl = Url::fromRoute('user.login')->toString(); | ||
| $this->messenger->addMessage($this->t('You have to login as real user, since you are an administrator.')); |
There was a problem hiding this comment.
Saying this user is an administrator could be a security risk. I'd suggest rewriting this to:
Please retry your login on this form.
There was a problem hiding this comment.
Updated message. Please review.
|
@AnastasiiaPys we have to QA it. |
|
Tested on macOS(Chrome). Please review my steps to reproduce whether I did it correct
Expected behavior: the user is redirected to the /user/login page Actual result: the user is logged in as Contributor The test failed. |
We are checking two user roles ( 'administrator', 'virtual_ymca_editor') to redirect to /user/login page when they try login from landing page user email option. |
| foreach ($userRolesArray as $role) { | ||
| if ($account->hasRole($role)) { | ||
| $loginUrl = Url::fromRoute('user.login')->toString(); | ||
| $this->messenger->addMessage($this->t('Please retry your login on this form.')); |
There was a problem hiding this comment.
This line is throwing an error:
Error: Call to undefined method Drupal\openy_gc_auth\GCUserAuthorizer::t() in Drupal\openy_gc_auth\GCUserAuthorizer->authorizeUser() (line 97 of /.../docroot/modules/contrib/openy_gated_content/modules/openy_gc_auth/src/GCUserAuthorizer.php)
We're reviewing for a fix.
There was a problem hiding this comment.
https://drupal.stackexchange.com/questions/257631/call-to-undefined-function-t-in-an-unit-test has some possible solutions to this.
There was a problem hiding this comment.
Updated the code as per suggestions, Please review.
|
Build 4251 |
|
Build 4251 |
|
Build 4252 |
|
Build 4252 |
|
Build 4253 |
|
Build 4253 |
|
@kairamkondarajesh ,please fix conflicts. |
|
Build 4414 |
|
Build 4414 |
ymcatwincities#140
Steps to test: