Use first available membership for password reset #1448
Conversation
| @@ -79,6 +81,20 @@ func (c *Controller) HandleSubmitResetPassword() http.Handler { | |||
| var resetComposer auth.ResetPasswordEmailFunc | |||
|
|
|||
| membership := controller.MembershipFromContext(ctx) | |||
|
|
|||
| // This is likely - most users reset password from un-authed context. | |||
| if membership == nil { | |||
There was a problem hiding this comment.
The middleware sets CurrentMemberships (plural) too - you can use that instead of doing a lookup.
There was a problem hiding this comment.
only for RequireAuth pages, which this is not. I don't believe the user will have a session (with currentmembership and realm set)
There was a problem hiding this comment.
It gets populated by LoadCurrentMembership, not requireauth. And I'm pretty sure this route calls LoadCurrentMembership
There was a problem hiding this comment.
These do:
this one is here:
There was a problem hiding this comment.
Ah blarg. I messed up that ordering then. Do you have bandwidth to refactor the middleware to load memberships and require that middleware here?
Loading memberships is really expensive (it's an O(n+1) query), so I'd prefer to do it in middleware and cache it on the context once.
There was a problem hiding this comment.
This is unauthenticated (the user types in their email address) so there is not user or membership yet. I've change this to be less expensive by db querying without loading all memberships.
| membership, err = user.SelectFirstMembership(c.db) | ||
| if err != nil { | ||
| if database.IsNotFound(err) { | ||
| logger.Infof("No membership found for %s", user.Email) |
There was a problem hiding this comment.
Can we make this Infow in another PR?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sethvargo, whaught The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Proposed Changes
Release Note