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

Extract authentication logic to redux state #517

Merged
merged 10 commits into from
Aug 28, 2018
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
3 changes: 3 additions & 0 deletions dashboard/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,15 @@
"@types/react-router-dom": "^4.2.3",
"@types/react-router-redux": "^5.0.11",
"@types/react-test-renderer": "^16.0.0",
"@types/redux-mock-store": "^1.0.0",
"axios-mock-adapter": "^1.15.0",
"husky": "0.14.3",
"jest-enzyme": "6.0.2",
"lint-staged": "^6.1.0",
"mock-socket": "^8.0.2",
"prettier": "^1.10.2",
"react-svg-loader": "^2.1.0",
"redux-mock-store": "^1.5.3",
"tslint": "^5.9.1",
"tslint-config-prettier": "^1.6.0",
"tslint-react": "^3.4.0",
Expand Down
65 changes: 65 additions & 0 deletions dashboard/src/actions/auth.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { IAuthState } from "reducers/auth";
import configureMockStore from "redux-mock-store";
import thunk from "redux-thunk";
import { getType } from "typesafe-actions";
import actions from ".";
import { Auth } from "../shared/Auth";

const mockStore = configureMockStore([thunk]);
const token = "abcd";
const validationErrorMsg = "Validation error";

let store: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should see if Partial<IStoreState> works here, if not we can try casting the mockStore thing to IStoreState


beforeEach(() => {
const state: IAuthState = {
authenticated: false,
authenticating: false,
};

Auth.validateToken = jest.fn();
Auth.setAuthToken = jest.fn();

store = mockStore({
auth: {
state,
},
});
});

describe("authenticate", () => {
it("dispatches authenticating and auth error if invalid", () => {
Auth.validateToken = jest.fn().mockImplementationOnce(() => {
throw new Error(validationErrorMsg);
});
const expectedActions = [
{
type: getType(actions.auth.authenticating),
},
{
errorMsg: `Error: ${validationErrorMsg}`,
type: getType(actions.auth.authenticationError),
},
];

return store.dispatch(actions.auth.authenticate(token)).then(() => {
expect(store.getActions()).toEqual(expectedActions);
});
});

it("dispatches authenticating and auth ok if valid", () => {
const expectedActions = [
{
type: getType(actions.auth.authenticating),
},
{
authenticated: true,
type: getType(actions.auth.setAuthenticated),
},
];

return store.dispatch(actions.auth.authenticate(token)).then(() => {
expect(store.getActions()).toEqual(expectedActions);
});
});
});
24 changes: 20 additions & 4 deletions dashboard/src/actions/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,30 @@ export const setAuthenticated = createAction("SET_AUTHENTICATED", (authenticated
type: "SET_AUTHENTICATED",
}));

const allActions = [setAuthenticated].map(getReturnOfExpression);
export const authenticating = createAction("AUTHENTICATING", () => ({
type: "AUTHENTICATING",
}));

export const authenticationError = createAction("AUTHENTICATION_ERROR", (errorMsg: string) => ({
errorMsg,
type: "AUTHENTICATION_ERROR",
}));

const allActions = [setAuthenticated, authenticating, authenticationError].map(
getReturnOfExpression,
);
export type AuthAction = typeof allActions[number];

export function authenticate(token: string) {
return async (dispatch: Dispatch<IStoreState>) => {
await Auth.validateToken(token);
Auth.setAuthToken(token);
return dispatch(setAuthenticated(true));
dispatch(authenticating());
try {
await Auth.validateToken(token);
Auth.setAuthToken(token);
return dispatch(setAuthenticated(true));
} catch (e) {
return dispatch(authenticationError(e.toString()));
}
};
}

Expand Down
1 change: 1 addition & 0 deletions dashboard/src/backport.d.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// This file contains a set of backports from typescript 3.x
// TODO(miguel) Remove backports once we upgrade typescript https://github.com/kubeapps/kubeapps/issues/534
// @ts-ignore
type EventHandlerNonNull = (event: Event) => any;

// @ts-ignore
Expand Down
76 changes: 27 additions & 49 deletions dashboard/src/components/LoginForm/LoginForm.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,48 +12,50 @@ const emptyLocation: Location = {
state: "",
};

const defaultProps = {
authenticate: jest.fn(),
authenticated: false,
authenticating: false,
authenticationError: undefined,
location: emptyLocation,
};

const authenticationError = "it's a trap";

it("renders a token login form", () => {
const wrapper = shallow(
<LoginForm authenticated={false} authenticate={jest.fn()} location={emptyLocation} />,
);
const wrapper = shallow(<LoginForm {...defaultProps} />);
expect(wrapper.find("input#token").exists()).toBe(true);
expect(wrapper.find(Redirect).exists()).toBe(false);
expect(wrapper).toMatchSnapshot();
});

it("renders a link to the access control documentation", () => {
const wrapper = shallow(
<LoginForm authenticated={false} authenticate={jest.fn()} location={emptyLocation} />,
);
const wrapper = shallow(<LoginForm {...defaultProps} />);
expect(wrapper.find("a").props()).toMatchObject({
href: "https://github.com/kubeapps/kubeapps/blob/master/docs/user/access-control.md",
target: "_blank",
});
});

it("updates the token in the state when the input is changed", () => {
const wrapper = shallow(
<LoginForm authenticated={false} authenticate={jest.fn()} location={emptyLocation} />,
);
const wrapper = shallow(<LoginForm {...defaultProps} />);
wrapper.find("input#token").simulate("change", { currentTarget: { value: "f00b4r" } });
expect(wrapper.state("token")).toBe("f00b4r");
});

describe("redirect if authenticated", () => {
it("redirects to / if no current location", () => {
const wrapper = shallow(
<LoginForm authenticated={true} authenticate={jest.fn()} location={emptyLocation} />,
);
const wrapper = shallow(<LoginForm {...defaultProps} authenticated={true} />);
const redirect = wrapper.find(Redirect);
expect(redirect.exists()).toBe(true);
expect(redirect.props()).toEqual({ push: false, to: { pathname: "/" } });
});

it("redirects to previous location", () => {
const location = emptyLocation;
const location = Object.assign({}, emptyLocation);
location.state = { from: "/test" };
const wrapper = shallow(
<LoginForm authenticated={true} authenticate={jest.fn()} location={location} />,
<LoginForm {...defaultProps} authenticated={true} location={location} />,
);
const redirect = wrapper.find(Redirect);
expect(redirect.exists()).toBe(true);
Expand All @@ -62,49 +64,25 @@ describe("redirect if authenticated", () => {
});

it("calls the authenticate handler when the form is submitted", () => {
const authenticate = jest.fn();
const wrapper = shallow(
<LoginForm authenticated={false} authenticate={authenticate} location={emptyLocation} />,
);
const wrapper = shallow(<LoginForm {...defaultProps} />);
wrapper.find("input#token").simulate("change", { currentTarget: { value: "f00b4r" } });
wrapper.find("form").simulate("submit", { preventDefault: jest.fn() });
expect(authenticate).toBeCalledWith("f00b4r");
expect(wrapper.state("authenticating")).toBe(true);
expect(defaultProps.authenticate).toBeCalledWith("f00b4r");
});

it("displays an error if the authenticate handler throws an error", async () => {
const authenticate = async () => {
throw new Error("it's a trap");
};
it("displays an error if the authentication error is passed", () => {
const wrapper = shallow(
<LoginForm authenticated={false} authenticate={authenticate} location={emptyLocation} />,
<LoginForm {...defaultProps} authenticationError={authenticationError} />,
);
wrapper.find("input#token").simulate("change", { currentTarget: { value: "f00b4r" } });
wrapper.find("form").simulate("submit", { preventDefault: jest.fn() });

// wait for promise to resolve
try {
await authenticate();
} catch (e) {
expect(wrapper.state()).toMatchObject({
authenticating: false,
error: e.toString(),
});

wrapper.update();
expect(wrapper.find(".alert-error").exists()).toBe(true);
expect(wrapper).toMatchSnapshot();
}
expect(wrapper.find(".alert-error").exists()).toBe(true);
expect(wrapper).toMatchSnapshot();
});

it("allows you to dismiss the error alert", () => {
const wrapper = shallow(
<LoginForm authenticated={false} authenticate={jest.fn()} location={emptyLocation} />,
);
wrapper.setState({ error: "it's a trap" });
expect(wrapper.find(".alert-error").exists()).toBe(true);
it("disables the input if authenticating", () => {
const wrapper = shallow(<LoginForm {...defaultProps} />);

wrapper.find("button.alert__close").simulate("click");
expect(wrapper.find(".alert-error").exists()).toBe(false);
expect(wrapper.state("error")).toBeUndefined();
expect(wrapper.find(".button").props().disabled).toBe(false);
wrapper.setProps({ authenticating: true });
expect(wrapper.find(".button")).toBeDisabled();
});
26 changes: 7 additions & 19 deletions dashboard/src/components/LoginForm/LoginForm.tsx
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
import { Location } from "history";
import * as React from "react";
import { Lock, X } from "react-feather";
import { Lock } from "react-feather";
import { Redirect } from "react-router";

import "./LoginForm.css";

interface ILoginFormProps {
authenticated: boolean;
authenticating: boolean;
authenticationError: string | undefined;
authenticate: (token: string) => any;
location: Location;
}

interface ILoginFormState {
authenticating: boolean;
token: string;
error?: string;
}

class LoginForm extends React.Component<ILoginFormProps, ILoginFormState> {
public state: ILoginFormState = { token: "", authenticating: false };
public state: ILoginFormState = { token: "" };
public render() {
if (this.props.authenticated) {
const { from } = this.props.location.state || { from: { pathname: "/" } };
Expand All @@ -28,13 +28,10 @@ class LoginForm extends React.Component<ILoginFormProps, ILoginFormState> {
<section className="LoginForm">
<div className="LoginForm__container padding-v-bigger bg-skew">
<div className="container container-tiny">
{this.state.error && (
{this.props.authenticationError && (
<div className="alert alert-error margin-c" role="alert">
There was an error connecting to the Kubernetes API. Please check that your token is
valid.
<button className="alert__close" onClick={this.handleAlertClose}>
<X />
</button>
</div>
)}
</div>
Expand Down Expand Up @@ -71,7 +68,7 @@ class LoginForm extends React.Component<ILoginFormProps, ILoginFormState> {
<button
type="submit"
className="button button-accent"
disabled={this.state.authenticating}
disabled={this.props.authenticating}
>
Login
</button>
Expand All @@ -87,22 +84,13 @@ class LoginForm extends React.Component<ILoginFormProps, ILoginFormState> {

private handleSubmit = async (e: React.FormEvent<HTMLFormElement>) => {
e.preventDefault();
this.setState({ authenticating: true });
const { token } = this.state;
try {
return token && (await this.props.authenticate(token));
} catch (e) {
this.setState({ error: e.toString(), token: "", authenticating: false });
}
return token && (await this.props.authenticate(token));
};

private handleTokenChange = (e: React.FormEvent<HTMLInputElement>) => {
this.setState({ token: e.currentTarget.value });
};

private handleAlertClose = (e: React.FormEvent<HTMLButtonElement>) => {
this.setState({ error: undefined });
};
}

export default LoginForm;
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
Copy link
Contributor

@andresmgot andresmgot Aug 22, 2018

Choose a reason for hiding this comment

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

I think I don't fully understand the purpose of snapshots. It seems that it's something that you just replace with the current state when it fails. Apparently it's not a very useful test. Am I missing something?


exports[`displays an error if the authenticate handler throws an error 1`] = `
exports[`displays an error if the authentication error is passed 1`] = `
<section
className="LoginForm"
>
Expand All @@ -15,15 +15,6 @@ exports[`displays an error if the authenticate handler throws an error 1`] = `
role="alert"
>
There was an error connecting to the Kubernetes API. Please check that your token is valid.
<button
className="alert__close"
onClick={[Function]}
>
<X
color="currentColor"
size="24"
/>
</button>
</div>
</div>
<div
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { shallow } from "enzyme";
import { Location } from "history";
import * as React from "react";
import { IAuthState } from "reducers/auth";
import configureMockStore from "redux-mock-store";
import thunk from "redux-thunk";
import LoginForm from "./LoginFormContainer";

const mockStore = configureMockStore([thunk]);

const makeStore = (
authenticated: boolean,
authenticating: boolean,
authenticationError: string,
) => {
const state: IAuthState = {
authenticated,
authenticating,
authenticationError,
};
return mockStore({ auth: state });
};

const emptyLocation: Location = {
hash: "",
pathname: "",
search: "",
state: "",
};

describe("LoginFormContainer props", () => {
it("maps authentication redux states to props", () => {
const store = makeStore(true, true, "It's a trap");
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just pass the IAuthState object directly? not sure why this helper is valuable, it's also not easy to read what the first two parameters relate to

Copy link
Contributor

Choose a reason for hiding this comment

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

also, shouldn't we test different variations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not see much value on passing variations since the mapStateToProps just takes the values, there are not defaults or anything.

const wrapper = shallow(<LoginForm store={store} location={emptyLocation} />);
const form = wrapper.find("LoginForm");
expect(form).toHaveProp({
authenticated: true,
authenticating: true,
authenticationError: "It's a trap",
});
});
});
Loading