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

feat(Proof): improve the permission strategy #603

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

raphael0202
Copy link
Contributor

@raphael0202 raphael0202 commented Dec 5, 2024

  • allow anonymous proof upload (a new proof.anonymous field tracks which proof was uploaded by an anonymous user)
  • allow to retrieve and list all proofs, irrespective of your authentication status
  • allow users to add a price to a proof they don't own
  • allow moderators to update/delete a proof

Tracking history for the proof table is meant to be made in a future PR

- allow anonymous proof upload (a new proof.anonymous field tracks
  which proof was uploaded by an anonymous user)
- allow to retrieve and list all proofs, irrespective of your
  authentication status
- allow users to add a price to a proof they don't own
- allow moderators to update/delete a proof

Tracking history for the proof table is meant to be made in a future PR
@raphael0202 raphael0202 requested a review from raphodn December 5, 2024 17:37
@github-actions github-actions bot added the Proofs label Dec 5, 2024
@raphodn raphodn changed the title feat: improve the permission strategy related to proof feat(Proof): improve the permission strategy Dec 5, 2024
Copy link
Member

@raphodn raphodn left a comment

Choose a reason for hiding this comment

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

Sorry but I think this PR should be split into multiple PRs, too many changes in one go it's hard to review and understand which line does what 😓

I'll start opening PRs in parallel 🙏

@@ -463,12 +463,6 @@ def clean(self, *args, **kwargs):
)

if proof:
if proof.owner != self.owner:
Copy link
Member

@raphodn raphodn Dec 5, 2024

Choose a reason for hiding this comment

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

we should not allow users to add prices on proofs they do not own such as RECEIPTs or GDPR_REQUESTs or SHOP_IMPORTs. It should only be possible for PRICE_TAGs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True!

open_prices/api/proofs/views.py Show resolved Hide resolved
@@ -33,9 +33,14 @@ class Meta:
verbose_name = "User"
verbose_name_plural = "Users"

@property
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raphodn This is important addition, there was a bug here, because we just checked that the User instance had a is_authenticated method when doing if user.is_authenticated (which is always true)

Copy link
Member

@raphodn raphodn Dec 6, 2024

Choose a reason for hiding this comment

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

this requires a dedicated "fix" PR 🙏
and to be tested, because it's probably here for a reason ? (that I can't remember, most likely because of our custom User model & auth) 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method is here because we use it during proof upload. It was a hidden bug, as we used to block all non authenticated requests (but not anymore), so the user was always authenticated.

@raphodn raphodn marked this pull request as draft December 6, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

2 participants