Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX-113] add "forgot password" feature #276

Merged
merged 36 commits into from
Oct 10, 2018
Merged

[FIX-113] add "forgot password" feature #276

merged 36 commits into from
Oct 10, 2018

Conversation

trzpiot
Copy link
Member

@trzpiot trzpiot commented Aug 21, 2018

This PR adds some new features. It is about issue #113.

  • Users must now enter a mail address when registering. This must be unique in the system.
  • Users will then receive a mail with a registration link.
  • This is only valid for 24 hours.
  • Users can now log in with their mail address or name.
  • Only if the user verifies his mail address via this link he can reset his pasword.
  • An expired registration link can be requested again.
  • The user can now edit his profile (name, mail address, password).
  • If the user changes his mail address, he must verify it again.
  • The user can now reset his password if he forgotten it.
  • To do this, he must enter his verified mail address on a page and then receive a mail with a reset link.
  • If he follows the link, he can change his password. The link is also only valid for 24 hours.
  • Users who have a non-verified email address in their profile will see a notification (PR [FIX-101] set automatic alarm #273 had to be merged)

The following needs to be done:

  • A notification must be displayed for existing users who have not yet entered a mail address
  • The texts for the mails must be changed
  • Refactoring
  • Adjust tests and write own
  • Write documentation and comments
  • Extending the Wiki with mail server

Tests for the EditUserPage are still missing, because there are some problems with the session.
@trzpiot trzpiot changed the title [FIX-113] WIP: add "forgot password" feature [FIX-113] add "forgot password" feature Sep 21, 2018
Copy link
Member

@maximAtanasov maximAtanasov left a comment

Choose a reason for hiding this comment

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

Great work Sebastian, everything worked on the first try, the wiki page is easy to follow and I think it's awesome.

Here are some suggestions/issues that I have:

  1. I suggest pre-filling the application.propreties with the default data (with mail.activate set to false)

  2. Does not work with an existing HSQLDB, maybe make mailVerified nullable? Because of this I could not test what happens when an existing user has no email.

  3. The first time I tried to verify my email Gmail sent it to spam (not an error, just maybe something to note somewhere in the app or in the docs, so that the user doesn't spend too long looking for the mail)

  4. In the Edit User form, Actual password could be renamed to Current password

  5. Is the text field in the Resend verification mail form needed? If I type in a different mail from the one I have registered it tells the mail could not be found.

  6. I'm not recieving any verification email on my Adesso work email (not in the junk folder either). Do you know why? It seems to be working perfectly fine with Gmail.
    (Maybe I configured something wrong)

  7. Maybe you could also add the instructions from the wiki page to a MD file in the repo.

Otherwise, the code looks good to me (although there are a lot of changes and I'm still looking through it) :)

Also, any ideas on how would we go about making this work with the adesso mail server / active directory and existing installations of the Budgeteer?

@trzpiot
Copy link
Member Author

trzpiot commented Sep 27, 2018

Hi @maximAtanasov, thanks for your review. 😄

To your suggestions:

  1. I have now set mail.activate to false by default. Filling the other data would make no sense for me (I can still do it if necessary) because they always depend on the system where Budgeteer is installed.

  2. This should work, because mail is nullable in the UserEntity. I have now also set mailVerified to nullable (but this shouldn't be the reason). Can you try it for me if it works now?

  3. In addition to the information that the mail was sent, the spam folder is now noticed.

  4. Done.

  5. Now the mail address of the user is directly in the textfield. This allows him to check if he has entered the correct mail address during registration.

  6. Maybe the adesso server blocks mails from a local mail server.

  7. Done.

No, I don't have any information about that, but maybe there are other projects that have also implemented similar features? We should discuss this internally.

@maximAtanasov
Copy link
Member

Unfortunately, I'm still unable to log in with an existing user that does not have an email yet.
I get the following exception with mailVerified set to either nullable or not:

Last cause: Can not set boolean field org.wickedsource.budgeteer.persistence.user.UserEntity.mailVerified to null value
WicketMessage: Method onFormSubmitted of interface org.apache.wicket.markup.html.form.IFormSubmitListener targeted at [Form [Component id = loginForm]] on component [Form [Component id = loginForm]] threw an exception

This is what my user looks like in the db:
unbenannt

@trzpiot
Copy link
Member Author

trzpiot commented Oct 5, 2018

Should work now. 😃

Copy link
Member

@maximAtanasov maximAtanasov left a comment

Choose a reason for hiding this comment

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

Yup, everything works :)

@maximAtanasov maximAtanasov merged commit d1906db into adessoSE:master Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants