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

Add cleaned option #3234

Merged
merged 2 commits into from
Dec 6, 2023
Merged

Add cleaned option #3234

merged 2 commits into from
Dec 6, 2023

Conversation

ksotik
Copy link
Contributor

@ksotik ksotik commented Jul 26, 2023

This option allow to disable server-side values validation for all types.
Also fix "safe" option docs and default value inverse logic.

Comment on lines 275 to 281
# This will disable (if True) values validation for all types of all entities / properties
# (will pass cleaned=True to the model.get_proxy() in the aleph/logic/processing.py)
cleaned = get_flag("cleaned", default=False)
# Flag is only available for admins:
if not request.authz.is_admin:
cleaned = False

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @ksotik, thanks for opening your first PR in the Aleph repo! :) 🎉

Could you add a test for the new flag?
https://github.com/alephdata/aleph/blob/develop/aleph/tests/test_collections_api.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

# loading of document data:
safe = get_flag("safe", default=True)
# Flag is only available for admins:
if not request.authz.is_admin:
safe = True

# This will disable (if True) values validation for all types of all entities / properties
# (will pass cleaned=True to the model.get_proxy() in the aleph/logic/processing.py)
cleaned = get_flag("cleaned", default=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly concerned about the fact that the two flags cleaned and safe do related things but behave in opposite ways when set to true:

  • cleaned=true: Assume that the data can be trusted and contains only valid values.
  • safe=true: Assume that the data can NOT be trusted, so perform additional security measures.

Not sure though if there is a better alternative and probably not a huge problem as this is mostly relevant in edge cases. A validate flag is used in some other endpoint. It’s has a slightly different behavior though (validate=true means that Aleph will return an error instead of silently discarding invalid values).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, maybe the flag could be clean instead of cleaned (and default to true)?

  • clean=true means that the data cannot be trusted and that the data should be cleaned from invalid values.
  • safe=true means that the data cannot be trusted and that file checksums should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@Rosencrantz
Copy link
Contributor

@tillprochaska Can you take another look at this, ensure that this is now up to your standards.

Copy link
Contributor

@tillprochaska tillprochaska left a comment

Choose a reason for hiding this comment

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

Really sorry this took so long, but thanks a lot for opening the PR and making these changes. I’ll also merge the respective alephclient PR.

@tillprochaska tillprochaska merged commit 0044f5b into alephdata:develop Dec 6, 2023
simonwoerpel pushed a commit to investigativedata/aleph that referenced this pull request Apr 22, 2024
* Add cleaned option

* Change cleaned=True to clean=False; add clean flag tests
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.

3 participants