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

package_search include_private flag #3191

Merged
merged 10 commits into from Aug 17, 2016

Conversation

Projects
None yet
5 participants
@wardi
Copy link
Contributor

wardi commented Aug 5, 2016

Proposed fix for #3176

Stop using a context variable in package_show to return private datasets (only works from inside ckan), instead allow users to pass include_private=True to have all datasets they have permission to see returned.

This is essentially what is already allowed when users browse to the organzation search page unless there's a customised package_show auth function. In that case we might be exposing more information than was available earlier (users can get the whole dataset metadata not just what's seen in search results). We'll have to change the way permissions on datasets are defined to fix that issue, see: #3192 for next steps.

@wardi

This comment has been minimized.

Copy link
Contributor

wardi commented Aug 5, 2016

This needs some more tests covering include_private as a normal parameter. We could also add a config option to default to showing private datasets in the normal search page here too.

@wardi

This comment has been minimized.

Copy link
Contributor

wardi commented Aug 5, 2016

@TkTech this is sure to conflict with #3118. It feels like lots of solr-specific code in this action should be moved into lib/search

@TkTech

This comment has been minimized.

Copy link
Member

TkTech commented Aug 6, 2016

@wardi there are a number of actions and methods relying on Solr syntax queries that I'm going to have to work through one-by-one. The changes proposed and upcoming in #3118 will take months to finish, so we shouldn't let that be a blocker or concern for this PR right now.

@amercader amercader self-assigned this Aug 9, 2016

@wardi

This comment has been minimized.

Copy link
Contributor

wardi commented Aug 12, 2016

Spoke to @amercader about this on Tuesday and we decided to enable display of private datasets on the default search page as part of this PR. There will be a new option ckan.search.default_include_private = False to revert to the old CKAN behaviour if you really want to.

@Aaron-M

This comment has been minimized.

Copy link

Aaron-M commented Aug 15, 2016

Question - would having the search results set to (true) include_private datasets affect if private datasets are indexed by google... (currently they are not)?

@wardi

This comment has been minimized.

Copy link
Contributor

wardi commented Aug 16, 2016

anonymous users (like google) shouldn't be able to see private datasets. If you can see private datasets without logging in with these changes please let me know!

super(cls, cls).setup_class()
helpers.reset_db()
search.clear_all()

This comment has been minimized.

@amercader

amercader Aug 17, 2016

Member

Why getting rid of these class setup methods?

This comment has been minimized.

@wardi

wardi Aug 17, 2016

Contributor

They're duplicating code already in the base class


Controls whether the default search page (``/dataset``) should include
private datasets visible to the current user or only public datasets
visible to everyone.

This comment has been minimized.

@amercader

amercader Aug 17, 2016

Member

Shall we add a note here about the behaviour on the API not being the same (ie you don't get private datasets by default and having to pass include_private)? I can see this being confusing

This comment has been minimized.

@wardi

wardi Aug 17, 2016

Contributor

The behaviour of the API stays the same unless you're relying on context['ignore_capacity_check'] = True somewhere in your own code. I'd suggest replacing that with context['user'] = site_user; data_dict['include_private'] = True

This comment has been minimized.

@amercader

amercader Aug 17, 2016

Member

I meant that the behaviour is not the same in the frontend than in the API. If I understand correctly by default package_search won't show private datasets, as opposed to the frontend.

This comment has been minimized.

@wardi

wardi Aug 17, 2016

Contributor

Could this be written more clearly? I say "the default search page (/dataset)", nothing about the API. Is that confusing?

This comment has been minimized.

@amercader

amercader Aug 17, 2016

Member

Fair enough, let's leave it like this


# Remove before these hit solr FIXME: whitelist instead
include_private = asbool(data_dict.pop('include_private', False))
include_drafts = asbool(data_dict.pop('include_drafts', False))

This comment has been minimized.

@amercader

amercader Aug 17, 2016

Member

We need to document these options in the docstring

This comment has been minimized.

@wardi

wardi Aug 17, 2016

Contributor

done in ede4e11

@amercader

This comment has been minimized.

Copy link
Member

amercader commented Aug 17, 2016

@wardi looks great, some minor comments

@nibecker

This comment has been minimized.

Copy link

nibecker commented Aug 17, 2016

Thanks @wardi. We should make sure that users with customised package_show will be informed about this change in advance as it might cause trouble for them. This includes of course careful documentation in the release notes. Any other suggestion where to place a notice?

@wardi

This comment has been minimized.

Copy link
Contributor

wardi commented Aug 17, 2016

@nibecker yes, we'll need to include the new parameters to package_search in the changelog, however If you've customised package_show you should know you're going to have trouble with upgrades. Overriding a core action is pretty much the same as monkey-patching CKAN code.

edit: parameters are new on package_search, not package_show

@amercader

This comment has been minimized.

Copy link
Member

amercader commented Aug 17, 2016

@wardi is that ready to go or did you want to add more tests?

@wardi

This comment has been minimized.

Copy link
Contributor

wardi commented Aug 17, 2016

@amercader I was just looking at that. Are there cases that aren't covered by the current tests?

@amercader

This comment has been minimized.

Copy link
Member

amercader commented Aug 17, 2016

Maybe "sysadmin can search private datasets" and "user from org1 can not see private datasets from org2"? I know auth testing of the underlying actions should cover that but just in case :)

@wardi

This comment has been minimized.

Copy link
Contributor

wardi commented Aug 17, 2016

@amercader sure, but I'd like to do those tests at the action level because there's no custom code for either of those cases in the controller

@amercader

This comment has been minimized.

Copy link
Member

amercader commented Aug 17, 2016

But this should be just a matter of setting the appropriate REMOTE_USER (a sysadmin and an org1 user) and check the output as you do in the other tests, right?

@wardi

This comment has been minimized.

Copy link
Contributor

wardi commented Aug 17, 2016

@amercader ok, added those too

@amercader

This comment has been minimized.

Copy link
Member

amercader commented Aug 17, 2016

PEEEEEEEEEEP 8 🎱 💀

@amercader amercader merged commit 5f16935 into master Aug 17, 2016

1 check was pending

ci/circleci CircleCI is running your tests
Details
@amercader

This comment has been minimized.

Copy link
Member

amercader commented Aug 17, 2016

@wardi fixed that myself, merged now

@amercader amercader deleted the 3191-package-search-include-private branch Aug 17, 2016

@wardi wardi referenced this pull request Sep 15, 2016

Merged

IPermissionLabels #3192

rufuspollock added a commit to rufuspollock/ckan-explorer that referenced this pull request Feb 9, 2017

Merge pull request #14 from HawaDawa/clean-version
[all][l]: Rework the CKAN Explorer to provide new features

- Single dataset operation: supply dataset in the URL and we go directly to that resource
- Support grabbing data from CKAN user's private datasets (requires a very recent version of CKAN)
- Don't append resource views but rather replace them so that ckan explorer becomes more of a single-page SQL tool; for opening multiple resources there are buttons to open them in new tabs
- Minor formatting changes (code and appearance)

See also:
ckan/ckan#3191
okfn/ckan.js#25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment