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.

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

#9251: Better handle of context access restriction #9298

Merged
merged 7 commits into from
Aug 1, 2023

Conversation

dsuren1
Copy link
Contributor

@dsuren1 dsuren1 commented Jul 24, 2023

Description

This PR enhances the context access restriction scenario with proper error message and redirection if configured.

Update: This configuration is not valid and agreed to keep design simple by coding redirect logic in the respective project and is not needed at the moment in mapstore ref: #9298 (review)

"miscSettings": {
     "loginPage": "/?login"  // example
}

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Enhancement

Issue

What is the current behavior?

What is the new behavior?
On context load error

  • Not logged in
    • Redirect to login page (when custom login page configured) or prompt login panel
  • Logged in
image
  • On back to home page, redirects to custom homePath (if configured) else to default root /

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

@dsuren1 dsuren1 added this to the 2023.02.00 milestone Jul 24, 2023
@dsuren1 dsuren1 requested a review from offtherailz July 24, 2023 15:01
@dsuren1 dsuren1 self-assigned this Jul 24, 2023
@dsuren1 dsuren1 linked an issue Jul 24, 2023 that may be closed by this pull request
2 tasks
@offtherailz
Copy link
Member

You have to solve conflicts @dsuren1

@dsuren1
Copy link
Contributor Author

dsuren1 commented Jul 25, 2023

You have to solve conflicts @dsuren1

Yes, permalink commit causing it

@dsuren1
Copy link
Contributor Author

dsuren1 commented Jul 25, 2023

@offtherailz I have resolved the conflicts. PR is ready for review now. Thanks!

Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Hi,
in general I think that the redirect logic is not needed at the moment in mapstore because:

  • geOrchestra has a custom login plugin, hidden that handles the logic of login) and anyway their CAS can not redirect to the correct page.
  • here in mapstore we don't have the login redirect but only openID or basic.

If it is anyway needed for some reason, I should use the existing authenticationProviders logic, instead of adding custom logic to this special case. So I should remove for sure the loginPage from misc settings, at least if needed use the existing logic.

For the redirect to the home page, I should centralize the logic in a unique action, instead of getting the homePage here and there.

web/client/epics/feedbackMask.js Outdated Show resolved Hide resolved
web/client/epics/feedbackMask.js Outdated Show resolved Hide resolved
web/client/plugins/FeedbackMask.jsx Outdated Show resolved Hide resolved
web/client/selectors/context.js Outdated Show resolved Hide resolved
web/client/actions/login.js Outdated Show resolved Hide resolved
docs/developer-guide/local-config.md Outdated Show resolved Hide resolved
web/client/epics/feedbackMask.js Outdated Show resolved Hide resolved
@@ -17,3 +20,8 @@ export function goToPage(page, router) {
page
};
}

export function goToHomePage() {
const homePath = ConfigUtils.getMiscSetting('homePath', '/');
Copy link
Member

Choose a reason for hiding this comment

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

can we use the goToPage action instead of push?

Copy link
Contributor Author

@dsuren1 dsuren1 Jul 28, 2023

Choose a reason for hiding this comment

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

In that case, we may need to pass router as a param from every place we trigger it. And GO_TO_PAGE doesn't perform any sideeffect

@dsuren1 dsuren1 requested a review from offtherailz July 28, 2023 14:45
@offtherailz
Copy link
Member

This looks ok for me. Just to be sure @allyoucanmap what do you think about these changes in the geonode context?

@offtherailz offtherailz merged commit 2a6113c into geosolutions-it:master Aug 1, 2023
4 checks passed
@offtherailz
Copy link
Member

offtherailz commented Aug 1, 2023

@ElenaGallo, could you please test this on DEV ? Thank you

@dsuren1 anyway I don't think we have something really to test here, while it should be on geOrchestra

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better handle of context access restriction
2 participants