Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/asub 8551 apple signin redirect #2184

Merged
merged 10 commits into from
Jul 29, 2024
Merged

Fix/asub 8551 apple signin redirect #2184

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ce5efd7
add localstorage to redirect url
accbjt Jul 10, 2024
01db3dd
add test for using localhost redirect
accbjt Jul 11, 2024
fd2fcc9
fix tests
accbjt Jul 17, 2024
78a93e1
remove debugger
accbjt Jul 17, 2024
78a34d6
add location.assign
accbjt Jul 17, 2024
c005f59
update tests for login link
accbjt Jul 19, 2024
d3379fc
fix typo
accbjt Jul 19, 2024
0eb7efa
revision updates
accbjt Jul 22, 2024
3b4842c
revision changes
accbjt Jul 24, 2024
2239833
Merge branch 'arc-themes-release-version-2.5.0' into fix/ASUB-8551-ap…
accbjt Jul 26, 2024
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
24 changes: 20 additions & 4 deletions blocks/identity-block/components/login/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,16 @@
const [isAppleAuthSuccess, setIsAppleAuthSuccess] = useState(false);
const { loginByOIDC } = useOIDCLogin();

const setRedirectUrl = (url) => {
setCurrentRedirectToURL(url);
sessionStorage.setItem('ArcXP_redirectUrl', url);
};

const getRedirectURL = () => {
const localStorageRedirectUrl = sessionStorage.getItem('ArcXP_redirectUrl');

return redirectQueryParam || localStorageRedirectUrl || currentRedirectToURL;
};

useEffect(() => {
const askForloginWithApple = async (code) => {
Expand Down Expand Up @@ -57,13 +67,15 @@
const redirectUrlLocation = new URL(document.referrer);

if (searchParams.has('reset_password')) {
setCurrentRedirectToURL(`${redirectURL}${redirectUrlLocation.search}`);
const newRedirectUrl = `${redirectURL}${redirectUrlLocation.search}`;

setRedirectUrl(newRedirectUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

@accbjt I think we can define a let newRedirectUrl and move this line out from the if-else statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LauraPinilla Okay, I made those updates.

} else {
const newRedirectUrl = redirectUrlLocation.pathname.includes('/pagebuilder/')
? redirectURL
: `${redirectUrlLocation.pathname}${redirectUrlLocation.search}`;

setCurrentRedirectToURL(newRedirectUrl);
setRedirectUrl(newRedirectUrl);
}
}
}, [redirectQueryParam, redirectToPreviousPage, redirectURL]);
Expand All @@ -88,7 +100,11 @@
if (isOIDC) {
loginByOIDC();
} else {
window.location = redirectQueryParam || validatedLoggedInPageLoc;
const localStorageRedirectUrl = sessionStorage.getItem('ArcXP_redirectUrl');
const validatedLocalRedirectURL = validateURL(localStorageRedirectUrl);
const newRedirectUrl = redirectQueryParam || validatedLocalRedirectURL || validatedLoggedInPageLoc;

window.location = newRedirectUrl;
Dismissed Show dismissed Hide dismissed
Dismissed Show dismissed Hide dismissed
}
}
};
Expand All @@ -98,7 +114,7 @@
}, [Identity, redirectQueryParam, loggedInPageLocation, isAdmin, loginByOIDC, isOIDC, isAppleAuthSuccess]);

return {
loginRedirect: redirectQueryParam || currentRedirectToURL,
loginRedirect: getRedirectURL(),
};
};

Expand Down
79 changes: 61 additions & 18 deletions blocks/identity-block/components/login/index.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,18 @@ const Test = (props) => {
);
};

const windowLocationValues = {
origin: 'http://localhost',
href: 'http://localhost',
search: '',
pathname: '/',
}

