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

[17.01] Fixes for users API #3771

Merged
merged 5 commits into from Mar 18, 2017

Conversation

Projects
None yet
4 participants
@martenson
Copy link
Member

commented Mar 17, 2017

Bunch of method invoked but not existent in API controller.

  • closes #3765
  • also fixes wrong import that had all datetime.utcnow() calls error (only used in password reset ping @dannon )
  • also fixes activation being requested before changes are saved (thus with the wrong email)

There are other improvements we should make (like deduplicaiton) to this API, controller, and UI but this should suffice for a release branch.

related: #3656
probably introduced at #3118

martenson added some commits Mar 17, 2017

add the activation functionality to API users controller
it was invoked but not included resulting in errors
@martenson

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2017

Also we really need tests for activation/password resets/account changes :'(

@dannon

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

Definitely agreed we need to add (or update) when converting functionality from web->api like this.


from datetime import datetime, timedelta

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 17, 2017

Member

timedelta is not used, will break flake8.

This comment has been minimized.

Copy link
@martenson

martenson Mar 17, 2017

Author Member

fixed, I did not port my flake setup to new machine yet :/

message = 'Unable to send activation email, please contact your local Galaxy administrator.'
if trans.app.config.error_email_to is not None:
message += ' Contact: %s' % trans.app.config.error_email_to
raise MessageException(message)

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 17, 2017

Member

By moving this piece of code, we would now send a verification email even when the email address was not changed, e.g. for changes to username or address, right?
Not sure that's correct.

This comment has been minimized.

Copy link
@martenson

martenson Mar 17, 2017

Author Member

You are right, I think I addressed that with 54fb1a4

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 17, 2017

Member

Thanks! I am wondering what should happen if send_verification_email() fails: should the email change and user deactivation be committed to the database anyway?
Maybe moving back this piece of code and changing:

if self.send_verification_email(trans, user.email, user.username):

to:

if self.send_verification_email(trans, email, username):

would solve the problem without disabling user accounts in case of a server misconfiguration?

This comment has been minimized.

Copy link
@martenson

martenson Mar 17, 2017

Author Member

@nsoranzo But then you will be sending to email that has not been saved to DB and there is a lot of code in between these two points that can fail I think.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 17, 2017

Member

You can add an additional:

trans.sa_session.add(user)

near:

trans.sa_session.add(private_role)

This comment has been minimized.

Copy link
@martenson

martenson Mar 17, 2017

Author Member

@nsoranzo I guess I could live with that.

martenson added some commits Mar 17, 2017

private_role = trans.app.security_agent.get_private_user_role(user)
private_role.name = email
private_role.description = 'Private role for ' + email
user.email = email
trans.sa_session.add(user)
trans.sa_session.add(private_role)
trans.sa_session.flush()

This comment has been minimized.

Copy link
@martenson

martenson Mar 17, 2017

Author Member

@nsoranzo I had to flush here or else L442 fails because user is None

Check for the activation token. Create new activation token and store it in the database if no token found.
"""
user = trans.sa_session.query( trans.app.model.User ).filter( trans.app.model.User.table.c.email == email ).first()
activation_token = user.activation_token

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 17, 2017

Member

It seems that activation_token is set once and for all for a user, not every time an email change is performed. Is that safe?

This comment has been minimized.

Copy link
@martenson

martenson Mar 17, 2017

Author Member

@nsoranzo definitely could be better, worth an issue I think

This comment has been minimized.

Copy link
@dannon

dannon Mar 17, 2017

Member

Should probably generate a new token and invalidate any others. Looking for the previous code that did this now.

This comment has been minimized.

Copy link
@martenson

martenson Mar 17, 2017

Author Member

@dannon that code might have not been created yet

This comment has been minimized.

Copy link
@dannon

dannon Mar 17, 2017

Member

Ahh, this doesn't use the same token schema that we use for password reset. We should probably combine these two token systems in the future.

For now, can you just make get_activation_token always set a new token?

This comment has been minimized.

Copy link
@dannon

This comment has been minimized.

Copy link
@dannon

This comment has been minimized.

Copy link
@dannon

dannon Mar 17, 2017

Member

(to follow up here, I don't think this is necessary for now, and maybe not important to do ever)

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 18, 2017

Member

Maybe we can reset the activation token to None after the activation has been completed.

This comment has been minimized.

Copy link
@martenson

martenson Mar 18, 2017

Author Member

@nsoranzo can we continue this particular line of comments in #3774 please?

@dannon

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

Thanks for all the review on this one @nsoranzo, and for all the tweaks and fixes @martenson. If we can merge the changes in martenson#21, I think we might be good to go.

@martenson

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2017

@dannon I think your PR is technically more correct but provides worse UX. People have trouble finding these activation emails due to filters/spam/trash and this means you need to find The_One_Email even when you clicked on 'resend' three times and first and third got eaten by a spamfilter and you only see the second email.

Moreover I wouldn't consider this a bugfix. Maybe a PR against dev would be a better start?

@dannon

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

It occurs to me that the worst possible thing someone could do using an old token is activate an account, anyway.

I'm thinking about this from the perspective of stale password reset tokens or something, and in that case I'd definitely want to argue the point, but since it's just user activation (so, no harm possible that I can think of, really), I'll happily concede for now.

@martenson

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2017

@galaxybot test this

@blankenberg

This comment has been minimized.

Copy link
Member

commented Mar 18, 2017

+1

(all tests did pass, but I clicked the wrong button)

@blankenberg blankenberg reopened this Mar 18, 2017

host = trans.request.host.split( ':' )[ 0 ]
if host in [ 'localhost', '127.0.0.1', '0.0.0.0' ]:
host = socket.getfqdn()
body = ("Hello %s,\n\n"

This comment has been minimized.

Copy link
@blankenberg

blankenberg Mar 18, 2017

Member

The 'heavy' lifting here should be handled by e.g. managers. Web & API controllers should be lightweight, handling the formatting of inputs and outputs, with the bulk of the operations being performed by some centralized/shared methodology.

This is a general comment and should not prevent merging the bug fix, however.

@dannon dannon merged commit 6ab5cc1 into galaxyproject:release_17.01 Mar 18, 2017

5 checks passed

api test Build finished. 257 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 136 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 24 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details

@martenson martenson deleted the martenson:fix-users-api branch Mar 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.