feat(api): Document add+remove member from team #50900
Merged
Conversation
This file contains 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
github-actions
bot
added
the
Scope: Backend
schew2381
force-pushed
the
seiji/feat/add-member-to-team-docs-update
branch
from
a0b1d06
to
d3a120f
Compare
schew2381
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #50900 +/- ##
==========================================
+ Coverage 79.62% 79.64% +0.01%
==========================================
Files 4982 4986 +4
Lines 211081 211434 +353
Branches 35960 36040 +80
==========================================
+ Hits 168082 168400 +318
- Misses 37831 37849 +18
- Partials 5168 5185 +17
|
schew2381
force-pushed
the
seiji/feat/add-member-to-team-docs-update
branch
from
f37c1cf
to
c32e0b8
Compare
| Note that the permission scopes also vary depending on the organization setting `"Open | ||
| Membership"` and the type of authorization token. The following table outlines the accepted | ||
| scopes. | ||
| <table style="width: 100%;"> |
There was a problem hiding this comment.
Do you think this can be used by other APIs as well? If so can you create a util method that would generate the html from a python map or something? Ideally can this be part of extend_schema?
There was a problem hiding this comment.
I don't think this is extensible to other APIs for now, probably just the ones in this file b/c they all involve Team Admin
dashed
approved these changes
Aug 4, 2023
cathteng
reviewed
Aug 8, 2023
| @region_silo_endpoint | ||
| class OrganizationMemberTeamDetailsEndpoint(OrganizationMemberEndpoint): | ||
| public = {"DELETE", "POST"} |
There was a problem hiding this comment.
just for my own sake -- how are we determining what is public? what is currently public?
There was a problem hiding this comment.
We're doing APIs that are most requested/used by our customers first in phases that you can fine in this notion doc. The delete+post are phase 1 endpoints
Comment on lines
+181
to
+192
| # We require a third Team Response TypedDict that inherits like so: | ||
| # TeamSerializerResponse | ||
| # * BaseTeamSerializerResponse | ||
| # * _TeamSerializerResponseOptional | ||
| # instead of having this inheritance: | ||
| # BaseTeamSerializerResponse | ||
| # * _TeamSerializerResponseOptional | ||
| # b/c of how drf-spectacular builds schema using @extend_schema. When specifying a DRF serializer | ||
| # as a response, the schema will include all optional fields even if the response body for that | ||
| # request never includes those fields. There is no way to have a single serializer that we can | ||
| # manipulate to exclude optional fields at will, so we need two separate serializers where one | ||
| # returns the base response fields, and the other returns the combined base+optional response fields |
There was a problem hiding this comment.
does _TeamSerializerResponseOptional exist? or is this just a placeholder to allow for optional fields in TeamSerializerResponse?
There was a problem hiding this comment.
Yes it exists here. We put optional fields in there that may not be set
schew2381
added a commit
that referenced
this pull request
Aug 8, 2023
## Duplicate Functionality There is another endpoint called `OrganizationMemberTeamDetailsEndpoint` that can also update a single member's Team Role like the endpoint this PR documents. This is the API that's called when you're actually on the `Team` tab pictured below. <img width="833" alt="Screenshot 2023-08-04 at 2 12 11 PM" src="https://github.com/getsentry/sentry/assets/67301797/d4252e6b-dfda-4e47-9b3a-501c34aeb261"> I decided to not document the duplicate endpoint because it runs into the same permission scopes issue as #50900 with regards to Team Admins. I also wanted to avoid 2 endpoints that do the same thing in our documentation. Unfortunately, this means that Team Admins that are regular org members cannot change roles for the teams they're on. I don't believe this is a huge issue because Team Admins do this through the UI, and changing team roles is not as common as adding/removing team members. ## Endpoint Title This is titled `Update an Organization Member's Roles` instead of `Update an Organization Member` because the only other thing this endpoint can do is resend an invite to an invited member who hasn't joined yet. I don't believe this is necessary to document, and including `roles` in the source makes the endpoint much easier to find.  ## Garbage in Response Unfortunately we interact with roles on the frontend by piping them through API responses. This leads to extremely long responses where the roles by themselves make up ~80% of body. Ideally we would add role information to the frontend, but that is a very difficult problem. Because the frontend expects roles, the `TypedDict` return type for the PUT response serializer cannot have these roles as optional. The example response is forced to include them or else the example validator errors. Below is what the roles look like in the example (very little is shown).  
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Requires #53364 + #54148
Background
Sentry has 2 types of tokens to hit our API, user and organization auth tokens. The organization tokens are also commonly referred to as integration tokens because you can only create them through the
Integrationflow (see here for walkthrough).User Auth Tokens
User auth tokens are tied to a Sentry user account, not an organization. You can create a single user token that can be used for multiple organizations here. User tokens can have non-hierarchical scopes (eg:
org:writedoesn't haveorg:readbecause the UI allows you to select individual scopes)When hitting an endpoint, we take the intersection of the user token's scope and the scopes allotted to the user's organization role. If any scope in this intersection is allowed to access the endpoint, we allow it to proceed. For example, if a regular organization member uses a user token with (
org:read,org:admin), the intersection will only include (org:admin).Organization Auth Tokens
Organization tokens are tied to a single organization. They can also have have non-hierarchical scopes but only when created through a non-public API. The UI only allows you to create scopes with hierarchy (selecting
adminincludesorg:readandorg:write)When hitting an endpoint, we compare the exact scopes on the token without any extra logic
Complications
The permissions scopes for both endpoints are complicated because they depend on the
Open Membershipsetting, the type of token, and the role of the user. The endpoint logic and documentation is tedious especially with user auth tokens which contain many edge cases. Additionally, we don't document which roles have which permissions, only what actions are allowed per role.Bugs - Fixed in separate PR #53364 + #54148
Open Membershipwas disabled, we allowed org tokens withorg:readto still add members to teamsteam:writeteam:adminper our scopes documentation and also allowed the above to occur (withteam:adminoverteam:write)Edge Cases
org:readw/ user tokenteam:write, but they don't have that permission on an org level so they can't even hit the endpoint with justteam:writeb/c their user auth token is deniedorg:readto make it past the base permission check, andteam:writeto pass the endpoint logicteam:write, but they should only be allowed delete members from teams they are a part of. This makes the second bugfix above more difficult, b/c we cannot simply check forteam:writeon the user token.Org Adminrole complicates things. The role hasteam:writeon an org-level, but we restrict some team actions depending on whether or not they are part of the team.Add member
Delete member