Skip to content
This repository was archived by the owner on Mar 13, 2026. It is now read-only.

easier password reset process, fixed #1781, plus some minor fixes#3178

Merged
Guite merged 5 commits intozikula:1.4from
Guite:issue-1781
Nov 15, 2016
Merged

easier password reset process, fixed #1781, plus some minor fixes#3178
Guite merged 5 commits intozikula:1.4from
Guite:issue-1781

Conversation

@Guite
Copy link
Member

@Guite Guite commented Nov 14, 2016

Q A
Bug fix? yes (email related)
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets #1781
Refs tickets -
License MIT
Changelog updated yes

Notes:

This does still use the verification entity and confirmation codes. So for example the expiry is still handled by that.

But there are some essential behavioural differences:

  1. The code is wrapped into an encoded identifier together with the user id, the user name and the email address. This identifier is appended to the url contained in the email text.
  2. After extracting the identifier arguments the user is selected using the user id.
  3. The code is checked before the form even displays.
  4. The form contains only two password fields now.
  5. A new helper class (LostPasswordVerificationHelper) encapsulates the details, making the controllers a bit more slim.

This PR also addresses some minor bugs I've found, like non-absolute urls in the email templates and wrong usage of the MailerApi in the MailHelper class.

@Guite Guite added this to the 1.4.4 milestone Nov 14, 2016
@Guite
Copy link
Member Author

Guite commented Nov 14, 2016

ready to review

'uid' => $requestDetails['userId'],
'uname' => $requestDetails['userName'],
'email' => $requestDetails['emailAddress']
]);
Copy link
Member

Choose a reason for hiding this comment

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

when using the uid you will only get one record or null. There is no possibility of more. you can simply use the uid and the find method and then check for null.

also this is from the user_repository. Anyone that has logged in since upgrade past 1.4.3 will have had their record migrated to the zauth_authentication_mapping table. So this will not work.

Copy link
Member Author

@Guite Guite Nov 14, 2016

Choose a reason for hiding this comment

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

well, I did not invent this, but rewrote the code which was there; so if it does not work it didn't work before, too

Copy link
Member

Choose a reason for hiding this comment

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

that's simply not true. The previous code only searched based on email. Such a search may possibly return several rows because in the past, the email has not been required to be unique. You are searching based on multiple params including the uid, which must be unique, therefore it will only return one row and using the find method is appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant

also this is from the user_repository. Anyone that has logged in since upgrade past 1.4.3 will have had their record migrated to the zauth_authentication_mapping table. So this will not work.

Copy link
Member

@craigh craigh Nov 14, 2016

Choose a reason for hiding this comment

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

please see comment below

@craigh
Copy link
Member

craigh commented Nov 14, 2016

to review in combination with the #3177: User data is migrated from the users table to the zauth_authentication_mapping table when each user logs in after that upgrade. This means that some user data will be in both places - the only thing this PR cares about - the password - is the main thing that is migrated.

This means you will need to have a 'new' method which uses zauth_authentication_mapping and a BC method using the users table - the easier it is to remove the BC layer, the better please.

I didn't walk though each line of this PR yet, but it appears you haven't considered the zauth_authentication_mapping location for passwords. perhaps I missed it

@Guite
Copy link
Member Author

Guite commented Nov 14, 2016

This means you will need to have a 'new' method which uses zauth_authentication_mapping and a
BC method using the users table - the easier it is to remove the BC layer, the better please.

I didn't walk though each line of this PR yet, but it appears you haven't considered the
zauth_authentication_mapping location for passwords. perhaps I missed it

again: I reused the code which was there; so if it has serious problems those are general and very probably subject of another issue

@craigh
Copy link
Member

craigh commented Nov 14, 2016

again: I reused the code which was there; so if it has serious problems those are general and very probably subject of another issue

if it works, great! I'm just asking you to check both possibilities:

  1. a legacy login (user has not logged in since the upgrade to 1.4.3) where the pwd will be stored in the users table
  2. a new login (user was created after 1.4.3 install or has logged in since) where the pwd will be store in the zauth table.

@Guite
Copy link
Member Author

Guite commented Nov 15, 2016

Tested both. Both work.

@Guite Guite merged commit a48be0c into zikula:1.4 Nov 15, 2016
@Guite Guite deleted the issue-1781 branch November 15, 2016 14:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants