Sysadmins locked-out of API without Right: (visitor, SITE_READ, System)

[dread]

The problem is that in ckan/controllers/rest.py the BaseApiController? has this method:

    def __before__(self, action, **env):
        BaseController.__before__(self, action, **env)
        if not self.authorizer.am_authorized(c, model.Action.SITE_READ, model.System):
            abort(401, _('Not authorized to see this page'))

which works on the basis of your c.user, rather than your apikey. All API users are treated as visitors (since API users don't get a login cookie) and even a sysadmin's apikey is blocked unless there is a right for a Visitor to SITE_READ.

Also needs tests.

(Also, why is this restriction only on the API, package search, group index and tags and agroup index? I'm guessing SITE_READ is only for places where other authz don't apply, but maybe it should not be called 'SITE_READ' but 'OTHER_READ' or something?)

[rgrp]

Think this would be resolved by refactoring implied in ticket:1001 (specifically checks for api key and remote user should lead to availability of same set of identification information).

[dread]

I agree with leaving #1001 solving this issue. Centralising this is good. This ticket can instead focus on:

• returning the correct 403 error (rather than the 302 currently)
• creating some tests
• documenting
• consider renaming the SITE_READ variable and rethinking purpose

[dread]

I've added in tests for SITE_READ and corrected the 403 response - see branch bug-1044-site-read.

The tests have uncovered some inconsistencies in the use of SITE_READ between reading/editing a package in the Web interface vs the REST API. Friedrich, do you want to take a look at ckan/tests/functional/test_authz.py:TestSiteRead.test_pkggroupadmin_read for details and have a think about how this should best work. How about making SITE_READ more widespread in the WUI, to protect read/editing/searching things, in addition to the READ and EDIT RoleActions?.

Once that is decided, it should be pretty easy to refer to the tests and document SITE_READ.

[pudo]

David, thanks for writing those tests - perhaps we should combine them with the ones below ("TestLockedDownUsage?") which attempt to basically do the same.

As for the inconsistency introduced by the global check in the REST API you're right: Of course it is strange that WUI access checks are more granular than the API checks. The alternative is that we either move authz checks into all relevant REST places (a major refactoring I would be suspicious of) or that we introduce duplicate checks on the WUI actions (I actually have performance concerns about that, authz is incredibly slow - and it introduces another level of special authz that I think we really should not have). Given the choice I'd opt for the REST refactor - there is no good reason to make SITE_READ a duplicate check where authz already applies.

On the other hand, this is a function we really don't want to enable or even have and that was only added to satisfy some very specific user demands. Given that these are fulfilled, I'm actually OK keeping the inconsistency for now - nobody will see it in normal operations and in a locked-down environment, users will need to have API keys anyway.

Regarding the naming, I'm pretty opionion-less: SITE_READ was proposed by rgrp and I think its pretty fitting, while OTHER_READ would just confuse me. PUBLIC_READ might work, though.

[dread]

I'm happy to leave the inconsistencies in SITE_READ for now, since this will be resolved when authz is centralised between WUI and API in the dictization refactor.

I'm also happy to leave both TestSiteRead? and TestLockedDownUsage? as they complement each other nicely actually.

So I think all that remains for this ticket is to add a paragraph to the authz documentation about SITE_READ. Pudo would you mind doing this? It's well worth noting where it applies and where it doesn't.

[dread]

I think all that remains for this ticket is to add a paragraph to the authz documentation about SITE_READ. Pudo would you mind doing this? It's well worth noting where it applies and where it doesn't.

[dread]

I've added in the docs for this in cset:013da53052d1 ready for release 1.3.3.