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

get_action is a confusing interface, let's improve it #3254

Closed
wardi opened this issue Sep 22, 2016 · 6 comments
Closed

get_action is a confusing interface, let's improve it #3254

wardi opened this issue Sep 22, 2016 · 6 comments

Comments

@wardi
Copy link
Contributor

wardi commented Sep 22, 2016

current patterns in ckan

# call as username
ckan.logic.get_action('action_name')(
    {'user': username}, {'arg1': val1, ...}) 

# call as c.user (dangerous if you're not running from a request)
ckan.logic.get_action('action_name')(
    {}, {'arg1': val1, ...})  # {} or None as context

# call as '127.0.0.1' with ignore_auth=True for tests, but not obvious from function name
ckan.tests.helpers.call_action('action_name', arg1=val1, ...)

# call as username with optional context parameter
ckan.tests.helpers.call_action('action_name',
    context={'user': username, 'ignore_auth': False}, arg1=val1, ...)

examples from ckanapi (not necesarily ideal)

# call as site user
ckanapi.LocalCKAN().action.action_name(arg1=val1, ...)

# call as site user with explicit action name, data_dict
ckanapi.LocalCKAN().call_action('action_name', {'arg1': val1, ...})

# call as username with explicit context (not a normal way to use ckanapi)
ckanapi.LocalCKAN().call_action('action_name', {'arg1': val1, ...},
    {'user': username})

# call as username
ckanapi.LocalCKAN(username=username).action.action_name(arg1=val1, ...)

# upload files (not supported by ckan.logic.get_action)
ckanapi.LocalCKAN().action.resource_create(
    package_id='my-dataset-with-files',
    upload=open('/path/to/file/to/upload.csv', 'rb'))

# upload files with explicit file list (not supported by ckan.logic.get_action)
ckanapi.LocalCKAN().call_action('resource_create',
    {'package_id': 'my-dataset-with-files'},
    None,
    {'upload': open('/path/to/file/to/upload.csv', 'rb'))
@wardi
Copy link
Contributor Author

wardi commented Sep 22, 2016

How about friendly calls with keyword arguments and auto-detected file objects,
similar to ckanapi.LocalCKAN.action.action_name but with explicit declaration of the user.

ckan.logic.request_action(action_name, **data_dict)
ckan.logic.sysadmin_action(action_name, **data_dict)
ckan.logic.anonymous_action(action_name, **data_dict)
ckan.logic.user_action(username, action_name, **data_dict)

request_action is for use just from controllers/views and template helpers, taking the user from c.user edit: if request_action is used outside of a request it must behave the same as anonymous_action

@jrods
Copy link
Contributor

jrods commented Sep 22, 2016

what about just having the permission level as an argument instead?

ckan.logic.get_action(action_name, 'sysadmin', **data_dict)
ckan.logic.get_action(action_name, 'anonymous', **data_dict)
ckan.logic.get_action(action_name, 'request', **data_dict) # this one might be confusing? 
# would still use c.user
ckan.logic.get_action(action_name, username, **data_dict) # or userobj

Actually, might also make sense to change it to call_action instead, to be consistent with helpers?

@wardi
Copy link
Contributor Author

wardi commented Sep 22, 2016

@jrods then we can't have users named 'sysadmin', 'anonymous' or 'request' :-) In general I don't like mixing different types of arguments in the same parameters to avoid surprising bugs like that.

@jrods
Copy link
Contributor

jrods commented Sep 22, 2016

Fair enough, I would have then suggested only taking an userobj instead of username, making it, pretty trivial to check, but then i don't know if there would be problems in the instance of when an userobj wouldn't be available.

It makes sense to have dedicated functions to specific authorization levels, but what about extensions that expand/create user roles different from core? Would extensions just sort of be self contained in that regard, managing non-default permissions within itself? I don't know what the current situation is like and what other extension do, so I would like to be enlighten, if it's worth being concerned about.

@wardi
Copy link
Contributor Author

wardi commented Sep 22, 2016

I don't think this list can actually grow. Other than anonymous (no user) and sysadmin (use the site user) I can't think of any permission level that could be given without giving a user. IPermissionLabels and IAuthFunctions plugins calculate permissions based on the user.

@kmbn
Copy link
Contributor

kmbn commented Jan 22, 2019

We decided to close old issues that are not actively worked on so that we can focus our effort and attention on issues affecting the current versions of CKAN.

If this issue is still affecting the version of CKAN you're working with now, please feel free to comment or reopen the issue.

If you do reopen this issue, please update it with new details. One reason it might not have been resolved in the past is that it wasn't clear how a contributor could address the issue.

@kmbn kmbn closed this as completed Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants