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

Wait for URL update on redirect #3163

Merged
merged 6 commits into from
Oct 6, 2023
+72 −35
Merged

Wait for URL update on redirect #3163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b7f603a
Wait for URL update on redirect
NSeydoux Oct 2, 2023
0302f79
Update changelog
NSeydoux Oct 2, 2023
ebf09a9
fixup! Wait for URL update on redirect
NSeydoux Oct 2, 2023
9ff804b
fixup! Update changelog
NSeydoux Oct 2, 2023
28285b1
Poll current location for query params cleanup
NSeydoux Oct 3, 2023
250862f
Remove console statements
NSeydoux Oct 4, 2023
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
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@ The following have been deprecated, and will be removed in future major releases

The following changes have been implemented but not released yet:

### Bugfixes

#### browser

- [Mismatching redirect URI](https://github.com/inrupt/solid-client-authn-js/issues/2891) on refresh: the root cause of the bug was a race
condition because of the asynchronous nature of updating the browser URL. The appropriate event is now awaited for, which should prevent
the issue from manifesting.

## [1.17.2](https://github.com/inrupt/solid-client-authn-js/releases/tag/v1.17.2) - 2023-09-15

### Bugfixes
Expand Down
59 changes: 40 additions & 19 deletions packages/browser/src/ClientAuthentication.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,12 @@ describe("ClientAuthentication", () => {
// TODO: add tests for events & errors

it("reverts back to un-authenticated fetch on logout", async () => {
// eslint-disable-next-line no-restricted-globals
history.replaceState = jest.fn();
window.history.replaceState = jest
.fn<typeof window.history.replaceState>()
.mockImplementationOnce((_data, _unused, url) => {
// Pretend the current location is updated
window.location.href = url as string;
});
const clientAuthn = getClientAuthentication();

const unauthFetch = clientAuthn.fetch;
Expand Down Expand Up @@ -402,13 +406,17 @@ describe("ClientAuthentication", () => {
mockEmitter.emit = jest.fn<typeof mockEmitter.emit>();

it("calls handle redirect", async () => {
// eslint-disable-next-line no-restricted-globals
history.replaceState = jest.fn();
window.history.replaceState = jest
.fn<typeof window.history.replaceState>()
.mockImplementationOnce((_data, _unused, url) => {
// Pretend the current location is updated
window.location.href = url as string;
});
const expectedResult = SessionCreatorCreateResponse;
const clientAuthn = getClientAuthentication();
const unauthFetch = clientAuthn.fetch;
const url =
"https://coolapp.com/redirect?state=userId&id_token=idToken&access_token=accessToken";
"https://example.org/redirect?state=userId&id_token=idToken&access_token=accessToken";
const redirectInfo = await clientAuthn.handleIncomingRedirect(
url,
mockEmitter,
Expand Down Expand Up @@ -436,8 +444,12 @@ describe("ClientAuthentication", () => {
});

it("clears the current IRI from OAuth query parameters in the auth code flow", async () => {
// eslint-disable-next-line no-restricted-globals
history.replaceState = jest.fn();
window.history.replaceState = jest
.fn<typeof window.history.replaceState>()
.mockImplementationOnce((_data, _unused, url) => {
// Pretend the current location is updated
window.location.href = url as string;
});
const clientAuthn = getClientAuthentication();
const url =
"https://coolapp.com/redirect?state=someState&code=someAuthCode&iss=someIssuer";
Expand All @@ -452,8 +464,12 @@ describe("ClientAuthentication", () => {
});

it("clears the current IRI from OAuth query parameters even if auth flow fails", async () => {
// eslint-disable-next-line no-restricted-globals
history.replaceState = jest.fn();
window.history.replaceState = jest
.fn<typeof window.history.replaceState>()
.mockImplementationOnce((_data, _unused, url) => {
// Pretend the current location is updated
window.location.href = url as string;
});

mockHandleIncomingRedirect.mockImplementationOnce(() =>
Promise.reject(new Error("Something went wrong")),
Expand All @@ -464,8 +480,7 @@ describe("ClientAuthentication", () => {
const url =
"https://coolapp.com/redirect?state=someState&code=someAuthCode";
await clientAuthn.handleIncomingRedirect(url, mockEmitter);
// eslint-disable-next-line no-restricted-globals
expect(history.replaceState).toHaveBeenCalledWith(
expect(window.history.replaceState).toHaveBeenCalledWith(
null,
"",
"https://coolapp.com/redirect",
Expand All @@ -479,14 +494,17 @@ describe("ClientAuthentication", () => {
});

it("clears the current IRI from OAuth query parameters in the implicit flow", async () => {
// eslint-disable-next-line no-restricted-globals
history.replaceState = jest.fn();
window.history.replaceState = jest
.fn<typeof window.history.replaceState>()
.mockImplementationOnce((_data, _unused, url) => {
// Pretend the current location is updated
window.location.href = url as string;
});
const clientAuthn = getClientAuthentication();
const url =
"https://coolapp.com/redirect?state=someState&id_token=idToken&access_token=accessToken";
await clientAuthn.handleIncomingRedirect(url, mockEmitter);
// eslint-disable-next-line no-restricted-globals
expect(history.replaceState).toHaveBeenCalledWith(
expect(window.history.replaceState).toHaveBeenCalledWith(
null,
"",
"https://coolapp.com/redirect",
Expand All @@ -495,14 +513,17 @@ describe("ClientAuthentication", () => {
});

it("preserves non-OAuth query strings", async () => {
// eslint-disable-next-line no-restricted-globals
history.replaceState = jest.fn();
window.history.replaceState = jest
.fn<typeof window.history.replaceState>()
.mockImplementationOnce((_data, _unused, url) => {
// Pretend the current location is updated
window.location.href = url as string;
});
const clientAuthn = getClientAuthentication();
const url =
"https://coolapp.com/redirect?state=someState&code=someAuthCode&someQuery=someValue";
await clientAuthn.handleIncomingRedirect(url, mockEmitter);
// eslint-disable-next-line no-restricted-globals
expect(history.replaceState).toHaveBeenCalledWith(
expect(window.history.replaceState).toHaveBeenCalledWith(
null,
"",
"https://coolapp.com/redirect?someQuery=someValue",
Expand Down
17 changes: 14 additions & 3 deletions packages/browser/src/ClientAuthentication.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
this.boundLogout = redirectInfo.getLogoutUrl;

// Strip the oauth params:
this.cleanUrlAfterRedirect(url);
await this.cleanUrlAfterRedirect(url);

return {
isLoggedIn: redirectInfo.isLoggedIn,
Expand All @@ -119,7 +119,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
};
} catch (err) {
// Strip the oauth params:
this.cleanUrlAfterRedirect(url);
await this.cleanUrlAfterRedirect(url);

// FIXME: EVENTS.ERROR should be errorCode, errorDescription
//
Expand All @@ -132,7 +132,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
}
};

private cleanUrlAfterRedirect(url: string): void {
private async cleanUrlAfterRedirect(url: string): Promise<void> {
const cleanedUpUrl = new URL(url);
cleanedUpUrl.searchParams.delete("state");
// For auth code flow
Expand All @@ -151,5 +151,16 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
// authentication library will be called again with what are now invalid
// query parameters!).
window.history.replaceState(null, "", cleanedUpUrl.toString());
while (window.location.href !== cleanedUpUrl.href) {
// Poll the current URL every ms. Active polling is required because
// window.history.replaceState is asynchronous, but the associated
// 'popstate' event which should be listened to is only sent on active
// navigation, which we will not have here.
// See https://developer.mozilla.org/en-US/docs/Web/API/Window/popstate_event#when_popstate_is_sent
// eslint-disable-next-line no-await-in-loop
await new Promise<void>((resolve) => {
setTimeout(() => resolve(), 1);
});
}
}
}
23 changes: 10 additions & 13 deletions packages/browser/src/Session.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,10 +246,9 @@ describe("Session", () => {
it("uses current window location as default redirect URL", async () => {
mockLocation("https://some.url");
const clientAuthentication = mockClientAuthentication();
const incomingRedirectHandler = jest.spyOn(
clientAuthentication,
"handleIncomingRedirect",
);
const incomingRedirectHandler = jest
.spyOn(clientAuthentication, "handleIncomingRedirect")
.mockResolvedValue(undefined);

const mySession = new Session({ clientAuthentication });
await mySession.handleIncomingRedirect();
Expand All @@ -259,13 +258,12 @@ describe("Session", () => {
);
});

it("wraps up ClientAuthentication handleIncomingRedirect", async () => {
it("wraps ClientAuthentication handleIncomingRedirect", async () => {
mockLocation("https://some.url");
const clientAuthentication = mockClientAuthentication();
const incomingRedirectHandler = jest.spyOn(
clientAuthentication,
"handleIncomingRedirect",
);
const incomingRedirectHandler = jest
.spyOn(clientAuthentication, "handleIncomingRedirect")
.mockResolvedValue(undefined);
const mySession = new Session({ clientAuthentication });
await mySession.handleIncomingRedirect("https://some.url");
expect(incomingRedirectHandler).toHaveBeenCalled();
Expand Down Expand Up @@ -401,10 +399,9 @@ describe("Session", () => {

it("preserves a binding to its Session instance", async () => {
const clientAuthentication = mockClientAuthentication();
const incomingRedirectHandler = jest.spyOn(
clientAuthentication,
"handleIncomingRedirect",
);
const incomingRedirectHandler = jest
.spyOn(clientAuthentication, "handleIncomingRedirect")
.mockResolvedValue(undefined);
const mySession = new Session({ clientAuthentication });
const objectWithHandleIncomingRedirect = {
handleIncomingRedirect: mySession.handleIncomingRedirect,
Expand Down