Issue #55: [UPD] make userkey respect suspended status of users #87
Conversation
|
Hi @IJOL ! Thanks for your patch. It mostly looks ok. Just a couple of things to fix before we can land it,
|
|
Hi @IJOL any chance you can fix issues before we can merge the code in? |
dmitriim
changed the title
…ing event will add a log record to an application log."
|
Hmm, is starting to be self evident that i need some help to pass the tests, is my first test case for php ever, and I'm little confused.. any help will be great.. |
|
|
||
| $user = $this->getDataGenerator()->create_user(); | ||
| $user->suspended = 1; | ||
| $this->setUser($user); |
There was a problem hiding this comment.
What are you trying to test here? I don't think you need set a user here and then try to login again, but using a key.
| public function test_throwing_exception_if_user_is_suspended() { | ||
| global $USER, $SESSION; | ||
|
|
||
| $user = $this->getDataGenerator()->create_user(); |
There was a problem hiding this comment.
there is already created user for the test. You can use $this->user.
| global $USER, $SESSION; | ||
|
|
||
| $user = $this->getDataGenerator()->create_user(); | ||
| $user->suspended = 1; |
There was a problem hiding this comment.
you need to save this state to DB.
| $this->create_user_private_key(); | ||
| $_POST['key'] = 'TestKey'; | ||
|
|
||
| $this->expectException(invalid_parameter_exception::class); |
There was a problem hiding this comment.
before checking this, can you please check that login failed event has been triggered?
| @@ -172,6 +172,15 @@ public function user_login_userkey() { | |||
| $this->userkeymanager->delete_keys($key->userid); | |||
|
|
|||
| $user = get_complete_user_data('id', $key->userid); | |||
| if (!empty($user->suspended)) { | |||
| $failurereason = AUTH_LOGIN_SUSPENDED; | |||
There was a problem hiding this comment.
do you really need this variable? Could be passed as is to the code below.
| $event = \core\event\user_login_failed::create(array('userid' => $user->id, | ||
| 'other' => array('username' => $user->username, 'reason' => $failurereason))); | ||
| $event->trigger(); | ||
| throw new invalid_parameter_exception('User suspended'); |
There was a problem hiding this comment.
If this error will be displayed to a user, can we please replace this with moodle_exception + add a lang string so it could be localised.
|
Thanks @IJOL |
No description provided.