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

Users API: add the update endpoint #2595

Merged
merged 8 commits into from Jul 18, 2016
4 changes: 2 additions & 2 deletions lib/galaxy/managers/base.py
Expand Up @@ -736,7 +736,7 @@ class ModelDeserializer( HasAModelManager ):
"""
# TODO:?? a larger question is: which should be first? Deserialize then validate - or - validate then deserialize?

def __init__( self, app, **kwargs ):
def __init__( self, app, validator=None, **kwargs ):
Copy link
Member

Choose a reason for hiding this comment

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

The new validator parameter does not seem to be used, is it for future functionalities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Future composition:
https://github.com/galaxyproject/galaxy/pull/2595/files#diff-d9bd7857711700c7d77c35956699096dR306
Up to this point ModelValidator was enough.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @carlfeberhard!

"""
Set up deserializers and validator.
"""
Expand All @@ -747,7 +747,7 @@ def __init__( self, app, **kwargs ):
self.deserializable_keyset = set([])
self.add_deserializers()
# a sub object that can validate incoming values
self.validate = ModelValidator( self.app )
self.validate = validator or ModelValidator( self.app )

def add_deserializers( self ):
"""
Expand Down
24 changes: 23 additions & 1 deletion lib/galaxy/managers/users.py
Expand Up @@ -11,6 +11,7 @@
from galaxy.managers import base
from galaxy.managers import deletable
from galaxy.managers import api_keys
from galaxy.security import validate_user_input

import logging
log = logging.getLogger( __name__ )
Expand All @@ -26,7 +27,6 @@ class UserManager( base.ModelManager, deletable.PurgableManagerMixin ):
# TODO: incorp BaseAPIController.validate_in_users_and_groups
# TODO: incorp CreatesUsersMixin
# TODO: incorp CreatesApiKeysMixin
# TODO: incorporate security/validate_user_input.py
# TODO: incorporate UsesFormDefinitionsMixin?

def create( self, webapp_name=None, **kwargs ):
Expand Down Expand Up @@ -288,6 +288,28 @@ def add_serializers( self ):
})


class UserDeserializer( base.ModelDeserializer ):
"""
Service object for validating and deserializing dictionaries that
update/alter users.
"""
model_manager_class = UserManager

def add_deserializers( self ):
super( UserDeserializer, self ).add_deserializers()
self.deserializers.update({
'username' : self.deserialize_username,
})

def deserialize_username( self, item, key, username, trans=None, **context ):
# TODO: validate_user_input requires trans and should(?) raise exceptions
# move validation to UserValidator and use self.app, exceptions instead
validation_error = validate_user_input.validate_publicname( trans, username, user=item )
if validation_error:
raise base.ModelDeserializingError( validation_error )
return self.default_deserializer( item, key, username, trans=trans, **context )


class CurrentUserSerializer( UserSerializer ):
model_manager_class = UserManager

Expand Down
17 changes: 7 additions & 10 deletions lib/galaxy/security/validate_user_input.py
Expand Up @@ -11,6 +11,9 @@

VALID_PUBLICNAME_RE = re.compile( "^[a-z0-9\-]+$" )
VALID_PUBLICNAME_SUB = re.compile( "[^a-z0-9\-]" )
PUBLICNAME_MIN_LEN = 3
PUBLICNAME_MAX_LEN = 255

# Basic regular expression to check email validity.
VALID_EMAIL_RE = re.compile( "[^@]+@[^@]+\.[^@]+" )
FILL_CHAR = '-'
Expand Down Expand Up @@ -41,16 +44,10 @@ def validate_publicname( trans, publicname, user=None ):
# letters, numbers, and the '-' character.
if user and user.username == publicname:
return ''
if trans.webapp.name == 'tool_shed':
if len( publicname ) < 3:
return "Public name must be at least 3 characters in length"
else:
# DCT - TODO - Simplify logic if 3 chars is okay for publicname for
# galaxy as well as toolshed
if len( publicname ) < 3:
return "Public name must be at least 3 characters in length"
if len( publicname ) > 255:
return "Public name cannot be more than 255 characters in length"
if len( publicname ) < PUBLICNAME_MIN_LEN:
return "Public name must be at least %d characters in length" % ( PUBLICNAME_MIN_LEN )
if len( publicname ) > PUBLICNAME_MAX_LEN:
return "Public name cannot be more than %d characters in length" % ( PUBLICNAME_MAX_LEN )
if not( VALID_PUBLICNAME_RE.match( publicname ) ):
return "Public name must contain only lower-case letters, numbers and '-'"
if trans.sa_session.query( trans.app.model.User ).filter_by( username=publicname ).first():
Expand Down
29 changes: 27 additions & 2 deletions lib/galaxy/webapps/galaxy/api/users.py
Expand Up @@ -27,6 +27,7 @@ def __init__(self, app):
super(UserAPIController, self).__init__(app)
self.user_manager = users.UserManager(app)
self.user_serializer = users.UserSerializer( app )
self.user_deserializer = users.UserDeserializer( app )

@expose_api
def index( self, trans, deleted='False', f_email=None, f_name=None, f_any=None, **kwd ):
Expand Down Expand Up @@ -175,8 +176,32 @@ def api_key( self, trans, user_id, **kwd ):
return key

@expose_api
def update( self, trans, **kwd ):
raise exceptions.NotImplemented()
def update( self, trans, id, payload, **kwd ):
"""
update( self, trans, id, payload, **kwd )
* PUT /api/users/{id}
updates the values for the item with the given ``id``

:type id: str
:param id: the encoded id of the item to update
:type payload: dict
:param payload: a dictionary of new attribute values

:rtype: dict
:returns: an error object if an error occurred or a dictionary containing
the serialized item after any changes
"""
current_user = trans.user
user_to_update = self.user_manager.by_id( self.decode_id( id ) )

# only allow updating other users if they're admin
editing_someone_else = current_user != user_to_update
is_admin = trans.api_inherit_admin or self.user_manager.is_admin( current_user )
if editing_someone_else and not is_admin:
raise exceptions.InsufficientPermissionsException( 'you are not allowed to update that user', id=id )

self.user_deserializer.deserialize( user_to_update, payload, user=current_user, trans=trans )
return self.user_serializer.serialize_to_view( user_to_update, view='detailed' )

@expose_api
@web.require_admin
Expand Down
61 changes: 60 additions & 1 deletion test/api/test_users.py
@@ -1,3 +1,5 @@
import json
from requests import put
from base import api

TEST_USER_EMAIL = "user_for_users_index_test@bx.psu.edu"
Expand All @@ -18,7 +20,64 @@ def test_index( self ):

def test_index_only_self_for_nonadmins( self ):
self._setup_user( TEST_USER_EMAIL )
with self._different_user( ):
with self._different_user():
all_users_response = self._get( "users" )
# Non admin users can only see themselves
assert len( all_users_response.json() ) == 1

def test_show( self ):
user = self._setup_user( TEST_USER_EMAIL )
with self._different_user( email=TEST_USER_EMAIL ):
show_response = self.__show( user )
self._assert_status_code_is( show_response, 200 )
self.__assert_matches_user( user, show_response.json() )

def test_update( self ):
new_name = 'linnaeus'
user = self._setup_user( TEST_USER_EMAIL )
not_the_user = self._setup_user( 'email@example.com' )
with self._different_user( email=TEST_USER_EMAIL ):

# working
update_response = self.__update( user, username=new_name )
self._assert_status_code_is( update_response, 200 )
update_json = update_response.json()
self.assertEqual( update_json[ 'username' ], new_name )

# too short
update_response = self.__update( user, username='mu' )
self._assert_status_code_is( update_response, 400 )

# not them
update_response = self.__update( not_the_user, username=new_name )
self._assert_status_code_is( update_response, 403 )

# non-existent
no_user_id = self.security.encode_id( 100 )
update_url = self._api_url( "users/%s" % ( no_user_id ), use_key=True )
update_response = put( update_url, data=json.dumps( dict( username=new_name ) ) )
self._assert_status_code_is( update_response, 404 )

def test_admin_update( self ):
new_name = 'flexo'
user = self._setup_user( TEST_USER_EMAIL )

update_url = self._api_url( "users/%s" % ( user[ "id" ] ), params=dict( key=self.master_api_key ) )
update_response = put( update_url, data=json.dumps( dict( username=new_name ) ) )
self._assert_status_code_is( update_response, 200 )
update_json = update_response.json()
self.assertEqual( update_json[ 'username' ], new_name )

def __show( self, user ):
return self._get( "users/%s" % ( user[ 'id' ] ) )

def __update( self, user, **new_data ):
update_url = self._api_url( "users/%s" % ( user[ "id" ] ), use_key=True )
# TODO: Awkward json.dumps required here because of https://trello.com/c/CQwmCeG6
body = json.dumps( new_data )
return put( update_url, data=body )

def __assert_matches_user( self, userA, userB ):
self._assert_has_keys( userB, "id", "username", "total_disk_usage" )
assert userA[ "id" ] == userB[ "id" ]
assert userA[ "username" ] == userB[ "username" ]
42 changes: 41 additions & 1 deletion test/unit/managers/test_UserManager.py
Expand Up @@ -12,9 +12,10 @@
from six import string_types

from galaxy import exceptions, model
from galaxy.managers import base as base_manager
from galaxy.managers import histories, users

from .base import BaseTestCase
from base import BaseTestCase
Copy link
Member

@nsoranzo nsoranzo Jul 8, 2016

Choose a reason for hiding this comment

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

Please revert to include the dot (for Python3).



# =============================================================================
Expand Down Expand Up @@ -215,6 +216,45 @@ def test_anonymous( self ):
self.assertIsJsonifyable( serialized )


# =============================================================================
class UserDeserializerTestCase( BaseTestCase ):

def set_up_managers( self ):
super( UserDeserializerTestCase, self ).set_up_managers()
self.deserializer = users.UserDeserializer( self.app )

def _assertRaises_and_return_raised( self, exception_class, fn, *args, **kwargs ):
try:
fn( *args, **kwargs )
except exception_class, exception:
Copy link
Member

Choose a reason for hiding this comment

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

except exception_class as exception:

self.assertTrue( True )
return exception
assert False, '%s not raised' % ( exception_class.__name__ )

def test_username_validation( self ):
user = self.user_manager.create( **user2_data )

# self.log( "usernames can be unicode" ) #TODO: nope they can't
# self.deserializer.deserialize( user, { 'username': 'Σίσυφος' }, trans=self.trans )

self.log( "usernames must be long enough and with no non-hyphen punctuation" )
exception = self._assertRaises_and_return_raised( base_manager.ModelDeserializingError,
self.deserializer.deserialize, user, { 'username': 'ed' }, trans=self.trans )
self.assertTrue( 'Public name must be at least' in str( exception ) )
self.assertRaises( base_manager.ModelDeserializingError, self.deserializer.deserialize,
user, { 'username': 'f.d.r.' }, trans=self.trans )

self.log( "usernames must be unique" )
self.user_manager.create( **user3_data )
self.assertRaises( base_manager.ModelDeserializingError, self.deserializer.deserialize,
user, { 'username': 'user3' }, trans=self.trans )

self.log( "username should be updatable" )
new_name = 'double-plus-good'
self.deserializer.deserialize( user, { 'username': new_name }, trans=self.trans )
self.assertEqual( self.user_manager.by_id( user.id ).username, new_name )


# =============================================================================
class AdminUserFilterParserTestCase( BaseTestCase ):

Expand Down