🔒️(back) restrict accesss to document accesses #801
Conversation
lunika
force-pushed
the
refactor/docs-accesses
branch
from
ffd4673 to
f242a57
Compare
| try: | ||
| document = models.Document.objects.get(pk=self.kwargs["resource_id"]) | ||
| except models.Document.DoesNotExist: | ||
| return queryset.none() |
There was a problem hiding this comment.
Isn't that weird the document is not always checked? Shouldn't we return 404 in those case (more global than just the list).
No change required in this PR anyway (because I guess it would have too much impact).
There was a problem hiding this comment.
I totally agree with you. I discover this while working on this PR. IMO it has been done this way to avoid one more query to test if the Document exists or not.
I wrote a test proving that we never have a 404, we should probably do something on this subject.
| self.is_current_user_owner_or_admin = is_owner_or_admin | ||
| if not is_owner_or_admin: | ||
| # Return only the document owner access | ||
| queryset = queryset.filter(role__in=models.PRIVILEGED_ROLES) |
There was a problem hiding this comment.
Do we really need to display those roles to readers? Should we display only the owner?
There was a problem hiding this comment.
I'm open on this suggestion. In a first time I limited only to owners. But as administrator have the same permissions I made the choice to also return both owners and administrators.
Every user having an access to a document, no matter its role have access to the entire accesses list with all the user details. Only owner or admin should be able to have the entire list, for the other roles, they have access to the list containing only owner and administrator with less information on the username. The email and its id is removed
lunika
force-pushed
the
refactor/docs-accesses
branch
from
40b433d to
12ffc25
Compare
Purpose
Every user having an access to a document, no matter its role have access to the entire accesses list with all the user details. Only owner or admin should be able to have the entire list, for the other roles, they have access to the list containing only owner and administrator with less information on the username. The email and its id is removed
Proposal