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

When your account is a builtin account, changing user info only changed the "authenticateduser" table #4565

Closed
scolapasta opened this issue Apr 3, 2018 · 12 comments
Assignees

Comments

@scolapasta
Copy link
Contributor

Steps:

  1. Create a built in account
  2. Change first name, last name, e-mail
  3. Changes are made to "authenticateduser" but not "builtinuser"

In particular, the builtinuser e-mail is what is used for "forgot password" functionality.

@scolapasta
Copy link
Contributor Author

Once thing to consider; when we first created this we treated the builtin (and all authentication providers) like we do with shib in that they are the source of truth for these details.

However, since then we have introduced oauth and user can (and sometimes have to) enter e-mail address via dataverse. All that is needed is some kind of unique token. We may want to consider this here and no longer store anything in builtinuser except for username and password.

@pdurbin
Copy link
Member

pdurbin commented Apr 3, 2018

Yes! I would love to remove the following fields from the builtinuser table:

  • affiliation
  • email
  • firstname
  • lastname
  • position

As @scolapasta indicated, we need to keep these fields:

  • username
  • encryptedpassword
  • passwordencryptionversion

@kcondon
Copy link
Contributor

kcondon commented Apr 19, 2018

yup, just discovered this the hard way by changing email.
Btw, THIS IS WHY PASSWORD RESET EMAIL IS NOT SENT/RECEIVED.
I manually set them to be the same in the db and password resent email was then sent correctly. So, it appears that many UI features use the address in authenticateduser table (dashboard, account info, log in detect/ sign up detect) but password reset appears to use the builtinuser table email. So, it you changed it and then forgot your password you are SOL.

@djbrooke
Copy link
Contributor

Hi @thaorell - is everything moving OK here? Feel free to drop into Slack or post a comment here if you have questions, we're happy to help!

@thaorell
Copy link
Contributor

@djbrooke It is going ok. I'm still learning about the code. I have dropped some columns (email and affiliation) and changed the code a bit, they are still giving errors and I am currently working on fixing them

@djbrooke
Copy link
Contributor

@thaorell - are you still working on this, or have you moved onto other items? Thanks!

@djbrooke
Copy link
Contributor

djbrooke commented Aug 6, 2018

assigning to @scolapasta to talk about this at next estimation session

@benjamin-martinez benjamin-martinez self-assigned this Aug 16, 2018
benjamin-martinez added a commit that referenced this issue Aug 20, 2018
…catedUser class but it seems like what I was trying to accomplish had already been done. All I think we need to do is remove certain columns from the builtinUser table. #4565
@benjamin-martinez benjamin-martinez removed their assignment Aug 20, 2018
@benjamin-martinez benjamin-martinez self-assigned this Aug 21, 2018
benjamin-martinez added a commit that referenced this issue Aug 22, 2018
…w. This wasn't the original proposed solution, but I think it's an easier and more effective one. #4565
matthew-a-dunlap pushed a commit that referenced this issue Aug 31, 2018
…dated now. This wasn't the original proposed solution, but I think it's an easier and more effective one. #4565"

This reverts commit 5630bf2.
matthew-a-dunlap added a commit that referenced this issue Sep 4, 2018
This included fixing tests with new mocks
matthew-a-dunlap added a commit that referenced this issue Sep 4, 2018
@matthew-a-dunlap matthew-a-dunlap removed their assignment Sep 4, 2018
@matthew-a-dunlap
Copy link
Contributor

matthew-a-dunlap commented Sep 4, 2018

The attached PR #4993 takes on solving the title of this story by removing the attributes from builtinUser entirely. AuthenticatedUser is now the goto for firstname, lastname, email, affiliation, position. Note there is an upgrade 4.9.2 to 4.9.3 script that drops these attributes from builtinUser.

Things touched:

  • Shib: Setting username
  • OAuth first login (and more?)
  • Login username & password, especially for builtin users
  • Creation of users via api.
  • General authentication / login
  • Password Reset
    • It looks like reset emails work fine now, but I may be misunderstanding the broken case

Do we know of any cases where the data was only updated in builtinUser and not AuthenticatedUser? If so we may need to look into some sort of migration.

@sekmiller
Copy link
Contributor

Looks good. The DataverseUserPage does not need/use the bean level builtin User.

@sekmiller sekmiller removed their assignment Sep 5, 2018
@kcondon
Copy link
Contributor

kcondon commented Sep 5, 2018

Issues found so far:

  1. When logged in as super user (dataverseAdmin) the dashboard link in navbar is missing but is present in account name pull down.
    Confirmed this was an intentional change as part of language toggle and existed in develop. As designed.

Things tested:

  • Shib: Setting username
  • OAuth first login (and more?) (github,google,orcid)
  • Login username & password, especially for builtin users
  • Creation of users via api.
  • Creation of users via UI
  • Chg user account info via UI
  • Verify email
  • Assign roles
  • Manager Users, incl. supe user, remove role
  • General authentication / login
  • Password Reset
    It looks like reset emails work fine now, but I may be misunderstanding the broken case

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

No branches or pull requests

10 participants