Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Option to lockdown user registration #399

Closed
wants to merge 40 commits into from

6 participants

@seanh
Owner

Originally based off #356

JoshData and others added some commits
@JoshData JoshData add ckan.auth.create_user option (default true) which when set to fal…
…se prevents all new user registrations (via site and API), which is useful in a locked-down publisher mode
6d15f09
@seanh seanh Merge branch 'master' of github.com:okfn/ckan into patch-create_user_…
…option
7631f58
@seanh seanh [#356] Tidy up some long lines
This makes the logic much easier to see I think
ffa4db3
@seanh seanh was assigned
@seanh
Owner

@tauberer Thanks for this, it looks good. I think it needs a couple more things:

  1. The Register button still appears in the web UI when registration is disabled, it should be hidden.
  2. There should be a couple of simple tests to test that users can and cannot register when this option is on and off.

If you can do these and just push them to your patch-create_user_option branch on your fork and then ping me, I'll pull them into this branch. You probably want to pull my extra commits on this branch into your branch first. It's a bit confusing with all the branches, sorry.

@JoshData

Just flagging you changed the logic, in case you didn't intend it.

I think people will expect that create_user=False implies create_user_via_api is also False, just because of the naming of the options. On line 106, I'd do:

if using_api and create_user_via_api and create_user:

@seanh seanh [#356] Tweak auth.create_user logic
If ckan.auth.create_user = False then don't let anyone create users,
either via the API or via the web interface.
4c47e1c
doc/organizations_and_groups.rst
@@ -230,6 +230,7 @@ The following config options have been created.
ckan.auth.user_create_organizations
ckan.auth.user_create_groups
+ckan.auth.create_user
@tobes
tobes added a note

should this be create_user_via_web to differentiate it from create_user_via_api

In my patch, create_user meant "create user by any means". The logic got changed in this pull req, so one or the other should change.

@tobes
tobes added a note

ok this also affects my above comment :)

@seanh Owner
seanh added a note

I'll change the logic back, just running the tests now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ckan/logic/auth/create.py
((13 lines not shown))
+ if using_api and create_user_via_api:
+ return {'success': True}
+ elif (not using_api) and create_user:
+ return {'success': True}
+ else:
+ return {'success': False, 'msg': _('User {user} not authorized to '
+ 'create users').format(user=user)}
@tobes
tobes added a note

I'd prefer this to be clearer eg the error would be user cannot create user via api etc as the message may say user cannot create user when they can but only via api or web interface - that feels more honest.

It would not make the code much more complicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@seanh seanh [#356] Better error messages for create user auth
If not allowed to create users via api then say 'not authorized to
create user via api', if not allowed to create users at all then say
'not authorized to create users'.
59db288
@seanh
Owner

Alright I think that addresses both points! But I haven't tested it, gotta run off, will come back to this tomorrow morning. This definitely needs a few simple tests

ckan/logic/auth/create.py
((18 lines not shown))
else:
return {'success': True}
-
@tobes
tobes added a note

I think pep8 likes 2 blank lines here but I wouldn't not merge because of this.

The above code looks nicer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ckan/logic/auth/create.py
@@ -98,13 +98,20 @@ def rating_create(context, data_dict):
def user_create(context, data_dict=None):
user = context['user']
- if ('api_version' in context
- and not new_authz.check_config_permission('create_user_via_api')):
- return {'success': False, 'msg': _('User %s not authorized to create users') % user}
+ create_user_via_api = new_authz.check_config_permission(
+ 'create_user_via_api')
+ create_user = new_authz.check_config_permission('create_user')
+ using_api = ('api_version' in context)
@tobes
tobes added a note

we don't need the brackets here using_api = 'api_version' in context works fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@seanh
Owner

@tauberer Ok I think the logic is right now, but feel free to change it if not. So if the Register button thing could be done, and some tests added, then we can merge this. Let me know if you want some tips on how to write the tests

@JoshData

Will do. I'll ask if I need help with tests.

@nigelbabu nigelbabu was assigned
ckan/templates/header.html
@@ -41,7 +41,9 @@
<nav class="account not-authed">
<ul class="unstyled">
<li>{% link_for _('Log in'), controller='user', action='login' %}</li>
- <li>{% link_for _('Register'), controller='user', action='register', class_='sub' %}</li>
+ {% if g['auth.create_user_via_web'] %}
@nigelbabu Owner

I'm not entirely happy how this one turned out and I could use some feedback on how to make this better.

@seanh Owner
seanh added a note

@nigelbabu Can you look at other links that appear/disappear on tthe frontend depending on whether the user is authorised to use them? e.g. the add dataset buttons. The logic, in the templates, would usually say: If the user is authorized to register then show the link and leave it up to the create_user() action function to decide (based on the config setting) whether this is authorised. Templates should not read auth config settings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ckan/lib/app_globals.py
@@ -57,6 +57,7 @@
'openid_enabled': {'default': 'true', 'type' : 'bool'},
'debug': {'default': 'false', 'type' : 'bool'},
'ckan.debug_supress_header' : {'default': 'false', 'type' : 'bool'},
+ 'ckan.auth.create_user_via_web': {'default': 'true', 'type': 'bool'},
@tobes
tobes added a note

please remove this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ckan/tests/functional/test_user.py
((13 lines not shown))
+ config['ckan.auth.create_user_via_web'] = False
+ wsgiapp = ckan.config.middleware.make_app(config['global_conf'],
+ **config)
+ cls.app = paste.fixture.TestApp(wsgiapp)
+ PylonsTestCase.setup_class()
+
+ @classmethod
+ def teardown_class(cls):
+ config.clear()
+ config.update(cls._original_config)
+ PylonsTestCase.teardown_class()
+ model.repo.rebuild_db()
+
+ def test_home_page(self):
+ res = self.app.get(url_for(controller='home', action='index', id=None))
+ assert 'Register' not in res
@nigelbabu Owner

This line fails, while the next one passes. @tobes / @seanh - Any idea what I'm doing wrong? Is there a better way to change the config for a test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@seanh
Owner

I agree with @tobes we should not have separate options for create_user_via_api and create_user_via_web it should be just create_user, web and api behaviour should be the same (sorry I realise we've gone back and forth on this one).

I don't know about the config-changing problem, @tobes?

@tobes

yeah changing config is a pain at the moment. I'd write the test and just have it disabled with a FIXME till we get that sorted some point in the next month.

As far as which options I don't know the usecase for having 2 options so maybe just a anon_create_user makes sense sysadmins will still be able to do anything.

@nigelbabu
Owner

I've merged create_user_via_api and create_user_via_web into anon_create_user. Turning this into one configuration option will change default behavior, though since it will enable creating users via the API. If that's not fine, happy to revert the last commit.

@tobes

These tests do not test what you think and I think should be removed. You are getting the old templates.

I think we should get the test done in 2.1 to use new templates etc but not yet. I'm still not entirely sure of the value of these tests as they are. anyway - maybe that's just me

Hah. I just ran into the old templates problem in my next pull request. This could also be why I couldn't get the config to work anyway. Happy to remove them but we should open a bug about how this should be tested after we switch tests to use the new templates.

Maybe just add as a FIXME in the test file as we will have to redo them all over time and then we don't clog the issue tracker

@seanh
Owner

For something like this -- testing a config option that turns create users on/off -- I would do the test via the API not the web frontend. Call the user_create() API and check that it succeeds or fails as expected based on the config (and in the case of failure, also test for correct error message).

I'm not sure we want tests for things like whether a certain button appears on the frontend or not. Maybe we do want those kind of tests as well as it's a common complaint "some button is showing when it shouldn't", but we can't add them yet anyway until the whole question of frontend tests for the new templates is resolved

ckan/logic/auth/create.py
((5 lines not shown))
def user_create(context, data_dict=None):
user = context['user']
- if ('api_version' in context
- and not new_authz.check_config_permission('create_user_via_api')):
- return {'success': False, 'msg': _('User %s not authorized to create users') % user}
+ create_user = new_authz.check_config_permission(
+ 'anon_create_user')
+
+ if not create_user:
+ return {'success': False, 'msg': _('User {user} not authorized to '
@seanh Owner
seanh added a note

you missed out the word is :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@seanh
Owner

Should the new config option also be added to the deployment_ini.tmpl? (Commented out and with a comment above it saying what it does, as with other settings.)

I would delete the frontend tests (not just comment them out) and add a test that works via the API instead.

Other than that I think this looks fine, although we may want to resubmit it to get a cleaner commit history

@nigelbabu Your commit message seem to assume a lot of context, eg. "First attempt at test (It fails)", test for what? What feature does this commit relate to? Also the issue number should be in the commit message. See https://github.com/okfn/ckan/blob/master/CONTRIBUTING.rst#commit-messages

Although in the case of this pull request the issue number is difficult. It started out as a pull request (#356) from an outside contributor on their own fork, this was just a standlone pull request without an issue, then got moved into another pull request (#399) for a branch on okfn's repo so okfn devs could add to it. I think this shows the difficulty with some of our processes:

  • Relying on issue numbers in commits. We all know to create an issue on github before we push any commits because we have to put the issue number in the commit logs, but can we expect outside contributors to know this?

  • Turning issues into pull requests (instead of making a new pull request), it seems to happen fairly often that one pull request gets resubmitted as another pull request, then what issue number should appear in the commits, the number of the first pull request or the second? What if the second pull request contains commits that are cherry-picked from the first and already have the issue number of the first PR in their commit messages? If there was a standalone issue with separate PRs you wouldn't have this problem all commits in all PRs have the issue number of the issue

nigelbabu added some commits
@nigelbabu nigelbabu Merge branch 'master' into 356-auth-create-user-option 49ff7d6
@nigelbabu nigelbabu [#356] Change the valiation message 86cabe9
@nigelbabu nigelbabu [#356] Functional API tests
* Remove the old commented functional tests testing against the
  templates.
* New tests that test against the API.
56a87cc
@nigelbabu
Owner

The new config option is documented in the configuration docs. I'm not sure if we should have this deployment.ini_tmpl. We don't want to make that file too busy with options.

I've moved the tests around and got them working. I always thought the notes about commit messages were actually for the pull requests and since I commit often I sometimes don't spend enough time on the commit messages themselves. I've noted this and I'll be careful from now on.

The issue numbers and pull requests thing seems like something we should talk about in Thursday's dev meeting.

I've had to change the error message a bit here. 1) The user doesn't actually see the error message when I call user_create from the API. The api controller catches the error and shows a different message. 2) Pretty sure this is an anonymous API call and doesn't need a user at all. Plus whether access is granted or not does not depend on the user. It depends on the config. If there's a cleaner way to put that into a message, I'd welcome that.

@nigelbabu
Owner

I found the cause my test failures. There are tests to check that users can't be created via the API. Those tests fail. I'm guessing the right course of action is to remove these tests?

@tobes

we do want to test these if we can't at the moment you should add FIXMEs to the test files saying what tests are needed

nigelbabu added some commits
@nigelbabu nigelbabu Merge branch 'master' into 356-auth-create-user-option
Conflicts:
	ckan/templates/header.html
	doc/organizations_and_groups.rst
2c1082a
@nigelbabu nigelbabu Fix broken test 4d22dee
@nigelbabu nigelbabu commented on the diff
ckan/tests/functional/api/test_user.py
((7 lines not shown))
+ 'email': 'testinganewuser@ckan.org',
+ 'password': 'random',
+ }
+ res = self.app.post('/api/3/action/user_create', json.dumps(params))
+ res_dict = json.loads(res.body)
+ assert res_dict['success'] is True
+
+
+class TestCreateUser(PylonsTestCase):
+ '''Tests for the creating user when anon_create_user is false.
+
+ '''
+ @classmethod
+ def setup_class(cls):
+ cls._original_config = config.copy()
+ config['ckan.auth.anon_create_user'] = False
@nigelbabu Owner

@tobes This is what is causing my test failures. I can the tests for this class on its own, but when I run the entire file, it fails. Am I doing something wrong in how I change the config option? The config option is correctly reset back in the teardown, but the auth setting isn't.

@tobes
tobes added a note

@nigelbabu I'll try and take a look this pm. If you don't hear from me give me a reminder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
nigelbabu added some commits
@nigelbabu nigelbabu Flush CONFIG_PERMISSIONS caching during teardown
The auth config was being cached to CONFIG_PERMISSIONS which caused
the subsequents tests to fail. This change flushes the
CONFIG_PERMISSIONS cache in the teardown function
6fa6a39
@nigelbabu nigelbabu Rename clear_auth_functions_cache
Rename the clear_auth_functions_cache to clear_auth_cache that will
clear the functions cache as well the CONFIG_PERMISSIONS cache. Use
function in the tear down function instead of manually clearing the
CONFIG_PERMISSIONS cache
f718ac8
@nigelbabu nigelbabu Call clear_auth_cache() when config is changed 6c48730
@nigelbabu nigelbabu Change expected status to STATUS_403_ACCESS_DENIED f043d1d
@nigelbabu
Owner

@tobes I could use some help. What fails on Travis, passes locally. And if that status is STATUS_403_ACCESS_DENIED, it causes a failure locally.

doc/organizations_and_groups.rst
@@ -0,0 +1,241 @@
+Organizations and Groups
@tobes
tobes added a note

I think this file should die it was killed in other branches - any useful info should be somewhere else in the docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ckan/tests/logic/test_tag.py
((7 lines not shown))
res = self.app.post('/api/action/user_create', params=postparams,
status=StatusCodes.STATUS_403_ACCESS_DENIED)
res_obj = json.loads(res.body)
assert res_obj['help'].startswith("Create a new user.")
assert res_obj['success'] is False
- assert res_obj['error'] == {'message': 'Access denied', '__type': 'Authorization Error'}
+ assert res_obj['error'] == {u'__type': u'Validation Error',
+ u'email': [u'Missing value']}
@tobes
tobes added a note

this test looks wrong we should not be changing what we return here. The auth rules may be different but externally it should behave identically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tobes

nigel can you provide more info like exactly what test fails for you locally and how you are running it is it just test_08_user_create_not_authorized()? . Are you running all tests or just a selection - the tests are all broken so running some vs all can have different results.

@nigelbabu
Owner

I tried running just the file, just the folder, and all the tests. I believe they return the same results.

@tobes

cool I think I won't have time today to look at this but remind me on monday and I'll take a look

@tobes

@nigelbabu bit late but now looking at this - I get a different error but the same test

@tobes

Ok,

diff --git a/ckan/tests/logic/test_tag.py b/ckan/tests/logic/test_tag.py
index 6cef2a1..81402c2 100644
--- a/ckan/tests/logic/test_tag.py
+++ b/ckan/tests/logic/test_tag.py
@@ -152,8 +152,6 @@ class TestAction(WsgiAppCase):
         res_obj = json.loads(res.body)
         assert res_obj['help'].startswith("Create a new user.")
         assert res_obj['success'] is False
-        assert res_obj['error'] == {u'__type': u'Validation Error',
-                u'email': [u'Missing value']}

     def test_09_user_create(self):
         user_dict = {'name':'test_create_from_action_api',

this fixes it for me if I just run the tests as
nosetests --ckan --with-pylons=test-core.ini --nologcapture ckan/tests/logic/test_tag.py
the call fails we don't even validate anything so who cares about the email

I have no idea why this test and the following are in test_tags.py they have nothing to do with tags.

really we want better tests here but it is a pain till we get #549 merged which fixes all these config issues etc and will allow full testing - the test here is hardly worth having as it doesn't really test much imho

not sure why tests/functional/api/test_user.py:TestCreateUser changes the config as this should already be set via test-core.ini

@tobes

Ahh actually I see the option was true in test-core.ini so move the tests from test_tags to tests/functional/api/test_user.py:TestCreateUser should fix things as that sets the option to false I hate the ckan tests they are so shit.

@tobes

@nigelbabu it'd be great if we could get this ready for Tuesday's pull request meeting

@nigelbabu
Owner

I don't even know what that test is doing there. We already test both conditions in test_user.py. Anyway, tests pass. This is ready for review now.

@tobes tobes was assigned
@tobes

@nigelbabu let's get this one finished off

can you remove doc/organizations_and_groups.rst as it is junk

Also we cannot just remove ckan.auth.create_user_via_api we need to at least inform users of this change needed to their config and mitigate the change ie if this is here and not the new option then respect the old but do not allow them to have both old and new. (or something similar)

@nigelbabu nigelbabu was assigned
@vitorbaptista

@tobes @nigelbabu

I've merged with master, and fixed the tests. What else is needed for this? Just deprecating ckan.auth.create_user_via_api before removing it entirely? Actually, it might be better to simply throw an error if you run CKAN with this option, telling the user that it was removed and asking him/her to change that to ckan.auth.anon_create_user, right? It's should be pretty straight-forward.

@JoshData

Thanks for merging this guys!

@nigelbabu
Owner

@vitorbaptista Yup, that's all that's needed. I would prefer not breaking it, to be honest.

@tobes

Yep we should just throw a deprecated warning for the first release with this.

@vitorbaptista

@nigelbabu Then this should be good to merge. I'll just wait @tobes to fix the build in master, then merge again... Should I assign one of you to review it?

@nigelbabu
Owner

Since I wrote most of the code for this pull request, I'm not very comfortable reviewing it. @tobes, would you be able to review it?

@vitorbaptista vitorbaptista referenced this pull request
Merged

Users invitations #1178

@johnglover johnglover was assigned
@johnglover johnglover commented on the diff
ckan/logic/auth/create.py
((9 lines not shown))
- return {'success': False, 'msg': _('User %s not authorized to create users') % user}
+def user_create(context, data_dict=None):
+ # create_user_via_api is deprecated
+ using_api = 'api_version' in context
+ create_user_via_api = new_authz.check_config_permission(
+ 'create_user_via_api')
+
+ create_user = new_authz.check_config_permission(
+ 'anon_create_user')
+
+ if not create_user:
+ return {'success': False, 'msg': _('Not authorized to '
+ 'create users')}
+ elif using_api and not create_user_via_api:
+ return {'success': False, 'msg': _('User {user} not authorized to '
+ 'create users via the API').format(user=context.get('user'))}
else:
return {'success': True}

@nigelbabu ckan.tests.functional.api.test_user.TestUserApi.test_user_create_default fails here if you do not set create_user_via_api=true in config. From the discussion it seems like this config option should not be needed any more, so this looks like a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nigelbabu
Owner

@johnglover The tests now pass. There was a spurious test failure (only in 1 of the 4 environments) when I merged master in. Just kicked off a new build and it should pass.

@johnglover

@nigelbabu I'm a bit confused by that commit. I think that there is a bug is in the logic, not the tests.

If I create a new CKAN instance with only the following auth config:

ckan.auth.anon_create_user = true

user_create will return False when using the API, as ckan.auth.create_user_via_api defaults to False when not specified. So to make this work you are required to include a deprecated config option, which seems like an anti-pattern. I thought that the point of this new option was to replace create_user_via_api entirely (unless someone is still using it, in which case we should respect it for a release or two), but perhaps I have missed something.

@nigelbabu
Owner

Closing this pull request to create a new one. See #1226

@nigelbabu nigelbabu closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 5, 2013
  1. @JoshData

    add ckan.auth.create_user option (default true) which when set to fal…

    JoshData authored
    …se prevents all new user registrations (via site and API), which is useful in a locked-down publisher mode
Commits on Feb 14, 2013
  1. @seanh
  2. @seanh

    [#356] Tidy up some long lines

    seanh authored
    This makes the logic much easier to see I think
  3. @seanh

    [#356] Tweak auth.create_user logic

    seanh authored
    If ckan.auth.create_user = False then don't let anyone create users,
    either via the API or via the web interface.
  4. @seanh

    [#356] Better error messages for create user auth

    seanh authored
    If not allowed to create users via api then say 'not authorized to
    create user via api', if not allowed to create users at all then say
    'not authorized to create users'.
Commits on Mar 17, 2013
  1. @nigelbabu

    pep8 fixes

    nigelbabu authored
  2. @nigelbabu
  3. @nigelbabu
  4. @nigelbabu
  5. @nigelbabu

    Merge branch 'master' into 356-auth-create-user-option

    nigelbabu authored
    Conflicts:
    	ckan/templates/header.html
Commits on Mar 21, 2013
  1. @nigelbabu
  2. @nigelbabu
  3. @nigelbabu
  4. @nigelbabu
Commits on Mar 22, 2013
  1. @nigelbabu
Commits on Mar 23, 2013
  1. @nigelbabu
  2. @nigelbabu
  3. @nigelbabu
  4. @nigelbabu
  5. @nigelbabu
  6. @nigelbabu
Commits on Mar 26, 2013
  1. @nigelbabu
  2. @nigelbabu
  3. @nigelbabu

    [#356] Functional API tests

    nigelbabu authored
    * Remove the old commented functional tests testing against the
      templates.
    * New tests that test against the API.
  4. @nigelbabu
Commits on May 30, 2013
  1. @nigelbabu

    Merge branch 'master' into 356-auth-create-user-option

    nigelbabu authored
    Conflicts:
    	ckan/templates/header.html
    	doc/organizations_and_groups.rst
Commits on Jun 4, 2013
  1. @nigelbabu

    Fix broken test

    nigelbabu authored
Commits on Jun 6, 2013
  1. @nigelbabu

    Flush CONFIG_PERMISSIONS caching during teardown

    nigelbabu authored
    The auth config was being cached to CONFIG_PERMISSIONS which caused
    the subsequents tests to fail. This change flushes the
    CONFIG_PERMISSIONS cache in the teardown function
  2. @nigelbabu

    Rename clear_auth_functions_cache

    nigelbabu authored
    Rename the clear_auth_functions_cache to clear_auth_cache that will
    clear the functions cache as well the CONFIG_PERMISSIONS cache. Use
    function in the tear down function instead of manually clearing the
    CONFIG_PERMISSIONS cache
  3. @nigelbabu
Commits on Jun 7, 2013
  1. @nigelbabu
Commits on Jun 18, 2013
  1. @nigelbabu
Commits on Jul 29, 2013
  1. @vitorbaptista

    Merge branch 'master' into 356-auth-create-user-option

    vitorbaptista authored
    Conflicts:
    	ckan/plugins/core.py
      ckan/config/environment.py
Commits on Jul 30, 2013
  1. @vitorbaptista
  2. @vitorbaptista
Commits on Jul 31, 2013
  1. @vitorbaptista
  2. @vitorbaptista
Commits on Aug 15, 2013
  1. @vitorbaptista

    Merge branch 'master' into 356-auth-create-user-option

    vitorbaptista authored
    Conflicts:
    	ckan/new_authz.py
Commits on Aug 28, 2013
  1. @nigelbabu

    Fix tests

    nigelbabu authored
  2. @nigelbabu
This page is out of date. Refresh to see the latest.
View
2  ckan/config/deployment.ini_tmpl
@@ -65,7 +65,7 @@ ckan.auth.user_create_groups = true
ckan.auth.user_create_organizations = true
ckan.auth.user_delete_groups = true
ckan.auth.user_delete_organizations = true
-ckan.auth.create_user_via_api = false
+ckan.auth.anon_create_user = true
## Search Settings
View
9 ckan/config/environment.py
@@ -99,11 +99,20 @@ def __getattr__(self, name):
return self.null_function
+def _warn_deprecated_configs(app_conf):
+ if 'ckan.auth.create_user_via_api' in app_conf:
+ log.warn('Configuration `ckan.auth.create_user_via_api` is deprecated '
+ 'and will be removed soon. Please use '
+ '`ckan.auth.anon_create_user` instead.')
+
+
def load_environment(global_conf, app_conf):
"""Configure the Pylons environment via the ``pylons.config``
object. This code should only need to be run once.
"""
+ _warn_deprecated_configs(app_conf)
+
###### Pylons monkey-patch
# this must be run at a time when the env is semi-setup, thus inlined here.
# Required by the deliverance plugin and iATI
View
20 ckan/logic/auth/create.py
@@ -103,12 +103,22 @@ def rating_create(context, data_dict):
# No authz check in the logic function
return {'success': True}
-def user_create(context, data_dict=None):
- user = context['user']
- if ('api_version' in context
- and not new_authz.check_config_permission('create_user_via_api')):
- return {'success': False, 'msg': _('User %s not authorized to create users') % user}
+def user_create(context, data_dict=None):
+ # create_user_via_api is deprecated
+ using_api = 'api_version' in context
+ create_user_via_api = new_authz.check_config_permission(
+ 'create_user_via_api')
+
+ create_user = new_authz.check_config_permission(
+ 'anon_create_user')
+
+ if not create_user:
+ return {'success': False, 'msg': _('Not authorized to '
+ 'create users')}
+ elif using_api and not create_user_via_api:
+ return {'success': False, 'msg': _('User {user} not authorized to '
+ 'create users via the API').format(user=context.get('user'))}
else:
return {'success': True}

@nigelbabu ckan.tests.functional.api.test_user.TestUserApi.test_user_create_default fails here if you do not set create_user_via_api=true in config. From the discussion it seems like this config option should not be needed any more, so this looks like a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
View
2  ckan/new_authz.py
@@ -86,6 +86,7 @@ def _build(self):
def clear_auth_functions_cache():
_AuthFunctions.clear()
+ CONFIG_PERMISSIONS.clear()
def auth_functions_list():
@@ -312,6 +313,7 @@ def get_user_id_for_username(user_name, allow_none=False):
# permission and default
# these are prefixed with ckan.auth. in config to override
'anon_create_dataset': False,
+ 'anon_create_user': True,
'create_dataset_if_not_in_organization': True,
'create_unowned_dataset': True,
'user_create_groups': True,
View
4 ckan/templates/header.html
@@ -49,7 +49,9 @@
<ul class="unstyled">
{% block header_account_notlogged %}
<li>{% link_for _('Log in'), controller='user', action='login' %}</li>
- <li>{% link_for _('Register'), controller='user', action='register', class_='sub' %}</li>
+ {% if h.check_access('user_create') %}
+ <li>{% link_for _('Register'), controller='user', action='register', class_='sub' %}</li>
+ {% endif %}
{% endblock %}
</ul>
</nav>
View
20 ckan/templates/user/login.html
@@ -18,15 +18,17 @@ <h1 class="page-heading">{% block page_heading %}{{ _('Login') }}{% endblock %}<
{% endblock %}
{% block secondary_content %}
- <section class="module module-narrow module-shallow">
- <h2 class="module-heading">{{ _('Need an Account?') }}</h2>
- <div class="module-content">
- <p>{% trans %}Then sign right up, it only takes a minute.{% endtrans %}</p>
- <p class="action">
- <a class="btn" href="{{ h.url_for(controller='user', action='register') }}">{{ _('Create an Account') }}</a>
- </p>
- </div>
- </section>
+ {% if h.check_access('user_create') %}
+ <section class="module module-narrow module-shallow">
+ <h2 class="module-heading">{{ _('Need an Account?') }}</h2>
+ <div class="module-content">
+ <p>{% trans %}Then sign right up, it only takes a minute.{% endtrans %}</p>
+ <p class="action">
+ <a class="btn" href="{{ h.url_for(controller='user', action='register') }}">{{ _('Create an Account') }}</a>
+ </p>
+ </div>
+ </section>
+ {% endif %}
<section class="module module-narrow module-shallow">
<h2 class="module-heading">{{ _('Forgotten your details?') }}</h2>
View
2  ckan/templates/user/request_reset.html
@@ -3,7 +3,7 @@
{% block subtitle %}{{ _('Reset Your Password') }}{% endblock %}
{% block breadcrumb_content %}
- <li class="active">{% link_for _('Password Reset'), controller='user', action='register' %}</li>
+ <li class="active">{% link_for _('Password Reset'), controller='user', action='request_reset' %}</li>
{% endblock %}
{% block primary_content %}
View
59 ckan/tests/functional/api/test_user.py
@@ -1,20 +1,26 @@
+import paste
+from pylons import config
from nose.tools import assert_equal
import ckan.logic as logic
+import ckan.new_authz as new_authz
from ckan import model
from ckan.lib.create_test_data import CreateTestData
from ckan.tests import TestController as ControllerTestCase
+from ckan.tests.pylons_controller import PylonsTestCase
from ckan.tests import url_for
+import ckan.config.middleware
+from ckan.common import json
class TestUserApi(ControllerTestCase):
@classmethod
def setup_class(cls):
CreateTestData.create()
-
+
@classmethod
def teardown_class(cls):
model.repo.rebuild_db()
-
+
def test_autocomplete(self):
response = self.app.get(
url=url_for(controller='api', action='user_autocomplete', ver=2),
@@ -51,8 +57,55 @@ def test_autocomplete_limit(self):
print response.json
assert_equal(len(response.json), 1)
+ def test_user_create_default(self):
+ params = {
+ 'name': 'testinganewuser',
+ 'email': 'testinganewuser@ckan.org',
+ 'password': 'random',
+ }
+ res = self.app.post('/api/3/action/user_create', json.dumps(params),
+ expect_errors=True)
+ res_dict = json.loads(res.body)
+ assert res_dict['success'] is False
+
+
+class TestCreateUser(PylonsTestCase):
+ '''Tests for the creating user when anon_create_user is false.
+
+ '''
+ @classmethod
+ def setup_class(cls):
+ cls._original_config = config.copy()
+ config['ckan.auth.anon_create_user'] = False
@nigelbabu Owner

@tobes This is what is causing my test failures. I can the tests for this class on its own, but when I run the entire file, it fails. Am I doing something wrong in how I change the config option? The config option is correctly reset back in the teardown, but the auth setting isn't.

@tobes
tobes added a note

@nigelbabu I'll try and take a look this pm. If you don't hear from me give me a reminder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ new_authz.clear_auth_functions_cache()
+ wsgiapp = ckan.config.middleware.make_app(config['global_conf'],
+ **config)
+ cls.app = paste.fixture.TestApp(wsgiapp)
+ PylonsTestCase.setup_class()
+
+ @classmethod
+ def teardown_class(cls):
+ config.clear()
+ config.update(cls._original_config)
+ new_authz.clear_auth_functions_cache()
+ PylonsTestCase.teardown_class()
+
+ model.repo.rebuild_db()
+
+ def test_user_create_disabled(self):
+ params = {
+ 'name': 'testinganewuser',
+ 'email': 'testinganewuser@ckan.org',
+ 'password': 'random',
+ }
+ res = self.app.post('/api/3/action/user_create', json.dumps(params),
+ expect_errors=True)
+ res_dict = res.json
+ assert res_dict['success'] is False
+
+
class TestUserActions(object):
-
+
@classmethod
def setup_class(cls):
CreateTestData.create()
View
1  ckan/tests/functional/test_user.py
@@ -11,6 +11,7 @@
from base import FunctionalTestCase
from ckan.lib.mailer import get_reset_link, create_reset_key
+
class TestUserController(FunctionalTestCase, HtmlCheckMethods, PylonsTestCase, SmtpServerHarness):
@classmethod
def setup_class(cls):
View
3  ckan/tests/logic/test_auth.py
@@ -6,13 +6,14 @@
INITIAL_TEST_CONFIG_PERMISSIONS = {
'anon_create_dataset': False,
+ 'anon_create_user': False,
'create_dataset_if_not_in_organization': False,
'user_create_groups': False,
'user_create_organizations': False,
'user_delete_groups': False,
'user_delete_organizations': False,
- 'create_user_via_api': False,
'create_unowned_dataset': False,
+ 'create_user_via_api': False,
}
View
16 ckan/tests/logic/test_tag.py
@@ -1,16 +1,24 @@
import json
+import paste
from pprint import pprint
from nose.tools import assert_equal, assert_raises
-import ckan.lib.search as search
+from pylons import config
+import ckan.lib.search as search
import ckan.model as model
from ckan.lib.create_test_data import CreateTestData
from ckan.tests import WsgiAppCase
from ckan.tests import StatusCodes
+from ckan.config.middleware import make_app
+
class TestAction(WsgiAppCase):
@classmethod
def setup_class(cls):
+ cls._original_config = config.copy()
+ config['ckan.auth.anon_create_user'] = False
+ wsgiapp = make_app(config['global_conf'], **config)
+ cls.app = paste.fixture.TestApp(wsgiapp)
search.clear()
CreateTestData.create()
cls.sysadmin_user = model.User.get('testsysadmin')
@@ -19,6 +27,8 @@ def setup_class(cls):
@classmethod
def teardown_class(cls):
+ config.clear()
+ config.update(cls._original_config)
model.repo.rebuild_db()
def test_06a_tag_list(self):
@@ -145,13 +155,13 @@ def test_07_tag_show_unknown_license(self):
assert_equal(pkg['isopen'], False)
def test_08_user_create_not_authorized(self):
- postparams = '%s=1' % json.dumps({'name':'test_create_from_action_api', 'password':'testpass'})
+ postparams = '%s=1' % json.dumps(
+ {'name':'test_create_from_action_api', 'password':'testpass'})
res = self.app.post('/api/action/user_create', params=postparams,
status=StatusCodes.STATUS_403_ACCESS_DENIED)
res_obj = json.loads(res.body)
assert res_obj['help'].startswith("Create a new user.")
assert res_obj['success'] is False
- assert res_obj['error'] == {'message': 'Access denied', '__type': 'Authorization Error'}
def test_09_user_create(self):
user_dict = {'name':'test_create_from_action_api',
View
19 doc/configuration.rst
@@ -255,6 +255,21 @@ Default value: ``False``
Allow users to create datasets without registering and logging in.
+.. _ckan.auth.anon_create_user:
+
+ckan.auth.anon_create_user
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Example::
+
+ ckan.auth.anon_create_user = True
+
+Default value: ``True``
+
+
+Allow visitors to create user accounts.
+
+
.. _ckan.auth.create_unowned_dataset:
ckan.auth.create_unowned_dataset
@@ -346,6 +361,10 @@ Allow users to delete organizations.
ckan.auth.create_user_via_api
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+.. deprecated:: 2.2
+ It's not possible to disable user creation just through the API anymore.
+ See :ref:`ckan.auth.anon_create_user`.
+
Example::
ckan.auth.create_user_via_api = False
View
4 test-core.ini
@@ -30,7 +30,7 @@ solr_url = http://127.0.0.1:8983/solr
ckan.auth.user_create_organizations = true
ckan.auth.user_create_groups = true
-ckan.auth.create_user_via_api = false
+ckan.auth.anon_create_user = true
ckan.auth.create_dataset_if_not_in_organization = true
ckan.auth.anon_create_dataset = false
ckan.auth.user_delete_groups=true
@@ -80,7 +80,7 @@ smtp.mail_from = info@test.ckan.net
ckan.locale_default = en
ckan.locale_order = en pt_BR ja it cs_CZ ca es fr el sv sr sr@latin no sk fi ru de pl nl bg ko_KR hu sa sl lv
-ckan.locales_filtered_out =
+ckan.locales_filtered_out =
ckan.datastore.enabled = 1
Something went wrong with that request. Please try again.