wip: Adapting AuthIdentityHandler to HC services #41821
Conversation
github-actions
bot
added
the
Scope: Backend
| class EmailService(InterfaceWithLifecycle): | ||
| def resolve_email_to_user( | ||
| self, email: str, organization: ApiOrganization | None = None | ||
| ) -> APIUser | None: |
There was a problem hiding this comment.
I'm experimentally trying something a little different with this service interface, as previously discussed with @corps.
With the other HC services, the set of abstract methods are also the public contract. Here, resolve_email_to_user is the only method we need to expose in the public contract; the two abstract methods are implementation details that it uses. That is, in Java-speak, get_user_emails and get_members_for_users could be considered "protected abstract" methods, while resolve_email_to_user is "public final".
| def get_members_for_users( | ||
| self, *, organization: ApiOrganization, users: Collection[APIUser] | ||
| ) -> Iterable[ApiOrganizationMember]: | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Resolved from sync discussion: move this to OrganizationService.
There was a problem hiding this comment.
ApiOrganizationMember will be unavailable in control silo. Would need to replace with minimal member mapping when it exists. Or, if we're interested only in existence of membership, just return a subset of the users arg.
| from sentry.services.hybrid_cloud.user import user_service | ||
|
|
||
| user = user_service.get_user(user_id=organization_member.user_id) | ||
| teams = self.get_teams(mt.team_id for mt in organization_member.member_teams) |
There was a problem hiding this comment.
To do: See if this is redundant to an existing service. Note that we already have an org ID.
| @@ -82,6 +85,33 @@ def get_organization_by_slug( | |||
|
|
|||
| return self.get_organization_by_id(id=org_id, user_id=user_id) | |||
|
|
|||
| @abstractmethod | |||
| def get_teams(self, team_ids: Iterable[int]) -> List[ApiTeam]: | |||
| pass | |||
There was a problem hiding this comment.
This interface needs to ensure that the teams come from only one region.
|
Now is a good time to introduce a speculative version of the organization member model as it will exist on the control silo (minimal mapping). Don't commit to a schema yet, just a placeholder dataclass. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Work in progress -- the new method works as intended but it regresses other stuff in AuthHelper as it's now getting an ApiAuthIdentity where AuthIdentity is expected.
src/sentry/pipeline/base.py
Outdated
| @@ -94,7 +97,7 @@ def __init__( | |||
| self, | |||
| request: Request, | |||
| provider_key: str, | |||
| organization: Organization | None = None, | |||
| organization: ApiOrganization | None = None, | |||
There was a problem hiding this comment.
Will need to see what is the blast radius on this change and whether it makes sense to try to convert all Pipeline subclasses at once. It doesn't seem to break a lot immediately, but that's probably only because the subclasses lack Mypy coverage.
As an alternative, it probably would work to type it as Organization | ApiOrganization | None temporarily.
|
Simplified and pulled into #43035 |
Broad work in progress, centered around adapting
sentry/auth/helper.pyto the Hybrid Cloud service interface system. There are a few chunks that I'll probably want to separate out; two major ones are:email.py, formerly a utility module, into a service centered around theUserEmailmodel.OrganizationService.