From 25c80f16f57ecc34fda2d52bbd8d3a41e0b33d66 Mon Sep 17 00:00:00 2001 From: Dominik Moritz Date: Sat, 6 Jul 2013 12:05:59 +0200 Subject: [PATCH 01/56] Add coverage reports with coveralls --- .coveragerc | 3 +++ .travis.yml | 4 ++++ bin/travis-build | 2 +- 3 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 .coveragerc diff --git a/.coveragerc b/.coveragerc new file mode 100644 index 00000000000..28f34e39273 --- /dev/null +++ b/.coveragerc @@ -0,0 +1,3 @@ +[run] +omit = /ckan/migration/* +source = ckan, ckanext diff --git a/.travis.yml b/.travis.yml index af450298b6c..b4f63b6c669 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,6 +5,8 @@ python: env: - PGVERSION=9.1 - PGVERSION=8.4 +install: + - pip install coveralls --use-mirrors script: ./bin/travis-build notifications: irc: @@ -14,3 +16,5 @@ notifications: on_failure: change template: - "%{repository} %{branch} %{commit} %{build_url} %{author}: %{message}" +after_success: + - coveralls diff --git a/bin/travis-build b/bin/travis-build index df7c398d1b6..de7599cdddd 100755 --- a/bin/travis-build +++ b/bin/travis-build @@ -48,4 +48,4 @@ fi cat test-core.ini # And finally, run the tests -nosetests --ckan --with-pylons=test-core.ini --nologcapture ckan ckanext +nosetests --ckan --with-pylons=test-core.ini --nologcapture ckan ckanext --with-coverage --cover-package=ckanext --cover-package=ckan From 7c60ba441dcbdb309e82344a6a981b87243a20a3 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Sun, 4 Aug 2013 13:30:18 -0300 Subject: [PATCH 02/56] [#1163] Users can be deleted To do this, I've configured the User model to be stateful using vdm.sqlalchemy. Right now, there're two states: active and deleted. If a user is deleted, he can't login, and is unauthorized to do anything. She also doesn't appear in the user's list anymore, but you can still access her profile page, if you know her username. If she was logged in when her user was deleted, the next time she goes into CKAN, she'll be logged off. Unfortunately, there's not a useful message like "Your user has been deleted." Yet. There's no way to undelete a user, but it should be simply creating an action to set her state to active. --- ckan/config/routing.py | 1 + ckan/controllers/user.py | 15 ++ ckan/lib/authenticator.py | 22 +- ckan/lib/base.py | 5 +- ckan/lib/create_test_data.py | 3 +- ckan/logic/action/delete.py | 22 ++ ckan/logic/action/get.py | 8 +- ckan/logic/auth/delete.py | 4 + .../070_add_state_column_to_user_table.py | 17 ++ ckan/model/follower.py | 201 +++++++----------- ckan/model/user.py | 9 +- ckan/new_authz.py | 35 +-- ckan/templates/user/edit_user_form.html | 6 + ckan/templates/user/read_base.html | 4 + ckan/tests/functional/test_user.py | 19 ++ ckan/tests/lib/test_authenticator.py | 106 +++++++++ ckan/tests/lib/test_dictization.py | 1 + ckan/tests/logic/test_action.py | 38 ++++ ckan/tests/logic/test_auth.py | 27 ++- ckan/tests/models/test_follower.py | 124 +++++++++++ ckan/tests/models/test_user.py | 7 + 21 files changed, 527 insertions(+), 147 deletions(-) create mode 100644 ckan/migration/versions/070_add_state_column_to_user_table.py create mode 100644 ckan/tests/lib/test_authenticator.py create mode 100644 ckan/tests/models/test_follower.py diff --git a/ckan/config/routing.py b/ckan/config/routing.py index 0fbb02bb886..8f78a6c099d 100644 --- a/ckan/config/routing.py +++ b/ckan/config/routing.py @@ -359,6 +359,7 @@ def make_map(): action='followers', ckan_icon='group') m.connect('user_edit', '/user/edit/{id:.*}', action='edit', ckan_icon='cog') + m.connect('user_delete', '/user/delete/{id}', action='delete') m.connect('/user/reset/{id:.*}', action='perform_reset') m.connect('register', '/user/register', action='register') m.connect('login', '/user/login', action='login') diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index ab162ef0659..e0b1ba6c762 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -178,6 +178,21 @@ def new(self, data=None, errors=None, error_summary=None): c.form = render(self.new_user_form, extra_vars=vars) return render('user/new.html') + def delete(self, id): + '''Delete user with id passed as parameter''' + context = {'model': model, + 'session': model.Session, + 'user': c.user} + data_dict = {'id': id} + + try: + get_action('user_delete')(context, data_dict) + user_index = h.url_for(controller='user', action='index') + h.redirect_to(user_index) + except NotAuthorized: + msg = _('Unauthorized to delete user with id "{user_id}".') + abort(401, msg.format(user_id=id)) + def _save_new(self, context): try: data_dict = logic.clean_dict(unflatten( diff --git a/ckan/lib/authenticator.py b/ckan/lib/authenticator.py index 6f061caad91..f64db4f5b8d 100644 --- a/ckan/lib/authenticator.py +++ b/ckan/lib/authenticator.py @@ -12,9 +12,9 @@ class OpenIDAuthenticator(object): def authenticate(self, environ, identity): if 'repoze.who.plugins.openid.userid' in identity: - openid = identity.get('repoze.who.plugins.openid.userid') + openid = identity['repoze.who.plugins.openid.userid'] user = User.by_openid(openid) - if user is None: + if user is None or user.is_deleted(): return None else: return user.name @@ -25,14 +25,20 @@ class UsernamePasswordAuthenticator(object): implements(IAuthenticator) def authenticate(self, environ, identity): - if not 'login' in identity or not 'password' in identity: + if not ('login' in identity and 'password' in identity): return None - user = User.by_name(identity.get('login')) + + login = identity['login'] + user = User.by_name(login) + if user is None: - log.debug('Login failed - username %r not found', identity.get('login')) - return None - if user.validate_password(identity.get('password')): + log.debug('Login failed - username %r not found', login) + elif user.is_deleted(): + log.debug('Login as %r failed - user is deleted', login) + elif not user.validate_password(identity['password']): + log.debug('Login as %r failed - password not valid', login) + else: return user.name - log.debug('Login as %r failed - password not valid', identity.get('login')) + return None diff --git a/ckan/lib/base.py b/ckan/lib/base.py index 1298e0665c7..bfffcaf2a5d 100644 --- a/ckan/lib/base.py +++ b/ckan/lib/base.py @@ -290,8 +290,9 @@ def _identify_user_default(self): if c.user: c.user = c.user.decode('utf8') c.userobj = model.User.by_name(c.user) - if c.userobj is None: - # This occurs when you are logged in, clean db + if c.userobj is None or c.userobj.is_deleted(): + # This occurs when a user that was still logged in is deleted, + # or when you are logged in, clean db # and then restart (or when you change your username) # There is no user object, so even though repoze thinks you # are logged in and your cookie has ckan_display_name, we diff --git a/ckan/lib/create_test_data.py b/ckan/lib/create_test_data.py index 3edf865c7c7..6c7a275c36f 100644 --- a/ckan/lib/create_test_data.py +++ b/ckan/lib/create_test_data.py @@ -519,8 +519,9 @@ def _create_user_without_commit(cls, name='', **user_dict): @classmethod def create_user(cls, name='', **kwargs): - cls._create_user_without_commit(name, **kwargs) + user = cls._create_user_without_commit(name, **kwargs) model.Session.commit() + return user @classmethod def flag_for_deletion(cls, pkg_names=[], tag_names=[], group_names=[], diff --git a/ckan/logic/action/delete.py b/ckan/logic/action/delete.py index 933e7624b43..9191151aa01 100644 --- a/ckan/logic/action/delete.py +++ b/ckan/logic/action/delete.py @@ -18,6 +18,28 @@ _get_or_bust = ckan.logic.get_or_bust _get_action = ckan.logic.get_action +def user_delete(context, data_dict): + '''Delete a user. + + Only sysadmins can delete users. + + :param id: the id or usernamename of the user to delete + :type id: string + ''' + + _check_access('user_delete', context, data_dict) + + model = context['model'] + user_id = _get_or_bust(data_dict, 'id') + user = model.User.get(user_id) + + if user is None: + raise NotFound('User "{id}" was not found.'.format(id=user_id)) + + user.delete() + model.repo.commit() + + def package_delete(context, data_dict): '''Delete a dataset (package). diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index c0fe9639764..a6cec306c91 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -7,6 +7,7 @@ from pylons import config import sqlalchemy +import vdm.sqlalchemy import ckan.lib.dictization import ckan.logic as logic @@ -664,6 +665,9 @@ def user_list(context, data_dict): else_=model.User.fullname) ) + # Filter deleted users + query = query.filter(model.User.state != vdm.sqlalchemy.State.DELETED) + ## hack for pagination if context.get('return_query'): return query @@ -1173,7 +1177,9 @@ def user_autocomplete(context, data_dict): q = data_dict['q'] limit = data_dict.get('limit', 20) - query = model.User.search(q).limit(limit) + query = model.User.search(q) + query = query.filter(model.User.state != vdm.sqlalchemy.State.DELETED) + query = query.limit(limit) user_list = [] for user in query.all(): diff --git a/ckan/logic/auth/delete.py b/ckan/logic/auth/delete.py index 61b44697407..a2b121c11ad 100644 --- a/ckan/logic/auth/delete.py +++ b/ckan/logic/auth/delete.py @@ -4,6 +4,10 @@ from ckan.logic.auth import get_resource_object from ckan.lib.base import _ +def user_delete(context, data_dict): + # Only sysadmins are authorized to purge organizations. + return {'success': False} + def package_delete(context, data_dict): user = context['user'] package = get_package_object(context, data_dict) diff --git a/ckan/migration/versions/070_add_state_column_to_user_table.py b/ckan/migration/versions/070_add_state_column_to_user_table.py new file mode 100644 index 00000000000..1dfadbe7d32 --- /dev/null +++ b/ckan/migration/versions/070_add_state_column_to_user_table.py @@ -0,0 +1,17 @@ +import vdm.sqlalchemy + + +def upgrade(migrate_engine): + migrate_engine.execute( + ''' + ALTER TABLE "user" ADD COLUMN "state" text NOT NULL DEFAULT '%s' + ''' % vdm.sqlalchemy.State.ACTIVE + ) + + +def downgrade(migrate_engine): + migrate_engine.exeecute( + ''' + ALTER TABLE "user" DROP COLUMN "state" + ''' + ) diff --git a/ckan/model/follower.py b/ckan/model/follower.py index 6e6096ed5cc..d14ffe667ae 100644 --- a/ckan/model/follower.py +++ b/ckan/model/follower.py @@ -1,30 +1,28 @@ -import sqlalchemy import meta import datetime -import domain_object +import sqlalchemy +import vdm.sqlalchemy -class UserFollowingUser(domain_object.DomainObject): - '''A many-many relationship between users. +import ckan.model +import domain_object - A relationship between one user (the follower) and another (the object), - that means that the follower is currently following the object. - ''' +class ModelFollowingModel(domain_object.DomainObject): def __init__(self, follower_id, object_id): self.follower_id = follower_id self.object_id = object_id self.datetime = datetime.datetime.now() @classmethod - def get(self, follower_id, object_id): - '''Return a UserFollowingUser object for the given follower_id and + def get(cls, follower_id, object_id): + '''Return a ModelFollowingModel object for the given follower_id and object_id, or None if no such follower exists. ''' - query = meta.Session.query(UserFollowingUser) - query = query.filter(UserFollowingUser.follower_id==follower_id) - query = query.filter(UserFollowingUser.object_id==object_id) - return query.first() + query = cls._get(follower_id, object_id) + following = cls._filter_following_objects(query) + if len(following) == 1: + return following[0] @classmethod def is_following(cls, follower_id, object_id): @@ -32,33 +30,76 @@ def is_following(cls, follower_id, object_id): otherwise. ''' - return UserFollowingUser.get(follower_id, object_id) is not None - + return cls.get(follower_id, object_id) is not None @classmethod def followee_count(cls, follower_id): - '''Return the number of users followed by a user.''' - return meta.Session.query(UserFollowingUser).filter( - UserFollowingUser.follower_id == follower_id).count() + '''Return the number of objects followed by the follower.''' + return cls._get_followees(follower_id).count() @classmethod def followee_list(cls, follower_id): - '''Return a list of users followed by a user.''' - return meta.Session.query(UserFollowingUser).filter( - UserFollowingUser.follower_id == follower_id).all() + '''Return a list of objects followed by the follower.''' + query = cls._get_followees(follower_id).all() + followees = cls._filter_following_objects(query) + return followees + + @classmethod + def follower_count(cls, object_id): + '''Return the number of followers of the object.''' + return cls._get_followers(object_id).count() + + @classmethod + def follower_list(cls, object_id): + '''Return a list of followers of the object.''' + query = cls._get_followers(object_id).all() + followers = cls._filter_following_objects(query) + return followers + + @classmethod + def _filter_following_objects(cls, query): + return [q[0] for q in query] + + @classmethod + def _get_followees(cls, follower_id): + return cls._get(follower_id) + + @classmethod + def _get_followers(cls, object_id): + return cls._get(None, object_id) + + @classmethod + def _get(cls, follower_id=None, object_id=None): + follower_alias = sqlalchemy.orm.aliased(cls._follower_class()) + object_alias = sqlalchemy.orm.aliased(cls._object_class()) + + follower_id = follower_id or cls.follower_id + object_id = object_id or cls.object_id + + query = meta.Session.query(cls, follower_alias, object_alias)\ + .filter(sqlalchemy.and_(follower_alias.id == follower_id,\ + cls.follower_id == follower_alias.id,\ + cls.object_id == object_alias.id,\ + follower_alias.state != vdm.sqlalchemy.State.DELETED,\ + object_alias.state != vdm.sqlalchemy.State.DELETED,\ + object_alias.id == object_id)) + return query + +class UserFollowingUser(ModelFollowingModel): + '''A many-many relationship between users. + + A relationship between one user (the follower) and another (the object), + that means that the follower is currently following the object. + ''' @classmethod - def follower_count(cls, user_id): - '''Return the number of followers of a user.''' - return meta.Session.query(UserFollowingUser).filter( - UserFollowingUser.object_id == user_id).count() + def _follower_class(cls): + return ckan.model.User @classmethod - def follower_list(cls, user_id): - '''Return a list of followers of a user.''' - return meta.Session.query(UserFollowingUser).filter( - UserFollowingUser.object_id == user_id).all() + def _object_class(cls): + return ckan.model.User user_following_user_table = sqlalchemy.Table('user_following_user', @@ -76,62 +117,20 @@ def follower_list(cls, user_id): meta.mapper(UserFollowingUser, user_following_user_table) -class UserFollowingDataset(domain_object.DomainObject): +class UserFollowingDataset(ModelFollowingModel): '''A many-many relationship between users and datasets (packages). A relationship between a user (the follower) and a dataset (the object), that means that the user is currently following the dataset. ''' - def __init__(self, follower_id, object_id): - self.follower_id = follower_id - self.object_id = object_id - self.datetime = datetime.datetime.now() - - @classmethod - def get(self, follower_id, object_id): - '''Return a UserFollowingDataset object for the given follower_id and - object_id, or None if no such follower exists. - - ''' - query = meta.Session.query(UserFollowingDataset) - query = query.filter(UserFollowingDataset.follower_id==follower_id) - query = query.filter(UserFollowingDataset.object_id==object_id) - return query.first() - - @classmethod - def is_following(cls, follower_id, object_id): - '''Return True if follower_id is currently following object_id, False - otherwise. - - ''' - return UserFollowingDataset.get(follower_id, object_id) is not None - - - @classmethod - def followee_count(cls, follower_id): - '''Return the number of datasets followed by a user.''' - return meta.Session.query(UserFollowingDataset).filter( - UserFollowingDataset.follower_id == follower_id).count() - - @classmethod - def followee_list(cls, follower_id): - '''Return a list of datasets followed by a user.''' - return meta.Session.query(UserFollowingDataset).filter( - UserFollowingDataset.follower_id == follower_id).all() - - @classmethod - def follower_count(cls, dataset_id): - '''Return the number of followers of a dataset.''' - return meta.Session.query(UserFollowingDataset).filter( - UserFollowingDataset.object_id == dataset_id).count() + def _follower_class(cls): + return ckan.model.User @classmethod - def follower_list(cls, dataset_id): - '''Return a list of followers of a dataset.''' - return meta.Session.query(UserFollowingDataset).filter( - UserFollowingDataset.object_id == dataset_id).all() + def _object_class(cls): + return ckan.model.Package user_following_dataset_table = sqlalchemy.Table('user_following_dataset', @@ -150,60 +149,20 @@ def follower_list(cls, dataset_id): meta.mapper(UserFollowingDataset, user_following_dataset_table) -class UserFollowingGroup(domain_object.DomainObject): +class UserFollowingGroup(ModelFollowingModel): '''A many-many relationship between users and groups. A relationship between a user (the follower) and a group (the object), that means that the user is currently following the group. ''' - def __init__(self, follower_id, object_id): - self.follower_id = follower_id - self.object_id = object_id - self.datetime = datetime.datetime.now() - - @classmethod - def get(self, follower_id, object_id): - '''Return a UserFollowingGroup object for the given follower_id and - object_id, or None if no such relationship exists. - - ''' - query = meta.Session.query(UserFollowingGroup) - query = query.filter(UserFollowingGroup.follower_id == follower_id) - query = query.filter(UserFollowingGroup.object_id == object_id) - return query.first() - - @classmethod - def is_following(cls, follower_id, object_id): - '''Return True if follower_id is currently following object_id, False - otherwise. - - ''' - return UserFollowingGroup.get(follower_id, object_id) is not None - - @classmethod - def followee_count(cls, follower_id): - '''Return the number of groups followed by a user.''' - return meta.Session.query(UserFollowingGroup).filter( - UserFollowingGroup.follower_id == follower_id).count() - @classmethod - def followee_list(cls, follower_id): - '''Return a list of groups followed by a user.''' - return meta.Session.query(UserFollowingGroup).filter( - UserFollowingGroup.follower_id == follower_id).all() + def _follower_class(cls): + return ckan.model.User @classmethod - def follower_count(cls, object_id): - '''Return the number of users following a group.''' - return meta.Session.query(UserFollowingGroup).filter( - UserFollowingGroup.object_id == object_id).count() - - @classmethod - def follower_list(cls, object_id): - '''Return a list of the users following a group.''' - return meta.Session.query(UserFollowingGroup).filter( - UserFollowingGroup.object_id == object_id).all() + def _object_class(cls): + return ckan.model.Group user_following_group_table = sqlalchemy.Table('user_following_group', meta.metadata, diff --git a/ckan/model/user.py b/ckan/model/user.py index d3eb3d6f1c1..d36dd5343cc 100644 --- a/ckan/model/user.py +++ b/ckan/model/user.py @@ -6,6 +6,7 @@ from sqlalchemy.sql.expression import or_ from sqlalchemy.orm import synonym from sqlalchemy import types, Column, Table +import vdm.sqlalchemy import meta import types as _types @@ -28,8 +29,11 @@ Column('sysadmin', types.Boolean, default=False), ) +vdm.sqlalchemy.make_table_stateful(user_table) -class User(domain_object.DomainObject): + +class User(vdm.sqlalchemy.StatefulObjectMixin, + domain_object.DomainObject): VALID_NAME = re.compile(r"^[a-zA-Z0-9_\-]{3,255}$") DOUBLE_SLASH = re.compile(':\/([^/])') @@ -162,6 +166,9 @@ def number_administered_packages(self): q = q.filter_by(user=self, role=model.Role.ADMIN) return q.count() + def is_deleted(self): + return self.state == vdm.sqlalchemy.State.DELETED + def is_in_group(self, group): return group in self.get_group_ids() diff --git a/ckan/new_authz.py b/ckan/new_authz.py index 01fb07927ac..bf1d0830d47 100644 --- a/ckan/new_authz.py +++ b/ckan/new_authz.py @@ -101,23 +101,30 @@ def clean_action_name(action_name): def is_sysadmin(username): - ''' returns True is username is a sysadmin ''' + ''' Returns True is username is a sysadmin ''' + user = _get_user(username) + return user and user.sysadmin + + +def is_deleted(username): + ''' Returns True if username is deleted ''' + user = _get_user(username) + return user and user.is_deleted() + + +def _get_user(username): + ''' Try to get the user from c, if possible, and fallback to using the DB ''' if not username: - return False - # see if we can authorise without touching the database + return None + # See if we can get the user without touching the DB try: if c.userobj and c.userobj.name == username: - if c.userobj.sysadmin: - return True - return False + return c.userobj except TypeError: # c is not available pass - # get user from the database - user = model.User.get(username) - if user and user.sysadmin: - return True - return False + # Get user from the DB + return model.User.get(username) def get_group_or_org_admin_ids(group_id): @@ -146,10 +153,14 @@ def is_authorized(action, context, data_dict=None): action = clean_action_name(action) auth_function = _AuthFunctions.get(action) if auth_function: + username = context.get('user') + # deleted users are always unauthorized + if is_deleted(username): + return {'success': False} # sysadmins can do anything unless the auth_sysadmins_check # decorator was used in which case they are treated like all other # users. - if is_sysadmin(context.get('user')): + elif is_sysadmin(username): if not getattr(auth_function, 'auth_sysadmins_check', False): return {'success': True} diff --git a/ckan/templates/user/edit_user_form.html b/ckan/templates/user/edit_user_form.html index 19b8755b2bd..ce31e34b583 100644 --- a/ckan/templates/user/edit_user_form.html +++ b/ckan/templates/user/edit_user_form.html @@ -33,6 +33,12 @@
+ {% block delete_button %} + {% if h.check_access('user_delete', {'id': data.id}) %} + {% set locale = h.dump_json({'content': _('Are you sure you want to delete this User?')}) %} + {% block delete_button_text %}{{ _('Delete') }}{% endblock %} + {% endif %} + {% endblock %}
diff --git a/ckan/templates/user/read_base.html b/ckan/templates/user/read_base.html index aa27a2b4119..f2b94e20782 100644 --- a/ckan/templates/user/read_base.html +++ b/ckan/templates/user/read_base.html @@ -76,6 +76,10 @@

{{ user.display_name }}

{{ _('Member Since') }}
{{ h.render_datetime(user.created) }}
+
+
{{ _('State') }}
+
{{ user.state }}
+
{% if c.is_myself %}
{{ _('API Key') }} {{ _('Private') }}
diff --git a/ckan/tests/functional/test_user.py b/ckan/tests/functional/test_user.py index 0ef528fd9ad..16795561f46 100644 --- a/ckan/tests/functional/test_user.py +++ b/ckan/tests/functional/test_user.py @@ -64,6 +64,25 @@ def test_user_read(self): 'rel="nofollow"') assert 'Edit Profile' not in main_res, main_res + def test_user_delete_redirects_to_user_index(self): + user = CreateTestData.create_user('a_user') + url = url_for(controller='user', action='delete', id=user.id) + extra_environ = {'REMOTE_USER': 'testsysadmin'} + + redirect_url = url_for(controller='user', action='index', + qualified=True) + res = self.app.get(url, status=302, extra_environ=extra_environ) + + assert user.is_deleted(), user + assert res.header('Location').startswith(redirect_url), res.header('Location') + + def test_user_delete_by_unauthorized_user(self): + user = model.User.by_name(u'annafan') + url = url_for(controller='user', action='delete', id=user.id) + extra_environ = {'REMOTE_USER': 'an_unauthorized_user'} + + self.app.get(url, status=401, extra_environ=extra_environ) + def test_user_read_without_id(self): offset = '/user/' res = self.app.get(offset, status=302) diff --git a/ckan/tests/lib/test_authenticator.py b/ckan/tests/lib/test_authenticator.py new file mode 100644 index 00000000000..c7c026112d2 --- /dev/null +++ b/ckan/tests/lib/test_authenticator.py @@ -0,0 +1,106 @@ +import ckan + +import ckan.lib.create_test_data as ctd +import ckan.lib.authenticator as authenticator + +CreateTestData = ctd.CreateTestData + + +class TestUsernamePasswordAuthenticator(object): + @classmethod + def setup_class(cls): + auth = authenticator.UsernamePasswordAuthenticator() + cls.authenticate = auth.authenticate + + @classmethod + def teardown(cls): + ckan.model.repo.rebuild_db() + + def test_authenticate_succeeds_if_login_and_password_are_correct(self): + environ = {} + password = 'somepass' + user = CreateTestData.create_user('a_user', **{'password': password}) + identity = {'login': user.name, 'password': password} + + username = self.authenticate(environ, identity) + assert username == user.name, username + + def test_authenticate_fails_if_user_is_deleted(self): + environ = {} + password = 'somepass' + user = CreateTestData.create_user('a_user', **{'password': password}) + identity = {'login': user.name, 'password': password} + user.delete() + + assert self.authenticate(environ, identity) is None + + def test_authenticate_fails_if_password_is_wrong(self): + environ = {} + user = CreateTestData.create_user('a_user') + identity = {'login': user.name, 'password': 'wrong-password'} + assert self.authenticate(environ, identity) is None + + def test_authenticate_fails_if_received_no_login_or_pass(self): + environ = {} + identity = {} + assert self.authenticate(environ, identity) is None + + def test_authenticate_fails_if_received_just_login(self): + environ = {} + identity = {'login': 'some-user'} + assert self.authenticate(environ, identity) is None + + def test_authenticate_fails_if_received_just_password(self): + environ = {} + identity = {'password': 'some-password'} + assert self.authenticate(environ, identity) is None + + def test_authenticate_fails_if_user_doesnt_exist(self): + environ = {} + identity = {'login': 'inexistent-user'} + assert self.authenticate(environ, identity) is None + + +class TestOpenIDAuthenticator(object): + @classmethod + def setup_class(cls): + auth = authenticator.OpenIDAuthenticator() + cls.authenticate = auth.authenticate + + @classmethod + def teardown(cls): + ckan.model.repo.rebuild_db() + + def test_authenticate_succeeds_if_openid_is_correct(self): + environ = {} + openid = 'some-openid-key' + user = CreateTestData.create_user('a_user', **{'openid': openid}) + identity = {'login': user.name, + 'repoze.who.plugins.openid.userid': openid} + + username = self.authenticate(environ, identity) + assert username == user.name, username + + def test_authenticate_fails_if_openid_is_incorrect(self): + environ = {} + openid = 'wrong-openid-key' + user = CreateTestData.create_user('a_user') + identity = {'login': user.name, + 'repoze.who.plugins.openid.userid': openid} + + assert self.authenticate(environ, identity) is None + + def test_authenticate_fails_if_user_is_deleted(self): + environ = {} + openid = 'some-openid-key' + user = CreateTestData.create_user('a_user', **{'openid': openid}) + user.delete() + identity = {'login': user.name, + 'repoze.who.plugins.openid.userid': openid} + + assert self.authenticate(environ, identity) is None + + def test_authenticate_fails_if_user_have_no_openid(self): + environ = {} + identity = {} + assert self.authenticate(environ, identity) is None diff --git a/ckan/tests/lib/test_dictization.py b/ckan/tests/lib/test_dictization.py index fb8bb49e628..72ca6fa31e5 100644 --- a/ckan/tests/lib/test_dictization.py +++ b/ckan/tests/lib/test_dictization.py @@ -939,6 +939,7 @@ def test_16_group_dictized(self): 'users': [{'about': u'I love reading Annakarenina. My site: http://anna.com', 'display_name': u'annafan', 'capacity' : 'public', + 'state': 'active', 'sysadmin': False, 'email_hash': 'd41d8cd98f00b204e9800998ecf8427e', 'fullname': None, diff --git a/ckan/tests/logic/test_action.py b/ckan/tests/logic/test_action.py index 73a3b838f0c..fecbdf96b53 100644 --- a/ckan/tests/logic/test_action.py +++ b/ckan/tests/logic/test_action.py @@ -7,6 +7,7 @@ from pylons import config import datetime +import vdm.sqlalchemy import ckan from ckan.lib.create_test_data import CreateTestData from ckan.lib.dictization.model_dictize import resource_dictize @@ -334,6 +335,11 @@ def test_42_create_resource_with_error(self): def test_04_user_list(self): + # Create deleted user to make sure he won't appear in the user_list + deleted_user = CreateTestData.create_user('deleted_user') + deleted_user.delete() + model.repo.commit() + postparams = '%s=1' % json.dumps({}) res = self.app.post('/api/action/user_list', params=postparams) res_obj = json.loads(res.body) @@ -555,6 +561,33 @@ def test_12_user_update_errors(self): for expected_message in test_call['messages']: assert expected_message[1] in ''.join(res_obj['error'][expected_message[0]]) + def test_user_delete(self): + name = 'normal_user' + CreateTestData.create_user(name) + user = model.User.get(name) + user_dict = {'id': user.id} + postparams = '%s=1' % json.dumps(user_dict) + + res = self.app.post('/api/action/user_delete', params=postparams, + extra_environ={'Authorization': str(self.sysadmin_user.apikey)}) + + res_obj = json.loads(res.body) + deleted_user = model.User.get(name) + assert res_obj['success'] is True + assert deleted_user.is_deleted(), deleted_user + + def test_user_delete_requires_data_dict_with_key_id(self): + user_dict = {'name': 'normal_user'} + postparams = '%s=1' % json.dumps(user_dict) + + res = self.app.post('/api/action/user_delete', params=postparams, + extra_environ={'Authorization': str(self.sysadmin_user.apikey)}, + status=StatusCodes.STATUS_409_CONFLICT) + + res_obj = json.loads(res.body) + assert res_obj['success'] is False + assert res_obj['error']['id'] == ['Missing value'] + def test_13_group_list(self): postparams = '%s=1' % json.dumps({}) res = self.app.post('/api/action/group_list', params=postparams) @@ -633,6 +666,11 @@ def test_14_group_show(self): assert res_obj['success'] is False def test_16_user_autocomplete(self): + # Create deleted user to make sure he won't appear in the user_list + deleted_user = CreateTestData.create_user('joe') + deleted_user.delete() + model.repo.commit() + #Empty query postparams = '%s=1' % json.dumps({}) res = self.app.post( diff --git a/ckan/tests/logic/test_auth.py b/ckan/tests/logic/test_auth.py index 3ea9f2db39f..0f0a63372b2 100644 --- a/ckan/tests/logic/test_auth.py +++ b/ckan/tests/logic/test_auth.py @@ -48,8 +48,33 @@ def create_user(self, name): self.apikeys[name] = str(json.loads(res.body)['result']['apikey']) -class TestAuthOrgs(TestAuth): +class TestAuthUsers(TestAuth): + def test_only_sysadmins_can_delete_users(self): + username = 'username' + user = {'id': username} + self.create_user(username) + + self._call_api('user_delete', user, username, 403) + self._call_api('user_delete', user, 'sysadmin', 200) + + def test_auth_deleted_users_are_always_unauthorized(self): + always_success = lambda x,y: {'success': True} + new_authz._AuthFunctions._build() + new_authz._AuthFunctions._functions['always_success'] = always_success + # We can't reuse the username with the other tests because we can't + # rebuild_db(), because in the setup_class we get the sysadmin. If we + # rebuild the DB, we would delete the sysadmin as well. + username = 'other_username' + self.create_user(username) + user = model.User.get(username) + user.delete() + + assert not new_authz.is_authorized_boolean('always_success', {'user': username}) + del new_authz._AuthFunctions._functions['always_success'] + + +class TestAuthOrgs(TestAuth): def test_01_create_users(self): # actual roles assigned later self.create_user('org_admin') diff --git a/ckan/tests/models/test_follower.py b/ckan/tests/models/test_follower.py new file mode 100644 index 00000000000..9f4eb51ea3f --- /dev/null +++ b/ckan/tests/models/test_follower.py @@ -0,0 +1,124 @@ +import ckan.model as model +import ckan.lib.create_test_data as ctd + +CreateTestData = ctd.CreateTestData + + +class FollowerClassesTests(object): + @classmethod + def teardown_class(cls): + model.repo.rebuild_db() + + def test_get(self): + following = self.FOLLOWER_CLASS.get(self.follower.id, self.followee.id) + assert following.follower_id == self.follower.id, following + assert following.object_id == self.followee.id, following + + def test_get_returns_none_if_couldnt_find_users(self): + following = self.FOLLOWER_CLASS.get('some-id', 'other-id') + assert following is None, following + + def test_is_following(self): + assert self.FOLLOWER_CLASS.is_following(self.follower.id, + self.followee.id) + + def test_is_following_returns_false_if_user_isnt_following(self): + assert not self.FOLLOWER_CLASS.is_following(self.followee.id, + self.follower.id) + + def test_followee_count(self): + count = self.FOLLOWER_CLASS.followee_count(self.follower.id) + assert count == 1, count + + def test_followee_list(self): + followees = self.FOLLOWER_CLASS.followee_list(self.follower.id) + object_ids = [f.object_id for f in followees] + assert object_ids == [self.followee.id], object_ids + + def test_follower_count(self): + count = self.FOLLOWER_CLASS.follower_count(self.followee.id) + assert count == 1, count + + def test_follower_list(self): + followers = self.FOLLOWER_CLASS.follower_list(self.followee.id) + follower_ids = [f.follower_id for f in followers] + assert follower_ids == [self.follower.id], follower_ids + + +class TestUserFollowingUser(FollowerClassesTests): + FOLLOWER_CLASS = model.UserFollowingUser + + @classmethod + def setup_class(cls): + model.repo.rebuild_db() + cls.follower = CreateTestData.create_user('follower') + cls.followee = CreateTestData.create_user('followee') + cls.FOLLOWER_CLASS(cls.follower.id, cls.followee.id).save() + cls._create_deleted_models() + + @classmethod + def _create_deleted_models(cls): + deleted_user = CreateTestData.create_user('deleted_user') + cls.FOLLOWER_CLASS(deleted_user.id, cls.followee.id).save() + cls.FOLLOWER_CLASS(cls.follower.id, deleted_user.id).save() + deleted_user.delete() + deleted_user.save() + + +class TestUserFollowingDataset(FollowerClassesTests): + FOLLOWER_CLASS = model.UserFollowingDataset + + @classmethod + def setup_class(cls): + model.repo.rebuild_db() + cls.follower = CreateTestData.create_user('follower') + cls.followee = cls._create_dataset('followee') + cls.FOLLOWER_CLASS(cls.follower.id, cls.followee.id).save() + cls._create_deleted_models() + + @classmethod + def _create_deleted_models(cls): + deleted_user = CreateTestData.create_user('deleted_user') + cls.FOLLOWER_CLASS(deleted_user.id, cls.followee.id).save() + deleted_user.delete() + deleted_user.save() + deleted_dataset = cls._create_dataset('deleted_dataset') + cls.FOLLOWER_CLASS(cls.follower.id, deleted_dataset.id).save() + deleted_dataset.delete() + deleted_dataset.save() + + @classmethod + def _create_dataset(self, name): + CreateTestData.create_arbitrary({'name': name}) + return model.Package.get(name) + + +class TestUserFollowingGroup(FollowerClassesTests): + FOLLOWER_CLASS = model.UserFollowingGroup + + @classmethod + def setup_class(cls): + model.repo.rebuild_db() + model.repo.new_revision() + cls.follower = CreateTestData.create_user('follower') + cls.followee = cls._create_group('followee') + cls.FOLLOWER_CLASS(cls.follower.id, cls.followee.id).save() + cls._create_deleted_models() + model.repo.commit_and_remove() + + @classmethod + def _create_deleted_models(cls): + deleted_user = CreateTestData.create_user('deleted_user') + cls.FOLLOWER_CLASS(deleted_user.id, cls.followee.id).save() + deleted_user.delete() + deleted_user.save() + deleted_group = cls._create_group('deleted_group') + cls.FOLLOWER_CLASS(cls.follower.id, deleted_group.id).save() + deleted_group.delete() + deleted_group.save() + + @classmethod + def _create_group(self, name): + group = model.Group(name) + group.save() + return group diff --git a/ckan/tests/models/test_user.py b/ckan/tests/models/test_user.py index b9ef7f0624f..c4b356ae854 100644 --- a/ckan/tests/models/test_user.py +++ b/ckan/tests/models/test_user.py @@ -56,6 +56,13 @@ def test_4_get_openid_missing_slash(self): assert out assert out.fullname == u'Sandra' + def test_is_deleted(self): + user = CreateTestData._create_user_without_commit('a_user') + assert not user.is_deleted(), user + user.delete() + assert user.is_deleted(), user + + def to_names(domain_obj_list): '''Takes a list of domain objects and returns a corresponding list of their names.''' From ad2de00a4e2c2772d1e59faee26ca818731168c9 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Fri, 9 Aug 2013 16:07:29 -0300 Subject: [PATCH 03/56] [#1163] Use ckan.model.State instead of vdm.sqlalchemy.State This makes us a bit less tied to vdm. --- ckan/logic/action/get.py | 5 ++--- .../migration/versions/070_add_state_column_to_user_table.py | 4 ++-- ckan/model/follower.py | 4 ++-- ckan/model/group.py | 4 ++-- ckan/model/user.py | 4 +++- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index a6cec306c91..9ede04d7a39 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -7,7 +7,6 @@ from pylons import config import sqlalchemy -import vdm.sqlalchemy import ckan.lib.dictization import ckan.logic as logic @@ -666,7 +665,7 @@ def user_list(context, data_dict): ) # Filter deleted users - query = query.filter(model.User.state != vdm.sqlalchemy.State.DELETED) + query = query.filter(model.User.state != model.State.DELETED) ## hack for pagination if context.get('return_query'): @@ -1178,7 +1177,7 @@ def user_autocomplete(context, data_dict): limit = data_dict.get('limit', 20) query = model.User.search(q) - query = query.filter(model.User.state != vdm.sqlalchemy.State.DELETED) + query = query.filter(model.User.state != model.State.DELETED) query = query.limit(limit) user_list = [] diff --git a/ckan/migration/versions/070_add_state_column_to_user_table.py b/ckan/migration/versions/070_add_state_column_to_user_table.py index 1dfadbe7d32..46c5c3d4cd5 100644 --- a/ckan/migration/versions/070_add_state_column_to_user_table.py +++ b/ckan/migration/versions/070_add_state_column_to_user_table.py @@ -1,11 +1,11 @@ -import vdm.sqlalchemy +import ckan.model def upgrade(migrate_engine): migrate_engine.execute( ''' ALTER TABLE "user" ADD COLUMN "state" text NOT NULL DEFAULT '%s' - ''' % vdm.sqlalchemy.State.ACTIVE + ''' % ckan.model.State.ACTIVE ) diff --git a/ckan/model/follower.py b/ckan/model/follower.py index d14ffe667ae..66ac1da435d 100644 --- a/ckan/model/follower.py +++ b/ckan/model/follower.py @@ -80,8 +80,8 @@ def _get(cls, follower_id=None, object_id=None): .filter(sqlalchemy.and_(follower_alias.id == follower_id,\ cls.follower_id == follower_alias.id,\ cls.object_id == object_alias.id,\ - follower_alias.state != vdm.sqlalchemy.State.DELETED,\ - object_alias.state != vdm.sqlalchemy.State.DELETED,\ + follower_alias.state != ckan.model.State.DELETED,\ + object_alias.state != ckan.model.State.DELETED,\ object_alias.id == object_id)) return query diff --git a/ckan/model/group.py b/ckan/model/group.py index c16424affee..d5fadab6c2c 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -195,8 +195,8 @@ def packages(self, with_private=False, limit=None, query = meta.Session.query(_package.Package).\ filter( - or_(_package.Package.state == vdm.sqlalchemy.State.ACTIVE, - _package.Package.state == vdm.sqlalchemy.State.PENDING)). \ + or_(_package.Package.state == model.State.ACTIVE, + _package.Package.state == model.State.PENDING)). \ filter(group_table.c.id == self.id).\ filter(member_table.c.state == 'active') diff --git a/ckan/model/user.py b/ckan/model/user.py index d36dd5343cc..82e1acd99b8 100644 --- a/ckan/model/user.py +++ b/ckan/model/user.py @@ -167,7 +167,9 @@ def number_administered_packages(self): return q.count() def is_deleted(self): - return self.state == vdm.sqlalchemy.State.DELETED + # have to import here to avoid circular imports + import ckan.model as model + return self.state == model.State.DELETED def is_in_group(self, group): return group in self.get_group_ids() From d6fde48e5cd73a2d0754b3ceb9fe6319deedaa20 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Fri, 9 Aug 2013 18:40:36 -0300 Subject: [PATCH 04/56] [#1163] Use ckan.model.core.State instead of ckan.model.State With this change, we're able to avoid having to load ckan.model inside methods, to avoid circular dependencies. --- ckan/logic/action/get.py | 5 +++-- .../migration/versions/070_add_state_column_to_user_table.py | 4 ++-- ckan/model/follower.py | 5 +++-- ckan/model/group.py | 4 ++-- ckan/model/user.py | 5 ++--- 5 files changed, 12 insertions(+), 11 deletions(-) diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index 9ede04d7a39..176096f4481 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -14,6 +14,7 @@ import ckan.logic.schema import ckan.lib.dictization.model_dictize as model_dictize import ckan.lib.navl.dictization_functions +import ckan.model.core as core import ckan.model.misc as misc import ckan.plugins as plugins import ckan.lib.search as search @@ -665,7 +666,7 @@ def user_list(context, data_dict): ) # Filter deleted users - query = query.filter(model.User.state != model.State.DELETED) + query = query.filter(model.User.state != core.State.DELETED) ## hack for pagination if context.get('return_query'): @@ -1177,7 +1178,7 @@ def user_autocomplete(context, data_dict): limit = data_dict.get('limit', 20) query = model.User.search(q) - query = query.filter(model.User.state != model.State.DELETED) + query = query.filter(model.User.state != core.State.DELETED) query = query.limit(limit) user_list = [] diff --git a/ckan/migration/versions/070_add_state_column_to_user_table.py b/ckan/migration/versions/070_add_state_column_to_user_table.py index 46c5c3d4cd5..367e3a48c32 100644 --- a/ckan/migration/versions/070_add_state_column_to_user_table.py +++ b/ckan/migration/versions/070_add_state_column_to_user_table.py @@ -1,11 +1,11 @@ -import ckan.model +import ckan.model.core def upgrade(migrate_engine): migrate_engine.execute( ''' ALTER TABLE "user" ADD COLUMN "state" text NOT NULL DEFAULT '%s' - ''' % ckan.model.State.ACTIVE + ''' % ckan.model.core.State.ACTIVE ) diff --git a/ckan/model/follower.py b/ckan/model/follower.py index 66ac1da435d..a8afd07706f 100644 --- a/ckan/model/follower.py +++ b/ckan/model/follower.py @@ -3,6 +3,7 @@ import sqlalchemy import vdm.sqlalchemy +import core import ckan.model import domain_object @@ -80,8 +81,8 @@ def _get(cls, follower_id=None, object_id=None): .filter(sqlalchemy.and_(follower_alias.id == follower_id,\ cls.follower_id == follower_alias.id,\ cls.object_id == object_alias.id,\ - follower_alias.state != ckan.model.State.DELETED,\ - object_alias.state != ckan.model.State.DELETED,\ + follower_alias.state != core.State.DELETED,\ + object_alias.state != core.State.DELETED,\ object_alias.id == object_id)) return query diff --git a/ckan/model/group.py b/ckan/model/group.py index d5fadab6c2c..86963983e9a 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -195,8 +195,8 @@ def packages(self, with_private=False, limit=None, query = meta.Session.query(_package.Package).\ filter( - or_(_package.Package.state == model.State.ACTIVE, - _package.Package.state == model.State.PENDING)). \ + or_(_package.Package.state == core.State.ACTIVE, + _package.Package.state == core.State.PENDING)). \ filter(group_table.c.id == self.id).\ filter(member_table.c.state == 'active') diff --git a/ckan/model/user.py b/ckan/model/user.py index 82e1acd99b8..ce882ca2b8f 100644 --- a/ckan/model/user.py +++ b/ckan/model/user.py @@ -9,6 +9,7 @@ import vdm.sqlalchemy import meta +import core import types as _types import domain_object @@ -167,9 +168,7 @@ def number_administered_packages(self): return q.count() def is_deleted(self): - # have to import here to avoid circular imports - import ckan.model as model - return self.state == model.State.DELETED + return self.state == core.State.DELETED def is_in_group(self, group): return group in self.get_group_ids() From 89a38101afe7b1415d431cdffc013e45bf90bf67 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Mon, 12 Aug 2013 17:04:00 -0300 Subject: [PATCH 05/56] [#1178] Add PENDING state to User --- ckan/lib/authenticator.py | 6 +++--- ckan/lib/base.py | 2 +- ckan/model/user.py | 11 +++++++++++ ckan/new_authz.py | 27 ++++++++++++--------------- ckan/tests/lib/test_authenticator.py | 19 +++++++++++++++++++ ckan/tests/logic/test_auth.py | 18 +++++++++++++++++- ckan/tests/models/test_user.py | 26 ++++++++++++++++++++++++++ 7 files changed, 89 insertions(+), 20 deletions(-) diff --git a/ckan/lib/authenticator.py b/ckan/lib/authenticator.py index f64db4f5b8d..cf6ed016694 100644 --- a/ckan/lib/authenticator.py +++ b/ckan/lib/authenticator.py @@ -14,7 +14,7 @@ def authenticate(self, environ, identity): if 'repoze.who.plugins.openid.userid' in identity: openid = identity['repoze.who.plugins.openid.userid'] user = User.by_openid(openid) - if user is None or user.is_deleted(): + if user is None or not user.is_active(): return None else: return user.name @@ -33,8 +33,8 @@ def authenticate(self, environ, identity): if user is None: log.debug('Login failed - username %r not found', login) - elif user.is_deleted(): - log.debug('Login as %r failed - user is deleted', login) + elif not user.is_active(): + log.debug('Login as %r failed - user isn\'t active', login) elif not user.validate_password(identity['password']): log.debug('Login as %r failed - password not valid', login) else: diff --git a/ckan/lib/base.py b/ckan/lib/base.py index bfffcaf2a5d..eabd9200b1d 100644 --- a/ckan/lib/base.py +++ b/ckan/lib/base.py @@ -290,7 +290,7 @@ def _identify_user_default(self): if c.user: c.user = c.user.decode('utf8') c.userobj = model.User.by_name(c.user) - if c.userobj is None or c.userobj.is_deleted(): + if c.userobj is None or not c.userobj.is_active(): # This occurs when a user that was still logged in is deleted, # or when you are logged in, clean db # and then restart (or when you change your username) diff --git a/ckan/model/user.py b/ckan/model/user.py index ce882ca2b8f..a6d6c44b612 100644 --- a/ckan/model/user.py +++ b/ckan/model/user.py @@ -167,9 +167,20 @@ def number_administered_packages(self): q = q.filter_by(user=self, role=model.Role.ADMIN) return q.count() + def activate(self): + ''' Activate the user ''' + self.state = core.State.ACTIVE + + def set_pending(self): + ''' Set the user as pending ''' + self.state = core.State.PENDING + def is_deleted(self): return self.state == core.State.DELETED + def is_pending(self): + return self.state == core.State.PENDING + def is_in_group(self, group): return group in self.get_group_ids() diff --git a/ckan/new_authz.py b/ckan/new_authz.py index bf1d0830d47..b98434a9b45 100644 --- a/ckan/new_authz.py +++ b/ckan/new_authz.py @@ -106,12 +106,6 @@ def is_sysadmin(username): return user and user.sysadmin -def is_deleted(username): - ''' Returns True if username is deleted ''' - user = _get_user(username) - return user and user.is_deleted() - - def _get_user(username): ''' Try to get the user from c, if possible, and fallback to using the DB ''' if not username: @@ -154,15 +148,18 @@ def is_authorized(action, context, data_dict=None): auth_function = _AuthFunctions.get(action) if auth_function: username = context.get('user') - # deleted users are always unauthorized - if is_deleted(username): - return {'success': False} - # sysadmins can do anything unless the auth_sysadmins_check - # decorator was used in which case they are treated like all other - # users. - elif is_sysadmin(username): - if not getattr(auth_function, 'auth_sysadmins_check', False): - return {'success': True} + user = _get_user(username) + + if user: + # inactive users are always unauthorized + if not user.is_active(): + return {'success': False} + # sysadmins can do anything unless the auth_sysadmins_check + # decorator was used in which case they are treated like all other + # users. + elif user.sysadmin: + if not getattr(auth_function, 'auth_sysadmins_check', False): + return {'success': True} return auth_function(context, data_dict) else: diff --git a/ckan/tests/lib/test_authenticator.py b/ckan/tests/lib/test_authenticator.py index c7c026112d2..8e7254d94c7 100644 --- a/ckan/tests/lib/test_authenticator.py +++ b/ckan/tests/lib/test_authenticator.py @@ -34,6 +34,15 @@ def test_authenticate_fails_if_user_is_deleted(self): assert self.authenticate(environ, identity) is None + def test_authenticate_fails_if_user_is_pending(self): + environ = {} + password = 'somepass' + user = CreateTestData.create_user('a_user', **{'password': password}) + identity = {'login': user.name, 'password': password} + user.set_pending() + + assert self.authenticate(environ, identity) is None + def test_authenticate_fails_if_password_is_wrong(self): environ = {} user = CreateTestData.create_user('a_user') @@ -100,6 +109,16 @@ def test_authenticate_fails_if_user_is_deleted(self): assert self.authenticate(environ, identity) is None + def test_authenticate_fails_if_user_is_deleted(self): + environ = {} + openid = 'some-openid-key' + user = CreateTestData.create_user('a_user', **{'openid': openid}) + user.set_pending() + identity = {'login': user.name, + 'repoze.who.plugins.openid.userid': openid} + + assert self.authenticate(environ, identity) is None + def test_authenticate_fails_if_user_have_no_openid(self): environ = {} identity = {} diff --git a/ckan/tests/logic/test_auth.py b/ckan/tests/logic/test_auth.py index 0f0a63372b2..2b5204e878a 100644 --- a/ckan/tests/logic/test_auth.py +++ b/ckan/tests/logic/test_auth.py @@ -64,7 +64,7 @@ def test_auth_deleted_users_are_always_unauthorized(self): # We can't reuse the username with the other tests because we can't # rebuild_db(), because in the setup_class we get the sysadmin. If we # rebuild the DB, we would delete the sysadmin as well. - username = 'other_username' + username = 'deleted_user' self.create_user(username) user = model.User.get(username) user.delete() @@ -73,6 +73,22 @@ def test_auth_deleted_users_are_always_unauthorized(self): del new_authz._AuthFunctions._functions['always_success'] + def test_auth_pending_users_are_always_unauthorized(self): + always_success = lambda x,y: {'success': True} + new_authz._AuthFunctions._build() + new_authz._AuthFunctions._functions['always_success'] = always_success + # We can't reuse the username with the other tests because we can't + # rebuild_db(), because in the setup_class we get the sysadmin. If we + # rebuild the DB, we would delete the sysadmin as well. + username = 'pending_user' + self.create_user(username) + user = model.User.get(username) + user.state = model.State.PENDING + + assert not new_authz.is_authorized_boolean('always_success', {'user': username}) + + del new_authz._AuthFunctions._functions['always_success'] + class TestAuthOrgs(TestAuth): def test_01_create_users(self): diff --git a/ckan/tests/models/test_user.py b/ckan/tests/models/test_user.py index c4b356ae854..bd34641b936 100644 --- a/ckan/tests/models/test_user.py +++ b/ckan/tests/models/test_user.py @@ -58,10 +58,36 @@ def test_4_get_openid_missing_slash(self): def test_is_deleted(self): user = CreateTestData._create_user_without_commit('a_user') + user.state = 'some-state' assert not user.is_deleted(), user user.delete() assert user.is_deleted(), user + def test_user_is_active_by_default(self): + user = CreateTestData._create_user_without_commit('a_user') + assert user.is_active(), user + + def test_activate(self): + user = CreateTestData._create_user_without_commit('a_user') + user.state = 'some-state' + assert not user.is_active(), user + user.activate() + assert user.is_active(), user + + def test_activate(self): + user = CreateTestData._create_user_without_commit('a_user') + user.state = 'some-state' + assert not user.is_active(), user + user.activate() + assert user.is_active(), user + + def test_is_pending(self): + user = CreateTestData._create_user_without_commit('a_user') + user.state = 'some-state' + assert not user.is_pending(), user + user.set_pending() + assert user.is_pending(), user + def to_names(domain_obj_list): '''Takes a list of domain objects and returns a corresponding list From c0c6803b574d48016af6185fd0a2a71605567a77 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Wed, 14 Aug 2013 18:13:36 -0300 Subject: [PATCH 06/56] [#1178] The user is activated when it performs the reset password --- ckan/controllers/user.py | 1 + ckan/tests/functional/test_user.py | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index e0b1ba6c762..4e76b098d37 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -479,6 +479,7 @@ def perform_reset(self, id): new_password = self._get_form_password() user_dict['password'] = new_password user_dict['reset_key'] = c.reset_key + user_dict['state'] = model.State.ACTIVE user = get_action('user_update')(context, user_dict) h.flash_success(_("Your password has been reset.")) diff --git a/ckan/tests/functional/test_user.py b/ckan/tests/functional/test_user.py index 16795561f46..f0ff38cfdac 100644 --- a/ckan/tests/functional/test_user.py +++ b/ckan/tests/functional/test_user.py @@ -955,3 +955,21 @@ def test_perform_reset_user_password_link_user_incorrect(self): id='randomness', # i.e. incorrect key='randomness') res = self.app.get(offset, status=404) + + def test_perform_reset_activates_pending_user(self): + password = 'password' + params = { 'password1': password, 'password2': password } + user = CreateTestData.create_user(name='username', + email='user@email.com') + user.set_pending() + create_reset_key(user) + assert user.is_pending(), user.state + + offset = url_for(controller='user', + action='perform_reset', + id=user.id, + key=user.reset_key) + res = self.app.post(offset, params=params, status=302) + + user = model.User.get(user.id) + assert user.is_active(), user From 74f649c9e3eb0690f5e48f939b4546b415b0777b Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Wed, 14 Aug 2013 21:42:22 -0300 Subject: [PATCH 07/56] [#1178] Admins can invite users The invited user starts in pending state, with the password reset key set. We still have to send an email to the user telling him/her to change the password and log in. I had to change authorization code to only automatically unauthorize deleted users, not pending. This was because the users needs to be able to perform the password reset when pending, to be able to become active. --- ckan/controllers/user.py | 2 +- ckan/logic/action/create.py | 37 ++++++++++++++++++++++ ckan/logic/auth/create.py | 2 ++ ckan/logic/schema.py | 1 + ckan/new_authz.py | 4 +-- ckan/tests/functional/test_user.py | 20 +++++++++++- ckan/tests/logic/test_action.py | 50 ++++++++++++++++++++++++++++++ ckan/tests/logic/test_auth.py | 22 ++++--------- dev-requirements.txt | 1 + 9 files changed, 119 insertions(+), 20 deletions(-) diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index 4e76b098d37..000f3db5a0c 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -447,7 +447,7 @@ def perform_reset(self, id): # FIXME We should reset the reset key when it is used to prevent # reuse of the url context = {'model': model, 'session': model.Session, - 'user': c.user, + 'user': c.user or id, 'keep_sensitive_data': True} data_dict = {'id': id} diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 1fc0fb6e058..346fd30e680 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -1,6 +1,8 @@ '''API functions for adding data to CKAN.''' import logging +import random +import re from pylons import config import paste.deploy.converters @@ -15,6 +17,7 @@ import ckan.lib.dictization.model_dictize as model_dictize import ckan.lib.dictization.model_save as model_save import ckan.lib.navl.dictization_functions +import ckan.lib.navl.validators as validators from ckan.common import _ @@ -836,6 +839,40 @@ def user_create(context, data_dict): log.debug('Created user {name}'.format(name=user.name)) return user_dict +def user_invite(context, data_dict): + '''docstring''' + _check_access('user_invite', context, data_dict) + + user_invite_schema = { + 'email': [validators.not_empty, unicode] + } + _, errors = _validate(data_dict, user_invite_schema, context) + if errors: + raise ValidationError(errors) + + while True: + try: + import ckan.lib.mailer + name = _get_random_username_from_email(data_dict['email']) + password = str(random.SystemRandom().random()) + data_dict['name'] = name + data_dict['password'] = password + data_dict['state'] = ckan.model.State.PENDING + user_dict = _get_action('user_create')(context, data_dict) + user = ckan.model.User.get(user_dict['id']) + ckan.lib.mailer.create_reset_key(user) + return model_dictize.user_dictize(user, context) + except ValidationError as e: + if 'name' not in e.error_dict: + raise e + +def _get_random_username_from_email(email): + localpart = email.split('@')[0] + cleaned_localpart = re.sub(r'[^\w]', '', localpart) + random_number = random.SystemRandom().random() * 10000 + name = '%s-%d' % (cleaned_localpart, random_number) + return name + ## Modifications for rest api def package_create_rest(context, data_dict): diff --git a/ckan/logic/auth/create.py b/ckan/logic/auth/create.py index bf9c3d17ea3..fafbc5a2e4c 100644 --- a/ckan/logic/auth/create.py +++ b/ckan/logic/auth/create.py @@ -112,6 +112,8 @@ def user_create(context, data_dict=None): else: return {'success': True} +def user_invite(context, data_dict=None): + return {'success': False} def _check_group_auth(context, data_dict): # FIXME This code is shared amoung other logic.auth files and should be diff --git a/ckan/logic/schema.py b/ckan/logic/schema.py index f8fe8df7fbd..a8cd4ce6779 100644 --- a/ckan/logic/schema.py +++ b/ckan/logic/schema.py @@ -395,6 +395,7 @@ def default_user_schema(): 'apikey': [ignore], 'reset_key': [ignore], 'activity_streams_email_notifications': [ignore_missing], + 'state': [ignore_missing], } return schema diff --git a/ckan/new_authz.py b/ckan/new_authz.py index b98434a9b45..62a4e88a978 100644 --- a/ckan/new_authz.py +++ b/ckan/new_authz.py @@ -151,8 +151,8 @@ def is_authorized(action, context, data_dict=None): user = _get_user(username) if user: - # inactive users are always unauthorized - if not user.is_active(): + # deleted users are always unauthorized + if user.is_deleted(): return {'success': False} # sysadmins can do anything unless the auth_sysadmins_check # decorator was used in which case they are treated like all other diff --git a/ckan/tests/functional/test_user.py b/ckan/tests/functional/test_user.py index f0ff38cfdac..fe30f53a505 100644 --- a/ckan/tests/functional/test_user.py +++ b/ckan/tests/functional/test_user.py @@ -959,7 +959,7 @@ def test_perform_reset_user_password_link_user_incorrect(self): def test_perform_reset_activates_pending_user(self): password = 'password' params = { 'password1': password, 'password2': password } - user = CreateTestData.create_user(name='username', + user = CreateTestData.create_user(name='pending_user', email='user@email.com') user.set_pending() create_reset_key(user) @@ -973,3 +973,21 @@ def test_perform_reset_activates_pending_user(self): user = model.User.get(user.id) assert user.is_active(), user + + def test_perform_reset_doesnt_activate_deleted_user(self): + password = 'password' + params = { 'password1': password, 'password2': password } + user = CreateTestData.create_user(name='deleted_user', + email='user@email.com') + user.delete() + create_reset_key(user) + assert user.is_deleted(), user.state + + offset = url_for(controller='user', + action='perform_reset', + id=user.id, + key=user.reset_key) + res = self.app.post(offset, params=params, status=302) + + user = model.User.get(user.id) + assert user.is_deleted(), user diff --git a/ckan/tests/logic/test_action.py b/ckan/tests/logic/test_action.py index fecbdf96b53..7bf17bd094e 100644 --- a/ckan/tests/logic/test_action.py +++ b/ckan/tests/logic/test_action.py @@ -6,6 +6,7 @@ from nose.plugins.skip import SkipTest from pylons import config import datetime +import mock import vdm.sqlalchemy import ckan @@ -561,6 +562,55 @@ def test_12_user_update_errors(self): for expected_message in test_call['messages']: assert expected_message[1] in ''.join(res_obj['error'][expected_message[0]]) + def test_user_invite(self): + email_username = 'invited_user$ckan' + email = '%s@email.com' % email_username + user_dict = {'email': email} + postparams = '%s=1' % json.dumps(user_dict) + extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} + + res = self.app.post('/api/action/user_invite', params=postparams, + extra_environ=extra_environ) + + res_obj = json.loads(res.body) + user = model.User.get(res_obj['result']['id']) + expected_username = email_username.replace('$', '') + assert res_obj['success'] is True, res_obj + assert user.email == email, (user.email, email) + assert user.name.startswith(expected_username), (user.name, expected_username) + assert user.is_pending(), user + assert user.reset_key is not None, user + + def test_user_invite_without_email_raises_error(self): + user_dict = {} + postparams = '%s=1' % json.dumps(user_dict) + extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} + + res = self.app.post('/api/action/user_invite', params=postparams, + extra_environ=extra_environ, + status=StatusCodes.STATUS_409_CONFLICT) + + res_obj = json.loads(res.body) + assert res_obj['success'] is False, res_obj + assert 'email' in res_obj['error'], res_obj + + @mock.patch('ckan.logic.action.create._get_random_username_from_email') + def test_user_invite_should_work_even_if_tried_username_already_exists(self, random_username_mock): + email = 'invited_user@email.com' + user_dict = {'email': email} + postparams = '%s=1' % json.dumps(user_dict) + extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} + + usernames = ['first', 'first', 'second'] + random_username_mock.side_effect = lambda email: usernames.pop(0) + + for _ in range(2): + res = self.app.post('/api/action/user_invite', params=postparams, + extra_environ=extra_environ) + + res_obj = json.loads(res.body) + assert res_obj['success'] is True, res_obj + def test_user_delete(self): name = 'normal_user' CreateTestData.create_user(name) diff --git a/ckan/tests/logic/test_auth.py b/ckan/tests/logic/test_auth.py index 2b5204e878a..13e7b38f2a5 100644 --- a/ckan/tests/logic/test_auth.py +++ b/ckan/tests/logic/test_auth.py @@ -49,6 +49,12 @@ def create_user(self, name): class TestAuthUsers(TestAuth): + def test_only_sysadmins_can_invite_users(self): + username = 'normal_user' + self.create_user(username) + + assert not new_authz.is_authorized_boolean('user_invite', {'user': username}) + def test_only_sysadmins_can_delete_users(self): username = 'username' user = {'id': username} @@ -73,22 +79,6 @@ def test_auth_deleted_users_are_always_unauthorized(self): del new_authz._AuthFunctions._functions['always_success'] - def test_auth_pending_users_are_always_unauthorized(self): - always_success = lambda x,y: {'success': True} - new_authz._AuthFunctions._build() - new_authz._AuthFunctions._functions['always_success'] = always_success - # We can't reuse the username with the other tests because we can't - # rebuild_db(), because in the setup_class we get the sysadmin. If we - # rebuild the DB, we would delete the sysadmin as well. - username = 'pending_user' - self.create_user(username) - user = model.User.get(username) - user.state = model.State.PENDING - - assert not new_authz.is_authorized_boolean('always_success', {'user': username}) - - del new_authz._AuthFunctions._functions['always_success'] - class TestAuthOrgs(TestAuth): def test_01_create_users(self): diff --git a/dev-requirements.txt b/dev-requirements.txt index a445712d239..a781573b0b8 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -7,3 +7,4 @@ nose==1.3.0 pep8==1.4.6 Sphinx==1.2b1 polib==1.0.3 +mock==1.0.1 From 31523cd27a35958c2f0660a2762bcfd0274c8006 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Thu, 15 Aug 2013 11:49:03 -0300 Subject: [PATCH 08/56] [#1178] perform_reset uses the received id as the context's user When performing a password reset, the user is probably (always?) not logged in. So c.user is an empty string. So, the auth functions have no way to tell which user is trying to reset his/her password. This worked fine before, because everyone was able to reset the password. But now that we've got users in DELETED state, it's not the case anymore. --- ckan/controllers/user.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index 000f3db5a0c..98033d9a0ab 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -447,17 +447,16 @@ def perform_reset(self, id): # FIXME We should reset the reset key when it is used to prevent # reuse of the url context = {'model': model, 'session': model.Session, - 'user': c.user or id, + 'user': id, 'keep_sensitive_data': True} - data_dict = {'id': id} - try: check_access('user_reset', context) except NotAuthorized: abort(401, _('Unauthorized to reset password.')) try: + data_dict = {'id': id} user_dict = get_action('user_show')(context, data_dict) # Be a little paranoid, and get rid of sensitive data that's From e5890740164c38ca0ebf10bf25a297b0b634e204 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Fri, 16 Aug 2013 10:39:25 -0300 Subject: [PATCH 09/56] [#1178] Send email to the invited user I removed the time.sleep(0.1) on TestMailer. Looking through the code, I couldn't find anywhere where a timer looked needed. And I ran these tests a hundred times without the timer to see if I could make them fail, but no. So, I guess they're not needed anymore. I also had to move the RESET_LINK_MESSAGE inside get_reset_link_body(). This was because, when importing ckan.lib.mailer in ckan.logic.action.create.py, I got: TypeError: No object (name: translator) has been registered for this thread This seems to be because we were using _() before pylons had a change to set up the translator. Moving it inside the method solves this. --- ckan/lib/mailer.py | 59 +++++++++++++++++++++++---------- ckan/logic/action/create.py | 18 +++++++--- ckan/tests/lib/test_mailer.py | 37 ++++++++++++++------- ckan/tests/logic/test_action.py | 28 ++++++++++++++-- 4 files changed, 105 insertions(+), 37 deletions(-) diff --git a/ckan/lib/mailer.py b/ckan/lib/mailer.py index f0e1978ac2c..72ebe33e2e0 100644 --- a/ckan/lib/mailer.py +++ b/ckan/lib/mailer.py @@ -100,21 +100,37 @@ def mail_user(recipient, subject, body, headers={}): mail_recipient(recipient.display_name, recipient.email, subject, body, headers=headers) +def get_reset_link_body(user): + reset_link_message = _( + '''You have requested your password on %(site_title)s to be reset. -RESET_LINK_MESSAGE = _( -'''You have requested your password on %(site_title)s to be reset. + Please click the following link to confirm this request: -Please click the following link to confirm this request: + %(reset_link)s + ''') - %(reset_link)s -''') + d = { + 'reset_link': get_reset_link(user), + 'site_title': g.site_title + } + return reset_link_message % d -def make_key(): - return uuid.uuid4().hex[:10] +def get_invite_body(user): + invite_message = _( + '''You have been invited to %(site_title)s. A user has already been created to + you with the username %(user_name)s. You can change it later. -def create_reset_key(user): - user.reset_key = unicode(make_key()) - model.repo.commit_and_remove() + To accept this invite, please reset your password at: + + %(reset_link)s + ''') + + d = { + 'reset_link': get_reset_link(user), + 'site_title': g.site_title, + 'user_name': user.name, + } + return invite_message % d def get_reset_link(user): return urljoin(g.site_url, @@ -123,17 +139,24 @@ def get_reset_link(user): id=user.id, key=user.reset_key)) -def get_reset_link_body(user): - d = { - 'reset_link': get_reset_link(user), - 'site_title': g.site_title - } - return RESET_LINK_MESSAGE % d - def send_reset_link(user): create_reset_key(user) body = get_reset_link_body(user) - mail_user(user, _('Reset your password'), body) + subject = _('Reset your password') + mail_user(user, subject, body) + +def send_invite(user): + create_reset_key(user) + body = get_invite_body(user) + subject = _('Invite for {site_title}'.format(site_title=g.site_title)) + mail_user(user, subject, body) + +def create_reset_key(user): + user.reset_key = unicode(make_key()) + model.repo.commit_and_remove() + +def make_key(): + return uuid.uuid4().hex[:10] def verify_reset_link(user, key): if not key: diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 346fd30e680..edef0a95f30 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -18,6 +18,7 @@ import ckan.lib.dictization.model_save as model_save import ckan.lib.navl.dictization_functions import ckan.lib.navl.validators as validators +import ckan.lib.mailer as mailer from ckan.common import _ @@ -840,7 +841,17 @@ def user_create(context, data_dict): return user_dict def user_invite(context, data_dict): - '''docstring''' + '''Invite a new user. + + You must be authorized to invite users. + + :param email: the email address for the new user + :type email: string + + :returns: the newly created yser + :rtype: dictionary + + ''' _check_access('user_invite', context, data_dict) user_invite_schema = { @@ -852,7 +863,6 @@ def user_invite(context, data_dict): while True: try: - import ckan.lib.mailer name = _get_random_username_from_email(data_dict['email']) password = str(random.SystemRandom().random()) data_dict['name'] = name @@ -860,7 +870,7 @@ def user_invite(context, data_dict): data_dict['state'] = ckan.model.State.PENDING user_dict = _get_action('user_create')(context, data_dict) user = ckan.model.User.get(user_dict['id']) - ckan.lib.mailer.create_reset_key(user) + mailer.send_invite(user) return model_dictize.user_dictize(user, context) except ValidationError as e: if 'name' not in e.error_dict: @@ -868,7 +878,7 @@ def user_invite(context, data_dict): def _get_random_username_from_email(email): localpart = email.split('@')[0] - cleaned_localpart = re.sub(r'[^\w]', '', localpart) + cleaned_localpart = re.sub(r'[^\w]', '-', localpart) random_number = random.SystemRandom().random() * 10000 name = '%s-%d' % (cleaned_localpart, random_number) return name diff --git a/ckan/tests/lib/test_mailer.py b/ckan/tests/lib/test_mailer.py index b95d4d290e3..22b3985930b 100644 --- a/ckan/tests/lib/test_mailer.py +++ b/ckan/tests/lib/test_mailer.py @@ -1,13 +1,12 @@ -import time from nose.tools import assert_equal, assert_raises from pylons import config from email.mime.text import MIMEText import hashlib -from ckan import model +import ckan.model as model +import ckan.lib.mailer as mailer from ckan.tests.pylons_controller import PylonsTestCase from ckan.tests.mock_mail_server import SmtpServerHarness -from ckan.lib.mailer import mail_recipient, mail_user, send_reset_link, add_msg_niceties, MailerException, get_reset_link_body, get_reset_link from ckan.lib.create_test_data import CreateTestData from ckan.lib.base import g @@ -35,7 +34,7 @@ def setup(self): def mime_encode(self, msg, recipient_name): sender_name = g.site_title sender_url = g.site_url - body = add_msg_niceties(recipient_name, msg, sender_name, sender_url) + body = mailer.add_msg_niceties(recipient_name, msg, sender_name, sender_url) encoded_body = MIMEText(body.encode('utf-8'), 'plain', 'utf-8').get_payload().strip() return encoded_body @@ -49,8 +48,7 @@ def test_mail_recipient(self): 'subject': 'Meeting', 'body': 'The meeting is cancelled.', 'headers': {'header1': 'value1'}} - mail_recipient(**test_email) - time.sleep(0.1) + mailer.mail_recipient(**test_email) # check it went to the mock smtp server msgs = self.get_smtp_messages() @@ -74,8 +72,7 @@ def test_mail_user(self): 'subject': 'Meeting', 'body': 'The meeting is cancelled.', 'headers': {'header1': 'value1'}} - mail_user(**test_email) - time.sleep(0.1) + mailer.mail_user(**test_email) # check it went to the mock smtp server msgs = self.get_smtp_messages() @@ -96,12 +93,11 @@ def test_mail_user_without_email(self): 'subject': 'Meeting', 'body': 'The meeting is cancelled.', 'headers': {'header1': 'value1'}} - assert_raises(MailerException, mail_user, **test_email) + assert_raises(mailer.MailerException, mailer.mail_user, **test_email) def test_send_reset_email(self): # send email - send_reset_link(model.User.by_name(u'bob')) - time.sleep(0.1) + mailer.send_reset_link(model.User.by_name(u'bob')) # check it went to the mock smtp server msgs = self.get_smtp_messages() @@ -110,7 +106,24 @@ def test_send_reset_email(self): assert_equal(msg[1], config['smtp.mail_from']) assert_equal(msg[2], [model.User.by_name(u'bob').email]) assert 'Reset' in msg[3], msg[3] - test_msg = get_reset_link_body(model.User.by_name(u'bob')) + test_msg = mailer.get_reset_link_body(model.User.by_name(u'bob')) + expected_body = self.mime_encode(test_msg, + u'bob') + assert expected_body in msg[3], '%r not in %r' % (expected_body, msg[3]) + + # reset link tested in user functional test + + def test_send_invite_email(self): + # send email + mailer.send_invite(model.User.by_name(u'bob')) + + # check it went to the mock smtp server + msgs = self.get_smtp_messages() + assert_equal(len(msgs), 1) + msg = msgs[0] + assert_equal(msg[1], config['smtp.mail_from']) + assert_equal(msg[2], [model.User.by_name(u'bob').email]) + test_msg = mailer.get_invite_body(model.User.by_name(u'bob')) expected_body = self.mime_encode(test_msg, u'bob') assert expected_body in msg[3], '%r not in %r' % (expected_body, msg[3]) diff --git a/ckan/tests/logic/test_action.py b/ckan/tests/logic/test_action.py index 7bf17bd094e..b6361803f0e 100644 --- a/ckan/tests/logic/test_action.py +++ b/ckan/tests/logic/test_action.py @@ -562,7 +562,8 @@ def test_12_user_update_errors(self): for expected_message in test_call['messages']: assert expected_message[1] in ''.join(res_obj['error'][expected_message[0]]) - def test_user_invite(self): + @mock.patch('ckan.lib.mailer.mail_user') + def test_user_invite(self, mail_user): email_username = 'invited_user$ckan' email = '%s@email.com' % email_username user_dict = {'email': email} @@ -574,14 +575,32 @@ def test_user_invite(self): res_obj = json.loads(res.body) user = model.User.get(res_obj['result']['id']) - expected_username = email_username.replace('$', '') + expected_username = email_username.replace('$', '-') assert res_obj['success'] is True, res_obj assert user.email == email, (user.email, email) assert user.name.startswith(expected_username), (user.name, expected_username) assert user.is_pending(), user assert user.reset_key is not None, user - def test_user_invite_without_email_raises_error(self): + @mock.patch('ckan.lib.mailer.send_invite') + def test_user_invite_sends_email(self, send_invite): + email_username = 'invited_user' + email = '%s@email.com' % email_username + user_dict = {'email': email} + postparams = '%s=1' % json.dumps(user_dict) + extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} + + res = self.app.post('/api/action/user_invite', params=postparams, + extra_environ=extra_environ) + + res_obj = json.loads(res.body) + user = model.User.get(res_obj['result']['id']) + assert res_obj['success'] is True, res_obj + assert send_invite.called + assert send_invite.call_args[0][0].id == res_obj['result']['id'] + + @mock.patch('ckan.lib.mailer.mail_user') + def test_user_invite_without_email_raises_error(self, mail_user): user_dict = {} postparams = '%s=1' % json.dumps(user_dict) extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} @@ -596,6 +615,8 @@ def test_user_invite_without_email_raises_error(self): @mock.patch('ckan.logic.action.create._get_random_username_from_email') def test_user_invite_should_work_even_if_tried_username_already_exists(self, random_username_mock): + patcher = mock.patch('ckan.lib.mailer.mail_user') + patcher.start() email = 'invited_user@email.com' user_dict = {'email': email} postparams = '%s=1' % json.dumps(user_dict) @@ -610,6 +631,7 @@ def test_user_invite_should_work_even_if_tried_username_already_exists(self, ran res_obj = json.loads(res.body) assert res_obj['success'] is True, res_obj + patcher.stop() def test_user_delete(self): name = 'normal_user' From f2f9095926e143ef7a1fe46f613c88bbf1096478 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Tue, 20 Aug 2013 19:27:49 -0300 Subject: [PATCH 10/56] [#1178] Adds user to organization when inviting, and only org admins can invite --- ckan/logic/action/create.py | 22 +++++++++++---- ckan/logic/auth/create.py | 2 +- ckan/model/user.py | 13 ++++----- ckan/tests/lib/test_mailer.py | 5 +++- ckan/tests/logic/test_action.py | 47 ++++++++++++++++----------------- ckan/tests/logic/test_auth.py | 11 ++++---- 6 files changed, 58 insertions(+), 42 deletions(-) diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index edef0a95f30..5e1e2a477e0 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -843,19 +843,25 @@ def user_create(context, data_dict): def user_invite(context, data_dict): '''Invite a new user. - You must be authorized to invite users. + You must be authorized to create organization members. - :param email: the email address for the new user + :param email: the email of the user to be invited to the organization :type email: string + :param organization_id: the id or name of the organization + :type organization_id: string + :param role: role of the user in the group. One of ``member``, ``editor``, + or ``admin`` + :type role: string :returns: the newly created yser :rtype: dictionary - ''' _check_access('user_invite', context, data_dict) user_invite_schema = { - 'email': [validators.not_empty, unicode] + 'email': [validators.not_empty, unicode], + 'organization_id': [validators.not_empty], + 'role': [validators.not_empty], } _, errors = _validate(data_dict, user_invite_schema, context) if errors: @@ -870,6 +876,12 @@ def user_invite(context, data_dict): data_dict['state'] = ckan.model.State.PENDING user_dict = _get_action('user_create')(context, data_dict) user = ckan.model.User.get(user_dict['id']) + member_dict = { + 'username': user.id, + 'id': data_dict['organization_id'], + 'role': data_dict['role'] + } + _get_action('organization_member_create')(context, member_dict) mailer.send_invite(user) return model_dictize.user_dictize(user, context) except ValidationError as e: @@ -1187,7 +1199,7 @@ def _group_or_org_member_create(context, data_dict, is_org=False): role = data_dict.get('role') group_id = data_dict.get('id') group = model.Group.get(group_id) - result = session.query(model.User).filter_by(name=username).first() + result = model.User.get(username) if result: user_id = result.id else: diff --git a/ckan/logic/auth/create.py b/ckan/logic/auth/create.py index fafbc5a2e4c..55ec2d7df6f 100644 --- a/ckan/logic/auth/create.py +++ b/ckan/logic/auth/create.py @@ -113,7 +113,7 @@ def user_create(context, data_dict=None): return {'success': True} def user_invite(context, data_dict=None): - return {'success': False} + return organization_member_create(context, data_dict) def _check_group_auth(context, data_dict): # FIXME This code is shared amoung other logic.auth files and should be diff --git a/ckan/model/user.py b/ckan/model/user.py index a6d6c44b612..4f87175edf4 100644 --- a/ckan/model/user.py +++ b/ckan/model/user.py @@ -181,20 +181,21 @@ def is_deleted(self): def is_pending(self): return self.state == core.State.PENDING - def is_in_group(self, group): - return group in self.get_group_ids() + def is_in_group(self, group_id): + return group_id in self.get_group_ids() - def is_in_groups(self, groupids): + def is_in_groups(self, group_ids): ''' Given a list of group ids, returns True if this user is in any of those groups ''' guser = set(self.get_group_ids()) - gids = set(groupids) + gids = set(group_ids) return len(guser.intersection(gids)) > 0 - def get_group_ids(self, group_type=None): + def get_group_ids(self, group_type=None, capacity=None): ''' Returns a list of group ids that the current user belongs to ''' - return [g.id for g in self.get_groups(group_type=group_type)] + return [g.id for g in + self.get_groups(group_type=group_type, capacity=capacity)] def get_groups(self, group_type=None, capacity=None): import ckan.model as model diff --git a/ckan/tests/lib/test_mailer.py b/ckan/tests/lib/test_mailer.py index 22b3985930b..83a35c3b92c 100644 --- a/ckan/tests/lib/test_mailer.py +++ b/ckan/tests/lib/test_mailer.py @@ -114,8 +114,10 @@ def test_send_reset_email(self): # reset link tested in user functional test def test_send_invite_email(self): + user = model.User.by_name(u'bob') + assert user.reset_key is None, user # send email - mailer.send_invite(model.User.by_name(u'bob')) + mailer.send_invite(user) # check it went to the mock smtp server msgs = self.get_smtp_messages() @@ -127,5 +129,6 @@ def test_send_invite_email(self): expected_body = self.mime_encode(test_msg, u'bob') assert expected_body in msg[3], '%r not in %r' % (expected_body, msg[3]) + assert user.reset_key is not None, user # reset link tested in user functional test diff --git a/ckan/tests/logic/test_action.py b/ckan/tests/logic/test_action.py index b6361803f0e..f2ef0df9f7d 100644 --- a/ckan/tests/logic/test_action.py +++ b/ckan/tests/logic/test_action.py @@ -562,12 +562,18 @@ def test_12_user_update_errors(self): for expected_message in test_call['messages']: assert expected_message[1] in ''.join(res_obj['error'][expected_message[0]]) - @mock.patch('ckan.lib.mailer.mail_user') - def test_user_invite(self, mail_user): + @mock.patch('ckan.lib.mailer.send_invite') + def test_user_invite(self, send_invite): email_username = 'invited_user$ckan' email = '%s@email.com' % email_username - user_dict = {'email': email} - postparams = '%s=1' % json.dumps(user_dict) + organization_name = 'an_organization' + CreateTestData.create_groups([{'name': organization_name}]) + role = 'member' + organization = model.Group.get(organization_name) + params = {'email': email, + 'organization_id': organization.id, + 'role': role} + postparams = '%s=1' % json.dumps(params) extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} res = self.app.post('/api/action/user_invite', params=postparams, @@ -575,27 +581,14 @@ def test_user_invite(self, mail_user): res_obj = json.loads(res.body) user = model.User.get(res_obj['result']['id']) - expected_username = email_username.replace('$', '-') assert res_obj['success'] is True, res_obj assert user.email == email, (user.email, email) - assert user.name.startswith(expected_username), (user.name, expected_username) assert user.is_pending(), user - assert user.reset_key is not None, user - - @mock.patch('ckan.lib.mailer.send_invite') - def test_user_invite_sends_email(self, send_invite): - email_username = 'invited_user' - email = '%s@email.com' % email_username - user_dict = {'email': email} - postparams = '%s=1' % json.dumps(user_dict) - extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} - - res = self.app.post('/api/action/user_invite', params=postparams, - extra_environ=extra_environ) - - res_obj = json.loads(res.body) - user = model.User.get(res_obj['result']['id']) - assert res_obj['success'] is True, res_obj + expected_username = email_username.replace('$', '-') + assert user.name.startswith(expected_username), (user.name, + expected_username) + group_ids = user.get_group_ids(capacity=role) + assert organization.id in group_ids, (group_ids, organization.id) assert send_invite.called assert send_invite.call_args[0][0].id == res_obj['result']['id'] @@ -618,8 +611,14 @@ def test_user_invite_should_work_even_if_tried_username_already_exists(self, ran patcher = mock.patch('ckan.lib.mailer.mail_user') patcher.start() email = 'invited_user@email.com' - user_dict = {'email': email} - postparams = '%s=1' % json.dumps(user_dict) + organization_name = 'an_organization' + CreateTestData.create_groups([{'name': organization_name}]) + role = 'member' + organization = model.Group.get(organization_name) + params = {'email': email, + 'organization_id': organization.id, + 'role': role} + postparams = '%s=1' % json.dumps(params) extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} usernames = ['first', 'first', 'second'] diff --git a/ckan/tests/logic/test_auth.py b/ckan/tests/logic/test_auth.py index 13e7b38f2a5..9ae69f32081 100644 --- a/ckan/tests/logic/test_auth.py +++ b/ckan/tests/logic/test_auth.py @@ -1,3 +1,5 @@ +import mock + import ckan.tests as tests from ckan.logic import get_action import ckan.model as model @@ -49,11 +51,10 @@ def create_user(self, name): class TestAuthUsers(TestAuth): - def test_only_sysadmins_can_invite_users(self): - username = 'normal_user' - self.create_user(username) - - assert not new_authz.is_authorized_boolean('user_invite', {'user': username}) + @mock.patch('ckan.logic.auth.create.organization_member_create') + def test_invite_user_delegates_to_organization_member_create(self, organization_member_create): + new_authz.is_authorized_boolean('user_invite', {}) + organization_member_create.assert_called() def test_only_sysadmins_can_delete_users(self): username = 'username' From b75524ee9b74166428555c6c5f76db7f58c67f14 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Thu, 29 Aug 2013 21:33:52 -0300 Subject: [PATCH 11/56] [#1178] Invite user on add member to organization/group I've added an "Email" field to the "Add Member" to Organization/Group page. If you add an e-mail there, we'll create a new user and invite her (doesn't matter if there's already a user with the same email). I still need to change the template and add help texts. --- ckan/controllers/group.py | 13 +++++++++ ckan/logic/action/create.py | 14 +++++----- ckan/logic/auth/create.py | 3 ++- ckan/model/user.py | 4 +++ ckan/templates/group/member_new.html | 1 + ckan/templates/organization/member_new.html | 1 + ckan/tests/functional/test_group.py | 30 +++++++++++++++++++++ ckan/tests/logic/test_action.py | 4 +-- ckan/tests/logic/test_auth.py | 13 ++++++--- 9 files changed, 69 insertions(+), 14 deletions(-) diff --git a/ckan/controllers/group.py b/ckan/controllers/group.py index ccef46bfe78..c8775168c8f 100644 --- a/ckan/controllers/group.py +++ b/ckan/controllers/group.py @@ -612,6 +612,19 @@ def member_new(self, id): data_dict = clean_dict(dict_fns.unflatten( tuplize_dict(parse_params(request.params)))) data_dict['id'] = id + + email = data_dict.get('email') + if email and email != '': + user_data_dict = { + 'email': email, + 'group_id': data_dict['id'], + 'role': data_dict['role'] + } + del data_dict['email'] + user_dict = self._action('user_invite')(context, + user_data_dict) + data_dict['username'] = user_dict['name'] + c.group_dict = self._action('group_member_create')(context, data_dict) self._redirect_to(controller='group', action='members', id=id) else: diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 5e1e2a477e0..7bc750fa10e 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -843,12 +843,12 @@ def user_create(context, data_dict): def user_invite(context, data_dict): '''Invite a new user. - You must be authorized to create organization members. + You must be authorized to create group members. - :param email: the email of the user to be invited to the organization + :param email: the email of the user to be invited to the group :type email: string - :param organization_id: the id or name of the organization - :type organization_id: string + :param group_id: the id or name of the group + :type group_id: string :param role: role of the user in the group. One of ``member``, ``editor``, or ``admin`` :type role: string @@ -860,7 +860,7 @@ def user_invite(context, data_dict): user_invite_schema = { 'email': [validators.not_empty, unicode], - 'organization_id': [validators.not_empty], + 'group_id': [validators.not_empty], 'role': [validators.not_empty], } _, errors = _validate(data_dict, user_invite_schema, context) @@ -878,10 +878,10 @@ def user_invite(context, data_dict): user = ckan.model.User.get(user_dict['id']) member_dict = { 'username': user.id, - 'id': data_dict['organization_id'], + 'id': data_dict['group_id'], 'role': data_dict['role'] } - _get_action('organization_member_create')(context, member_dict) + _get_action('group_member_create')(context, member_dict) mailer.send_invite(user) return model_dictize.user_dictize(user, context) except ValidationError as e: diff --git a/ckan/logic/auth/create.py b/ckan/logic/auth/create.py index 55ec2d7df6f..25e9f18988c 100644 --- a/ckan/logic/auth/create.py +++ b/ckan/logic/auth/create.py @@ -113,7 +113,8 @@ def user_create(context, data_dict=None): return {'success': True} def user_invite(context, data_dict=None): - return organization_member_create(context, data_dict) + context['id'] = context.get('group_id') + return group_member_create(context, data_dict) def _check_group_auth(context, data_dict): # FIXME This code is shared amoung other logic.auth files and should be diff --git a/ckan/model/user.py b/ckan/model/user.py index 4f87175edf4..da92a57c11a 100644 --- a/ckan/model/user.py +++ b/ckan/model/user.py @@ -44,6 +44,10 @@ def by_openid(cls, openid): obj = meta.Session.query(cls).autoflush(False) return obj.filter_by(openid=openid).first() + @classmethod + def by_email(cls, email): + return meta.Session.query(cls).filter_by(email=email).all() + @classmethod def get(cls, user_reference): # double slashes in an openid often get turned into single slashes diff --git a/ckan/templates/group/member_new.html b/ckan/templates/group/member_new.html index 87c6dbb60c2..2385a1957b9 100644 --- a/ckan/templates/group/member_new.html +++ b/ckan/templates/group/member_new.html @@ -19,6 +19,7 @@

{% set format_attrs = {'data-module': 'autocomplete', 'data-module-source': '/api/2/util/user/autocomplete?q=?'} %} {{ form.input('username', id='field-username', label=_('User'), placeholder=_('Username'), value='', error='', classes=['control-medium'], attrs=format_attrs) }} {% endif %} + {{ form.input('email', label=_('Email'), value=user.email, classes=['control-medium']) }} {% set format_attrs = {'data-module': 'autocomplete'} %} {{ form.select('role', label=_('Role'), options=c.roles, selected=c.user_role, error='', attrs=format_attrs) }}
diff --git a/ckan/templates/organization/member_new.html b/ckan/templates/organization/member_new.html index dd2c4e50bfa..74487100199 100644 --- a/ckan/templates/organization/member_new.html +++ b/ckan/templates/organization/member_new.html @@ -21,6 +21,7 @@

{% set format_attrs = {'data-module': 'autocomplete', 'data-module-source': '/api/2/util/user/autocomplete?q=?'} %} {{ form.input('username', id='field-username', label=_('User'), placeholder=_('Username'), value='', error='', classes=['control-medium'], attrs=format_attrs) }} {% endif %} + {{ form.input('email', label=_('Email'), value=user.email, classes=['control-medium']) }} {% set format_attrs = {'data-module': 'autocomplete'} %} {{ form.select('role', label=_('Role'), options=c.roles, selected=c.user_role, error='', attrs=format_attrs) }}
diff --git a/ckan/tests/functional/test_group.py b/ckan/tests/functional/test_group.py index b07bbe7ea6f..c5dc63b1097 100644 --- a/ckan/tests/functional/test_group.py +++ b/ckan/tests/functional/test_group.py @@ -1,6 +1,7 @@ import re from nose.tools import assert_equal +import mock import ckan.tests.test_plugins as test_plugins import ckan.model as model @@ -660,3 +661,32 @@ def test_2_atom_feed(self): assert '' in res, res + +class TestMemberInvite(FunctionalTestCase): + @classmethod + def setup_class(self): + model.Session.remove() + model.repo.rebuild_db() + + def teardown(self): + model.repo.rebuild_db() + + @mock.patch('ckan.lib.mailer.mail_user') + def test_member_new_invites_user_if_received_email(self, mail_user): + user = CreateTestData.create_user('a_user', sysadmin=True) + group_name = 'a_group' + CreateTestData.create_groups([{'name': group_name}], user.name) + group = model.Group.get(group_name) + url = url_for(controller='group', action='member_new', id=group.id) + email = 'invited_user@mailinator.com' + role = 'member' + + params = {'email': email, 'role': role} + res = self.app.post(url, params, + extra_environ={'REMOTE_USER': str(user.name)}) + + users = model.User.by_email(email) + assert len(users) == 1, users + user = users[0] + assert user.email == email, user + assert group.id in user.get_group_ids(capacity=role) diff --git a/ckan/tests/logic/test_action.py b/ckan/tests/logic/test_action.py index f2ef0df9f7d..8c1d3e0586d 100644 --- a/ckan/tests/logic/test_action.py +++ b/ckan/tests/logic/test_action.py @@ -571,7 +571,7 @@ def test_user_invite(self, send_invite): role = 'member' organization = model.Group.get(organization_name) params = {'email': email, - 'organization_id': organization.id, + 'group_id': organization.id, 'role': role} postparams = '%s=1' % json.dumps(params) extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} @@ -616,7 +616,7 @@ def test_user_invite_should_work_even_if_tried_username_already_exists(self, ran role = 'member' organization = model.Group.get(organization_name) params = {'email': email, - 'organization_id': organization.id, + 'group_id': organization.id, 'role': role} postparams = '%s=1' % json.dumps(params) extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} diff --git a/ckan/tests/logic/test_auth.py b/ckan/tests/logic/test_auth.py index 9ae69f32081..ff8a18ccdf3 100644 --- a/ckan/tests/logic/test_auth.py +++ b/ckan/tests/logic/test_auth.py @@ -51,10 +51,15 @@ def create_user(self, name): class TestAuthUsers(TestAuth): - @mock.patch('ckan.logic.auth.create.organization_member_create') - def test_invite_user_delegates_to_organization_member_create(self, organization_member_create): - new_authz.is_authorized_boolean('user_invite', {}) - organization_member_create.assert_called() + @mock.patch('ckan.logic.auth.create.group_member_create') + def test_invite_user_prepares_context_and_delegates_to_group_member_create(self, group_member_create): + context = {'group_id': 42} + group_member_create_context = context + group_member_create_context['id'] = context['group_id'] + + new_authz.is_authorized_boolean('user_invite', context) + + group_member_create.assert_called(group_member_create_context, None) def test_only_sysadmins_can_delete_users(self): username = 'username' From 89db2ed23ca5cb0136a44d2829f2e713f2b2558d Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Sat, 31 Aug 2013 11:26:31 -0300 Subject: [PATCH 12/56] [#1178] Move migration from 070 to 071, as we have a new 070 --- ...umn_to_user_table.py => 071_add_state_column_to_user_table.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename ckan/migration/versions/{070_add_state_column_to_user_table.py => 071_add_state_column_to_user_table.py} (100%) diff --git a/ckan/migration/versions/070_add_state_column_to_user_table.py b/ckan/migration/versions/071_add_state_column_to_user_table.py similarity index 100% rename from ckan/migration/versions/070_add_state_column_to_user_table.py rename to ckan/migration/versions/071_add_state_column_to_user_table.py From 87d0b8b520e1f4860bec04b2a6e5edd67a40b33e Mon Sep 17 00:00:00 2001 From: joetsoi Date: Fri, 6 Sep 2013 14:20:23 +0100 Subject: [PATCH 13/56] [#1228] strip whitespace from title in model_dictize.package_dictize --- ckan/lib/dictization/model_dictize.py | 3 +++ ckan/tests/lib/test_dictization.py | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/ckan/lib/dictization/model_dictize.py b/ckan/lib/dictization/model_dictize.py index 793f0733c95..c55908fad25 100644 --- a/ckan/lib/dictization/model_dictize.py +++ b/ckan/lib/dictization/model_dictize.py @@ -209,6 +209,9 @@ def package_dictize(pkg, context): if not result: raise logic.NotFound result_dict = d.table_dictize(result, context) + #strip whitespace from title + if result_dict.get('title'): + result_dict['title'] = result_dict['title'].strip() #resources res_rev = model.resource_revision_table resource_group = model.resource_group_table diff --git a/ckan/tests/lib/test_dictization.py b/ckan/tests/lib/test_dictization.py index fb8bb49e628..a2ea317f18a 100644 --- a/ckan/tests/lib/test_dictization.py +++ b/ckan/tests/lib/test_dictization.py @@ -1222,3 +1222,21 @@ def test_25_user_dictize_as_anonymous(self): # Passwords should never be available assert 'password' not in user_dict + + def test_26_package_dictize_whitespace_strippped_from_title(self): + + context = {"model": model, + "session": model.Session} + + pkg = model.Session.query(model.Package).first() + original_title = pkg.title + pkg.title = " whitespace title \t" + model.Session.add(pkg) + model.Session.commit() + + result = package_dictize(pkg, context) + assert result['title'] == 'whitespace title' + pkg.title = original_title + model.Session.add(pkg) + model.Session.commit() + From 40397fe5c9bd0e2703f0d4e26227863c6cbc6b4a Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Mon, 9 Sep 2013 16:10:10 -0300 Subject: [PATCH 14/56] [#1178] Add help text to the add member forms --- ckan/public/base/less/forms.less | 30 ++++++++++++++ ckan/templates/group/member_new.html | 46 ++++++++++++++++----- ckan/templates/organization/member_new.html | 46 ++++++++++++++++----- 3 files changed, 102 insertions(+), 20 deletions(-) diff --git a/ckan/public/base/less/forms.less b/ckan/public/base/less/forms.less index 289439c291a..a641129f705 100644 --- a/ckan/public/base/less/forms.less +++ b/ckan/public/base/less/forms.less @@ -670,3 +670,33 @@ textarea { .box-shadow(none); } } + +.add-member-form .control-label { + width: 100%; + text-align: left; +} + +.add-member-form .controls { + margin-left: auto; +} + +.add-member-or { + float: left; + line-height: 100px; + margin-left: 10px; + margin-right: 10px; + text-transform: uppercase; + color: grey; + font-weight: bold; +} + +.add-member-form .row-fluid .control-group { + float: left; + width: 45%; +} + +.add-member-form .row-fluid { + .select2-container, input { + width: 100% !important; + } +} diff --git a/ckan/templates/group/member_new.html b/ckan/templates/group/member_new.html index 2385a1957b9..efa3235461a 100644 --- a/ckan/templates/group/member_new.html +++ b/ckan/templates/group/member_new.html @@ -10,16 +10,42 @@

{% block page_heading %}{{ _('Edit Member') if user else _('Add Member') }}{% endblock %}

{% block form %} -
- {% if user %} - - {% set format_attrs = {'disabled': true} %} - {{ form.input('username', label=_('User'), value=user.name, classes=['control-medium'], attrs=format_attrs) }} - {% else %} - {% set format_attrs = {'data-module': 'autocomplete', 'data-module-source': '/api/2/util/user/autocomplete?q=?'} %} - {{ form.input('username', id='field-username', label=_('User'), placeholder=_('Username'), value='', error='', classes=['control-medium'], attrs=format_attrs) }} - {% endif %} - {{ form.input('email', label=_('Email'), value=user.email, classes=['control-medium']) }} + +
+
+ + + {{ _('If you wish to add an existing user, search for their username below.') }} + +
+ {% if user %} + + + {% else %} + + {% endif %} +
+
+
+ {{ _('or') }} +
+
+ + + {{ _('If you wish to invite a new user, enter their email address.') }} + +
+ +
+
+
{% set format_attrs = {'data-module': 'autocomplete'} %} {{ form.select('role', label=_('Role'), options=c.roles, selected=c.user_role, error='', attrs=format_attrs) }}
diff --git a/ckan/templates/organization/member_new.html b/ckan/templates/organization/member_new.html index 74487100199..b76de7663fa 100644 --- a/ckan/templates/organization/member_new.html +++ b/ckan/templates/organization/member_new.html @@ -12,16 +12,42 @@

{% block page_heading %}{{ _('Edit Member') if user else _('Add Member') }}{% endblock %}

{% block form %} - - {% if user %} - - {% set format_attrs = {'disabled': true} %} - {{ form.input('username', label=_('User'), value=user.name, classes=['control-medium'], attrs=format_attrs) }} - {% else %} - {% set format_attrs = {'data-module': 'autocomplete', 'data-module-source': '/api/2/util/user/autocomplete?q=?'} %} - {{ form.input('username', id='field-username', label=_('User'), placeholder=_('Username'), value='', error='', classes=['control-medium'], attrs=format_attrs) }} - {% endif %} - {{ form.input('email', label=_('Email'), value=user.email, classes=['control-medium']) }} + +
+
+ + + {{ _('If you wish to add an existing user, search for her username below.') }} + +
+ {% if user %} + + + {% else %} + + {% endif %} +
+
+
+ {{ _('or') }} +
+
+ + + {{ _('If you wish to invite a new user, enter her email address.') }} + +
+ +
+
+
{% set format_attrs = {'data-module': 'autocomplete'} %} {{ form.select('role', label=_('Role'), options=c.roles, selected=c.user_role, error='', attrs=format_attrs) }}
From 2adda64767ea178e558ccd6844b84efe267a6dcf Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Mon, 9 Sep 2013 17:00:45 -0300 Subject: [PATCH 15/56] [#1178] Fix typo exeecute -> execute in migration --- ckan/migration/versions/071_add_state_column_to_user_table.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/migration/versions/071_add_state_column_to_user_table.py b/ckan/migration/versions/071_add_state_column_to_user_table.py index 367e3a48c32..791a046bad2 100644 --- a/ckan/migration/versions/071_add_state_column_to_user_table.py +++ b/ckan/migration/versions/071_add_state_column_to_user_table.py @@ -10,7 +10,7 @@ def upgrade(migrate_engine): def downgrade(migrate_engine): - migrate_engine.exeecute( + migrate_engine.execute( ''' ALTER TABLE "user" DROP COLUMN "state" ''' From 6327e2454ae5a98df9e71ecd27725aa0ea9f2e2d Mon Sep 17 00:00:00 2001 From: Ian Ward Date: Mon, 9 Sep 2013 16:34:07 -0400 Subject: [PATCH 16/56] [#772] explaion plugin loading order --- doc/configuration.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/configuration.rst b/doc/configuration.rst index 91b73781058..19b7f338ca7 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -508,6 +508,10 @@ Specify which CKAN plugins are to be enabled. Format as a space-separated list of the plugin names. The plugin name is the key in the ``[ckan.plugins]`` section of the extension's ``setup.py``. For more information on plugins and extensions, see :doc:`extensions/index`. +The order that plugin names are specified influences the order that plugins are loaded. Plugins in separate python modules are loaded in exactly the order specified, but plugins in the same module will always be loaded in the order their classes appear in the module. + +Plugin load order is important for plugins that add templates directories: Templates found in template directories added earlier will override templates in template directories added later. + .. _ckan.datastore.enabled: ckan.datastore.enabled From b0f3e643b2640c2a717d56f802a5d61477fcf7f3 Mon Sep 17 00:00:00 2001 From: amercader Date: Wed, 11 Sep 2013 12:32:26 +0100 Subject: [PATCH 17/56] [#1237] Improve facet.limit search option A negative value should be allowed if you don't want to limit the facet values, but the search params schema didn't allow it. Also mention in the docs that this can be set via configuration and that the default is 50. Added some tests. --- ckan/logic/action/get.py | 5 ++-- ckan/logic/schema.py | 2 +- ckan/tests/logic/test_action.py | 42 +++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index ab50329c608..a810cfa3cbe 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -1289,8 +1289,9 @@ def package_search(context, data_dict): :param facet.mincount: the minimum counts for facet fields should be included in the results. :type facet.mincount: int - :param facet.limit: the maximum number of constraint counts that should be - returned for the facet fields. A negative value means unlimited + :param facet.limit: the maximum number of values the facet fields. A + negative value means unlimited. This can be set instance-wide with + the :ref:`search.facets.limit` config option. Default is 50. :type facet.limit: int :param facet.field: the fields to facet upon. Default empty. If empty, then the returned facet information is empty. diff --git a/ckan/logic/schema.py b/ckan/logic/schema.py index f8fe8df7fbd..75d49808234 100644 --- a/ckan/logic/schema.py +++ b/ckan/logic/schema.py @@ -539,7 +539,7 @@ def default_package_search_schema(): 'qf': [ignore_missing, unicode], 'facet': [ignore_missing, unicode], 'facet.mincount': [ignore_missing, natural_number_validator], - 'facet.limit': [ignore_missing, natural_number_validator], + 'facet.limit': [ignore_missing, int_validator], 'facet.field': [ignore_missing, list_of_strings], 'extras': [ignore_missing] # Not used by Solr, but useful for extensions } diff --git a/ckan/tests/logic/test_action.py b/ckan/tests/logic/test_action.py index 89e030beac8..8b4fe3dca14 100644 --- a/ckan/tests/logic/test_action.py +++ b/ckan/tests/logic/test_action.py @@ -1377,6 +1377,48 @@ def test_1_basic(self): assert_equal(result['count'], 1) assert_equal(result['results'][0]['name'], 'annakarenina') + def test_1_facet_limit(self): + params = { + 'q':'*:*', + 'facet.field': ['groups', 'tags', 'res_format', 'license'], + 'rows': 20, + 'start': 0, + } + postparams = '%s=1' % json.dumps(params) + res = self.app.post('/api/action/package_search', params=postparams) + res = json.loads(res.body) + assert_equal(res['success'], True) + + assert_equal(len(res['result']['search_facets']['groups']['items']), 2) + + params = { + 'q':'*:*', + 'facet.field': ['groups', 'tags', 'res_format', 'license'], + 'facet.limit': 1, + 'rows': 20, + 'start': 0, + } + postparams = '%s=1' % json.dumps(params) + res = self.app.post('/api/action/package_search', params=postparams) + res = json.loads(res.body) + assert_equal(res['success'], True) + + assert_equal(len(res['result']['search_facets']['groups']['items']), 1) + + params = { + 'q':'*:*', + 'facet.field': ['groups', 'tags', 'res_format', 'license'], + 'facet.limit': -1, # No limit + 'rows': 20, + 'start': 0, + } + postparams = '%s=1' % json.dumps(params) + res = self.app.post('/api/action/package_search', params=postparams) + res = json.loads(res.body) + assert_equal(res['success'], True) + + assert_equal(len(res['result']['search_facets']['groups']['items']), 2) + def test_1_basic_no_params(self): postparams = '%s=1' % json.dumps({}) res = self.app.post('/api/action/package_search', params=postparams) From 2d76140db869595b83d2d501380bbe9f248192f5 Mon Sep 17 00:00:00 2001 From: John Martin Date: Wed, 11 Sep 2013 16:58:41 +0100 Subject: [PATCH 18/56] [#1178] Small CSS tweaks and fix for premature closing div --- ckan/public/base/less/forms.less | 8 ++++---- ckan/templates/organization/member_new.html | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/ckan/public/base/less/forms.less b/ckan/public/base/less/forms.less index a641129f705..1263abdd219 100644 --- a/ckan/public/base/less/forms.less +++ b/ckan/public/base/less/forms.less @@ -682,11 +682,11 @@ textarea { .add-member-or { float: left; - line-height: 100px; - margin-left: 10px; - margin-right: 10px; + margin-top: 75px; + width: 7%; + text-align: center; text-transform: uppercase; - color: grey; + color: @grayLight; font-weight: bold; } diff --git a/ckan/templates/organization/member_new.html b/ckan/templates/organization/member_new.html index b76de7663fa..352cb9f3790 100644 --- a/ckan/templates/organization/member_new.html +++ b/ckan/templates/organization/member_new.html @@ -65,7 +65,6 @@

{% endblock %} -
{% endblock %} {% block secondary_content %} From f03e409b6e8eed6633cf530ff8cf08b882d1b726 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Thu, 12 Sep 2013 17:44:30 -0300 Subject: [PATCH 19/56] [#848] Fix configuration name search.facet.limits -> search.facet.limit --- doc/configuration.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/configuration.rst b/doc/configuration.rst index 91b73781058..beaf9a69956 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -450,14 +450,14 @@ Default value: ``false`` Controls whether the default search page (``/dataset``) should show only standard datasets or also custom dataset types. -.. _search.facets.limits: +.. _search.facets.limit: -search.facets.limits -^^^^^^^^^^^^^^^^^^^^ +search.facets.limit +^^^^^^^^^^^^^^^^^^^ Example:: - search.facets.limits = 100 + search.facets.limit = 100 Default value: ``50`` From 43afc32641e9d09293ebb5276f1098598770ff3a Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Thu, 12 Sep 2013 17:46:46 -0300 Subject: [PATCH 20/56] [#848] extra_template_paths and extra_public_paths aren't deprecated --- doc/configuration.rst | 4 ---- 1 file changed, 4 deletions(-) diff --git a/doc/configuration.rst b/doc/configuration.rst index beaf9a69956..a0af8ec5769 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -914,8 +914,6 @@ To customise the display of CKAN you can supply replacements for the Genshi temp For more information on theming, see :doc:`theming`. -.. note:: This is only for legacy code, and shouldn't be used anymore. - .. _extra_public_paths: extra_public_paths @@ -929,8 +927,6 @@ To customise the display of CKAN you can supply replacements for static files su For more information on theming, see :doc:`theming`. -.. note:: This is only for legacy code, and shouldn't be used anymore. - .. end_config-theming Storage Settings From fca56dceb35bae9889b2b017af0a8b38afddec15 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Thu, 12 Sep 2013 17:48:57 -0300 Subject: [PATCH 21/56] [#848] ckan.template_footer_end is deprecated --- doc/configuration.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/configuration.rst b/doc/configuration.rst index a0af8ec5769..7c2aef44f3d 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -888,6 +888,8 @@ Example (showing insertion of Google Analytics code):: +.. note:: This is only for legacy code, and shouldn't be used anymore. + .. _ckan.template_title_deliminater: ckan.template_title_deliminater From 2994a3c3232a8134746337ff528ff613c1844565 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Thu, 12 Sep 2013 18:35:26 -0300 Subject: [PATCH 22/56] [#848] Reorganize and add examples of FileStore's configuration parameters --- doc/configuration.rst | 54 +++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/doc/configuration.rst b/doc/configuration.rst index 7c2aef44f3d..55301872056 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -945,20 +945,7 @@ Example:: Default value: ``None`` -This setting will change the bucket name for the uploaded files. - -.. _ckan.storage.key_prefix: - -ckan.storage.key_prefix -^^^^^^^^^^^^^^^^^^^^^^^ - -Example:: - - ckan.storage.key_prefix = ckan-file/ - -Default value: ``file/`` - -This setting will change the prefix for the uploaded files. This is only for ``pairtree``. +This changes the bucket name for the uploaded files. .. _ckan.storage.max_content_length: @@ -999,6 +986,19 @@ Default value: ``None`` Only used with the local storage backend. Use this to specify where uploaded files should be stored, and also to turn on the handling of file storage. The folder should exist, and will automatically be turned into a valid pairtree repository if it is not already. +.. _ckan.storage.key_prefix: + +ckan.storage.key_prefix +^^^^^^^^^^^^^^^^^^^^^^^ + +Example:: + + ckan.storage.key_prefix = ckan-file/ + +Default value: ``file/`` + +Only used with the local storage backend. This changes the prefix for the uploaded files. + .. _ofs.aws_access_key_id: ofs.aws_access_key_id @@ -1006,13 +1006,11 @@ ofs.aws_access_key_id Example:: - ofs.aws_access_key_id = your_key_id_here + ofs.aws_access_key_id = 022QF06E7MXBSH9DHM02 Default value: ``None`` -Only used with the Amazon S3 storage backend. - -.. todo:: Expand +Only used with the Amazon S3 storage backend. Configure with your AWS Access Key ID. .. _ofs.aws_secret_access_key: @@ -1021,13 +1019,11 @@ ofs.aws_secret_access_key Example:: - ofs.aws_secret_access_key = your_secret_access_key_here + ofs.aws_secret_access_key = kWcrlUX5JEDGM/LtmEENI/aVmYvHNif5zB+d9+ct Default value: ``None`` -Only used with the Amazon S3 storage backend. - -.. todo:: Expand +Only used with the Amazon S3 storage backend. Configure with your AWS Secret Access Key. .. _ofs.gs_access_key_id: @@ -1036,13 +1032,12 @@ ofs.gs_access_key_id Example:: - ofs.gs_access_key_id = your_key_id_here + ofs.gs_access_key_id = GOOGTS7C7FUP3AIRVJTE Default value: ``None`` -Only used with the Google storage backend. - -.. todo:: Expand +Only used with the Google storage backend. Configure with your Google Storage +Access Key ID. .. _ofs.gs_secret_access_key: @@ -1051,13 +1046,12 @@ ofs.gs_secret_access_key Example:: - ofs.gs_secret_access_key = your_secret_access_key_here + ofs.gs_secret_access_key = bGoa+V7g/yqDXvKRqq+JTFn4uQZbPiQJo4pf9RzJ Default value: ``None`` -Only used with the Google storage backend. - -.. todo:: Expand +Only used with the Google storage backend. Configure with your Google Storage +Secret Access Key. DataPusher Settings From e7db320c5f514048f18e892e3158bfe0c4c83c1f Mon Sep 17 00:00:00 2001 From: Guy Sheffer Date: Wed, 25 Sep 2013 13:02:44 +0300 Subject: [PATCH 23/56] Add notes and changes to new language submission, since pull requests are not accepted anymore --- doc/i18n.rst | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/doc/i18n.rst b/doc/i18n.rst index 9d90ce18b23..2718298b925 100644 --- a/doc/i18n.rst +++ b/doc/i18n.rst @@ -27,8 +27,9 @@ Adding a New Language If you want to add an entirely new language to CKAN, you have two options. * :ref:`i18n-transifex`. Creating translation files using Transifex, the open source translation software. -* :ref:`i18n-manual`. Creating translation files manually. +* :ref:`i18n-manual`. Creating translation files manually in your own branch. +.. note:: Translating CKAN in Transifex is only enabled when a 'call for translations' is issued. .. _i18n-transifex: @@ -64,8 +65,6 @@ The Transifex workflow is as follows: Manual Setup ------------ -If you prefer not to use Transifex, you can create translation files manually. - .. note:: Please keep the CKAN core developers aware of new languages created in this way. All the English strings in CKAN are extracted into the ``ckan.pot`` file, which can be found in ``ckan/i18n``. @@ -106,13 +105,13 @@ We recommend using a translation tool, such as `poedit ` 3. Commit the Translation ++++++++++++++++++++++++++ -When the po is complete, commit it to the CKAN i18n repo:: - +When the po is complete, create a branch in your source, then commit it to the CKAN i18n repo:: + + git checkout master + git branch translation-YOUR_LANGUAGE git add ckan/i18n/YOUR_LANGUAGE/LC_MESSAGES/ckan.po git commit -m '[i18n]: New language po added: YOUR_LANGUAGE' ckan/i18n/YOUR_LANGUAGE/LC_MESSAGES/ckan.po - git push - -.. note:: You need to be given credentials to do this - to request these, contact the `ckan-discuss `_ list. + git push origin translation-YOUR_LANGUAGE 4. Compile a Translation ++++++++++++++++++++++++ From c8a483a77f82c4c8a545c569e87cd7843a3d92dc Mon Sep 17 00:00:00 2001 From: kindly Date: Mon, 30 Sep 2013 11:58:35 +0100 Subject: [PATCH 24/56] [#1221] use write url for search as this uses correct priviledges --- ckanext/datastore/logic/action.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ckanext/datastore/logic/action.py b/ckanext/datastore/logic/action.py index 47a464518ce..d9d291b9df3 100644 --- a/ckanext/datastore/logic/action.py +++ b/ckanext/datastore/logic/action.py @@ -300,9 +300,7 @@ def datastore_search(context, data_dict): raise p.toolkit.ValidationError(errors) res_id = data_dict['resource_id'] - data_dict['connection_url'] = pylons.config.get( - 'ckan.datastore.read_url', - pylons.config['ckan.datastore.write_url']) + data_dict['connection_url'] = pylons.config['ckan.datastore.write_url'] resources_sql = sqlalchemy.text(u'''SELECT alias_of FROM "_table_metadata" WHERE name = :id''') From c339162836695677f02e9090f2ce40f6a65ad194 Mon Sep 17 00:00:00 2001 From: John Glover Date: Wed, 2 Oct 2013 16:59:33 +0200 Subject: [PATCH 25/56] [#1178] PEP8 (and correct a comment) --- ckan/logic/action/create.py | 3 +++ ckan/logic/action/delete.py | 1 + ckan/logic/auth/delete.py | 4 +++- ckan/model/follower.py | 13 +++++++------ 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 371ca62e625..066ed7bcc87 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -841,6 +841,7 @@ def user_create(context, data_dict): log.debug('Created user {name}'.format(name=user.name)) return user_dict + def user_invite(context, data_dict): '''Invite a new user. @@ -889,6 +890,7 @@ def user_invite(context, data_dict): if 'name' not in e.error_dict: raise e + def _get_random_username_from_email(email): localpart = email.split('@')[0] cleaned_localpart = re.sub(r'[^\w]', '-', localpart) @@ -896,6 +898,7 @@ def _get_random_username_from_email(email): name = '%s-%d' % (cleaned_localpart, random_number) return name + ## Modifications for rest api def package_create_rest(context, data_dict): diff --git a/ckan/logic/action/delete.py b/ckan/logic/action/delete.py index 9191151aa01..210f9d72387 100644 --- a/ckan/logic/action/delete.py +++ b/ckan/logic/action/delete.py @@ -18,6 +18,7 @@ _get_or_bust = ckan.logic.get_or_bust _get_action = ckan.logic.get_action + def user_delete(context, data_dict): '''Delete a user. diff --git a/ckan/logic/auth/delete.py b/ckan/logic/auth/delete.py index a2b121c11ad..08974144a71 100644 --- a/ckan/logic/auth/delete.py +++ b/ckan/logic/auth/delete.py @@ -4,10 +4,12 @@ from ckan.logic.auth import get_resource_object from ckan.lib.base import _ + def user_delete(context, data_dict): - # Only sysadmins are authorized to purge organizations. + # sysadmins only return {'success': False} + def package_delete(context, data_dict): user = context['user'] package = get_package_object(context, data_dict) diff --git a/ckan/model/follower.py b/ckan/model/follower.py index a8afd07706f..eb54cd9ca67 100644 --- a/ckan/model/follower.py +++ b/ckan/model/follower.py @@ -1,7 +1,6 @@ import meta import datetime import sqlalchemy -import vdm.sqlalchemy import core import ckan.model @@ -78,15 +77,17 @@ def _get(cls, follower_id=None, object_id=None): object_id = object_id or cls.object_id query = meta.Session.query(cls, follower_alias, object_alias)\ - .filter(sqlalchemy.and_(follower_alias.id == follower_id,\ - cls.follower_id == follower_alias.id,\ - cls.object_id == object_alias.id,\ - follower_alias.state != core.State.DELETED,\ - object_alias.state != core.State.DELETED,\ + .filter(sqlalchemy.and_( + follower_alias.id == follower_id, + cls.follower_id == follower_alias.id, + cls.object_id == object_alias.id, + follower_alias.state != core.State.DELETED, + object_alias.state != core.State.DELETED, object_alias.id == object_id)) return query + class UserFollowingUser(ModelFollowingModel): '''A many-many relationship between users. From 2e0266f12003244bc32795e82a9825574b93bf72 Mon Sep 17 00:00:00 2001 From: John Martin Date: Wed, 2 Oct 2013 16:04:32 +0100 Subject: [PATCH 26/56] [#1070] Makes recaptcha work with the jinja2 templates --- ckan/lib/app_globals.py | 2 +- ckan/public/base/less/forms.less | 5 +++++ ckan/templates/user/new_user_form.html | 5 +++++ ckan/templates/user/snippets/recaptcha.html | 12 ++++++++++++ 4 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 ckan/templates/user/snippets/recaptcha.html diff --git a/ckan/lib/app_globals.py b/ckan/lib/app_globals.py index 0739274c671..1b95264ad1e 100644 --- a/ckan/lib/app_globals.py +++ b/ckan/lib/app_globals.py @@ -38,7 +38,7 @@ # has been setup in load_environment(): 'ckan.site_id': {}, 'ckan.recaptcha.publickey': {'name': 'recaptcha_publickey'}, - 'ckan.recaptcha.privatekey': {'name': 'recaptcha_publickey'}, + 'ckan.recaptcha.privatekey': {'name': 'recaptcha_privatekey'}, 'ckan.template_title_deliminater': {'default': '-'}, 'ckan.template_head_end': {}, 'ckan.template_footer_end': {}, diff --git a/ckan/public/base/less/forms.less b/ckan/public/base/less/forms.less index 289439c291a..c9de5810ee4 100644 --- a/ckan/public/base/less/forms.less +++ b/ckan/public/base/less/forms.less @@ -670,3 +670,8 @@ textarea { .box-shadow(none); } } + +#recaptcha_table { + table-layout: inherit; + line-height: 1; +} diff --git a/ckan/templates/user/new_user_form.html b/ckan/templates/user/new_user_form.html index 3879f2dde6c..57a0f1d85e0 100644 --- a/ckan/templates/user/new_user_form.html +++ b/ckan/templates/user/new_user_form.html @@ -7,6 +7,11 @@ {{ form.input("email", id="field-email", label=_("Email"), type="email", placeholder="joe@example.com", value=data.email, error=errors.email, classes=["control-medium"]) }} {{ form.input("password1", id="field-password", label=_("Password"), type="password", placeholder="••••••••", value=data.password1, error=errors.password1, classes=["control-medium"]) }} {{ form.input("password2", id="field-confirm-password", label=_("Confirm"), type="password", placeholder="••••••••", value=data.password2, error=errors.password1, classes=["control-medium"]) }} + + {% if g.recaptcha_publickey %} + {% snippet "user/snippets/recaptcha.html", public_key=g.recaptcha_publickey %} + {% endif %} +
diff --git a/ckan/templates/user/snippets/recaptcha.html b/ckan/templates/user/snippets/recaptcha.html new file mode 100644 index 00000000000..994867dc97f --- /dev/null +++ b/ckan/templates/user/snippets/recaptcha.html @@ -0,0 +1,12 @@ +
+
+ + + + +
+
From afc80ed59d6173a96980576fc4760702413ac8dc Mon Sep 17 00:00:00 2001 From: John Martin Date: Thu, 3 Oct 2013 10:49:20 +0100 Subject: [PATCH 27/56] [#1259] Fixes a JS error with autocomplete module and improves UX 1. Basically adds an extra check to make sure that 'abort()' is defined 2. Then changes the module to behave a little nicer when inbetween ajax requests --- .../base/javascript/modules/autocomplete.js | 41 ++++++++++--------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/ckan/public/base/javascript/modules/autocomplete.js b/ckan/public/base/javascript/modules/autocomplete.js index 768a708c9c3..adea1628f49 100644 --- a/ckan/public/base/javascript/modules/autocomplete.js +++ b/ckan/public/base/javascript/modules/autocomplete.js @@ -86,6 +86,8 @@ this.ckan.module('autocomplete', function (jQuery, _) { $('.select2-choice', select2.container).on('click', function() { return false; }); + + this._select2 = select2; }, /* Looks up the completions for the current search term and passes them @@ -140,29 +142,30 @@ this.ckan.module('autocomplete', function (jQuery, _) { // old data. this._lastTerm = string; + // Kills previous timeout + clearTimeout(this._debounced); + + // OK, wipe the dropdown before we start ajaxing the completions + fn({results:[]}); + if (string) { - if (!this._debounced) { - // Set a timer to prevent the search lookup occurring too often. - this._debounced = setTimeout(function () { - var term = module._lastTerm; + // Set a timer to prevent the search lookup occurring too often. + this._debounced = setTimeout(function () { + var term = module._lastTerm; - delete module._debounced; + // Cancel the previous request if it hasn't yet completed. + if (module._last && typeof module._last.abort == 'function') { + module._last.abort(); + } - // Cancel the previous request if it hasn't yet completed. - if (module._last) { - module._last.abort(); - } + module._last = module.getCompletions(term, function (terms) { + fn(terms); + }); + }, this.options.interval); - module._last = module.getCompletions(term, function (terms) { - fn(module._lastResults = terms); - }); - }, this.options.interval); - } else { - // Re-use the last set of terms. - fn(this._lastResults || {results: []}); - } - } else { - fn({results: []}); + // This forces the ajax throbber to appear, because we've called the + // callback already and that hides the throbber + $('.select2-search input', this._select2.dropdown).addClass('select2-active'); } }, From 70bff16b4408e614933cfaf42fdbf4d2a774ad94 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Mon, 7 Oct 2013 15:43:45 +0200 Subject: [PATCH 28/56] [#772] Clarify plugin-loading order and best practice --- doc/configuration.rst | 27 +++++++++++++++++++++++++-- doc/extensions/best-practices.rst | 21 ++++++++++++++++----- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/doc/configuration.rst b/doc/configuration.rst index 8d5e3e56946..2a6e3aa592e 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -508,9 +508,32 @@ Specify which CKAN plugins are to be enabled. Format as a space-separated list of the plugin names. The plugin name is the key in the ``[ckan.plugins]`` section of the extension's ``setup.py``. For more information on plugins and extensions, see :doc:`extensions/index`. -The order that plugin names are specified influences the order that plugins are loaded. Plugins in separate python modules are loaded in exactly the order specified, but plugins in the same module will always be loaded in the order their classes appear in the module. +.. note:: -Plugin load order is important for plugins that add templates directories: Templates found in template directories added earlier will override templates in template directories added later. + The order of the plugin names in the configuration file influences the + order that CKAN will load the plugins in. As long as each plugin class is + implemented in a separate Python module (i.e. in a separate Python source + code file), the plugins will be loaded in the order given in the + configuration file. + + When multiple plugins are implemented in the same Python module, CKAN will + process the plugins in the order that they're given in the config file, but as + soon as it reaches one plugin from a given Python module, CKAN will load all + plugins from that Python module, in the order that the plugin classes are + defined in the module. + + For simplicity, we recommend implementing each plugin class in its own Python + module. + + Plugin loading order can be important, for example for plugins that add custom + template files: templates found in template directories added earlier will + override templates in template directories added later. + + .. todo:: + + Fix CKAN's plugin loading order to simply load all plugins in the order + they're given in the config file, regardless of which Python modules + they're implemented in. .. _ckan.datastore.enabled: diff --git a/doc/extensions/best-practices.rst b/doc/extensions/best-practices.rst index 8d95fe7ba49..ecde15a8152 100644 --- a/doc/extensions/best-practices.rst +++ b/doc/extensions/best-practices.rst @@ -1,14 +1,18 @@ -------------------------------------- +===================================== Best practices for writing extensions -------------------------------------- +===================================== + +------------------------------ Follow CKAN's coding standards -============================== +------------------------------ See :ref:`coding standards`. + +------------------------------------------------- Use the plugins toolkit instead of importing CKAN -================================================= +------------------------------------------------- Try to limit your extension to interacting with CKAN only through CKAN's :doc:`plugin interfaces ` and @@ -17,10 +21,17 @@ extension code separate from CKAN as much as possible, so that internal changes in CKAN from one release to the next don't break your extension. +--------------------------------- Don't edit CKAN's database tables -================================= +--------------------------------- An extension can create its own tables in the CKAN database, but it should *not* write to core CKAN tables directly, add columns to core tables, or use foreign keys against core tables. + +------------------------------------------------------- +Implement each plugin class in a separate Python module +------------------------------------------------------- + +This keeps CKAN's plugin loading order simple, see :ref:`ckan.plugins`. From 4b2c46b59bd6cb5ed1235b60e1555d776c5456b2 Mon Sep 17 00:00:00 2001 From: amercader Date: Mon, 7 Oct 2013 17:46:18 +0100 Subject: [PATCH 29/56] [#1265] Keep visibility value on form errors The wrong value was checked to choose the selected option. --- ckan/templates/package/snippets/package_basic_fields.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ckan/templates/package/snippets/package_basic_fields.html b/ckan/templates/package/snippets/package_basic_fields.html index 044b9ebe5bc..d1ac7e25577 100644 --- a/ckan/templates/package/snippets/package_basic_fields.html +++ b/ckan/templates/package/snippets/package_basic_fields.html @@ -86,8 +86,8 @@
From dad667eb4233b00561167db2aff41092afecfd2d Mon Sep 17 00:00:00 2001 From: Dominik Moritz Date: Mon, 7 Oct 2013 13:35:25 -0700 Subject: [PATCH 30/56] Correct encoding to make coveralls work --- ckan/config/environment.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/config/environment.py b/ckan/config/environment.py index 49be9b726a6..409f77435a4 100644 --- a/ckan/config/environment.py +++ b/ckan/config/environment.py @@ -159,7 +159,7 @@ def find_controller(self, controller): ''' This code is based on Genshi code - Copyright © 2006-2012 Edgewall Software + Copyright © 2006-2012 Edgewall Software All rights reserved. Redistribution and use in source and binary forms, with or From a903ef999d8d67db48118ea8413595707c2e3080 Mon Sep 17 00:00:00 2001 From: Dominik Moritz Date: Mon, 7 Oct 2013 14:20:57 -0700 Subject: [PATCH 31/56] Add test coverage banner to readme --- README.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.rst b/README.rst index 93d80e668cc..2eafd662e07 100644 --- a/README.rst +++ b/README.rst @@ -5,6 +5,10 @@ CKAN: The Open Source Data Portal Software :target: http://travis-ci.org/okfn/ckan :alt: Build Status +.. image:: https://coveralls.io/repos/okfn/ckan/badge.png?branch=coveralls + :target: https://coveralls.io/r/okfn/ckan?branch=coveralls + :alt: Test coverage + **CKAN is the world’s leading open-source data portal platform**. CKAN makes it easy to publish, share and work with data. It's a data management system that provides a powerful platform for cataloging, storing and accessing From 5ad2c9bc8d651c31b302499780b6926b7a798ebc Mon Sep 17 00:00:00 2001 From: Dominik Moritz Date: Mon, 7 Oct 2013 14:25:57 -0700 Subject: [PATCH 32/56] Don't cover tests files --- .coveragerc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.coveragerc b/.coveragerc index 28f34e39273..ed2c3b6bc6f 100644 --- a/.coveragerc +++ b/.coveragerc @@ -1,3 +1,3 @@ [run] -omit = /ckan/migration/* +omit = /ckan/migration/*, /ckan/tests/*, */tests/* source = ckan, ckanext From 1ab6ff449faef790a4f160d30aedd08a82b726fb Mon Sep 17 00:00:00 2001 From: amercader Date: Tue, 8 Oct 2013 13:06:11 +0100 Subject: [PATCH 33/56] [#1268] Add 'python setup.py develop' step to source upgrade instructions --- doc/upgrade-source.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/doc/upgrade-source.rst b/doc/upgrade-source.rst index af70ac4ea05..539b96dcb62 100644 --- a/doc/upgrade-source.rst +++ b/doc/upgrade-source.rst @@ -37,6 +37,12 @@ CKAN release you're upgrading to: pip install --upgrade -r requirements.txt +#. Register any new or updated plugins: + + :: + + python setup.py develop + #. If you are upgrading to a new :ref:`major release ` you need to update your Solr schema symlink. From 485f681f6f5d5fe890dc339d57a8563844e2fde2 Mon Sep 17 00:00:00 2001 From: joetsoi Date: Wed, 9 Oct 2013 14:02:39 +0100 Subject: [PATCH 34/56] [#1272] prevent keyerror when looking up 'url_type' in datastore before_show --- ckanext/datastore/plugin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckanext/datastore/plugin.py b/ckanext/datastore/plugin.py index 4477ad70526..04f201f9eb0 100644 --- a/ckanext/datastore/plugin.py +++ b/ckanext/datastore/plugin.py @@ -285,7 +285,7 @@ def before_show(self, resource_dict): ''' Modify the resource url of datastore resources so that they link to the datastore dumps. ''' - if resource_dict['url_type'] == 'datastore': + if resource_dict.get('url_type') == 'datastore': resource_dict['url'] = p.toolkit.url_for( controller='ckanext.datastore.controller:DatastoreController', action='dump', resource_id=resource_dict['id']) From 16924d937af4207a585fd34eaf2be083bcc2d6d2 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Fri, 11 Oct 2013 20:23:41 -0300 Subject: [PATCH 35/56] [#1178] Remove backward migrations, as we don't support it --- .../versions/071_add_state_column_to_user_table.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/ckan/migration/versions/071_add_state_column_to_user_table.py b/ckan/migration/versions/071_add_state_column_to_user_table.py index 791a046bad2..c4f3f5013aa 100644 --- a/ckan/migration/versions/071_add_state_column_to_user_table.py +++ b/ckan/migration/versions/071_add_state_column_to_user_table.py @@ -7,11 +7,3 @@ def upgrade(migrate_engine): ALTER TABLE "user" ADD COLUMN "state" text NOT NULL DEFAULT '%s' ''' % ckan.model.core.State.ACTIVE ) - - -def downgrade(migrate_engine): - migrate_engine.execute( - ''' - ALTER TABLE "user" DROP COLUMN "state" - ''' - ) From 0bd163e75eacc65fee0546fa356f195f1e109713 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Fri, 11 Oct 2013 20:40:46 -0300 Subject: [PATCH 36/56] [#1178] Fix repeated test name --- ckan/tests/lib/test_authenticator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/tests/lib/test_authenticator.py b/ckan/tests/lib/test_authenticator.py index 8e7254d94c7..8b2c6139f0f 100644 --- a/ckan/tests/lib/test_authenticator.py +++ b/ckan/tests/lib/test_authenticator.py @@ -109,7 +109,7 @@ def test_authenticate_fails_if_user_is_deleted(self): assert self.authenticate(environ, identity) is None - def test_authenticate_fails_if_user_is_deleted(self): + def test_authenticate_fails_if_user_is_pending(self): environ = {} openid = 'some-openid-key' user = CreateTestData.create_user('a_user', **{'openid': openid}) From d073ce7f6b56361cf8c5452c2f969e4d45356437 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Fri, 11 Oct 2013 20:54:24 -0300 Subject: [PATCH 37/56] [#1178] Use the data returned by validate, instead of the one passed as param --- ckan/logic/action/create.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 066ed7bcc87..f4ab9fa4ba1 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -865,23 +865,23 @@ def user_invite(context, data_dict): 'group_id': [validators.not_empty], 'role': [validators.not_empty], } - _, errors = _validate(data_dict, user_invite_schema, context) + data, errors = _validate(data_dict, user_invite_schema, context) if errors: raise ValidationError(errors) while True: try: - name = _get_random_username_from_email(data_dict['email']) + name = _get_random_username_from_email(data['email']) password = str(random.SystemRandom().random()) - data_dict['name'] = name - data_dict['password'] = password - data_dict['state'] = ckan.model.State.PENDING - user_dict = _get_action('user_create')(context, data_dict) + data['name'] = name + data['password'] = password + data['state'] = ckan.model.State.PENDING + user_dict = _get_action('user_create')(context, data) user = ckan.model.User.get(user_dict['id']) member_dict = { 'username': user.id, - 'id': data_dict['group_id'], - 'role': data_dict['role'] + 'id': data['group_id'], + 'role': data['role'] } _get_action('group_member_create')(context, member_dict) mailer.send_invite(user) From 1da817c40b59d7dfa866abd9e68ed844442f34d4 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Fri, 11 Oct 2013 20:58:11 -0300 Subject: [PATCH 38/56] [#1178] Move user_invite_schema to ckan.logic.schema --- ckan/logic/action/create.py | 8 ++------ ckan/logic/schema.py | 8 ++++++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index f4ab9fa4ba1..e4f13b3baad 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -860,12 +860,8 @@ def user_invite(context, data_dict): ''' _check_access('user_invite', context, data_dict) - user_invite_schema = { - 'email': [validators.not_empty, unicode], - 'group_id': [validators.not_empty], - 'role': [validators.not_empty], - } - data, errors = _validate(data_dict, user_invite_schema, context) + schema = context.get('schema') or ckan.logic.schema.default_user_invite_schema() + data, errors = _validate(data_dict, schema, context) if errors: raise ValidationError(errors) diff --git a/ckan/logic/schema.py b/ckan/logic/schema.py index a8cd4ce6779..60f36411c7b 100644 --- a/ckan/logic/schema.py +++ b/ckan/logic/schema.py @@ -424,6 +424,14 @@ def default_update_user_schema(): return schema +def default_user_invite_schema(): + schema = { + 'email': [not_empty, unicode], + 'group_id': [not_empty], + 'role': [not_empty], + } + return schema + def default_task_status_schema(): schema = { 'id': [ignore], From c2e4f767eed6485383a7ef2efd40a8e990d0a623 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Fri, 11 Oct 2013 22:59:53 -0300 Subject: [PATCH 39/56] [#1178] Refactor user name creation I've isolated the loop where it needs to be. This makes the code clearer, but makes it susceptible to errors. For example, after the random name was generated but before the user is created, another user with the same name might be created, breaking our code. This should be rare enough to bother us. --- ckan/logic/action/create.py | 43 +++++++++++++++------------------ ckan/tests/logic/test_action.py | 7 +++--- 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index e4f13b3baad..9dca2940773 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -865,34 +865,31 @@ def user_invite(context, data_dict): if errors: raise ValidationError(errors) - while True: - try: - name = _get_random_username_from_email(data['email']) - password = str(random.SystemRandom().random()) - data['name'] = name - data['password'] = password - data['state'] = ckan.model.State.PENDING - user_dict = _get_action('user_create')(context, data) - user = ckan.model.User.get(user_dict['id']) - member_dict = { - 'username': user.id, - 'id': data['group_id'], - 'role': data['role'] - } - _get_action('group_member_create')(context, member_dict) - mailer.send_invite(user) - return model_dictize.user_dictize(user, context) - except ValidationError as e: - if 'name' not in e.error_dict: - raise e + name = _get_random_username_from_email(data['email']) + password = str(random.SystemRandom().random()) + data['name'] = name + data['password'] = password + data['state'] = ckan.model.State.PENDING + user_dict = _get_action('user_create')(context, data) + user = ckan.model.User.get(user_dict['id']) + member_dict = { + 'username': user.id, + 'id': data['group_id'], + 'role': data['role'] + } + _get_action('group_member_create')(context, member_dict) + mailer.send_invite(user) + return model_dictize.user_dictize(user, context) def _get_random_username_from_email(email): localpart = email.split('@')[0] cleaned_localpart = re.sub(r'[^\w]', '-', localpart) - random_number = random.SystemRandom().random() * 10000 - name = '%s-%d' % (cleaned_localpart, random_number) - return name + while True: + random_number = random.SystemRandom().random() * 10000 + name = '%s-%d' % (cleaned_localpart, random_number) + if not ckan.model.User.get(name): + return name ## Modifications for rest api diff --git a/ckan/tests/logic/test_action.py b/ckan/tests/logic/test_action.py index 1fb96e83ca1..d5a043c0b9a 100644 --- a/ckan/tests/logic/test_action.py +++ b/ckan/tests/logic/test_action.py @@ -613,8 +613,8 @@ def test_user_invite_without_email_raises_error(self, mail_user): assert res_obj['success'] is False, res_obj assert 'email' in res_obj['error'], res_obj - @mock.patch('ckan.logic.action.create._get_random_username_from_email') - def test_user_invite_should_work_even_if_tried_username_already_exists(self, random_username_mock): + @mock.patch('random.SystemRandom') + def test_user_invite_should_work_even_if_tried_username_already_exists(self, system_random_mock): patcher = mock.patch('ckan.lib.mailer.mail_user') patcher.start() email = 'invited_user@email.com' @@ -628,8 +628,7 @@ def test_user_invite_should_work_even_if_tried_username_already_exists(self, ran postparams = '%s=1' % json.dumps(params) extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} - usernames = ['first', 'first', 'second'] - random_username_mock.side_effect = lambda email: usernames.pop(0) + system_random_mock.return_value.random.side_effect = [1000, 1000, 2000, 3000] for _ in range(2): res = self.app.post('/api/action/user_invite', params=postparams, From dfd8ae9fb07ff551707bdc08c6e23bd424c7335c Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Fri, 11 Oct 2013 23:07:25 -0300 Subject: [PATCH 40/56] [#1178] Simplify "if" expression, removing redundancy --- ckan/controllers/group.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/controllers/group.py b/ckan/controllers/group.py index c8775168c8f..66bd533fd43 100644 --- a/ckan/controllers/group.py +++ b/ckan/controllers/group.py @@ -614,7 +614,7 @@ def member_new(self, id): data_dict['id'] = id email = data_dict.get('email') - if email and email != '': + if email: user_data_dict = { 'email': email, 'group_id': data_dict['id'], From bae4d47c3227bd5f7581ac5859edcb9badd027c0 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Sat, 12 Oct 2013 16:52:19 -0300 Subject: [PATCH 41/56] [#1178] Use model.State instead of core.State --- ckan/logic/action/get.py | 6 +++--- .../versions/071_add_state_column_to_user_table.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index 57c62a2916c..0621f55cf9a 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -15,7 +15,7 @@ import ckan.logic.schema import ckan.lib.dictization.model_dictize as model_dictize import ckan.lib.navl.dictization_functions -import ckan.model.core as core +import ckan.model as model import ckan.model.misc as misc import ckan.plugins as plugins import ckan.lib.search as search @@ -690,7 +690,7 @@ def user_list(context, data_dict): ) # Filter deleted users - query = query.filter(model.User.state != core.State.DELETED) + query = query.filter(model.User.state != model.State.DELETED) ## hack for pagination if context.get('return_query'): @@ -1246,7 +1246,7 @@ def user_autocomplete(context, data_dict): limit = data_dict.get('limit', 20) query = model.User.search(q) - query = query.filter(model.User.state != core.State.DELETED) + query = query.filter(model.User.state != model.State.DELETED) query = query.limit(limit) user_list = [] diff --git a/ckan/migration/versions/071_add_state_column_to_user_table.py b/ckan/migration/versions/071_add_state_column_to_user_table.py index c4f3f5013aa..828ebfb5f44 100644 --- a/ckan/migration/versions/071_add_state_column_to_user_table.py +++ b/ckan/migration/versions/071_add_state_column_to_user_table.py @@ -1,9 +1,9 @@ -import ckan.model.core +import ckan.model def upgrade(migrate_engine): migrate_engine.execute( ''' ALTER TABLE "user" ADD COLUMN "state" text NOT NULL DEFAULT '%s' - ''' % ckan.model.core.State.ACTIVE + ''' % ckan.model.State.ACTIVE ) From 3218b581443ad07cd09a93d364bf5085fb0f7884 Mon Sep 17 00:00:00 2001 From: amercader Date: Mon, 14 Oct 2013 17:31:04 +0100 Subject: [PATCH 42/56] [#1237] Fix docstring --- ckan/logic/action/get.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index a810cfa3cbe..384026a64e8 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -1289,8 +1289,8 @@ def package_search(context, data_dict): :param facet.mincount: the minimum counts for facet fields should be included in the results. :type facet.mincount: int - :param facet.limit: the maximum number of values the facet fields. A - negative value means unlimited. This can be set instance-wide with + :param facet.limit: the maximum number of values the facet fields return. + A negative value means unlimited. This can be set instance-wide with the :ref:`search.facets.limit` config option. Default is 50. :type facet.limit: int :param facet.field: the fields to facet upon. Default empty. If empty, From 6431ba3dc6a117926101a97ae859132a906cf0f2 Mon Sep 17 00:00:00 2001 From: John Glover Date: Tue, 15 Oct 2013 16:00:30 +0200 Subject: [PATCH 43/56] [#1178] Add auth_user_obj back to context in User controller methods. --- ckan/controllers/user.py | 48 ++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index 98033d9a0ab..1c6fefff344 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -38,7 +38,8 @@ class UserController(base.BaseController): def __before__(self, action, **env): base.BaseController.__before__(self, action, **env) try: - context = {'model': model, 'user': c.user or c.author} + context = {'model': model, 'user': c.user or c.author, + 'auth_user_obj': c.userobj} check_access('site_read', context) except NotAuthorized: if c.action not in ('login', 'request_reset', 'perform_reset',): @@ -89,7 +90,8 @@ def index(self): c.q = request.params.get('q', '') c.order_by = request.params.get('order_by', 'name') - context = {'return_query': True} + context = {'return_query': True, 'user': c.user or c.author, + 'auth_user_obj': c.userobj} data_dict = {'q': c.q, 'order_by': c.order_by} @@ -111,7 +113,8 @@ def index(self): def read(self, id=None): context = {'model': model, 'session': model.Session, - 'user': c.user or c.author, 'for_view': True} + 'user': c.user or c.author, 'auth_user_obj': c.userobj, + 'for_view': True} data_dict = {'id': id, 'user_obj': c.userobj} try: @@ -140,7 +143,8 @@ def me(self, locale=None): id=user_ref) def register(self, data=None, errors=None, error_summary=None): - context = {'model': model, 'session': model.Session, 'user': c.user} + context = {'model': model, 'session': model.Session, 'user': c.user, + 'auth_user_obj': c.userobj} try: check_access('user_create', context) except NotAuthorized: @@ -154,6 +158,7 @@ def new(self, data=None, errors=None, error_summary=None): ''' context = {'model': model, 'session': model.Session, 'user': c.user or c.author, + 'auth_user_obj': c.userobj, 'schema': self._new_form_to_db_schema(), 'save': 'save' in request.params} @@ -182,7 +187,8 @@ def delete(self, id): '''Delete user with id passed as parameter''' context = {'model': model, 'session': model.Session, - 'user': c.user} + 'user': c.user, + 'auth_user_obj': c.userobj} data_dict = {'id': id} try: @@ -234,7 +240,7 @@ def edit(self, id=None, data=None, errors=None, error_summary=None): context = {'save': 'save' in request.params, 'schema': self._edit_form_to_db_schema(), 'model': model, 'session': model.Session, - 'user': c.user, + 'user': c.user, 'auth_user_obj': c.userobj } if id is None: if c.userobj: @@ -391,7 +397,8 @@ def logged_out_page(self): return render('user/logout.html') def request_reset(self): - context = {'model': model, 'session': model.Session, 'user': c.user} + context = {'model': model, 'session': model.Session, 'user': c.user, + 'auth_user_obj': c.userobj} data_dict = {'id': request.params.get('user')} try: check_access('request_reset', context) @@ -511,7 +518,8 @@ def _get_form_password(self): raise ValueError(_('You must provide a password')) def followers(self, id=None): - context = {'for_view': True} + context = {'for_view': True, 'user': c.user or c.author, + 'auth_user_obj': c.userobj} data_dict = {'id': id, 'user_obj': c.userobj} self._setup_template_variables(context, data_dict) f = get_action('user_follower_list') @@ -525,7 +533,8 @@ def activity(self, id, offset=0): '''Render this user's public activity stream page.''' context = {'model': model, 'session': model.Session, - 'user': c.user or c.author, 'for_view': True} + 'user': c.user or c.author, 'auth_user_obj': c.userobj, + 'for_view': True} data_dict = {'id': id, 'user_obj': c.userobj} try: check_access('user_show', context, data_dict) @@ -553,7 +562,8 @@ def display_name(followee): if (filter_type and filter_id): context = { 'model': model, 'session': model.Session, - 'user': c.user or c.author, 'for_view': True + 'user': c.user or c.author, 'auth_user_obj': c.userobj, + 'for_view': True } data_dict = {'id': filter_id} followee = None @@ -595,7 +605,8 @@ def display_name(followee): def dashboard(self, id=None, offset=0): context = {'model': model, 'session': model.Session, - 'user': c.user or c.author, 'for_view': True} + 'user': c.user or c.author, 'auth_user_obj': c.userobj, + 'for_view': True} data_dict = {'id': id, 'user_obj': c.userobj, 'offset': offset} self._setup_template_variables(context, data_dict) @@ -618,19 +629,22 @@ def dashboard(self, id=None, offset=0): return render('user/dashboard.html') def dashboard_datasets(self): - context = {'for_view': True} + context = {'for_view': True, 'user': c.user or c.author, + 'auth_user_obj': c.userobj} data_dict = {'user_obj': c.userobj} self._setup_template_variables(context, data_dict) return render('user/dashboard_datasets.html') def dashboard_organizations(self): - context = {'for_view': True} + context = {'for_view': True, 'user': c.user or c.author, + 'auth_user_obj': c.userobj} data_dict = {'user_obj': c.userobj} self._setup_template_variables(context, data_dict) return render('user/dashboard_organizations.html') def dashboard_groups(self): - context = {'for_view': True} + context = {'for_view': True, 'user': c.user or c.author, + 'auth_user_obj': c.userobj} data_dict = {'user_obj': c.userobj} self._setup_template_variables(context, data_dict) return render('user/dashboard_groups.html') @@ -639,7 +653,8 @@ def follow(self, id): '''Start following this user.''' context = {'model': model, 'session': model.Session, - 'user': c.user or c.author} + 'user': c.user or c.author, + 'auth_user_obj': c.userobj} data_dict = {'id': id} try: get_action('follow_user')(context, data_dict) @@ -658,7 +673,8 @@ def unfollow(self, id): '''Stop following this user.''' context = {'model': model, 'session': model.Session, - 'user': c.user or c.author} + 'user': c.user or c.author, + 'auth_user_obj': c.userobj} data_dict = {'id': id} try: get_action('unfollow_user')(context, data_dict) From 4ac411b61cfae7048cdc6cc5cace42aec20a13e9 Mon Sep 17 00:00:00 2001 From: John Glover Date: Tue, 15 Oct 2013 16:28:12 +0200 Subject: [PATCH 44/56] [#1178] Fix typo in org member_new template --- ckan/templates/organization/member_new.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ckan/templates/organization/member_new.html b/ckan/templates/organization/member_new.html index 352cb9f3790..a121549dab4 100644 --- a/ckan/templates/organization/member_new.html +++ b/ckan/templates/organization/member_new.html @@ -19,7 +19,7 @@

{{ _('Existing User') }} - {{ _('If you wish to add an existing user, search for her username below.') }} + {{ _('If you wish to add an existing user, search for their username below.') }}
{% if user %} @@ -41,7 +41,7 @@

{{ _('New User') }} - {{ _('If you wish to invite a new user, enter her email address.') }} + {{ _('If you wish to invite a new user, enter their email address.') }}
From 322149c6934a574589f37ffe8317c444ad0885ff Mon Sep 17 00:00:00 2001 From: John Glover Date: Tue, 15 Oct 2013 16:51:07 +0200 Subject: [PATCH 45/56] [#1178] Constrain number of attempts to generate a random user name in user_invite --- ckan/logic/action/create.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 9dca2940773..693845398d0 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -860,7 +860,8 @@ def user_invite(context, data_dict): ''' _check_access('user_invite', context, data_dict) - schema = context.get('schema') or ckan.logic.schema.default_user_invite_schema() + schema = context.get('schema', + ckan.logic.schema.default_user_invite_schema()) data, errors = _validate(data_dict, schema, context) if errors: raise ValidationError(errors) @@ -885,7 +886,12 @@ def user_invite(context, data_dict): def _get_random_username_from_email(email): localpart = email.split('@')[0] cleaned_localpart = re.sub(r'[^\w]', '-', localpart) - while True: + + # if we can't create a unique user name within this many attempts + # then something else is probably wrong and we should give up + max_name_creation_attempts = 100 + + for i in range(max_name_creation_attempts): random_number = random.SystemRandom().random() * 10000 name = '%s-%d' % (cleaned_localpart, random_number) if not ckan.model.User.get(name): From a480ace5ae73532627ad9b04cc3416f855373fc6 Mon Sep 17 00:00:00 2001 From: John Glover Date: Tue, 15 Oct 2013 17:09:29 +0200 Subject: [PATCH 46/56] [#1178] handle edge case where _get_random_username_from_email fails by returning the cleaned local part of the email address. This is expected to fail validation in user_create. --- ckan/logic/action/create.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 693845398d0..c7fb98be30a 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -897,6 +897,8 @@ def _get_random_username_from_email(email): if not ckan.model.User.get(name): return name + return cleaned_localpart + ## Modifications for rest api From 971026bf286aee490e8080bab715870d65b84cf7 Mon Sep 17 00:00:00 2001 From: John Martin Date: Tue, 15 Oct 2013 16:22:49 +0100 Subject: [PATCH 47/56] [#1263] Remove the 'clear all' from the faceter on the left nav --- ckan/public/base/less/iehacks.less | 5 ----- ckan/public/base/less/module.less | 11 ----------- ckan/templates/development/snippets/facet.html | 2 +- ckan/templates/development/snippets/module.html | 2 +- ckan/templates/snippets/facet_list.html | 1 - 5 files changed, 2 insertions(+), 19 deletions(-) diff --git a/ckan/public/base/less/iehacks.less b/ckan/public/base/less/iehacks.less index 232ee72ee9c..7d9bc5cc274 100644 --- a/ckan/public/base/less/iehacks.less +++ b/ckan/public/base/less/iehacks.less @@ -200,11 +200,6 @@ .media-content { position: relative; } - .action { - position: absolute; - top: 9px; - right: 10px; - } .media-image img { float: left; } diff --git a/ckan/public/base/less/module.less b/ckan/public/base/less/module.less index a575abcbda4..aa964cf5ea3 100644 --- a/ckan/public/base/less/module.less +++ b/ckan/public/base/less/module.less @@ -13,17 +13,6 @@ border-bottom: 1px solid @moduleHeadingBorderColor; } -.module-heading .action { - float: right; - color: @moduleHeadingActionTextColor; - font-size: 12px; - line-height: @baseLineHeight; - text-decoration: underline; - &:hover { - color: @layoutTextColor; - } -} - .module-content { padding: 0 @gutterX; margin: 20px 0; diff --git a/ckan/templates/development/snippets/facet.html b/ckan/templates/development/snippets/facet.html index ac9b9df1a0c..7280bf9a3d2 100644 --- a/ckan/templates/development/snippets/facet.html +++ b/ckan/templates/development/snippets/facet.html @@ -1,6 +1,6 @@
{% with items=(("First", true), ("Second", false), ("Third", true), ("Fourth", false), ("Last", false)) %} -

Facet List Clear All

+

Facet List