-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add support for CKAN 2.10+ #116
Comments
Hi @themowski, thanks so much for reporting this and also for taking the time to propose a solution as well, it's always nice to get a thorough issue! ❤️ |
Sure, I'll start work on a PR when I find some time. 👍 As a result of hacking on this, I ended up logging ckan/ckan #7777. Based on what I found during the deeper dive I did while logging that ticket & finding edge cases, I might consider changing the approach I suggested above slightly, so that we can more seamlessly support "in-between" deployments (i.e., ones that are 2.10.0 alpha builds). In particular, instead of using the I'll also try to dig a little bit into the "Remember Me" & CSRF stuff as part of this PR, but no promises. |
…in LdapPlugin#logout()
…0+ to login_success()
Overview
With the release of CKAN 2.10, the original authentication technology provided by
repoze.who
has been replaced withflask-login
. These changes were introduced in ckan/ckan PR#6560. As a result of these changes, theckanext-ldap
plugin does not work out of the box with CKAN 2.10.0+.I installed the latest version of the plugin (
v3.2.10
) in CKAN 2.10.1 via the official Docker setup provided at ckan/ckan-docker. When I entered my credentials on the login page, they seemed to have been correctly verified against the server -- an invalid username/password combination failed as expected, but the correct credentials routed me to a different page. I received the generic "Internal Server Error" message in the CKAN UI, and saw this in the logs:The
/user/logged_in
endpoint that this references was removed in the upstream ckan/ckan PR#6560, so the plugin's logic needs to be updated accordingly.Suggested Implementation
Adding support for CKAN 2.10 seems to be pretty straightforward. From what I can tell, we need to handle the fact that
/user/logged_in
does not exist anymore, and we need to integrate with the newtoolkit.login_user()
andtoolkit.logout_user()
functions to manage the user's login status.Changes are required in two files. The diffs below were written against
[email protected]
, the latest version as of this writing.These suggestions use the
toolkit.check_ckan_version()
function to only call the new functionality when running CKAN 2.10+. This means that the plugin should continue to work without issue in CKAN < 2.10 (but note that I did not test that case). That said, one thing to note about thecheck_ckan_version()
function is that alpha/beta releases of CKAN are considered to be the same version as the corresponding major release (an issue that I will be raising with CKAN separately). This means that if users are working with an alpha build of CKAN 2.10 from before PR#6560 was merged, then this functionality will not work as expected. I think it is probably safe to say that that is an unsupported edge case, though.ckanext/ldap/plugin.py
The
logout()
function inplugin.py
needs to be modified to call thetoolkit.logout_user()
function when the CKAN version is at least 2.10. Git diff below:ckanext/ldap/routes/_helpers.py
The bulk of the work needs to be done in the
login_success()
function in_helpers.py
. In CKAN 2.10+, we need to call thetoolkit.login_user()
function, which requires as input theckan.model.User
object that represents the user logging in. This object needs to be looked up, and there are a few edge cases that could result in errors and need to be handled. Git diff below:Final Notes
I am by no means an expert in CKAN, LDAP, or even authentication in general. I learned just enough about how this plugin and CKAN work to implement the initial support for logging into CKAN 2.10+ demonstrated above. My changes do not touch the "Remember Me" functionality, and also do not do anything with CSRF (which is new in CKAN 2.10, and they have a best-practices page in their official documentation).
Both of these seem to be handled by the
login()
function inckan/views/user.py
, so their code may be a starting place if this is desirable to implement. Per the release notes for CKAN 2.10, CSRF is optional for extensions for now, but will eventually be required in a future version of CKAN.The text was updated successfully, but these errors were encountered: