Skip to content
This repository has been archived by the owner on Dec 15, 2020. It is now read-only.

Email-based password reset #54

Merged
merged 27 commits into from
Aug 12, 2016
Merged

Email-based password reset #54

merged 27 commits into from
Aug 12, 2016

Conversation

marpaia
Copy link
Contributor

@marpaia marpaia commented Aug 9, 2016

No description provided.

@marpaia marpaia added this to the Sprint #1 - "The Enrollening" milestone Aug 9, 2016
@marpaia marpaia self-assigned this Aug 9, 2016
@marpaia
Copy link
Contributor Author

marpaia commented Aug 11, 2016

Testing with MailHog

The request

reset

HTML

html

Text

txt

Email source

mime

@marpaia
Copy link
Contributor Author

marpaia commented Aug 11, 2016

@zwass this is ready for review. I'm going to add integration testing and react rendering in another PR to keep reviews manageable.

@@ -44,15 +44,21 @@ func (vc *ViewerContext) UserID() (uint, error) {
// CanPerformActions returns a bool indicating the current user's ability to
// perform the most basic actions on the site
func (vc *ViewerContext) CanPerformActions() bool {
if vc.user == nil {
return false
if vc.IsLoggedIn() && !vc.user.NeedsPasswordReset {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to see return vc.IsLoggedIn() && !vc.user.NeedsPasswordReset

@marpaia
Copy link
Contributor Author

marpaia commented Aug 11, 2016

OK, other than the spam catcher evasion question, the rest of this should be good to go. Moving the email function out even allowed me to use a mock pool object to get another test in. I think I'm using *KolideError properly in func ResetUserPassword(c *gin.Context), but let me know.

I'm going to punt on spam detection avoidance for now and tackle it with DKIM.

}

err = GetSMTPConnectionPool(c).Send(&e, time.Second*10)
kerr = SendEmail(GetSMTPConnectionPool(c), user.Email, subject, html, text)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be kerr as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes!

@marpaia marpaia merged commit 736bce5 into kolide:master Aug 12, 2016
@marpaia marpaia deleted the password-reset branch August 12, 2016 19:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants