[GH-1092] Redirect the user to correct URL after login #1094
Conversation
| @@ -15,6 +16,13 @@ export const AuthGuard = (props) => { | |||
| setRequestedLocation(location.pathname); | |||
| } | |||
|
|
|||
| if ( | |||
| !sessionStorage.getItem('window-location') && | |||
| location.pathname !== '/' | |||
There was a problem hiding this comment.
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
|
As a suggestion for enhancement, related to the window path. There can be a situation:
May be UserA should be redirected to another "Access denied" page? |
| 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}</>; |
There was a problem hiding this comment.
What's wrong with long URLs? Why 50? Maybe it's better to make it a param, not magic constant.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@TejasRGitHub in this case, constants is a good place for it.
There was a problem hiding this comment.
Hi @SofiaSazonova , Updated code to make it a constant . Please let me know if it looks good
Hi @SofiaSazonova , I like this enhancement and maybe could be added in the future releases |
|
Addressed remaining review comments. |
|
@TejasRGitHub Thanks! |
Feature or Bugfix
Detail
Relates
Security
Please answer the questions below briefly where applicable, or write
N/A. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)? No
evalor similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.