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

Avoid using oidc callback URL as post-login URL #1384

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Avoid using oidc callback URL as post-login URL #1384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 7 additions & 6 deletions app/client/models/gristUrlState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,15 @@ export function getWelcomeHomeUrl() {
return _buildUrl('welcome/home').href;
}

const FINAL_PATHS = ['/signed-out', '/account-deleted'];
const PATHS_TO_EXCLUDE_FROM_NEXT = ['/signed-out', '/account-deleted', '/oauth2/callback'];
Copy link
Member

@paulfitz paulfitz Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Putting OIDC-specific logic in the front-end makes me feel a little itchy. Is there any way to quarantine the OIDC behavior to OIDCConfig.ts? Via a change to the CALLBACK_URL handler there perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a very good point. I'll try to see if there's a way to delegate this to login middleware such as OIDC.


// Returns the relative URL (i.e. path) of the current page, except when it's the
// "/signed-out" page or "/account-deleted", in which case it returns the home page ("/").
// Returns the relative URL (i.e. path) of the current page, except when it's a page
// that does not make sense to return to after login, such as "/signed-out"
// or "/account-deleted", in which case it returns the home page("/").
// This is a good URL to use for a post-login redirect.
function _getCurrentUrl(): string {
function _getPostLoginTargetUrl(): string {
const {hash, pathname, search} = new URL(window.location.href);
if (FINAL_PATHS.some(final => pathname.endsWith(final))) { return '/'; }
if (PATHS_TO_EXCLUDE_FROM_NEXT.some(path => pathname.endsWith(path))) { return '/'; }

return parseFirstUrlPart('o', pathname).path + search + hash;
}
Expand All @@ -114,7 +115,7 @@ function _getLoginLogoutUrl(
page: 'login'|'logout'|'signin'|'signup'|'account-deleted',
options: GetLoginOrSignupUrlOptions = {}
): string {
const {srcDocId, nextUrl = _getCurrentUrl()} = options;
const {srcDocId, nextUrl = _getPostLoginTargetUrl()} = options;
const startUrl = _buildUrl(page);
if (srcDocId) { startUrl.searchParams.set('srcDocId', srcDocId); }
if (nextUrl) { startUrl.searchParams.set('next', nextUrl); }
Expand Down
7 changes: 7 additions & 0 deletions test/client/models/gristUrlState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,5 +399,12 @@ describe('gristUrlState', function() {
setWindowLocation('https://docs.getgrist.com/signed-out');
assert.equal(getLoginUrl(), 'https://docs.getgrist.com/login?next=%2F');
});

it('getLoginUrl should skip encoding redirect url on oauth2 callback page', function() {
setWindowLocation('http://localhost:8080/oauth2/callback?error=something');
assert.equal(getLoginUrl(), 'http://localhost:8080/login?next=%2F');
setWindowLocation('https://docs.getgrist.com/oauth2/callback?error=something');
assert.equal(getLoginUrl(), 'https://docs.getgrist.com/login?next=%2F');
});
});
});