Skip to content

Commit b32ba9f

Browse files
authored
fix: exchange token authz, remove n+1 tenant lookup for organizations, slack oauth casing (#3631)
* fix: remove n+1 tenant lookup for organizations * fix build * remove loading state * fix: exchange tokens should not case on restrictedWithBearerToken * add slack webhook sync * Revert "add slack webhook sync" This reverts commit 42557ae. * make security on list integrations endpoint optional * case slack alert linking on control plane * update api generation * fix: exchange token PAT behavior
1 parent 92a3779 commit b32ba9f

5 files changed

Lines changed: 42 additions & 51 deletions

File tree

api/v1/server/authn/middleware.go

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -213,16 +213,7 @@ func (a *AuthN) handleCookieAuth(c echo.Context) error {
213213
func (a *AuthN) handleBearerAuth(c echo.Context) error {
214214
forbidden := echo.NewHTTPError(http.StatusForbidden, "Please provide valid credentials")
215215

216-
// a tenant id must exist in the context in order for the bearer auth to succeed, since
217-
// these tokens are tenant-scoped
218216
ctx := c.Request().Context()
219-
queriedTenant, ok := c.Get("tenant").(*sqlcv1.Tenant)
220-
221-
if !ok {
222-
a.l.Debug().Ctx(ctx).Msgf("tenant not found in context")
223-
224-
return fmt.Errorf("tenant not found in context")
225-
}
226217

227218
token, isExchangeToken, err := getBearerTokenFromRequest(c.Request())
228219

@@ -232,6 +223,8 @@ func (a *AuthN) handleBearerAuth(c echo.Context) error {
232223
return forbidden
233224
}
234225

226+
queriedTenant, isTenantScoped := c.Get("tenant").(*sqlcv1.Tenant)
227+
235228
if isExchangeToken {
236229
if a.config.Auth.ExchangeTokenClient == nil {
237230
a.l.Error().Msgf("exchange token client is not configured")
@@ -247,7 +240,9 @@ func (a *AuthN) handleBearerAuth(c echo.Context) error {
247240
return forbidden
248241
}
249242

250-
if *tenantId != queriedTenant.ID {
243+
// we permit exchange token auth if the token is valid and represents a user if the endpoint is not tenant-scoped, because
244+
// this is effectively a PAT without the tenant scoping
245+
if isTenantScoped && *tenantId != queriedTenant.ID {
251246
a.l.Error().Msgf("tenant id in token does not match tenant id in context")
252247

253248
return forbidden
@@ -284,6 +279,13 @@ func (a *AuthN) handleBearerAuth(c echo.Context) error {
284279
return nil
285280
}
286281

282+
// If we've reached this point, and it's not tenant-scoped, this endpoint is forbidden
283+
if !isTenantScoped {
284+
a.l.Debug().Ctx(ctx).Msgf("bearer token auth attempted on non-tenant-scoped endpoint")
285+
286+
return forbidden
287+
}
288+
287289
// Validate the token.
288290
tenantId, tokenUUID, err := a.config.Auth.JWTManager.ValidateTenantToken(c.Request().Context(), token)
289291

api/v1/server/authz/middleware.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,8 @@ var restrictedWithBearerToken = []string{
9292
// and we check that the bearer token has access to the tenant in the authn step.
9393
func (a *AuthZ) handleBearerAuth(c echo.Context, r *middleware.RouteInfo) error {
9494
// check for is_exchange_token set in the context, in which case we need to validate the user set in the context
95+
// exchange tokens are subject to the same RBAC restrictions as cookie auth, since they represent a user. only
96+
// regular bearer tokens should be subject to the additional restrictions in restrictedWithBearerToken
9597
if isExchangeToken, ok := c.Get(middleware.IsExchangeTokenContextKey).(bool); ok && isExchangeToken {
9698
if a.config.Auth.ExchangeTokenClient == nil {
9799
a.l.Error().Msgf("exchange token client is not configured, but is_exchange_token is set in context")
@@ -107,10 +109,10 @@ func (a *AuthZ) handleBearerAuth(c echo.Context, r *middleware.RouteInfo) error
107109
a.l.Debug().Ctx(c.Request().Context()).Err(err).Msgf("error validating user tenant permissions for exchange token user")
108110
return echo.NewHTTPError(http.StatusUnauthorized, "Not authorized to view this resource")
109111
}
110-
}
111-
112-
if rbac.OperationIn(r.OperationID, restrictedWithBearerToken) {
113-
return echo.NewHTTPError(http.StatusUnauthorized, "Not authorized to perform this operation")
112+
} else if !isExchangeToken {
113+
if rbac.OperationIn(r.OperationID, restrictedWithBearerToken) {
114+
return echo.NewHTTPError(http.StatusUnauthorized, "Not authorized to perform this operation")
115+
}
114116
}
115117

116118
return nil

frontend/app/src/lib/api/generated/control-plane/data-contracts.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,10 @@ export interface OrganizationTenant {
126126
* @format uuid
127127
*/
128128
id: string;
129+
/** Name of the tenant */
130+
name?: string;
131+
/** Slug of the tenant */
132+
slug?: string;
129133
/** Status of the tenant */
130134
status: TenantStatusType;
131135
/**

frontend/app/src/pages/main/v1/tenant-settings/alerting/index.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { SimpleTable } from '@/components/v1/molecules/simple-table/simple-table
1313
import { Button } from '@/components/v1/ui/button';
1414
import { Spinner } from '@/components/v1/ui/loading';
1515
import { Separator } from '@/components/v1/ui/separator';
16+
import useControlPlane from '@/hooks/use-control-plane';
1617
import { useCurrentTenantId, useTenantDetails } from '@/hooks/use-tenant';
1718
import api, {
1819
CreateTenantAlertEmailGroupRequest,
@@ -311,6 +312,7 @@ function DeleteEmailGroup({
311312

312313
function SlackWebhooksList() {
313314
const { tenantId } = useCurrentTenantId();
315+
const { isControlPlaneEnabled } = useControlPlane();
314316
const [deleteSlack, setDeleteSlack] = useState<SlackWebhook | null>(null);
315317

316318
const listWebhooksQuery = useQuery({
@@ -356,7 +358,13 @@ function SlackWebhooksList() {
356358
<h3 className="text-xl font-semibold leading-tight text-foreground">
357359
Slack Webhooks
358360
</h3>
359-
<a href={'/api/v1/tenants/' + tenantId + '/slack/start'}>
361+
<a
362+
href={
363+
isControlPlaneEnabled
364+
? '/api/v1/control-plane/tenants/' + tenantId + '/slack/start'
365+
: '/api/v1/tenants/' + tenantId + '/slack/start'
366+
}
367+
>
360368
<Button key="create-slack-webhook">Add Slack Webhook</Button>
361369
</a>
362370
</div>

frontend/app/src/pages/organizations/$organization/index.tsx

Lines changed: 11 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import {
2323
} from '@/components/v1/ui/tooltip';
2424
import { useCurrentUser } from '@/hooks/use-current-user';
2525
import { useOrganizations } from '@/hooks/use-organizations';
26-
import api from '@/lib/api';
2726
import {
2827
OrganizationMember,
2928
ManagementToken,
@@ -50,7 +49,7 @@ import {
5049
ArrowRightIcon,
5150
} from '@heroicons/react/24/outline';
5251
import { EllipsisVerticalIcon, TrashIcon } from '@heroicons/react/24/outline';
53-
import { useQuery, useQueries, useQueryClient } from '@tanstack/react-query';
52+
import { useQuery, useQueryClient } from '@tanstack/react-query';
5453
import { useParams, useNavigate } from '@tanstack/react-router';
5554
import { isAxiosError } from 'axios';
5655
import { formatDistanceToNow } from 'date-fns';
@@ -145,25 +144,6 @@ export default function OrganizationPage() {
145144
enabled: !!orgId,
146145
});
147146

148-
const tenantQueries = useQueries({
149-
queries: (organizationQuery.data?.tenants || [])
150-
.filter((tenant) => tenant.status !== TenantStatusType.ARCHIVED)
151-
.map((tenant) => ({
152-
queryKey: ['tenant:get', tenant.id],
153-
queryFn: async () => {
154-
const result = await api.tenantGet(tenant.id);
155-
return result.data;
156-
},
157-
enabled: !!tenant.id && !!organizationQuery.data,
158-
})),
159-
});
160-
161-
const tenantsLoading = tenantQueries.some((query) => query.isLoading);
162-
163-
const detailedTenants = tenantQueries
164-
.filter((query) => query.data)
165-
.map((query) => query.data);
166-
167147
const managementTokensQuery = useQuery({
168148
...orgApi.managementTokenListQuery(orgId),
169149
enabled: !!orgId,
@@ -233,10 +213,7 @@ export default function OrganizationPage() {
233213
cellRenderer: (
234214
row: OrganizationTenant & { metadata: { id: string } },
235215
) => {
236-
const detailed = detailedTenants.find((t) => t?.metadata.id === row.id);
237-
return (
238-
<span className="font-medium">{detailed?.name || 'Loading...'}</span>
239-
);
216+
return <span className="font-medium">{row.name || row.id}</span>;
240217
},
241218
},
242219
{
@@ -255,10 +232,7 @@ export default function OrganizationPage() {
255232
cellRenderer: (
256233
row: OrganizationTenant & { metadata: { id: string } },
257234
) => {
258-
const detailed = detailedTenants.find((t) => t?.metadata.id === row.id);
259-
return (
260-
<span className="text-muted-foreground">{detailed?.slug || '-'}</span>
261-
);
235+
return <span className="text-muted-foreground">{row.slug || '-'}</span>;
262236
},
263237
},
264238
{
@@ -578,11 +552,7 @@ export default function OrganizationPage() {
578552
<div className="flex-1 overflow-y-auto px-8">
579553
{activeSection === 'tenants' && (
580554
<>
581-
{tenantsLoading ? (
582-
<div className="flex items-center justify-center py-8">
583-
<Loading />
584-
</div>
585-
) : organization.tenants && organization.tenants.length > 0 ? (
555+
{organization.tenants && organization.tenants.length > 0 ? (
586556
<SimpleTable
587557
data={organization.tenants
588558
.filter(
@@ -738,8 +708,13 @@ export default function OrganizationPage() {
738708

739709
{(() => {
740710
const foundTenant = tenantToArchive
741-
? detailedTenants.find((t) => t?.metadata.id === tenantToArchive.id)
711+
? organization.tenants?.find((t) => t?.id === tenantToArchive.id)
742712
: undefined;
713+
714+
if (!foundTenant) {
715+
return null;
716+
}
717+
743718
return (
744719
tenantToArchive &&
745720
organization &&
@@ -748,7 +723,7 @@ export default function OrganizationPage() {
748723
open={!!tenantToArchive}
749724
onOpenChange={(open) => !open && setTenantToArchive(null)}
750725
tenant={tenantToArchive}
751-
tenantName={foundTenant.name}
726+
tenantName={foundTenant.name || foundTenant.id}
752727
organizationName={organization.name}
753728
onSuccess={() => {
754729
queryClient.invalidateQueries({

0 commit comments

Comments
 (0)