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

[GH-1092] Redirect the user to correct URL after login #1094

Merged
merged 8 commits into from
Apr 5, 2024
Merged

[GH-1092] Redirect the user to correct URL after login #1094

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6bbe404
Setup for URL redirect
TejasRGitHub Mar 7, 2024
26fc0af
Lint fixes
TejasRGitHub Mar 7, 2024
8de0066
Merge branch 'main' into gh-redirect-to-correct-url
Mar 25, 2024
cdfe85b
Adding Comments and removing sessionStorage.getItmem check in Generic…
TejasRGitHub Mar 25, 2024
27d311f
Merge branch 'main' into gh-redirect-to-correct-url
Apr 5, 2024
5f177e9
Adding window path length as a constant
TejasRGitHub Apr 5, 2024
288e5e9
Capitalizing as per standard
TejasRGitHub Apr 5, 2024
2b398d2
Fixing linting changes
TejasRGitHub Apr 5, 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: 24 additions & 0 deletions frontend/src/authentication/components/AuthGuard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { useState } from 'react';
import { Navigate, useLocation } from 'react-router-dom';
import { Login } from '../views/Login';
import { useAuth } from '../hooks';
import { regexToValidateWindowPathName } from '../../utils';

export const AuthGuard = (props) => {
const { children } = props;
Expand All @@ -15,6 +16,13 @@ export const AuthGuard = (props) => {
setRequestedLocation(location.pathname);
}

if (
!sessionStorage.getItem('window-location') &&
location.pathname !== '/'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking this explicitly so that we do not set the '/' which will happen in case a user logsout and then pastes the URL in the browser and again tries to logback with the new URL . As the user logged out the redirect URL is the base URL and then this gets stored in the window-location which should not be the case. We only want to store window location path pointing to specific path

) {
sessionStorage.setItem('window-location', location.pathname);
}

return <Login />;
}

Expand All @@ -23,6 +31,22 @@ export const AuthGuard = (props) => {
return <Navigate to={requestedLocation} />;
}

if (
sessionStorage.getItem('window-location') &&
SofiaSazonova marked this conversation as resolved.
Show resolved Hide resolved
location.pathname !== sessionStorage.getItem('window-location')
) {
const windowPathLocation = sessionStorage.getItem('window-location');
sessionStorage.removeItem('window-location');
// Check if the window-location only contains alphanumeric and / in it and its not tampered
if (!regexToValidateWindowPathName.test(windowPathLocation))
return <>{children}</>;
// A guardrail to limit the string of the pathname to a certain characters
if (windowPathLocation.length > 50) return <>{children}</>;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with long URLs? Why 50? Maybe it's better to make it a param, not magic constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @SofiaSazonova , Thanks for the review. I added this number by heuristically checking the length of the windowpath for the longest URL. I think it was the URL to table in a dataset and it was approx 33 - 37 characters.

I can make it a param and store it in the contants.js . What are your thoughts ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather make it a setting, rather then a constant.
The maximum URL length is something the customer may want to define before the deployment, and it's more an attribute of solution rather then code constant.
But maybe @dlpzx can correct me here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @SofiaSazonova , Thanks for re-reviewing it. In the length check we are actually checking the windowPath length . The window path length is excluding the URL domain. Thus, the window path length will contain all the resource paths to the data.all related items ( datasets, shares, tables, etc ) and won't contain anything else that might be customer specific. Unless a customer integrates a custom module ( not on data.all O.S. ) by themselves which have very large windowpath names I don't think we need to make it configurable. Please let me know if you want to still make it configurable.

@dlpzx , would also like to know your thoughts on this

Copy link
Contributor

Choose a reason for hiding this comment

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

@TejasRGitHub in this case, constants is a good place for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @SofiaSazonova , Updated code to make it a constant . Please let me know if it looks good

return <Navigate to={windowPathLocation} replace={true} />;
} else {
sessionStorage.removeItem('window-location');
}

return <>{children}</>;
};

Expand Down
7 changes: 7 additions & 0 deletions frontend/src/authentication/contexts/GenericAuthContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@
};

initialize().catch((e) => dispatch({ type: SET_ERROR, error: e.message }));
}, []);

Check warning on line 108 in frontend/src/authentication/contexts/GenericAuthContext.js

View workflow job for this annotation

GitHub Actions / es-lint (16.x) 

React Hook useEffect has missing dependencies: 'getAuthenticatedUser' and 'processLoadingStateChange'. Either include them or remove the dependency array

// useEffect needed for React OIDC context
// Process OIDC state when isLoading state changes
Expand All @@ -113,7 +113,7 @@
if (CUSTOM_AUTH) {
processLoadingStateChange();
}
}, [isLoading]);

Check warning on line 116 in frontend/src/authentication/contexts/GenericAuthContext.js

View workflow job for this annotation

GitHub Actions / es-lint (16.x) 

React Hook useEffect has a missing dependency: 'processLoadingStateChange'. Either include it or remove the dependency array

// useEffect to process when a user is loaded by react OIDC
// This is triggered when the userProfile ( i.e. auth.user ) is loaded by react OIDC
Expand Down Expand Up @@ -149,7 +149,7 @@
dispatch({ type: SET_ERROR, error: e.message })
);
}
}, [userProfile]);

Check warning on line 152 in frontend/src/authentication/contexts/GenericAuthContext.js

View workflow job for this annotation

GitHub Actions / es-lint (16.x) 

React Hook useEffect has a missing dependency: 'getAuthenticatedUser'. Either include it or remove the dependency array

// useEffect to process auth events generated by react OIDC
// This is used to logout user when the token expires
Expand All @@ -167,7 +167,7 @@
});
});
}
}, [authEvents]);

Check warning on line 170 in frontend/src/authentication/contexts/GenericAuthContext.js

View workflow job for this annotation

GitHub Actions / es-lint (16.x) 

React Hook useEffect has a missing dependency: 'auth'. Either include it or remove the dependency array

const getAuthenticatedUser = async () => {
if (CUSTOM_AUTH) {
Expand Down Expand Up @@ -249,6 +249,9 @@
}
});
}
if (sessionStorage.getItem('window-location')) {
sessionStorage.removeItem('window-location');
}
} catch (error) {
console.error('Failed to signout', error);
}
Expand Down Expand Up @@ -279,6 +282,10 @@
}
});
}

if (sessionStorage.getItem('window-location')) {
SofiaSazonova marked this conversation as resolved.
Show resolved Hide resolved
sessionStorage.removeItem('window-location');
}
};

return (
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/utils/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,5 @@ export const AwsRegions = [
{ name: 'AWS GovCloud (US-East)', code: 'us-gov-east-1' },
{ name: 'AWS GovCloud (US)', code: 'us-gov-west-1' }
];

export const regexToValidateWindowPathName = /^[a-zA-Z0-9/]*$/;
Loading