feat: add guards to stripe teams controller

[SomayChauhan]

What does this PR do?

• Apply the team guards for stripe controllers when installing it on a team. Currently, anyone can install them—restrict this to admins and owners only.

• separate org/team endpoints from user endpoints, instead of passing them as a query -- created a separate organizations/stripe-controller and service

• Fixes #XXXX (GitHub issue number)

• Fixes CAL-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

Visual Demo (For contributors especially)

A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).

Video Demo (if applicable):

• Show screen recordings of the issue or feature.
• Demonstrate how to reproduce the issue, the behavior before and after the change.

Image Demo (if applicable):

• Add side-by-side screenshots of the original and updated change.
• Highlight any significant change(s).

Mandatory Tasks (DO NOT REMOVE)

• I have self-reviewed the code (A decent size PR without self-review might be rejected).
• I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
• I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

• Are there environment variables that should be set?
• What are the minimal test data to have?
• What is expected (happy path) to have (input and output)?
• Any other important info that could help to test that PR

Checklist

• I haven't read the contributing guide
• My code doesn't follow the style guidelines of this project
• I haven't commented my code, particularly in hard-to-understand areas
• I haven't checked if my changes generate no new warnings

[graphite-app]

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (04/04/25)

1 reviewer was added to this PR based on Keith Williams's automation.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
cal	⬜️ Ignored (Inspect)	Visit Preview		Apr 8, 2025 8:48am
calcom-web-canary	⬜️ Ignored (Inspect)	Visit Preview		Apr 8, 2025 8:48am

[supalarry]

I don't know how and why but we have ended up with a huge organizations.module.ts. Can you create a new organizations-stripe.module.ts where you setup an isolated module that then is imported into the organizations.module.ts just like with organizations-organizations.module.ts.

[supalarry]

I think we should move this type to stripe.service.ts aka the very root of where the structure of this type is actually "defined".

[supalarry]

Hmm, not 100% sure what and why but StripeController importing OrganizationsStripeService makes me think something could be restructured - can StripeController import only StripeService and then strip organization controller and service re-uses them so we have core service and then org ones that wrap around them? is that possible?

I see we have // If we have an orgId, this is an organization-level operation below but then I have a question - why / when would an org request end up in stripe.controller not organizations one? Is it 100% not possible to have org logic in org controller only so that we also don't have to use forwardRef below?

[SomayChauhan]

The reason for this is that both regular users and teams/orgs are redirected to the same URL from stripe.
Since Stripe does not support dynamic redirect URLs like api/v2/organizations/:orgId/teams/:teamId/stripe/save, we use a single shared redirect URL for both cases. and pass the teamId as query parameters in OAuthCallbackState.

[supalarry]

The code from line 95 to 130 should be moved to stripe service - it's too much code for a controller - controllers should only extract information, pass it to service and then return value of the service.

[supalarry]

If we have save stripe account on the org stripe service, can we get rid or teamId logic here? Shouldn't it be that since its for individual user there is no trace / mention of team stuff?

[supalarry]

I see there is teamId within this function but I find it hard to reason - we are trying to split team with individual logic but somehow there is still team logic here.

I think there should be separate function saveTeamStripeAccount that has teamId as mandatory and this one saveStripeAccount has teamId removed - if a function has an optional parameter then 99% of time it is an indication that is should be split into individual functions.

[SomayChauhan]

#20540 (comment)

[supalarry]

Hmm, can we move the verifyAccess to a separate service? Something like OrgsPlanService or something or PlatformSubscriptionService - it looks weird that organizations stripe service is calling conferencing service.

[SomayChauhan]

867e088

[supalarry]

Now that I think more I have a question - why do we need this service and why can't we rely on already existing platform-plan.guard.ts ? This service is called by OrganizationsStripeService and OrganizationsConferencingService - why can't we get rid of this service and rely on guards that run before OrganizationsStripeService and OrganizationsConferencingService are run in the first place?

[SomayChauhan]

Here’s how the Stripe Connect flow works:

Endpoint Invocation:

For individual users, we call the /v2/stripe/connect endpoint in stripe.controller.

For teams, we use the /v2/organizations/:orgId/teams/:teamId/stripe/connect endpoint defined in organizations-stripe.controller.

Access Control:

The organizations-stripe.controller has guards in place (reference) to ensure that only team admins or owners can access the team-specific endpoint.

OAuth Callback:

After the user signs into their Stripe account, Stripe redirects back to a common callback endpoint for both users and teams (see callback handler).

In this handler, we inspect the OAuthCallbackState. If it contains orgId and teamId, we route the request to organizationsStripeService; otherwise, it goes to stripeService.
We can't add the guards directly to the callback endpoint because we only want them to be applied when both orgId and teamId are present. Therefore, I implemented the guard logic inside the organizationsStripeService function instead.

Extra Validation:

While the platformSubscriptionService.verifyAccess check in organizationsStripeService might seem redundant—since access control is already enforced at the URL creation step—we included it as an additional safeguard, in case someone tampers with the callback URL manually.

That said, it’s not strictly necessary and could be removed.

[supalarry]

Gotcha, I am now thinking - what do you think if in the common callback endpoint we check decodedCallbackState and if it is there we manually make request to a newly added organizations stripe save endpoint?

import { HttpService } from '@nestjs/common';
...
constructor(private readonly httpService: HttpService) {}
...
if (decodedCallbackState.orgId) {
    const {orgId, teamId} = decodedCallbackState;
    return this.httpservice.post(`/organizations/${orgId}/teams/${teamId}/stripe/save`, body (or whatever we need to pass));
}

I am thinking about code maintainability in the future and I think it's best if we would have this while still adressing my previous comment and comment you quote replied to. Yesterday we talked about custom middleware which might complicate and hide implementation details too much but I think a simple request re-routing like this would be great - it would allow us to completely either split stripe specific functions just for org / user and let stripe org service re-use stripe service and also will allow us not copying logic of endpoint guards.