Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make context['user'] always username or None #2817

Merged
merged 1 commit into from Jan 20, 2016

Conversation

Projects
None yet
4 participants
@wardi
Copy link
Contributor

commented Dec 31, 2015

This removes the need to detect IP addresses and other non-username strings in context['user']. This is important because this value is used to determine authorization to perform actions.

@wardi

This comment has been minimized.

Copy link
Contributor Author

commented Dec 31, 2015

@vitorbaptista does this change fix the tests for you, like #2801 ?

@davidread

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2016

👍

@vitorbaptista

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2016

@wardi Unfortunately it doesn't. The test is simply:

    @helpers.change_config('ckan.auth.anon_create_dataset', False)
    def test_new_requires_user_to_be_able_to_create_packages(self):
        url = toolkit.url_for('import_datapackage')
        response = self.app.get(url)
        assert_equals(302, response.status_int)

Where the import_datapackage page requires that the user is able to package_create. Even though the user is u'Unknown IP Address' at that moment (using your branch), toolkit.check_access('package_create', context) still returns True. The issue appears to be on:

# ckan/authz.py
def auth_is_anon_user(context):
    ''' Is this an anonymous user?
        eg Not logged in if a web request and not user defined in context
        if logic functions called directly

        See ckan/lib/base.py:232 for pylons context object logic
    '''
    context_user = context.get('user')  # context_user == u"Unknown IP Address"
    is_anon_user = not bool(context_user)  # is_anon_user == False (should be True)

    return is_anon_user
@wardi

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2016

Hmm. I wonder how we're getting "Unknown IP Address" in context['user']. @vitorbaptista Is the import_datapackage controller code putting 'user': c.user or c.author in the context?

@vitorbaptista

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2016

@wardi

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2016

@vitorbaptista I see. That's being stored in c.remote_addr, I'm trying to figure out how that value ends up in context['user']. The code I changed is trying to prevent it ending up there via c.author

@wardi

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

@vitorbaptista

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2016

@wardi you're right! Removing the toolkit.c.author fixed the tests 👍

I don't know the implications of this PR but, as far as I'm concerned, it's good to go.

@amercader amercader merged commit ab685c7 into ckan:master Jan 20, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

vitorbaptista added a commit to frictionlessdata/ckanext-datapackager that referenced this pull request Jan 27, 2016

Don't put `c.author` on context's "user"
For more information about it, check ckan/ckan#2817
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.