Skip to content

Conversation

@zaxbux
Copy link
Contributor

@zaxbux zaxbux commented Apr 9, 2021

This PR removes the plain-text password in the initial invite email for backend admins. Sure, a password reset link in an unencrypted email is not much more secure than including the password, but it does help improve "cyber hygiene" by preventing that password from being stored in whatever systems see that email on the way to the user's inbox. It also allows the user to choose their own password immediately, instead of remembering to change it after signing in.

I also added a "Send Password Restore" button to the user update form, giving other admins the ability to send a password reset email for another user.

Tested:

  • Creating new user without password sends email with reset link.
  • Creating new user with password sends email with normal login link.
  • Send password restore button sends a password reset email to the user.

@LukeTowers LukeTowers added needs review Issues/PRs that require a review from a maintainer maintenance PRs that fix bugs, are translation changes or make only minor changes labels Apr 9, 2021
@bennothommo
Copy link
Member

@zaxbux thanks for the PR. We're prioritising finishing off 1.1.3 and releasing the website and marketplace. Once these are done, we'll be happy to consider implementing this.

We'll be in touch soon.

@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@github-actions github-actions bot added the stale Issues/PRs that have had no activity and may be archived label Jun 13, 2021
@LukeTowers LukeTowers removed the stale Issues/PRs that have had no activity and may be archived label Jun 13, 2021
@bennothommo bennothommo added this to the v1.1.5 milestone Jun 17, 2021
@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@github-actions github-actions bot added the stale Issues/PRs that have had no activity and may be archived label Aug 17, 2021
@LukeTowers LukeTowers removed the stale Issues/PRs that have had no activity and may be archived label Aug 17, 2021
@bennothommo
Copy link
Member

@zaxbux sorry for the delay in getting back to this. I've had a look through and this looks fantastic - well done. I've tested the email out and works as expected.

Could we perhaps move the restore button to be a text link underneath the Password fields so that it is contextual?

Screen Shot 2021-09-30 at 10 30 19 am

Also, not sure how possible this is, but I notice currently that it contains the user's ID in the password restore link. Would it be possible to remove this and just rely on the code, so we're not inadvertently revealing user IDs?

This is more of a nice-to-have change, but I'd also prefer if we could time limit the password restoration so that they have, say, 24 hours to restore their password. I'm not sure if this is already the case or not.

@bennothommo bennothommo added needs review Issues/PRs that require a review from a maintainer Status: Revision Needed and removed needs review Issues/PRs that require a review from a maintainer labels Sep 30, 2021
@LukeTowers
Copy link
Member

I've got some ideas around improvements in this area so just hold off for now

@bennothommo bennothommo modified the milestones: v1.1.7, v1.1.8 Oct 12, 2021
@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@github-actions github-actions bot added the stale Issues/PRs that have had no activity and may be archived label Dec 12, 2021
@bennothommo bennothommo removed the stale Issues/PRs that have had no activity and may be archived label Dec 12, 2021
@LukeTowers LukeTowers modified the milestones: v1.1.8, v1.1.9 Feb 2, 2022
@github-actions
Copy link

github-actions bot commented Apr 4, 2022

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@github-actions github-actions bot added the stale Issues/PRs that have had no activity and may be archived label Apr 4, 2022
@bennothommo bennothommo removed the stale Issues/PRs that have had no activity and may be archived label Apr 4, 2022
@multiwebinc
Copy link
Contributor

Also, not sure how possible this is, but I notice currently that it contains the user's ID in the password restore link. Would it be possible to remove this and just rely on the code, so we're not inadvertently revealing user IDs?

Since the password is hashed with a random salt, you need some way of identifying which user the hash belongs to before being able to verify the hash. Two things come to mind to accomplish this:

  1. Have a separate password recovery table and the ID sent to the user is the password recovery ID instead of the user id.
  2. Add another column to the users table with a random string (maybe combined with uniqid() to guarantee uniqueness) stored in plain text. Both the random string and the temporary password are mailed to the user and the random string is used instead of the user ID to select the user.

@LukeTowers
Copy link
Member

Ideally we handle this with 1. since that's the way Laravel handles it nowadays I believe.

@LukeTowers LukeTowers modified the milestones: v1.1.9, v1.2.1 Jul 15, 2022
@bennothommo bennothommo modified the milestones: v1.2.1, v1.2.2 Sep 12, 2022
@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in this pull request for the last 6 months.
If you intend to continue working on this pull request, please respond within 3 days.
If this pull request is critical for your business, please reach out to us at wintercms@luketowers.ca.

@github-actions github-actions bot added the stale Issues/PRs that have had no activity and may be archived label Mar 14, 2023
@bennothommo bennothommo removed the stale Issues/PRs that have had no activity and may be archived label Mar 15, 2023
@LukeTowers LukeTowers modified the milestones: v1.2.2, v1.2.3 Apr 20, 2023
@LukeTowers LukeTowers modified the milestones: v1.2.3, v1.2.4 Jul 7, 2023
@LukeTowers LukeTowers modified the milestones: v1.2.4, 1.2.5 Dec 27, 2023
@mjauvin mjauvin modified the milestones: 1.2.5, 1.2.6 Feb 18, 2024
@LukeTowers LukeTowers modified the milestones: 1.2.6, 1.3.0 Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance PRs that fix bugs, are translation changes or make only minor changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants