Skip to content

Commit

Permalink
Merge #26053
Browse files Browse the repository at this point in the history
26053:  ui: redirect to login page when api call is unauthorized r=couchand a=couchand

#### ui: pass both api handlers in a single then call

Previously the API handlers in CachedDataReducer were attached with
a then followed by a catch, which meant errors in the dispatch of
success would show up as API errors erroneously.  This change moves
both handlers into a single invocation of then to fix the issue.

Release note: None

#### ui: redirect to login page when api call is unauthorized

When we get a 401 HTTP status code, redirect to the login
page.  Once logout is implemented, this should do that
instead, to make sure client-side state is clean.

Fixes: #25785
Release note: None

Co-authored-by: Andrew Couch <[email protected]>
  • Loading branch information
craig[bot] and couchand committed May 29, 2018
2 parents 0002c5c + 6526970 commit d403920
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 18 deletions.
37 changes: 27 additions & 10 deletions pkg/ui/src/redux/cachedDataReducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import _ from "lodash";
import { Action, Dispatch } from "redux";
import { assert } from "chai";
import moment from "moment";
import { hashHistory } from "react-router";
import { push } from "react-router-redux";

import { getLoginPage } from "src/redux/login";
import { APIRequestFn } from "src/util/api";

import { PayloadAction, WithRequest } from "src/interfaces/action";
Expand Down Expand Up @@ -177,16 +180,30 @@ export class CachedDataReducer<TRequest, TResponseMessage> {
// Note that after dispatching requestData, state.inFlight is true
dispatch(this.requestData(req));
// Fetch data from the servers. Return the promise for use in tests.
return this.apiEndpoint(req, this.requestTimeout).then((data) => {
// Dispatch the results to the store.
dispatch(this.receiveData(data, req));
}).catch((error: Error) => {
// If an error occurred during the fetch, add it to the store.
// Wait 1s to record the error to avoid spamming errors.
// TODO(maxlang): Fix error handling more comprehensively.
// Tracked in #8699
setTimeout(() => dispatch(this.errorData(error, req)), 1000);
}).then(() => {
return this.apiEndpoint(req, this.requestTimeout).then(
(data) => {
// Dispatch the results to the store.
dispatch(this.receiveData(data, req));
},
(error: Error) => {
// TODO(couchand): This is a really myopic way to check for HTTP
// codes. However, at the moment that's all that the underlying
// timeoutFetch offers. Major changes to this plumbing are warranted.
if (error.message === "Unauthorized") {
// TODO(couchand): This is an unpleasant dependency snuck in here...
const location = hashHistory.getCurrentLocation();
if (location && !location.pathname.startsWith("/login")) {
dispatch(push(getLoginPage(location)));
}
}

// If an error occurred during the fetch, add it to the store.
// Wait 1s to record the error to avoid spamming errors.
// TODO(maxlang): Fix error handling more comprehensively.
// Tracked in #8699
setTimeout(() => dispatch(this.errorData(error, req)), 1000);
},
).then(() => {
// Invalidate data after the invalidation period if one exists.
if (this.invalidationPeriod) {
setTimeout(() => dispatch(this.invalidateData(req)), this.invalidationPeriod.asMilliseconds());
Expand Down
15 changes: 15 additions & 0 deletions pkg/ui/src/redux/login.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { createPath } from "history/lib/PathUtils";
import { ThunkAction } from "redux-thunk";
import { createSelector } from "reselect";

import { Action } from "redux";
import { userLogin } from "src/util/api";
import { AdminUIState } from "src/redux/state";
import { LOGIN_PAGE } from "src/routes/login";
import { cockroach } from "src/js/protos";
import { getDataFromServer } from "src/util/dataFromServer";

Expand Down Expand Up @@ -97,6 +99,19 @@ export const selectLoginState = createSelector(
},
);

export function getLoginPage(location) {
const query = !location ? undefined : {
redirectTo: createPath({
pathname: location.pathname,
search: location.search,
}),
};
return {
pathname: LOGIN_PAGE,
query: query,
};
}

// Redux implementation.

// State
Expand Down
4 changes: 2 additions & 2 deletions pkg/ui/src/redux/state.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import _ from "lodash";
import { hashHistory } from "react-router";
import { syncHistoryWithStore, routerReducer, RouterState } from "react-router-redux";
import { syncHistoryWithStore, routerMiddleware, routerReducer, RouterState } from "react-router-redux";
import { createStore, combineReducers, applyMiddleware, compose, GenericStoreEnhancer, Store } from "redux";
import createSagaMiddleware from "redux-saga";
import thunk from "redux-thunk";
Expand Down Expand Up @@ -44,7 +44,7 @@ export function createAdminUIStore() {
login: loginReducer,
}),
compose(
applyMiddleware(thunk, sagaMiddleware),
applyMiddleware(thunk, sagaMiddleware, routerMiddleware(hashHistory)),
// Support for redux dev tools
// https://chrome.google.com/webstore/detail/redux-devtools/lmhkpmbekcpmknklioeibfkpmmfibljd
(window as any).devToolsExtension ? (window as any).devToolsExtension({
Expand Down
8 changes: 2 additions & 6 deletions pkg/ui/src/views/login/requireLogin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ import { withRouter, WithRouterProps } from "react-router";
import { connect } from "react-redux";

import { AdminUIState } from "src/redux/state";
import { selectLoginState, LoginState } from "src/redux/login";
import { LOGIN_PAGE } from "src/routes/login";
import { selectLoginState, LoginState, getLoginPage } from "src/redux/login";

interface RequireLoginProps {
loginState: LoginState;
Expand All @@ -23,10 +22,7 @@ class RequireLogin extends React.Component<WithRouterProps & RequireLoginProps>
const { location, router } = this.props;

if (!this.hasAccess()) {
router.push({
pathname: LOGIN_PAGE,
query: { redirectTo: router.createPath(location.pathname, location.query) },
});
router.push(getLoginPage(location));
}
}

Expand Down

0 comments on commit d403920

Please sign in to comment.