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 #970: adjust dbpatch and password tables #3062

Merged
merged 6 commits into from
Sep 29, 2014

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Sep 25, 2014

The constraint on the dbpatch table is adjusted so that, while attempts to repeat the same DB upgrade are still prevented, the user may switch between different server branches easily enough even if they have different revisions of Bio-Formats, the server starting each time without trouble.

The password table acquires a new changed column, noting the latest time that a user's password was set. First, try a server that includes #3061 (or even 5.0.5) but not this PR: change some users' passwords, including some more than once. Now, run the upgrade script and ensure that the changed column of the password table contains correct times for when those passwords were most recently changed. (Times for root might be a bit newer than seems correct, as that user is a bit odd.) Finally, start a server with this PR included, and make sure that further password changes, and passwords of newly created users, continue to have appropriate times updated in this column.

Fixes http://trac.openmicroscopy.org.uk/ome/ticket/970.
--no-rebase

@joshmoore
Copy link
Member

psql -U omero OMERO-5.1-breaking-deploy -c "select * from password"
 experimenter_id |           hash           | dn |          changed           
-----------------+--------------------------+----+----------------------------
               0 | kYMkc9SyhupHyJP1Cl2Alg== |    | 
               1 |                          |    | 
               2 |                          |    | 2014-09-26 14:22:16.011287
(3 rows)

# Change root and user 2 password....

psql -U omero OMERO-5.1-breaking-deploy -c "select * from password"
 experimenter_id |           hash           | dn |          changed           
-----------------+--------------------------+----+----------------------------
               1 |                          |    | 
               0 | kYMkc9SyhupHyJP1Cl2Alg== |    | 2014-09-26 14:25:45.038617
               2 | bzMnl/GqUqD710i3VUOJcg== |    | 2014-09-26 14:25:52.761563
(3 rows)

i.e. new user properly got a timestamp, and changing the user's and root's password properly bumped. 👍 Removing breaking.

@bpindelski
Copy link

All changes look OK here and the password change timestamp is indeed present in the merge build, after upgrading the DB from 5.0.5 to 5.1-DEV10. In this sense, this looks good to merge.

@jburel The only thing I've noticed while testing this PR is that root cannot change passwords of users. This has been tested in 5.0.4, 5.0.5 and the develop merge build. First I get a dialog saying that the password was changed, and then another one saying that the password can't be changed. I've tried out 5.0.4 to make sure it's not a result of the secvuln work.

@mtbc
Copy link
Member Author

mtbc commented Sep 29, 2014

@joshmoore
Copy link
Member

Listed on https://trello.com/c/WDilK2L3/68-api-model-db-changes-please-update-when-work-is-done since the change will need propagating elsewhere (API, etc.).

joshmoore added a commit that referenced this pull request Sep 29, 2014
fix #970: adjust dbpatch and password tables
@joshmoore joshmoore merged commit 6df970c into ome:develop Sep 29, 2014
@mtbc mtbc deleted the password-and-dbpatch-sql branch September 30, 2014 07:39
@joshmoore
Copy link
Member

NB: perhaps a little late, but thoughts on using a trigger to update "changed" instead?

@mtbc
Copy link
Member Author

mtbc commented Oct 6, 2014

At this point I doubt the gain is worth the overhead of testing another breaking PR but if there were some convenient way to slip that in, say when one's otherwise fiddling again with the password table anyway, I'd say sure, go for it, apart only from the detail that it might leave the system harder to understand if when we change the row one of the column changes comes by one means and the other comes by wholly different means.

@joshmoore
Copy link
Member

Agreed. Wasn't sure myself, but was mostly wondering about the case which I was just using to test of resetting via SQL based on the output of bin/omero db password.

@sbesson sbesson added this to the 5.1.0-m1 milestone Oct 14, 2014
rgozim pushed a commit to rgozim/openmicroscopy that referenced this pull request Oct 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants