#27 FEATURE: Configure expiration time for passwords #50
Conversation
|
@LukasReschke @nickvergessen @MorrisJobke any reason we never gave any feedback to this pull request? |
nickvergessen
left a comment
nickvergessen
left a comment
There was a problem hiding this comment.
Not sure why no one reacted. But I'm not subscribed to this repository so I didn't receive a notification for it.
| */ | ||
| public function deleteUserExpirationData($userUid) { | ||
| $sql = 'DELETE FROM oc_password_policy_expiration'; | ||
| $sql .= ' WHERE uid="' . $userUid . '"'; |
There was a problem hiding this comment.
This is a SQL injection.
Must use a parameter at least.
Best would be to use $this->db->getQueryBuilder() instead.
Same applies to all other queries.
| @@ -0,0 +1,43 @@ | |||
| <?php | |||
| namespace OCA\Password_Policy\Hook; | |||
There was a problem hiding this comment.
Missing header with Licence etc.
Same for all other files.
| $sql = "INSERT INTO oc_password_policy_expiration"; | ||
| $sql .= " (" . implode(',', array_keys($fields)) . ")"; | ||
| $sql .= ' VALUES ("' . implode('","', $fields) .'")'; | ||
| $sql .= " ON DUPLICATE KEY UPDATE "; |
There was a problem hiding this comment.
This does not work on all supported databases.
Either use $this->db->insertIfNotExist() and depending on the result do the update or manually select before the update/insert
| } | ||
|
|
||
| public function findAllUsers() { | ||
| $sql = 'SELECT * FROM oc_users LEFT JOIN oc_password_policy_expiration ON oc_users.uid = oc_password_policy_expiration.uid'; |
There was a problem hiding this comment.
columns need to be escaped with ` or you use the query builder which does it automatically.
| $message = $this->mailer->createMessage(); | ||
| $message->setFrom([$from => $this->defaults->getName()]); | ||
| $message->setTo([$userEmail => $userId]); | ||
| $message->setSubject($subject); |
There was a problem hiding this comment.
In the meantime setTemplate was added, which comes with nice and easy HTML emails.
Maybe you could also have a look at this.
| $this->setInterval(24 * 60 * 60); | ||
|
|
||
| $this->config = $config; | ||
| $this->logger = \OC::$server->getLogger(); |
| $this->groupManager = $groupManager; | ||
|
|
||
| $connection = \OC::$server->getDatabaseConnection(); | ||
| $this->userDAO = new UserDAO($connection); |
|
ping @dgrammlich |
|
Well sionce this is almost two years old, I guess it needs some more changes. Additionally to the above list, emails should use the email template system and database.xml is not recommended anymore. |
|
Hi, thanks for your review, @nickvergessen, and sorry for the late feedback. Due to a longer illness at the time of your review I couldn't respond and then just lost track of this issue. Nevertheless I'm no longer a member of the project and the company. Maybe @alex-boehm can tell more, or you can just close this MR due to obsolete code. |
|
/me just pushing the button here. 🙈 |
5 participants
Hi!
We implemented the expiration time for passwords for a customer and wanted to show you the solution. Maybe you can give us feedback on our code and / or merge it if you like it.
We are excited to hear from you!
Cheers
Alexander and Daniela