Skip to content

Commit d7dae8c

Browse files
therealbradclaude
andauthored
fix(integrations): unblock Jira OAuth authorize + align admin action icons (#463)
* fix(integrations): allow OAuth authorize on inactive integrations A new OAuth 2.0 (3LO) integration is intentionally INACTIVE until a user completes authorization, but IntegrationManager.getAdapter threw "Integration is not active" for any non-ACTIVE integration. Both the OAuth authorize and callback routes go through getAdapter, so the very flow that activates an integration could never run: clicking Authorize 500'd and the integration was stuck at "Awaiting authorization". Add an allowInactive option to getAdapter and pass it from the OAuth auth and callback routes; the ACTIVE guard still applies to every issue operation. Regression-tested at both layers — getAdapter builds an inactive adapter only when allowInactive is set, and the auth route redirects an inactive integration to the provider's authorize URL instead of 500ing (the new test fails if the guard fix is reverted). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(integrations): keep admin action icons aligned when Authorize is hidden The Authorize (shield) action only renders for unauthorized OAuth integrations, so on every other row the remaining icons shifted left. The prior fixed-width spacer (w-8) only approximated the button's footprint, so the icons still didn't line up exactly. Render the real Authorize button in an invisible, inert placeholder mode (same Button + icon, visibility:hidden, pointer-events-none, aria-hidden) on rows where it doesn't apply. That gives a pixel-identical slot, so the Sync/Test/Edit/Delete icons align across all rows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style(integrations): apply prettier to oauth auth route Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 1a5ad16 commit d7dae8c

7 files changed

Lines changed: 189 additions & 11 deletions

File tree

testplanit/app/[locale]/admin/integrations/AuthorizeIntegrationButton.tsx

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,12 @@ import { useTranslations } from "next-intl";
1212

1313
interface AuthorizeIntegrationButtonProps {
1414
integration: Integration;
15+
/**
16+
* When true, render an invisible, non-interactive button that occupies the
17+
* exact same footprint as the real one, so the remaining action icons stay
18+
* aligned on rows where authorization doesn't apply.
19+
*/
20+
placeholder?: boolean;
1521
}
1622

1723
/**
@@ -22,6 +28,7 @@ interface AuthorizeIntegrationButtonProps {
2228
*/
2329
export function AuthorizeIntegrationButton({
2430
integration,
31+
placeholder = false,
2532
}: AuthorizeIntegrationButtonProps) {
2633
const t = useTranslations("admin.integrations");
2734

@@ -32,6 +39,21 @@ export function AuthorizeIntegrationButton({
3239
window.location.href = `/api/integrations/oauth/${integration.provider.toLowerCase()}/auth?${params.toString()}`;
3340
};
3441

42+
// Reserve the slot with the same Button + icon (identical size) but hidden
43+
// and inert, so action icons line up across rows that don't show Authorize.
44+
if (placeholder) {
45+
return (
46+
<Button
47+
variant="ghost"
48+
className="px-2 py-1 h-auto invisible pointer-events-none"
49+
aria-hidden="true"
50+
tabIndex={-1}
51+
>
52+
<ShieldCheck className="h-4 w-4" />
53+
</Button>
54+
);
55+
}
56+
3557
return (
3658
<Tooltip>
3759
<TooltipTrigger asChild>

testplanit/app/[locale]/admin/integrations/columns.tsx

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -249,13 +249,19 @@ export const useColumns = (
249249
meta: { isPinned: "right" },
250250
cell: ({ row }) => (
251251
<div className="bg-primary-foreground whitespace-nowrap flex justify-center gap-1">
252-
{row.original.authType === "OAUTH2" &&
253-
row.original.status === "INACTIVE" && (
254-
<AuthorizeIntegrationButton
255-
key={`authorize-${row.original.id}`}
256-
integration={row.original}
257-
/>
258-
)}
252+
{/* Always render the Authorize slot. Rows that aren't an
253+
unauthorized OAuth integration get an invisible, same-size
254+
placeholder so the remaining action icons stay aligned. */}
255+
<AuthorizeIntegrationButton
256+
key={`authorize-${row.original.id}`}
257+
integration={row.original}
258+
placeholder={
259+
!(
260+
row.original.authType === "OAUTH2" &&
261+
row.original.status === "INACTIVE"
262+
)
263+
}
264+
/>
259265
<SyncIntegrationButton
260266
key={`sync-${row.original.id}`}
261267
integration={row.original}
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
import { beforeEach, describe, expect, it, vi } from "vitest";
2+
import { NextRequest } from "next/server";
3+
4+
// Shared mock fns (hoisted so the vi.mock factories can reference them).
5+
const {
6+
getServerSessionMock,
7+
getEnhancedDbMock,
8+
integrationFindUniqueMock,
9+
generateStateMock,
10+
storeOAuthStateMock,
11+
getAdapterMock,
12+
getAuthorizationUrlMock,
13+
} = vi.hoisted(() => ({
14+
getServerSessionMock: vi.fn(),
15+
getEnhancedDbMock: vi.fn(),
16+
integrationFindUniqueMock: vi.fn(),
17+
generateStateMock: vi.fn(),
18+
storeOAuthStateMock: vi.fn(),
19+
getAdapterMock: vi.fn(),
20+
getAuthorizationUrlMock: vi.fn(),
21+
}));
22+
23+
vi.mock("next-auth", () => ({
24+
getServerSession: getServerSessionMock,
25+
}));
26+
vi.mock("~/server/auth", () => ({ authOptions: {} }));
27+
vi.mock("@/lib/auth/utils", () => ({ getEnhancedDb: getEnhancedDbMock }));
28+
vi.mock("~/lib/integrations/AuthenticationService", () => ({
29+
AuthenticationService: {
30+
generateState: generateStateMock,
31+
storeOAuthState: storeOAuthStateMock,
32+
},
33+
}));
34+
vi.mock("~/lib/integrations/IntegrationManager", () => ({
35+
IntegrationManager: {
36+
getInstance: () => ({ getAdapter: getAdapterMock }),
37+
},
38+
}));
39+
40+
import { GET } from "./route";
41+
42+
const buildGet = (query: string) =>
43+
new NextRequest(
44+
`http://localhost:3000/api/integrations/oauth/jira/auth${query}`
45+
);
46+
const params = { params: Promise.resolve({ type: "jira" }) };
47+
48+
describe("GET /api/integrations/oauth/[type]/auth", () => {
49+
beforeEach(() => {
50+
vi.clearAllMocks();
51+
getServerSessionMock.mockResolvedValue({ user: { id: "user-1" } });
52+
getEnhancedDbMock.mockResolvedValue({
53+
integration: { findUnique: integrationFindUniqueMock },
54+
});
55+
// The integration is INACTIVE — the exact state an OAuth integration is in
56+
// before its first authorization, and the state that previously made
57+
// getAdapter throw "Integration is not active" → a 500 on this route.
58+
integrationFindUniqueMock.mockResolvedValue({
59+
id: 2,
60+
provider: "JIRA",
61+
status: "INACTIVE",
62+
authType: "OAUTH2",
63+
});
64+
generateStateMock.mockReturnValue("state-123");
65+
storeOAuthStateMock.mockResolvedValue(undefined);
66+
getAuthorizationUrlMock.mockReturnValue(
67+
"https://auth.atlassian.com/authorize?client_id=abc&redirect_uri=https%3A%2F%2Fapp%2Fapi%2Fintegrations%2Foauth%2Fjira%2Fcallback&state=state-123&response_type=code"
68+
);
69+
getAdapterMock.mockResolvedValue({
70+
supportsOAuth: true,
71+
getAuthorizationUrl: getAuthorizationUrlMock,
72+
});
73+
});
74+
75+
it("redirects an INACTIVE OAuth integration to the provider authorize URL (no 500 deadlock)", async () => {
76+
const response = await GET(buildGet("?integrationId=2"), params);
77+
78+
// Must be a redirect to the provider, never a 500. Before the fix,
79+
// getAdapter threw on the inactive integration and this route 500'd.
80+
expect([302, 307]).toContain(response.status);
81+
expect(response.headers.get("location") ?? "").toContain(
82+
"https://auth.atlassian.com/authorize"
83+
);
84+
});
85+
86+
it("builds the adapter with allowInactive so the active-check doesn't block setup", async () => {
87+
await GET(buildGet("?integrationId=2"), params);
88+
89+
// The wiring that breaks the deadlock: the route must ask getAdapter to
90+
// allow an inactive integration during the authorization handshake.
91+
expect(getAdapterMock).toHaveBeenCalledWith(
92+
"2",
93+
undefined,
94+
undefined,
95+
expect.objectContaining({ allowInactive: true })
96+
);
97+
expect(storeOAuthStateMock).toHaveBeenCalledWith("user-1", 2, "state-123");
98+
});
99+
100+
it("returns 400 when integrationId is missing", async () => {
101+
const response = await GET(buildGet(""), params);
102+
expect(response.status).toBe(400);
103+
});
104+
});

testplanit/app/api/integrations/oauth/[type]/auth/route.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,16 @@ export async function GET(
5252

5353
// Get the appropriate adapter based on the integration
5454
const manager = IntegrationManager.getInstance();
55-
const adapter = await manager.getAdapter(integrationId);
55+
// The integration is intentionally inactive until this authorization
56+
// completes, so allow building the adapter to generate the authorize URL.
57+
const adapter = await manager.getAdapter(
58+
integrationId,
59+
undefined,
60+
undefined,
61+
{
62+
allowInactive: true,
63+
}
64+
);
5665

5766
if (!adapter) {
5867
return NextResponse.json(

testplanit/app/api/integrations/oauth/[type]/callback/route.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,15 @@ export async function GET(
7878

7979
// Get the appropriate adapter
8080
const manager = IntegrationManager.getInstance();
81-
const adapter = await manager.getAdapter(integrationId!.toString());
81+
// The integration is still inactive at callback time (it's flipped to
82+
// ACTIVE below, once a token is stored), so allow building the adapter to
83+
// exchange the authorization code for tokens.
84+
const adapter = await manager.getAdapter(
85+
integrationId!.toString(),
86+
undefined,
87+
undefined,
88+
{ allowInactive: true }
89+
);
8290

8391
if (!adapter || !adapter.exchangeCodeForTokens) {
8492
return NextResponse.redirect(

testplanit/lib/integrations/IntegrationManager.test.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,28 @@ describe("IntegrationManager", () => {
287287
);
288288
});
289289

290+
it("builds an adapter for an inactive integration when allowInactive is set (OAuth authorization flow)", async () => {
291+
// OAuth integrations are inactive until authorized; the authorize and
292+
// callback routes must still be able to build the adapter to run the
293+
// handshake. Without allowInactive this would throw and deadlock setup.
294+
mockPrisma.integration.findUnique.mockResolvedValue({
295+
id: 2,
296+
name: "Jira OAuth",
297+
provider: "JIRA",
298+
status: "INACTIVE",
299+
authType: "OAUTH2",
300+
credentials: { clientId: "abc", clientSecret: "shh" },
301+
settings: { baseUrl: "https://test.atlassian.net" },
302+
userIntegrationAuths: [],
303+
});
304+
305+
const adapter = await manager.getAdapter("2", undefined, undefined, {
306+
allowInactive: true,
307+
});
308+
309+
expect(adapter).toBeTruthy();
310+
});
311+
290312
it("should throw error when no adapter registered for provider", async () => {
291313
mockPrisma.integration.findUnique.mockResolvedValue({
292314
id: 1,

testplanit/lib/integrations/IntegrationManager.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,8 @@ export class IntegrationManager {
6868
async getAdapter(
6969
integrationId: string,
7070
prismaClient?: typeof prisma,
71-
userId?: string
71+
userId?: string,
72+
options?: { allowInactive?: boolean }
7273
): Promise<IssueAdapter | null> {
7374
// OAuth adapters carry a per-user token, so they must be cached per user
7475
const cacheKey = userId ? `${integrationId}:${userId}` : integrationId;
@@ -95,7 +96,13 @@ export class IntegrationManager {
9596
throw new Error(`Integration not found: ${integrationId}`);
9697
}
9798

98-
if (integration.status !== "ACTIVE") {
99+
// OAuth 2.0 (3LO) integrations are intentionally inactive until a user
100+
// completes authorization — and the authorization flow itself needs the
101+
// adapter to build the authorize URL and to exchange the code for tokens.
102+
// Those two routes pass allowInactive so the setup handshake isn't blocked
103+
// by the very state it exists to resolve (otherwise an OAuth integration
104+
// can never become active). Every other caller still requires ACTIVE.
105+
if (integration.status !== "ACTIVE" && !options?.allowInactive) {
99106
throw new Error(`Integration is not active: ${integrationId}`);
100107
}
101108

0 commit comments

Comments
 (0)