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

Allow chained auth functions to chain to built-in auth functions. #4248

Closed
wants to merge 1 commit into from

Conversation

ashleysommer
Copy link
Contributor

@ashleysommer ashleysommer commented May 11, 2018

Proposed fixes:

Some authentication plugins implement chained auth functions on actions such as user_create and user_update. If no other plugins provide these functions, then CKAN raises the exception "The auth 'user_create' is not found for chained auth". The CKAN built-in auth system however does provide these functions. So it makes sense to fallback to having the chained auth function chain to the corresponding built-in function.

This is an example of one such plugin which attempts to chain off 'user_create' and 'user_update' and fails:
https://github.com/NaturalHistoryMuseum/ckanext-ldap/blob/6f9aec4edadaf792e347b4d20adad2c2322c167f/ckanext/ldap/logic/auth/create.py#L12

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

@ashleysommer ashleysommer changed the base branch from master to 2.8 May 11, 2018 04:57
@amercader
Copy link
Member

I've found this limitation frustrating in the past myself. @wardi can you recall a reason to not chain core auth functions?

@wardi
Copy link
Contributor

wardi commented May 15, 2018

No, just an oversight. We should absolutely be able to chain core auth functions

Copy link
Member

@amercader amercader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashleysommer Just some minor style comments but otherwise good to go!

@@ -102,13 +102,19 @@ def _build(self):
fetched_auth_functions[name] = auth_function

for name, func_list in chained_auth_functions.iteritems():
if name not in fetched_auth_functions:
if name not in fetched_auth_functions and\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind using parentheses instead of \ to wrap the line? Just to be consistent with the styling we use elsewhere in the code.

else:
# fallback to chaining off the builtin auth function
prev_func = self._functions[name]
fetched_auth_functions[name] =\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@amercader amercader self-assigned this May 25, 2018
@wardi
Copy link
Contributor

wardi commented May 25, 2018

We should have a simple test of this functionality too. The only current chained_auth tests are in the datastore extension, I'd be fine with adding another test there instead of creating a whole new plugin just for the tests. https://github.com/ckan/ckan/blob/master/ckanext/datastore/tests/test_chained_auth_functions.py

@amercader
Copy link
Member

@ashleysommer do you have time to add the little test @wardi mentions?

@frafra
Copy link
Contributor

frafra commented Nov 20, 2018

Same issue here. Is there something I can do to speed up things?

@frafra
Copy link
Contributor

frafra commented Nov 23, 2018

I implement a simple fix in addition to what has been proposed, because otherwise there would be an error (__name__ was missing):
https://github.com/frafra/ckan/tree/chain-builtin-auth-fixed

This was referenced Dec 6, 2018
@wardi
Copy link
Contributor

wardi commented Dec 18, 2018

closed in favour of #4579

@wardi wardi closed this Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants