Add support for CKAN 2.10.0+ #120
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…in LdapPlugin#logout()
…0+ to login_success()
themowski
commented
Sep 24, 2023
| @@ -7,3 +7,4 @@ build/ | |||
| dist/ | |||
| .idea | |||
| **/node_modules/ | |||
| .vscode/ | |||
There was a problem hiding this comment.
I can roll this back if needed -- added it because I'm a VS Code user and had some local configs I wanted to ignore.
There was a problem hiding this comment.
Not a problem at all, thanks for adding it! 🙂
jrdh
approved these changes
Sep 25, 2023
|
No problem, thanks for your patience while I got things set up to contribute! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR adds support for CKAN 2.10.0+ It addresses #116 -- see the issue report for much more detailed information & background.
Notes
Compatibility
This code updates
ckanext-ldapto work with released versions of CKAN 2.10. It's worth nothing that alpha versions of CKAN -- those identified as2.10.0a0in the upstream CKAN code -- may or may not work, depending on whether or not that version contains the commit that merged ckan/ckan#6560.If the alpha commit is from after that merge, then this plugin will work as expected.
If not (i.e., the end-user is using CKAN 2.10.0a0 at a commit from before that PR was merged), then the user is functionally running CKAN 2.9.x in terms of authentication. The plugin will not work correctly, and to fix it, they would need to undo the changes from this PR by removing the
if toolkit.check_ckan_version()blocks that this PR adds.(As a note -- I logged ckan/ckan#7777 to hopefully improve this versioning situation in the future.)
I thought about putting notes about this in the code, but decided it was too deep in the weeds -- my thought is that if someone encounters issues this particular issue, they will either upgrade CKAN to a released 2.10 version, or find this PR and roll back the changes locally.
Other changes
I updated the CKAN badge in the README to include 2.10, and emphasize that you support 2.9.7 and higher. Not sure if that's the approach you feel is best; figured I'd at least throw something out there to start with.
Remember Me & CSRF not included
I opted not to implement "Remember Me". I'm not sure if this existed in 2.9, or if it's new to 2.10. The upstream
login()code in ckan/ckan v2.9.9 and ckan/ckan v2.10.1 differs quite a bit (because of the migration to flask-login). If this is 2.10-only, we can probably do something similar to what CKAN does. Otherwise, we'll need to understand how CKAN 2.9 is doing it, and probably add some conditional logic.Similarly, I opted not to try implementing the CSRF protection. After some quick reading, I still don't fully understand what it's about, so I don't feel confident enough to tackle that. I'm not really sure how to test it, since I'm not a front-end developer.
Regardless of all this, I think both of those features should be handled via separate issues/PR's, as they are not strictly necessary for 2.10 support.