Merge pull request galaxyproject#2595 from carlfeberhard/users.api-up…

…date

Users API: add the update endpoint

lib/galaxy/managers/base.py

@@ -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 ):
         """
         Set up deserializers and validator.
         """
@@ -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 ):
         """

lib/galaxy/managers/users.py

@@ -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__ )
@@ -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 ):
@@ -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
 

lib/galaxy/security/validate_user_input.py

@@ -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 = '-'
@@ -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():

lib/galaxy/webapps/galaxy/api/users.py

@@ -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 ):
@@ -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

test/api/test_users.py

@@ -1,3 +1,5 @@
+import json
+from requests import put
 from base import api
 
 TEST_USER_EMAIL = "[email protected]"
@@ -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 protected]' )
+        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" ]

test/unit/managers/test_UserManager.py

@@ -1,5 +1,8 @@
 # -*- coding: utf-8 -*-
 """
+User Manager testing.
+
+Executable directly using: python -m test.unit.managers.test_UserManager
 """
 import imp
 import os
@@ -12,6 +15,7 @@
 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
@@ -215,6 +219,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 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 ):
 
@@ -237,5 +280,4 @@ def test_parsable( self ):
 
 # =============================================================================
 if __name__ == '__main__':
-    # or more generally, nosetests test_resourcemanagers.py -s -v
     unittest.main()