describe("useLogin()", () => {
beforeEach(() => {
Object.defineProperty(window, "location", {
writable: true,
value: {
origin: 'http://localhost',
href: 'http://localhost',
search: '',
pathname: '/',
}
value: windowLocationValues,
});
useIdentity.mockImplementation(() => ({
isInitialized: true,
Expand All @@ -64,6 +66,7 @@ describe("useLogin()", () => {

it("uses redirect query", async () => {
Object.defineProperty(window, "location", {
...windowLocationValues,
writable: true,
value: {
search: "?test=123&redirect=/new-account/",
Expand Down Expand Up @@ -97,6 +100,7 @@ describe("useLogin()", () => {
Object.defineProperty(window, "location", {
writable: true,
value: {
...windowLocationValues,
origin: 'http://localhost',
href: 'http://localhost',
search: '?reset_password=true',
Expand All @@ -118,13 +122,15 @@ describe("useLogin()", () => {
},
}));
await render(<Test />);
expect(window.location).toBe(`http://localhost${defaultParams.loggedInPageLocation}`);

expect(window.location).toBe(`http://localhost${defaultParams.redirectURL}`);
});

it("replaces potentially unsafe URLs in query param", async () => {
Object.defineProperty(window, "location", {
writable: true,
value: {
...windowLocationValues,
search: "?test=123&redirect=https://somewhere.com",
pathname: "/",
},
Expand All @@ -134,21 +140,23 @@ describe("useLogin()", () => {
expect(window.location).toBe("/");
});

it("replaces potentially unsafe URLs in redirectURL parameter", async () => {
await render(<Test redirectURL="https://somewhere.com" />);
it("replaces potentially unsafe URLs in query param", async () => {
Object.defineProperty(window, "location", {
writable: true,
value: {
...windowLocationValues,
search: "",
pathname: "/",
},
});
await render(<Test loggedInPageLocation="https://somewhere.com" />);
fireEvent.click(screen.getByRole("button"));
expect(window.location).toBe("/");
});

it("replaces potentially unsafe URLs in loggedInPageLocation parameter", async () => {
useIdentity.mockImplementation(() => ({
isInitialized: true,
Identity: {
isLoggedIn: jest.fn(() => true),
getConfig: jest.fn(() => ({})),
},
}));
await render(<Test loggedInPageLocation="https://somewhere.com" />);
it("replaces potentially unsafe URLs in redirectURL parameter", async () => {
await render(<Test redirectURL="https://somewhere.com" />);
fireEvent.click(screen.getByRole("button"));
expect(window.location).toBe("/");
});

Expand All @@ -161,6 +169,7 @@ describe("useLogin()", () => {
Object.defineProperty(window, "location", {
writable: true,
value: {
...windowLocationValues,
origin: 'http://localhost',
href: 'http://localhost',
search: '',
Expand All @@ -172,4 +181,38 @@ describe("useLogin()", () => {
expect(window.location).toBe("/account/");
delete document.referrer;
});

it("should use redirectUrl from sessionStorage", async () => {
const referrerURL = "http://localhost/featured-articles/";
Object.defineProperty(document, "referrer", {
value: referrerURL,
configurable: true,
});
Object.defineProperty(window, "location", {
writable: true,
value: {
...windowLocationValues,
origin: 'http://localhost',
href: 'http://localhost',
search: '',
pathname: '/'
}
});

useIdentity.mockImplementation(() => ({
isInitialized: true,
Identity: {
isLoggedIn: jest.fn(() => true),
getConfig: jest.fn(() => ({})),
},
}));

await render(<Test />);

expect(sessionStorage.getItem("ArcXP_redirectUrl")).toBe('/featured-articles/');

fireEvent.click(screen.getByRole("button"));
expect(window.location).toBe("/featured-articles/");
delete document.referrer;
});
});
26 changes: 13 additions & 13 deletions blocks/identity-block/components/social-sign-on/_children/AppleSignIn.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,19 @@ function AppleSignIn({ socialSignOnIn, className }) {
const { Identity } = useIdentity();

return (
<Button
id="apple-btn"
variant="secondary-reverse"
onClick={() => Identity.initAppleSignOn()}
iconLeft={AppleIcon}
className={`${className}__Apple`}
>
{socialSignOnIn !== SIGN_UP ? (
<span>{phrases.t("identity-block.social-signOn-apple-login")}</span>
) : (
<span>{phrases.t("identity-block.social-signOn-apple-signUp")}</span>
)}
</Button>
<Button
id="apple-btn"
variant="secondary-reverse"
onClick={() => Identity.initAppleSignOn()}
iconLeft={AppleIcon}
className={`${className}__Apple`}
>
{socialSignOnIn !== SIGN_UP ? (
<span>{phrases.t("identity-block.social-signOn-apple-login")}</span>
) : (
<span>{phrases.t("identity-block.social-signOn-apple-signUp")}</span>
)}
</Button>
);
}

Expand Down
4 changes: 2 additions & 2 deletions blocks/identity-block/components/social-sign-on/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import AppleSignIn from "./_children/AppleSignIn";
import useSocialSignIn from "./utils/useSocialSignIn";

const SocialSignOn = ({ className, onError, redirectURL, isOIDC, socialSignOnIn }) => {
const { facebookAppId, googleClientId, appleTeamId, appleKeyId, appleUrlToReceiveAuthToken} = useSocialSignIn(redirectURL, isOIDC, socialSignOnIn, onError);
const { facebookAppId, googleClientId, appleTeamId, appleKeyId, appleUrlToReceiveAuthToken } = useSocialSignIn(redirectURL, isOIDC, socialSignOnIn, onError);
return (
<section className={className}>
{googleClientId ? <GoogleSignIn onError={onError} redirectURL={redirectURL} socialSignOnIn={socialSignOnIn} className={className} /> : null}
{facebookAppId ? <FacebookSignIn socialSignOnIn={socialSignOnIn}/> : null}
{facebookAppId ? <FacebookSignIn socialSignOnIn={socialSignOnIn} /> : null}
{appleTeamId && appleKeyId && appleUrlToReceiveAuthToken ? <AppleSignIn socialSignOnIn={socialSignOnIn} className={className} /> : null}
</section>
);
Expand Down
4 changes: 3 additions & 1 deletion blocks/identity-block/components/social-sign-on/utils/useSocialSignIn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ function useSocialSignIn(redirectURL, isOIDC, socialSignOnIn, onError = () => {}
loginByOIDC();
} else {
const validatedURL = validateURL(redirectURL);

Copy link
Contributor

Choose a reason for hiding this comment

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

@accbjt Can you please revert this new line

window.location = validatedURL;
}
} catch (e) {
Expand Down Expand Up @@ -51,7 +52,8 @@ function useSocialSignIn(redirectURL, isOIDC, socialSignOnIn, onError = () => {}
loginByOIDC();
} else {
const validatedURL = validateURL(redirectURL);
window.location = validatedURL;

Copy link
Contributor

Choose a reason for hiding this comment

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

@accbjt Same here

window.location.hred = validatedURL;
}
}),
auto_select: true,
Expand Down
1 change: 1 addition & 0 deletions blocks/identity-block/features/account-management/default.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ function AccountManagement({ customFields }) {
const checkLoggedInStatus = () =>
Identity.isLoggedIn().then((isLoggedIn) => {
if (!isLoggedIn) {

Copy link
Contributor

Choose a reason for hiding this comment

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

@accbjt same here

window.location = redirectURL;
return;
}
Expand Down
1 change: 1 addition & 0 deletions blocks/identity-block/features/login/default.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ const Login = ({ customFields }) => {
loginByOIDC();
} else {
const validatedURL = validateURL(loginRedirect);

Copy link
Contributor

Choose a reason for hiding this comment

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

@accbjt same here

window.location = validatedURL;
}
})
Expand Down
2 changes: 1 addition & 1 deletion blocks/identity-block/features/social-sign-on/default.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ const SocialSignOnBlock = ({ customFields }) => {

return (
<Stack className={BLOCK_CLASS_NAME} data-testid="social-sign-on-container">
{!hideDiv ? <div className={`${BLOCK_CLASS_NAME}__dividerWithText`}>{phrases.t("identity-block.or")}</div> : null}
{!hideDiv ? <div className={`${BLOCK_CLASS_NAME}__dividerWithText`}>{phrases.t("identity-block.or")}</div> : null}
<GoogleSignInProvider>
<SocialSignOn
className={`${BLOCK_CLASS_NAME}__button-container`}
Expand Down
1 change: 1 addition & 0 deletions blocks/identity-block/utils/validate-redirect-url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const validateURL = (url) => {
return url;
}

sessionStorage.setItem("ArcXP_redirectUrl", "/");
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @accbjt, why are you setting ArcXP_redirectUrl to "/". Sounds like the value we are setting here should depend on the custom fields or the page in which the user is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LauraPinilla The original validateURL return '/' if the url is not valid so I was just following what they set in that function. I think if the url is not a valid url then it is okay to set it as /. That is the original logic and I don't want to change that and make it more complicated.

return "/";
};

Expand Down
Loading