Advanced shared feature #383
Open
Conversation
This file contains hidden or 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
lunika
force-pushed
the
refactor/advanced-shared
branch
from
72f806d to
d1dee97
Compare
lunika
commented
Oct 30, 2025
|
|
||
| class AccessPermission(permissions.BasePermission): | ||
| class ItemAccessPermission(IsAuthenticated): | ||
| """Permission class for access objects.""" |
Member
Author
There was a problem hiding this comment.
Update docstring, not accurate anymore.
| class BaseAccessSerializer(serializers.ModelSerializer): | ||
| """Serialize template accesses.""" | ||
| class ItemLightSerializer(serializers.ModelSerializer): | ||
| """Minial item serializer for nesting in document accesses.""" |
Member
Author
There was a problem hiding this comment.
Suggested change
| """Minial item serializer for nesting in document accesses.""" | |
| """Minial item serializer for nesting in item accesses.""" |
Member
There was a problem hiding this comment.
Suggested change
| """Minial item serializer for nesting in document accesses.""" | |
| """Minimal item serializer for nesting in item accesses.""" |
lunika
force-pushed
the
refactor/advanced-shared
branch
2 times, most recently
from
7a13f24 to
df3ec69
Compare
qbey
reviewed
Nov 12, 2025
| class BaseAccessSerializer(serializers.ModelSerializer): | ||
| """Serialize template accesses.""" | ||
| class ItemLightSerializer(serializers.ModelSerializer): | ||
| """Minial item serializer for nesting in document accesses.""" |
Member
There was a problem hiding this comment.
Suggested change
| """Minial item serializer for nesting in document accesses.""" | |
| """Minimal item serializer for nesting in item accesses.""" |
Comment on lines
+548
to
+579
| if not link_reach: | ||
| raise serializers.ValidationError( | ||
| {"link_reach": _("This field is required.")} | ||
| ) |
Member
There was a problem hiding this comment.
How would it be possible to reach this? Are not the fields validated before?
|
|
||
| if not link_reach: | ||
| raise serializers.ValidationError( | ||
| {"link_reach": _("This field is required.")} |
Member
There was a problem hiding this comment.
Other validation errors are not translated, I guess the frontend never use the values from the backend, maybe no translation needed here.
| if request: | ||
| return item.get_roles(request.user) | ||
| return item.get_role(request.user) | ||
| return [] |
Member
There was a problem hiding this comment.
Is there a type inconsistency here? get_user_role should not return a list I guess
Comment on lines
+1196
to
+1258
| request_target_keys = {f"user:{user.id}"} | ||
| request_target_keys.update( | ||
| f"team:{team}" for team in getattr(user, "teams", []) | ||
| ) |
We need version 0.0.17 to use new PriorityTextChoices for Role, LinkRoke and LinkReach
The choices RoleChoices, LinkReachChoices and LinkRoleChoices are moved in the djano-lasuite project, we can remove them from drive and use them from the library.
We need to compute the abilities based on ancestors link_reach and link_role. Gor this we added methods on the Item model computing them and storing them in a local cache to not compute them again and again in the same request.
We want to display on an item the ancestors and computed links (reach and role) as inherited from its ancestors.
The methods to annotate an item queryset were factorized on the viewset but the correct place is the custom queryset itself now that we have one.
Onlyt the ItemAccessViewset was using it, we remove it and copy in the ItemAccessViewset all the methods.
Some permissions were made in the BaseAccessSerializer. Permissions should not occur in serializers. We refactor this, moving permission check in permissions classes but also in the ItemAccessViewset. Also, there is no need to keep the BaseAccessPermission
There is a delay between the time the signature is issued and the time it is checked. Although this delay is minimal, if the signature is issued at the end of a second, both timestamps can differ of 1s.
The itemAccessViewset.get_queryset was the one used by the, now removed, ResourceAccessViewsetMixin. We don't need it's complexity anymore and it can be refactor to be clearer. The list function is added in order to complexify it later.
The BaseAccess model was only used by the ItemAccess model. We remove it in order to refactor later the get_abilities method.
We need to manage inheritence on item accesses. We backport all the work made on the docs project. To list accesses, we look for all accesses on all the item ancestors and the item itself.
The max_ancestors_role was only computes in the ItemAccessViewSet.list method to avoid having to compute it multiple times in the list and then in the serializer. The result is save in the model instance has a cache, but if not present whe computing the abilities outside the list method, then the expected abilities are wrong. The set_role_to can't be computed without it. So we search for the max_ancestors_role in the class itself if not set.
when an explicit access is creating and an other explicit access exists higher in the tree, then we must validate the role and ensure this new role is strictly higher than previous explicit accesses.
Ensure that it is not possible to set a role lower than existing on other accesses preset higher in the hierarchy tree.
When an access is created or updated, we want to syncronize descendants accesses to remove ones with lower or equal roles.
We open a specific endpoint to update items link configuration because it makes it more secure and simple to limit access rights to administrators/owners whereas other item fields like title can be edited by anonymous or authenticated users with much less access rights.
We don't need the whole accesses list for all the users on the list endpoint. We want to return the last two accesses, meaning the accesses on the two items the deepest in the items tree.
We need to know, for a given if the related access is explicit or inherited. This is usefull on the list endpoint. This endpoint returns the entire list of accesses add on the entire tree, we need to know when retrieving this list if each access is explicit on the item or inherited.
The set_role_to was computed to have only roles strictly higher than the max_ancestors_role. We want this behavior only for inherited accesses. When the role is explicit, we want to allow the lower role in set_role_to equal to max_ancestors_role
When an access role is updated and the new role is equal to the access max_ancestors_role, we want to resync accesses. For this, the current access is deleted instead of updating it. We don't want to have 2 explicits accesses sharing the same role.
For a given access we are able to compute the max ancestors role. We also need the item_id related to this max_ancestors_role. We compute it both in the vewset and in the models like we already did for the max_ancestors_role
We don't need the whole accesses list for all the users on the list endpoint. We want to return the last access, meaning the access on the deepest item in the items tree.
The ItemAvvessViewset list endpoint is really complicated and half of the algo is made to cache the role the current user have on the tree. This code is now removed and replaced by a queryset annotation allowing to fetch the user_roles and then determine in the model the max role the user have on the tree. It reduces the number of sql query by 1 and ease the code comprehension
lunika
force-pushed
the
refactor/advanced-shared
branch
from
33f4cca to
ea2d10d
Compare
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.
Purpose
Manage advanced shared in its own workspace and shared workspace.
specs (in french) : https://docs.numerique.gouv.fr/docs/ab704b19-18e3-46e3-9eb1-39bb2548dd35/
https://www.figma.com/design/XDjWHDLIiNY4mtpyj21EVK/Drive?node-id=9171-8103
Proposal
Implement #827