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

Fix a login error when using a password manager #5261

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
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
5 changes: 5 additions & 0 deletions .changeset/good-actors-yawn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"saleor-dashboard": patch
---

Now there can only be one login request running at a time. This means using a password manager to log in no longer cause an error. If there are too many login requests Dashboard now shows a corresponding error message.
4 changes: 4 additions & 0 deletions locale/defaultMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -4623,6 +4623,10 @@
"context": "order does not require shipping",
"string": "does not apply"
},
"RyZd9J": {
"context": "error message",
"string": "Please wait a moment before trying again."
},
"Ryh3iR": {
"context": "header",
"string": "Create Webhook"
Expand Down
45 changes: 45 additions & 0 deletions src/auth/AuthProvider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -211,4 +211,49 @@ describe("AuthProvider", () => {
expect(hook.result.current.errors).toEqual(["noPermissionsError"]);
expect(hook.result.current.authenticated).toBe(false);
});

it("should handle concurrent login attempts correctly", async () => {
const intl = useIntl();
const notify = useNotifier();
const apolloClient = useApolloClient();
Comment on lines +216 to +218
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: IMO you should mock them

Copy link
Member Author

Choose a reason for hiding this comment

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

I just checked, they're already mocked in the file


(useAuthState as jest.Mock).mockImplementation(() => ({
authenticated: false,
authenticating: false,
}));

const loginMock = jest.fn(
() =>
new Promise(resolve => {
return resolve({
data: {
tokenCreate: {
errors: [],
user: {
userPermissions: [
{
code: "MANAGE_USERS",
name: "Handle checkouts",
},
],
},
},
},
});
}),
);

(useAuth as jest.Mock).mockImplementation(() => ({
login: loginMock,
logout: jest.fn(),
}));

const { result } = renderHook(() => useAuthProvider({ intl, notify, apolloClient }));

// Simulate two concurrent login attempts
result.current.login!("email", "password");
result.current.login!("email", "password");

expect(loginMock).toHaveBeenCalledTimes(1);
});
});
7 changes: 7 additions & 0 deletions src/auth/components/LoginPage/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ export const errorMessages = defineMessages({
defaultMessage: "You don't have permission to login.",
description: "error message",
},
loginAttemptDelay: {
defaultMessage: "Please wait a moment before trying again.",
description: "error message",
id: "RyZd9J",
},
});

export function getErrorMessage(err: UserContextError, intl: IntlShape): string {
Expand All @@ -36,5 +41,7 @@ export function getErrorMessage(err: UserContextError, intl: IntlShape): string
return intl.formatMessage(errorMessages.serverError);
case "noPermissionsError":
return intl.formatMessage(errorMessages.noPermissionsError);
case "loginAttemptDelay":
return intl.formatMessage(errorMessages.loginAttemptDelay);
}
}
28 changes: 25 additions & 3 deletions src/auth/hooks/useAuthProvider.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { ApolloClient, ApolloError } from "@apollo/client";
import { IMessageContext } from "@dashboard/components/messages";
import { DEMO_MODE } from "@dashboard/config";
import { useUserDetailsQuery } from "@dashboard/graphql";
import { AccountErrorCode, useUserDetailsQuery } from "@dashboard/graphql";
import useLocalStorage from "@dashboard/hooks/useLocalStorage";
import useNavigator from "@dashboard/hooks/useNavigator";
import { commonMessages } from "@dashboard/intl";
Expand Down Expand Up @@ -33,12 +33,14 @@ export interface UseAuthProviderOpts {
notify: IMessageContext;
apolloClient: ApolloClient<any>;
}
type AuthErrorCodes = `${AccountErrorCode}`;

export function useAuthProvider({ intl, notify, apolloClient }: UseAuthProviderOpts): UserContext {
const { login, getExternalAuthUrl, getExternalAccessToken, logout } = useAuth();
const navigate = useNavigator();
const { authenticated, authenticating, user } = useAuthState();
const [requestedExternalPluginId] = useLocalStorage("requestedExternalPluginId", null);
const [isAuthRequestRunning, setIsAuthRequestRunning] = useState(false);
const [errors, setErrors] = useState<UserContextError[]>([]);
const permitCredentialsAPI = useRef(true);

Expand Down Expand Up @@ -112,27 +114,45 @@ export function useAuthProvider({ intl, notify, apolloClient }: UseAuthProviderO
}
}
};

const handleLogin = async (email: string, password: string) => {
if (isAuthRequestRunning) {
return;
}

try {
setIsAuthRequestRunning(true);

const result = await login({
email,
password,
includeDetails: false,
});

const errorList = result.data?.tokenCreate?.errors?.map(
({ code }) => code,
// SDK is deprecated and has outdated types - we need to use ones from Dashboard
) as AuthErrorCodes[];

if (isEmpty(result.data?.tokenCreate?.user?.userPermissions)) {
setErrors(["noPermissionsError"]);
await handleLogout();
}

if (result && !result.data?.tokenCreate?.errors.length) {
if (result && !errorList?.length) {
if (DEMO_MODE) {
displayDemoMessage(intl, notify);
}

saveCredentials(result.data?.tokenCreate?.user!, password);
} else {
setErrors(["loginError"]);
// While login page can show multiple errors, "loginError" doesn't match "attemptDelay"
// and should be shown when no other error is present
if (errorList?.includes(AccountErrorCode.LOGIN_ATTEMPT_DELAYED)) {
setErrors(["loginAttemptDelay"]);
} else {
setErrors(["loginError"]);
}
}

await logoutNonStaffUser(result.data?.tokenCreate!);
Expand All @@ -144,6 +164,8 @@ export function useAuthProvider({ intl, notify, apolloClient }: UseAuthProviderO
} else {
setErrors(["unknownLoginError"]);
}
} finally {
setIsAuthRequestRunning(false);
}
};
const handleRequestExternalLogin = async (pluginId: string, input: RequestExternalLoginInput) => {
Expand Down
1 change: 1 addition & 0 deletions src/auth/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const UserContextError = {
serverError: "serverError",
noPermissionsError: "noPermissionsError",
externalLoginError: "externalLoginError",
loginAttemptDelay: "loginAttemptDelay",
unknownLoginError: "unknownLoginError",
} as const;

Expand Down
6 changes: 5 additions & 1 deletion src/auth/views/Login.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ const LoginView: React.FC<LoginViewProps> = ({ params }) => {
setRequestedExternalPluginId,
} = useAuthParameters();
const handleSubmit = async (data: LoginFormData) => {
const result = await login!(data.email, data.password);
if (!login) {
return;
}

const result = await login(data.email, data.password);
const errors = result?.errors || [];

return errors;
Expand Down
Loading