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
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 69 additions & 9 deletions lib/galaxy/webapps/galaxy/api/users.py
Expand Up @@ -3,9 +3,12 @@
"""

import logging
import random
import re
import socket

from datetime import datetime, timedelta
Copy link
Member

Choose a reason for hiding this comment

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

timedelta is not used, will break flake8.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

from markupsafe import escape
from sqlalchemy import false, true, and_, or_

from galaxy import exceptions, util, web
Expand All @@ -14,6 +17,7 @@
from galaxy.security.validate_user_input import validate_email
from galaxy.security.validate_user_input import validate_password
from galaxy.security.validate_user_input import validate_publicname
from galaxy.web import url_for
from galaxy.web import _future_expose_api as expose_api
from galaxy.web import _future_expose_api_anonymous as expose_api_anonymous
from galaxy.web.base.controller import BaseAPIController
Expand All @@ -24,7 +28,7 @@
from galaxy.web.base.controller import UsesFormDefinitionsMixin
from galaxy.web.form_builder import AddressField
from galaxy.tools.toolbox.filters import FilterFactory
from galaxy.util import docstring_trim, listify
from galaxy.util import docstring_trim, listify, hash_util
from galaxy.util.odict import odict


Expand Down Expand Up @@ -333,16 +337,10 @@ def set_information(self, trans, id, payload={}, **kwd):
private_role.name = email
private_role.description = 'Private role for ' + email
user.email = email
trans.sa_session.add(private_role)
if trans.app.config.user_activation_on:
# Deactivate the user if email was changed and activation is on.
user.active = False
if self.send_verification_email(trans, user.email, user.username):
message = 'The login information has been updated with the changes.<br>Verification email has been sent to your new email address. Please verify it by clicking the activation link in the email.<br>Please check your spam/trash folder in case you cannot find the message.'
else:
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)
trans.sa_session.add(private_role)
# Update public name
if user.username != username:
user.username = username
Expand Down Expand Up @@ -390,9 +388,71 @@ def set_information(self, trans, id, payload={}, **kwd):
trans.sa_session.add(user_address)
trans.sa_session.add(user)
trans.sa_session.flush()
if trans.app.config.user_activation_on:
if self.send_verification_email(trans, user.email, user.username):
message = 'The login information has been updated with the changes.<br>Verification email has been sent to your new email address. Please verify it by clicking the activation link in the email.<br>Please check your spam/trash folder in case you cannot find the message.'
else:
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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

You can add an additional:

trans.sa_session.add(user)

near:

trans.sa_session.add(private_role)

Copy link
Member Author

@martenson martenson Mar 17, 2017

Choose a reason for hiding this comment

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

@nsoranzo I guess I could live with that.

trans.log_event('User information added')
return {'message': 'User information has been saved.'}

def send_verification_email( self, trans, email, username ):
"""
Send the verification email containing the activation link to the user's email.
"""
if username is None:
username = trans.user.username
activation_link = self.prepare_activation_link( trans, escape( email ) )

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"
Copy link
Member

Choose a reason for hiding this comment

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

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.

"In order to complete the activation process for %s begun on %s at %s, please click on the following link to verify your account:\n\n"
"%s \n\n"
"By clicking on the above link and opening a Galaxy account you are also confirming that you have read and agreed to Galaxy's Terms and Conditions for use of this service (%s). This includes a quota limit of one account per user. Attempts to subvert this limit by creating multiple accounts or through any other method may result in termination of all associated accounts and data.\n\n"
"Please contact us if you need help with your account at: %s. You can also browse resources available at: %s. \n\n"
"More about the Galaxy Project can be found at galaxyproject.org\n\n"
"Your Galaxy Team" % (escape( username ), escape( email ),
datetime.utcnow().strftime( "%D"),
trans.request.host, activation_link,
trans.app.config.terms_url,
trans.app.config.error_email_to,
trans.app.config.instance_resource_url))
to = email
frm = trans.app.config.email_from or 'galaxy-no-reply@' + host
subject = 'Galaxy Account Activation'
try:
util.send_mail( frm, to, subject, body, trans.app.config )
return True
except Exception:
log.exception( 'Unable to send the activation email.' )
return False

def prepare_activation_link( self, trans, email ):
"""
Prepare the account activation link for the user.
"""
activation_token = self.get_activation_token( trans, email )
activation_link = url_for( controller='user', action='activate', activation_token=activation_token, email=email, qualified=True )
return activation_link

def get_activation_token( self, trans, email ):
"""
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
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@dannon that code might have not been created yet

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

if activation_token is None:
activation_token = hash_util.new_secure_hash( str( random.getrandbits( 256 ) ) )
user.activation_token = activation_token
trans.sa_session.add( user )
trans.sa_session.flush()
return activation_token

def _validate_email_publicname(self, email, username):
''' Validate email and username using regex '''
if email == '' or not isinstance( email, basestring ):
Expand Down