API: use restricted serializer for related projects #11820
Conversation
readthedocs/api/v3/serializers.py
Outdated
| model = Project | ||
| fields = [ | ||
| "id", | ||
| "name", |
There was a problem hiding this comment.
I wonder if we should exclude the name from restricted objects as well (restricted organization is the other serializer like this)
| @@ -736,6 +736,28 @@ def get_admin(self, obj): | |||
| return AdminPermission.is_admin(user, obj) | |||
|
|
|||
|
|
|||
| class RestrictedProjectSerializer(serializers.ModelSerializer): | |||
There was a problem hiding this comment.
I'd prefer if we name this UnAuthedProjectSerializer or AnonymousProjectSerializer which is more explicit when this serializer is going to be used.
| Stripped version of the ProjectSerializer to be used when including related projects. | ||
|
|
||
| This serializer is used to avoid leaking information about a private project through | ||
| a public project. Instead of checking if user has access to the project, | ||
| we just show the name and slug. |
There was a problem hiding this comment.
In our meeting we talked about differentiating authed from un-authend/anonymous and based on the permissions used the full serializer or the un-authed one. Also, we talked about removing only those sensitive fields, like repo, instead of removing almost everything.
I want to keep as much consistency as possible in the returned objects here to match APIv3 responses.
We are planning to remove all these objects from the Addons API response in readthedocs/addons#356 and make usage of the APIv3 directly. We will need these ProjectSerializer and AnonymousProjectSerializer there as well.
There was a problem hiding this comment.
That was about addons in the case where we are leaking the project information if a project is private, but has a public version. This case is about the translation_of/subproject_of fields that are returned with the project, if the related projects are private, but the parent project is public.
There was a problem hiding this comment.
In this implementation if the parent project is public and the related project is public, we are going to strip all the data as well. That's what I want to avoid.
There was a problem hiding this comment.
In any case, this code is temporal and is going to be removed since the Addons API won't return this data anymore and everything will be retrieved using APIv3 in readthedocs/addons#356. In those endpoints, we will implement auth/anonymous serializers. Do we agree on that?
There was a problem hiding this comment.
this code is temporal and is going to be removed since the Addons API won't return this data anymore and everything will be retrieved using APIv3
Yeah, but this is still a problem for the projects/ endpoint of API v3 where we are still leaking that information.
There was a problem hiding this comment.
You didn't reply to the last part of my comment:
In those endpoints, we will implement auth/anonymous serializers. Do we agree on that?
Is that correct? We will those serializers there eventually?
There was a problem hiding this comment.
For the detail views maybe (if they are really needed by addons), but this is for a related field. We can also check for authz here, but that introduces two extra calls for each object, and that will be leaking authz checks into the serializer.
There was a problem hiding this comment.
For the detail views maybe (if they are really needed by addons),
OK, yeah, we will require the permission checks there 👍🏼
but this is for a related field. We can also check for authz here, but that introduces two extra calls for each object,
Addons will require a few fields on these related objects as well; in particular documentation urls and API links, which I understand you are keeping those in the serializers here. We can probably perform another request to the detail view if it needs more extra data or re-consider adding more fields here.
and that will be leaking authz checks into the serializer.
What "leaking authz checks" means?
There was a problem hiding this comment.
We can probably perform another request to the detail view if it needs more extra data or re-consider adding more fields here.
Yeah, that's my idea, just expose the necessary information to avoid doing a separate call /projects/<project>/main-translation/, but also don't expose everything, just enough to query the whole object if needed (the slug in this case).
What "leaking authz checks" means?
Serializers just render an object to json, they shouldn't have authorization checks, those should be done before rendering the object.
Ref https://github.com/readthedocs/readthedocs-corporate/issues/1845