API V3: avoid leaking information through expandable fields #11381
Conversation
There are still more things to do, but first I'm removing the ones that aren't documented, so this shouldn't be a breaking change, and the information from those fields can already be obtained from the main object, and if another user with lower permissions was relying on those fields, that was actually a bug. Ref readthedocs/readthedocs-corporate#1736. Expandable fields may look cool, but once you have to deal with permissions, they are a pain. Sorry, had to vent.
| # NOTE: we cannot have a Project with multiple organizations. | ||
| {"source": "organizations.first"}, | ||
| ), | ||
| # NOTE: we are leaking the slugs of all teams linked to this project | ||
| # to anyone with access to this prtoject. It's only the slug, but still. | ||
| "teams": ( |
There was a problem hiding this comment.
I think we can just have a teams method field (expanded by default, but with all the correct permissions checked) or a projects/{project}/teams/ endpoint.
There was a problem hiding this comment.
I understand it isn't possible to have permission check on expandable fields, right?
There was a problem hiding this comment.
yeah, it's hard/impossible to filter related fields of an object using expandable fields. This is since the related field doesn't know about the current user or whatever checks we need, and we can't override that.
We could implement our own expandable fields, but in a more limited way, like just for detail endpoints (implementing the checks on list views may have an impact on performance), and without recursion (just allow expanding one level).
This also reminds me of this post about graphql https://bessey.dev/blog/2024/05/24/why-im-over-graphql/
| expandable_fields = { | ||
| "projects": (ProjectSerializer, {"many": True}), | ||
| # TODO: we are leaking all teams and its members to anyone who is | ||
| # an admin member of the organization. | ||
| "teams": (TeamSerializer, {"many": True}), |
There was a problem hiding this comment.
Similar here, having another endpoint will be easier organization/{organization}/teams, similar to organization/{organization}/projects.
😂 |
There was a problem hiding this comment.
This looks fine for now 👍🏼
Expandable fields are a good feature because we can expose a lot of relevant data on the same endpoint without compromising it by making it pretty slow (some queries could perform slow) --in case we return all the same data by default.
We've been talking more about APIv3 in the last weeks since we are using it more from the new dashboard. It would be good to have a conversation around APIv3 soon, and decide a path to move forward around this and other limitations we've found:
- Expandable fields
- Allow public list endpoints without API token
- Endpoint missing fields
- Missing endpoints
- ... others
| # NOTE: we cannot have a Project with multiple organizations. | ||
| {"source": "organizations.first"}, | ||
| ), | ||
| # NOTE: we are leaking the slugs of all teams linked to this project | ||
| # to anyone with access to this prtoject. It's only the slug, but still. | ||
| "teams": ( |
There was a problem hiding this comment.
I understand it isn't possible to have permission check on expandable fields, right?
There are still more things to do, but first I'm removing the ones that aren't documented, so this shouldn't be a breaking change, and the information from those fields can already be obtained from the main object or from other endpoints, and if another user with lower permissions was relying on those fields, that was actually a bug.
Ref https://github.com/readthedocs/readthedocs-corporate/issues/1736.
Tests are on .com.
Expandable fields may look cool, but once you have to deal with permissions, they are a pain. Sorry, had to vent.