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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion docs/developer-guide/local-config.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ This is the main structure:
// optionals misc settings
"miscSettings": {
// Use POST requests for each WMS length URL highter than this value.
"maxURLLength": 5000
"maxURLLength": 5000,
// Custom path to home page
"homePath": '/home'
},
// optional state initializer (it will override the one defined in appConfig.js)
"initialState": {
Expand Down
8 changes: 8 additions & 0 deletions web/client/actions/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree.
*/
import ConfigUtils from "../utils/ConfigUtils";
import { push } from 'connected-react-router';

export const GO_TO_PAGE = 'GO_TO_PAGE';


Expand All @@ -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

return push(homePath);
}
10 changes: 7 additions & 3 deletions web/client/components/errors/ResourceUnavailable.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,22 +25,26 @@ import HTML from '../I18N/HTML';
* @prop {string} mode mode types referred to the page view, default 'map'
* @prop {object} glyphs key represents the mode and the value the name of glyph, default {map: '1-map', dashboard: 'dashboard'}
* @prop {string} errorMessage error message to display in description
* @prop {string} errorMessageType error message type. One of "text|html". Default type is "text"
* @prop {boolean} showHomeButton enable/disable home button
* @prop {node} homeButton home button to render in content section of empty state
*
*/

const ResourceUnavailable = emptyState(
({enabled, login, status, alwaysVisible, isSharedStory}) => enabled && alwaysVisible || enabled && login || enabled && status !== 403 || enabled && status === 403 && isSharedStory,
({status, mode = 'map', glyphs = {map: '1-map', dashboard: 'dashboard'}, errorMessage, errorMessageParams, showHomeButton, homeButton}) => ({
({status, mode = 'map', glyphs = {map: '1-map', dashboard: 'dashboard'}, errorMessage, errorMessageType = "text", errorMessageParams, showHomeButton, homeButton}) => ({
glyph: glyphs[mode] || '1-map',
title: status === 403 && <Message msgId={`${mode}.errors.loading.notAccessible`} />
|| status === 404 && <Message msgId={`${mode}.errors.loading.notFound`} />
|| <Message msgId={`${mode}.errors.loading.title`} />,
description: (
<div className="text-center">
{errorMessage && <Message msgId={errorMessage} msgParams={errorMessageParams} />
|| <HTML msgId={`${mode}.errors.loading.unknownError`} />}
{errorMessage
? errorMessageType === "text"
? <Message msgId={errorMessage} msgParams={errorMessageParams} />
: <HTML msgId={errorMessage} msgParams={errorMessageParams} />
: <HTML msgId={`${mode}.errors.loading.unknownError`} />}
</div>
),
content: showHomeButton && homeButton && <div className="text-center">{homeButton}</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,14 @@ describe('ResourceUnavailable component', () => {
expect(glyphicon).toExist();
});

it('ResourceUnavailable with error message type', () => {
ReactDOM.render(<ResourceUnavailable enabled login status={404} errorMessage={'<p>Error 404</p>'} mode="testMode" errorMessageType="html"/>, document.getElementById("container"));
const container = document.getElementById('container');
const desc = container.querySelector('.empty-state-description > .text-center > span');
expect(desc).toBeTruthy();
expect(desc.innerHTML).toBe('&lt;p&gt;Error 404&lt;/p&gt;');
});

it('ResourceUnavailable glyph and mode (missing glyph)', () => {
ReactDOM.render(<ResourceUnavailable enabled login status={404} errorMessage={'Error 404'} mode="newTestMode" glyphs={{testMode: 'list'}}/>, document.getElementById("container"));
const container = document.getElementById('container');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import React from 'react';
import ReactDOM from 'react-dom';
import ReactTestUtils from "react-dom/test-utils";
import { Provider } from 'react-redux';
import configureMockStore from 'redux-mock-store';

import STORY from '../../../../test-resources/geostory/sampleStory_1.json';
import expect from 'expect';
Expand Down Expand Up @@ -61,7 +63,7 @@ describe('GeoStory Navigation component', () => {
expect(selectedElement.innerText).toBe("Abstract");
});
it('should render home icon', () => {
ReactDOM.render(<Navigation router={{ pathname: '/geostory/shared/1', search: '?showHome=true' }} />, document.getElementById('container'));
ReactDOM.render(<Provider store={configureMockStore()()}><Navigation router={{ pathname: '/geostory/shared/1', search: '?showHome=true' }} /></Provider>, document.getElementById('container'));
expect(document.querySelector('#home-button')).toBeTruthy();
});
it('should hide home icon when showHome is false', () => {
Expand Down
16 changes: 8 additions & 8 deletions web/client/components/home/Home.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,16 @@
*/

import React from 'react';
import { connect } from 'react-redux';
import Button from '../misc/Button';
import PropTypes from 'prop-types';
import { Glyphicon, Tooltip } from 'react-bootstrap';
import OverlayTrigger from '../misc/OverlayTrigger';
import Message from '../../components/I18N/Message';
import ConfirmModal from '../../components/misc/ResizableModal';
import { get, pick } from "lodash";
import ConfigUtils from "../../utils/ConfigUtils";
export const getPath = () => {
const miscSettings = ConfigUtils.getConfigProp('miscSettings');
return get(miscSettings, ['homePath'], '/');
};
import { pick } from "lodash";
import { goToHomePage } from '../../actions/router';

class Home extends React.Component {
static propTypes = {
icon: PropTypes.string,
Expand Down Expand Up @@ -94,8 +92,10 @@ class Home extends React.Component {
}

goHome = () => {
this.context.router.history.push(getPath());
this.props.goToHomePage();
};
}

export default Home;
export default connect(null, {
goToHomePage
})(Home);
15 changes: 9 additions & 6 deletions web/client/components/home/__tests__/Home-test.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
import expect from 'expect';
import React from 'react';
import ReactDOM from 'react-dom';
import { Provider } from 'react-redux';
import configureMockStore from 'redux-mock-store';
import Home from '../Home';

const mockStore = configureMockStore();

describe("Test Home component", () => {
let store;
beforeEach((done) => {
store = mockStore();
document.body.innerHTML = '<div id="container"></div>';
setTimeout(done);
});
Expand All @@ -16,21 +22,18 @@ describe("Test Home component", () => {
});

it('creates component with defaults', () => {
const cmp = ReactDOM.render(<Home/>, document.getElementById("container"));
const cmp = ReactDOM.render(<Provider store={store}><Home/></Provider>, document.getElementById("container"));
expect(cmp).toBeTruthy();
expect(cmp.props.icon).toEqual("home");
const icons = document.querySelectorAll(".glyphicon-home");
expect(icons.length).toEqual(1);
expect(icons[0]).toBeTruthy();
});
it('creates component with custom icon text', () => {
const cmp = ReactDOM.render(
<Home
<Provider store={store}><Home
icon="pencil"
/>, document.getElementById("container"));
/></Provider>, document.getElementById("container"));
expect(cmp).toBeTruthy();
expect(cmp.props.icon).toEqual("pencil");

const buttons = document.querySelectorAll("button");
expect(buttons.length).toEqual(1);
expect(buttons[0]).toBeTruthy();
Expand Down
38 changes: 32 additions & 6 deletions web/client/epics/__tests__/context-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -179,14 +179,14 @@ describe('context epics', () => {
/*
* check error actions
*/
const checkLoadErrors = (startActions, initialState, messageId, done) => {
const checkLoadErrors = (startActions, initialState, messageId, errorStatus, done) => {
testEpic(loadContextAndMap, 5, startActions, ([loadingAction, clearMapTemplatesAction, loadMapAction, errorAction, loadEndAction]) => {
expect(loadingAction.type).toBe(LOADING);
expect(loadingAction.value).toBe(true);
expect(clearMapTemplatesAction.type).toBe(CLEAR_MAP_TEMPLATES);
expect(loadMapAction.type).toBe(LOAD_MAP_CONFIG);
expect(errorAction.type).toBe(CONTEXT_LOAD_ERROR);
expect(errorAction.error.status).toBe(403);
expect(errorAction.error.status).toBe(errorStatus);
expect(errorAction.error.messageId).toBe(messageId);
expect(loadEndAction.type).toBe(LOADING);
expect(loadEndAction.value).toBe(false);
Expand All @@ -208,7 +208,7 @@ describe('context epics', () => {
loadContext({ mapId, contextName }),
configureError({status: 403}) // THIS ACTION FAKES MAP LOAD FLOW END
];
checkLoadErrors(actions, NOT_LOGGED_STATE, 'context.errors.map.pleaseLogin', done);
checkLoadErrors(actions, NOT_LOGGED_STATE, 'context.errors.map.pleaseLogin', 403, done);

});
it('403 forbidden, logged in', done => {
Expand All @@ -217,7 +217,7 @@ describe('context epics', () => {
loadContext({ mapId, contextName }),
configureError({ status: 403 }) // THIS ACTION FAKES MAP LOAD FLOW END
];
checkLoadErrors(actions, LOGGED_STATE, 'context.errors.map.notAccessible', done);
checkLoadErrors(actions, LOGGED_STATE, 'context.errors.map.notAccessible', 403, done);

});
});
Expand All @@ -228,7 +228,7 @@ describe('context epics', () => {
loadContext({ mapId, contextName }),
configureMap() // THIS ACTION FAKES MAP LOAD FLOW END
];
checkLoadErrors(actions, NOT_LOGGED_STATE, 'context.errors.context.pleaseLogin', done);
checkLoadErrors(actions, NOT_LOGGED_STATE, 'context.errors.context.pleaseLogin', 403, done);

});
it('403 forbidden, logged in', done => {
Expand All @@ -237,7 +237,24 @@ describe('context epics', () => {
loadContext({ mapId, contextName }),
configureMap() // THIS ACTION FAKES MAP LOAD FLOW END
];
checkLoadErrors(actions, LOGGED_STATE, 'context.errors.context.notAccessible', done);
checkLoadErrors(actions, LOGGED_STATE, 'context.errors.context.notAccessible', 403, done);
});
it('404, logged in', done => {
createContextResponse(404);
const actions = [
loadContext({ mapId, contextName }),
configureMap() // THIS ACTION FAKES MAP LOAD FLOW END
];
checkLoadErrors(actions, LOGGED_STATE, 'context.errors.context.unknownError', 404, done);
});
it('404, not logged in', done => {
createContextResponse(404);
const actions = [
loadContext({ mapId, contextName }),
configureMap() // THIS ACTION FAKES MAP LOAD FLOW END
];
checkLoadErrors(actions, NOT_LOGGED_STATE, 'context.errors.context.notFound', 404, done);

});
});
});
Expand All @@ -251,6 +268,15 @@ describe('context epics', () => {
done();
});
});
it('reload when 404, then the user login', done => {
const actions = [loadContext({ mapId, contextName }), contextLoadError({ error: { status: 404 } }), loginSuccess()];
testEpic(handleLoginLogoutContextReload, 1, actions, ([reloadAction]) => {
expect(reloadAction.type).toBe(LOAD_CONTEXT);
expect(reloadAction.mapId).toBe(mapId);
expect(reloadAction.contextName).toBe(contextName);
done();
});
});
it('reload when loaded, then the user logout', done => {
const actions = [loadContext({ mapId, contextName }), loadFinished(), logout()];
testEpic(handleLoginLogoutContextReload, 1, actions, ([reloadAction]) => {
Expand Down
25 changes: 25 additions & 0 deletions web/client/epics/__tests__/feedbackMask-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { loadGeostory, geostoryLoaded, loadGeostoryError } from '../../actions/g
import { LOGIN_REQUIRED } from '../../actions/security';
import { onLocationChanged } from 'connected-react-router';
import { testEpic, addTimeoutEpic, TEST_TIMEOUT } from './epicTestUtils';
import { contextLoadError } from '../../actions/context';
import { loadPermalink, loadPermalinkError, permalinkLoaded } from '../../actions/permalink';

describe('feedbackMask Epics', () => {
Expand Down Expand Up @@ -226,6 +227,30 @@ describe('feedbackMask Epics', () => {
}
});
});
it('test feedbackMaskPromptLogin on context 404', done => {
const ACTIONS_EMITTED = 2;
const error = {status: 404, statusText: 'Forbidden'};
testEpic(addTimeoutEpic(feedbackMaskPromptLogin, 10), ACTIONS_EMITTED, contextLoadError({error}), actions => {
expect(actions.length).toBe(ACTIONS_EMITTED);
actions.map((action) => {
switch (action.type) {
case TEST_TIMEOUT:
break;
case LOGIN_REQUIRED:
break;
default:
done(new Error("Action not recognized"));
}
});
done();
},
{router: {
location: {
pathname: '/context/Context/1'
}
}
});
});
it('should navigate to homepage when user made some changes to new map, and prompt appears', (done) => {
const epicResponse = (actions) => {
expect(actions.length).toBe(1);
Expand Down
24 changes: 15 additions & 9 deletions web/client/epics/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,22 @@ const createMapFlow = (mapId = 'new', mapConfig, session, action$) => {
);
};

const errorToMessageId = (name, e, getState = () => {}) => {
let message = `context.errors.${name}.unknownError`;
const errorToMessageId = (name = "loading", e, getState = () => {}) => {
let messageId = `context.errors.${name}.unknownError`;
let errorMessageType = "text";
if (e.status === 403) {
message = `context.errors.${name}.pleaseLogin`;
messageId = `context.errors.${name}.pleaseLogin`;
if (isLoggedIn(getState())) {
message = `context.errors.${name}.notAccessible`;
messageId = `context.errors.${name}.notAccessible`;
}
} if (e.status === 404) {
message = `context.errors.${name}.notFound`;
messageId = `context.errors.${name}.notFound`;
if (isLoggedIn(getState())) {
messageId = `context.errors.${name}.unknownError`;
errorMessageType = "html";
}
}
return message;
return { messageId, errorMessageType };
};

/**
Expand Down Expand Up @@ -161,9 +166,10 @@ export const loadContextAndMap = (action$, { getState = () => { } } = {}) =>
loading(true, "loading"),
[loading(false, "loading")],
e => {
const messageId = errorToMessageId(e.name, e.originalError, getState);
const error = {...e.originalError, status: e.originalError?.status ?? e.status};
const { messageId, errorMessageType } = errorToMessageId(e.name, error, getState);
// prompt login should be triggered here
return Observable.of(contextLoadError({ error: {...e.originalError, messageId} }) );
return Observable.of(contextLoadError({ error: {...e.originalError, messageId, errorMessageType} }) );
}
)
);
Expand All @@ -183,7 +189,7 @@ export const loadContextAndMap = (action$, { getState = () => { } } = {}) =>
*/
export const handleLoginLogoutContextReload = action$ =>
// If the was a forbidden error (access denied to the given context)
action$.ofType(CONTEXT_LOAD_ERROR).filter(({ error }) => error.status === 403)
action$.ofType(CONTEXT_LOAD_ERROR).filter(({ error }) => [404, 403].includes(error.status)) // getResourceIdByName returns 404 in both the cases (not permitted/not found)
// or in case of context successfully loaded
.merge(action$.ofType(LOAD_FINISHED))
// on login-logout event
Expand Down
11 changes: 6 additions & 5 deletions web/client/epics/feedbackMask.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import Rx from 'rxjs';

import { split, get, isNil } from 'lodash';
import { LOCATION_CHANGE, push } from 'connected-react-router';
import { LOCATION_CHANGE } from 'connected-react-router';

import {
FEEDBACK_MASK_ENABLED,
Expand Down Expand Up @@ -40,6 +40,7 @@ import { isLoggedIn } from '../selectors/security';
import { isSharedStory } from '../selectors/geostory';
import { pathnameSelector } from '../selectors/router';
import { getGeostoryMode } from '../utils/GeoStoryUtils';
import { goToHomePage } from '../actions/router';


/**
Expand Down Expand Up @@ -221,8 +222,8 @@ export const feedbackMaskPromptLogin = (action$, store) => // TODO: separate log
.filter((action) => {
const pathname = pathnameSelector(store.getState());
return action.error
&& [403].concat([LOAD_PERMALINK_ERROR].includes(action.type) ? 404 : []).includes(action.error.status)
&& pathname.indexOf("new") === -1 && !(pathname.match(/(dashboard)/) !== null && pathname.match(/(dashboard)\/[0-9]+/) === null); // new map geostory and dashboard has different handling (see redirectUnauthorizedUserOnNewLoadError, TODO: uniform different behaviour)
&& [403].concat([CONTEXT_LOAD_ERROR, LOAD_PERMALINK_ERROR].includes(action.type) ? 404 : []).includes(action.error.status)
&& pathname.indexOf("new") === -1 && !(pathname.match(/dashboard/) !== null && pathname.match(/dashboard\/[0-9]+/) === null); // new map geostory and dashboard has different handling (see redirectUnauthorizedUserOnNewLoadError, TODO: uniform different behaviour)
})
.filter(() => !isLoggedIn(store.getState()) && !isSharedStory(store.getState()))
.exhaustMap(
Expand All @@ -231,7 +232,7 @@ export const feedbackMaskPromptLogin = (action$, store) => // TODO: separate log
.concat(
action$.ofType(LOGIN_PROMPT_CLOSED) // then if for login close
.take(1)
.switchMap(() => Rx.Observable.of(feedbackMaskLoading(), push('/'))) // go to home page
.switchMap(() => Rx.Observable.of(feedbackMaskLoading(), goToHomePage()))

).takeUntil(action$.ofType(LOGIN_SUCCESS, LOCATION_CHANGE))
);
Expand All @@ -244,7 +245,7 @@ export const redirectUnauthorizedUserOnNewLoadError = (action$, { getState = ()
pathnameSelector(getState()).match(/dashboard\/[0-9]+/) === null &&
pathnameSelector(getState()).match(/dashboard/) !== null))
.filter(() => !isLoggedIn(getState()))
.switchMap(() => Rx.Observable.of(push('/'))); // go to home page
.switchMap(() => Rx.Observable.of(goToHomePage()));

/**
* Epics for feedbackMask functionality
Expand Down
Loading
Loading