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

Feature/ckanext iauth #200

Merged
merged 78 commits into from
Feb 8, 2021
Merged

Conversation

jbrown-xentity
Copy link
Contributor

Integrating CKAN authentication customizations. Related to GSA/data.gov#2575.

Copy link
Contributor

@adborden adborden left a comment

Choose a reason for hiding this comment

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

Grab me if some of this doesn't make sense. There's a lot here. This is looking good, but I have some questions/issues about the approach.

I'm concerned we're overwriting the existing auth functions instead of augmenting them (chaining them). We don't want to accidentally allow access because the user exists when the controls should be more nuanced (only allow if user exists and is a member of an organization).

Adding more comprehensive tests would allow us to be sure. We should be testing all CKAN roles as a matrix to be sure we've captured all the cases. When dealing with authorization, we should be thorough in our tests.

@avdata99
Copy link
Contributor

My two cents here @jbrown-xentity @adborden
The functions at get_auth_functions will override (not chain) the default ones (usually just return {'success': True} for read functions). If the only logic (for read functions) is that only logged in users can read all then we probably don't require to chain any action.
I think the decorator auth_disallow_anonymous_access will be enough to all functions that don't require change the logic.

Have we already identified any case where it is required to preserve/chain the previous logic?
If we identify specific cases, then we need to ensure we chain that actions
I don't think we need to chain any action so far

@adborden
Copy link
Contributor

Thanks @avdata99, yeah I think you're right since we're really only touching the read functions, we can get away with validate_user. But if we default to chaining it's less risky and less work. We don't need to track down each built-in auth function to review whether or not to use chaining. If we chain every time, we have our bases covered and makes it fool proof if we add additional auth functions in the future.

@jbrown-xentity
Copy link
Contributor Author

Create new users:

  • GSA admin
  • GSA editor
  • GSA member
  • different org user (admin?) DOI
  • no org user

Create dataset with certain parameters on the fly:

  • private (true/false)
  • name

@chris-macdermaid chris-macdermaid marked this pull request as ready for review February 5, 2021 19:06
Copy link
Contributor

@adborden adborden left a comment

Choose a reason for hiding this comment

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

Looks great! I left a few minor suggestions.

Now that we've dug into all the actions... Is it possible to deny package_show to anonymous users while still allowing access to resources? That way we can get rid of the private -> public upload hack.

@chris-macdermaid chris-macdermaid merged commit 4656ca6 into inventory_ckan_2.8 Feb 8, 2021
@chris-macdermaid chris-macdermaid deleted the feature/ckanext-iauth branch February 8, 2021 15:45
adborden added a commit that referenced this pull request Feb 8, 2021
Bad merge with #200

- add the datagov_inventory to plugins in development
- comment out ckan.resource_formats to match production
- set root logger to DEBUG level output
- update auth settings from datagov_inventory work
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.

5 participants