Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions src/components/Settings/integrations/useOauthUrl.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import React, { PropsWithChildren } from 'react';
import { renderHook } from '@testing-library/react';
import { Session } from 'next-auth';
import { useSession } from 'next-auth/react';
import { renderToString } from 'react-dom/server';
import TestRouter from '__tests__/util/TestRouter';
import { IntegrationAccordion } from 'src/components/Shared/Forms/Accordions/AccordionEnum';
import { useOauthUrl } from './useOauthUrl';

jest.mock('next-auth/react');

const accountListId = 'account-list-1';
const apiToken = 'apiToken';

const Wrapper = ({ children }: PropsWithChildren) => (
<TestRouter router={{ query: { accountListId } }}>{children}</TestRouter>
);

const renderUseOauthUrl = () =>
renderHook(() => useOauthUrl(), { wrapper: Wrapper });

beforeEach(() => {
process.env.OAUTH_URL = 'https://auth.mpdx.org';

@zweatshirt zweatshirt Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@canac Hi Caleb, I had a quick question if that's okay. I assume this wouldn't cause cross-file leakage of process.env.OAUTH_URL? In this case, is it best to add an afterEach anyway?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine

(useSession as jest.Mock).mockReturnValue({
data: { user: { apiToken } } as Session,
});
});

describe('useOauthUrl', () => {
it('builds the Google OAuth url with the redirect back to integrations', () => {
const { result } = renderUseOauthUrl();

expect(result.current.getGoogleOauthUrl()).toBe(
`https://auth.mpdx.org/auth/user/google?account_list_id=${accountListId}&redirect_to=${encodeURIComponent(
`http://localhost/accountLists/${accountListId}/settings/integrations?selectedTab=${IntegrationAccordion.Google}`,
)}&access_token=${apiToken}`,
);
});

it('includes the organization id in the DonorHub OAuth url', () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] `getMailChimpOauthUrl` and `getPrayerlettersOauthUrl` aren't asserted here, so their distinct path segments (`/auth/user/mailchimp`, `/auth/user/prayer_letters` — note the underscore) aren't pinned by this unit test. They are covered with full-URL assertions at the component level (`MailchimpAccordion.test.tsx:143`, `PrayerlettersAccordion.test.tsx:112-117`), so this is minor. An `it.each` over all four getters would close the gap cheaply (`getOrganizationOauthUrl` stays separate since it takes an arg).

const { result } = renderUseOauthUrl();

expect(result.current.getOrganizationOauthUrl('org-1')).toBe(
`https://auth.mpdx.org/auth/user/donorhub?account_list_id=${accountListId}&redirect_to=${encodeURIComponent(
`http://localhost/accountLists/${accountListId}/settings/integrations?selectedTab=${IntegrationAccordion.Organization}`,
)}&access_token=${apiToken}&organization_id=org-1`,
);
});

it('renders on the server when window is undefined', () => {
const Probe = () => {
const { getGoogleOauthUrl } = useOauthUrl();
return <a href={getGoogleOauthUrl()}>{'link'}</a>;
};

const originalWindow = global.window;
// @ts-expect-error simulating a server environment with no window
delete global.window;

try {
let html = '';
expect(() => {
html = renderToString(
<Wrapper>
<Probe />
</Wrapper>,
);
}).not.toThrow();

expect(html).toContain(
`redirect_to=${encodeURIComponent(
`/accountLists/${accountListId}/settings/integrations?selectedTab=${IntegrationAccordion.Google}`,
)}`,
);
} finally {
global.window = originalWindow;
}
});
});
14 changes: 9 additions & 5 deletions src/components/Settings/integrations/useOauthUrl.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { useEffect, useState } from 'react';
import { IntegrationAccordion } from 'src/components/Shared/Forms/Accordions/AccordionEnum';
import { useAccountListId } from 'src/hooks/useAccountListId';
import { useRequiredSession } from 'src/hooks/useRequiredSession';
Expand All @@ -6,12 +7,15 @@ export const useOauthUrl = () => {
const { apiToken } = useRequiredSession();
const accountListId = useAccountListId();

const getRedirectUrl = (accordion: IntegrationAccordion) => {
const domain = window.location.origin;
return encodeURIComponent(
`${domain}/accountLists/${accountListId}/settings/integrations?selectedTab=${accordion}`,
const [origin, setOrigin] = useState('');
useEffect(() => {
setOrigin(window.location.origin);
}, []);

const getRedirectUrl = (accordion: IntegrationAccordion) =>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] During SSR and the first client render `origin` is `''`, so `redirect_to` is host-relative until the post-mount effect fires. This is functionally safe — these getters are only navigated on user click (post-hydration), and the Organization flows build the URL at click time, so `origin` is always populated by then; no open-redirect risk (no user input in the target). Noted for awareness only. If you ever want server-rendered links to be absolute, you could fall back to `window.location.origin` at call time when `origin === ''` — but that adds branching for a near-impossible race and isn't recommended here.

encodeURIComponent(
`${origin}/accountLists/${accountListId}/settings/integrations?selectedTab=${accordion}`,
);
};

return {
getGoogleOauthUrl: () =>
Expand Down
Loading