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

HL-891: Fix CSRF token issues #2182

Merged
merged 6 commits into from
Aug 22, 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
1 change: 1 addition & 0 deletions backend/benefit/helsinkibenefit/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@
CSRF_TRUSTED_ORIGINS = env.list("CSRF_TRUSTED_ORIGINS")
CSRF_COOKIE_NAME = env.str("CSRF_COOKIE_NAME")
CSRF_COOKIE_SECURE = True
CSRF_USE_SESSIONS = True

# Audit logging
AUDIT_LOG_ORIGIN = env.str("AUDIT_LOG_ORIGIN")
Expand Down
2 changes: 1 addition & 1 deletion backend/benefit/helsinkibenefit/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
path("v1/company/", GetUsersOrganizationView.as_view()),
path("v1/company/search/<str:name>/", SearchOrganisationsView.as_view()),
path("v1/company/get/<str:business_id>/", GetOrganisationByIdView.as_view()),
path("v1/users/me/", CurrentUserView.as_view()),
path("v1/users/me/", CurrentUserView.as_view(), name="users-me"),
path("v1/users/options/", UserOptionsView.as_view()),
path(
"v1/handlerapplications/<str:application_id>/review/", ReviewStateView.as_view()
Expand Down
5 changes: 4 additions & 1 deletion backend/benefit/users/api/v1/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from django.contrib.auth.base_user import AbstractBaseUser
from django.core.exceptions import PermissionDenied
from django.db import DatabaseError, transaction
from django.middleware.csrf import get_token
from django.shortcuts import get_object_or_404
from drf_spectacular.utils import extend_schema
from helsinki_gdpr.views import DeletionNotAllowed, DryRunException, GDPRAPIView
Expand Down Expand Up @@ -30,7 +31,9 @@ def get(self, request):
serializer = UserSerializer(
self._get_current_user(request), context={"request": request}
)
return Response(serializer.data)
response = serializer.data
response["csrf_token"] = get_token(request)
return Response(response)

def _get_current_user(self, request):
if not request.user.is_authenticated:
Expand Down
13 changes: 13 additions & 0 deletions backend/benefit/users/tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,23 @@
import factory
import pytest
from rest_framework.test import APIClient

from applications.tests.factories import ApplicationFactory
from common.tests.conftest import * # noqa
from companies.tests.conftest import * # noqa
from helsinkibenefit.tests.conftest import * # noqa


@pytest.fixture
def gdpr_api_client():
return APIClient()


@pytest.fixture
def application(mock_get_organisation_roles_and_create_company):
# Application which belongs to logged in user company
with factory.Faker.override_default_locale("fi_FI"):
app = ApplicationFactory()
app.company = mock_get_organisation_roles_and_create_company
app.save()
return app
11 changes: 11 additions & 0 deletions backend/benefit/users/tests/test_user_api.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from rest_framework.reverse import reverse

from common.tests.conftest import get_client_user


def test_applications_unauthorized(api_client, application):
response = api_client.get(reverse("users-me"))
user = get_client_user(api_client)
assert response.status_code == 200
assert response.data["id"] == str(user.id)
assert len(response.data["csrf_token"]) > 0
1 change: 1 addition & 0 deletions frontend/benefit/applicant/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,4 +113,5 @@ export const SUBMITTED_STATUSES = [

export enum LOCAL_STORAGE_KEYS {
IS_TERMS_OF_SERVICE_APPROVED = 'isTermsOfServiceApproved',
CSRF_TOKEN = 'csrfToken',
}
5 changes: 3 additions & 2 deletions frontend/benefit/applicant/src/hooks/useUserQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { useQuery, UseQueryResult } from 'react-query';
import showErrorToast from 'shared/components/toast/show-error-toast';
import useBackendAPI from 'shared/hooks/useBackendAPI';
import useLocale from 'shared/hooks/useLocale';
import { setLocalStorageItem } from 'shared/utils/localstorage.utils';

import { LOCAL_STORAGE_KEYS } from '../constants';

Expand Down Expand Up @@ -51,9 +52,9 @@ const useUserQuery = (
select: (data) => camelcaseKeys(data, { deep: true }),
onError: (error) => handleError(error),
onSuccess: (data) => {
setLocalStorageItem(LOCAL_STORAGE_KEYS.CSRF_TOKEN, data.csrfToken);
if (data.id && data.termsOfServiceApprovalNeeded)
// eslint-disable-next-line scanjs-rules/identifier_localStorage
localStorage.setItem(
setLocalStorageItem(
LOCAL_STORAGE_KEYS.IS_TERMS_OF_SERVICE_APPROVED,
'false'
);
Expand Down
1 change: 1 addition & 0 deletions frontend/benefit/applicant/src/pages/_app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ const App: React.FC<AppProps> = (appProps) => {
<BackendAPIProvider
baseURL={getBackendDomain()}
headers={getHeaders(locale)}
isLocalStorageCsrf
>
<QueryClientProvider client={queryClient}>
<AuthProvider>
Expand Down
8 changes: 4 additions & 4 deletions frontend/benefit/applicant/src/pages/login.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ import {
$GridCell,
} from 'shared/components/forms/section/FormSection.sc';
import getServerSideTranslations from 'shared/i18n/get-server-side-translations';
import { removeLocalStorageItem } from 'shared/utils/localstorage.utils';

import { IS_CLIENT, LOCAL_STORAGE_KEYS } from '../constants';
import { LOCAL_STORAGE_KEYS } from '../constants';

type NotificationProps =
| (Pick<HDSNotificationProps, 'type' | 'label'> & {
Expand Down Expand Up @@ -60,9 +61,8 @@ const Login: NextPage = () => {
}, [logout, queryClient]);

useEffect(() => {
if (IS_CLIENT)
// eslint-disable-next-line scanjs-rules/identifier_localStorage
localStorage.removeItem(LOCAL_STORAGE_KEYS.IS_TERMS_OF_SERVICE_APPROVED);
removeLocalStorageItem(LOCAL_STORAGE_KEYS.IS_TERMS_OF_SERVICE_APPROVED);
removeLocalStorageItem(LOCAL_STORAGE_KEYS.CSRF_TOKEN);
}, []);

return (
Expand Down
4 changes: 4 additions & 0 deletions frontend/benefit/handler/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,3 +193,7 @@ export const ALL_APPLICATION_STATUSES: APPLICATION_STATUSES[] = [
APPLICATION_STATUSES.DRAFT,
APPLICATION_STATUSES.REJECTED,
];

export enum LOCAL_STORAGE_KEYS {
CSRF_TOKEN = 'csrfToken',
}
10 changes: 8 additions & 2 deletions frontend/benefit/handler/src/hooks/useUserQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ import { useQuery, UseQueryResult } from 'react-query';
import useBackendAPI from 'shared/hooks/useBackendAPI';
import useLocale from 'shared/hooks/useLocale';
import User from 'shared/types/user';
import { setLocalStorageItem } from 'shared/utils/localstorage.utils';

import { ROUTES } from '../constants';
import { LOCAL_STORAGE_KEYS, ROUTES } from '../constants';
import useLogout from './useLogout';

// check that authentication is still alive in every 5 minutes
Expand Down Expand Up @@ -46,6 +47,11 @@ const useUserQuery = <T extends User>(
}
};

const onSuccessHandler = (user: User): void => {
checkForStaffStatus(user);
setLocalStorageItem(LOCAL_STORAGE_KEYS.CSRF_TOKEN, user.csrf_token);
};

return useQuery(
`${BackendEndpoint.USER_ME}`,
() => handleResponse<User>(axios.get(BackendEndpoint.USER_ME)),
Expand All @@ -54,7 +60,7 @@ const useUserQuery = <T extends User>(
enabled: !logout,
retry: false,
select,
onSuccess: checkForStaffStatus,
onSuccess: onSuccessHandler,
onError: (error) => handleError(error),
}
);
Expand Down
1 change: 1 addition & 0 deletions frontend/benefit/handler/src/pages/_app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const App: React.FC<AppProps> = (appProps) => {
<BackendAPIProvider
baseURL={getBackendDomain()}
headers={getHeaders(locale)}
isLocalStorageCsrf
>
<AppContextProvider>
<QueryClientProvider client={queryClient}>
Expand Down
7 changes: 7 additions & 0 deletions frontend/benefit/handler/src/pages/login.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@ import React, { useEffect } from 'react';
import { useQueryClient } from 'react-query';
import Container from 'shared/components/container/Container';
import getServerSideTranslations from 'shared/i18n/get-server-side-translations';
import { removeLocalStorageItem } from 'shared/utils/localstorage.utils';
import { useTheme } from 'styled-components';

import { LOCAL_STORAGE_KEYS } from '../constants';

type NotificationProps = Pick<HDSNotificationProps, 'type' | 'label'> & {
content?: string;
};
Expand Down Expand Up @@ -61,6 +64,10 @@ const Login: NextPage = () => {
}
}, [router.query.logout, router.query.userStateError, queryClient]);

useEffect(() => {
removeLocalStorageItem(LOCAL_STORAGE_KEYS.CSRF_TOKEN);
}, []);

return (
<Container>
<Notification
Expand Down
2 changes: 2 additions & 0 deletions frontend/benefit/shared/src/types/application.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ export type UserData = {
last_name: string;
terms_of_service_approval_needed: boolean;
terms_of_service_in_effect: TermsOfServiceInEffectData;
csrf_token: string;
};

export type ApplicantConsents = {
Expand All @@ -528,6 +529,7 @@ export type User = {
lastName: string;
termsOfServiceApprovalNeeded: boolean;
termsOfServiceInEffect: TermsOfServiceInEffect;
csrfToken: string;
};

export type ApproveTermsOfServiceResponseData = {
Expand Down
7 changes: 6 additions & 1 deletion frontend/shared/src/backend-api/BackendAPIProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,30 @@ import Axios from 'axios';
import React from 'react';
import { getLastCookieValue } from 'shared/cookies/get-last-cookie-value';
import { Headers } from 'shared/types/common';
import { getLocalStorageItem } from 'shared/utils/localstorage.utils';

import BackendAPIContext from './BackendAPIContext';

export interface BackendAPIProviderProps {
baseURL: string;
headers?: Headers;
isLocalStorageCsrf?: boolean;
}

const BackendAPIProvider: React.FC<BackendAPIProviderProps> = ({
baseURL,
headers,
isLocalStorageCsrf = false,
children,
}): JSX.Element => (
<BackendAPIContext.Provider
value={Axios.create({
baseURL,
headers: {
'Content-Type': 'application/json',
'X-CSRFToken': getLastCookieValue('yjdhcsrftoken'),
'X-CSRFToken': isLocalStorageCsrf
? getLocalStorageItem('csrfToken')
: getLastCookieValue('yjdhcsrftoken'),
...headers,
},
withCredentials: true,
Expand Down
1 change: 1 addition & 0 deletions frontend/shared/src/types/user.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ type User = {
name: string;
organization_name?: string;
is_staff?: boolean;
csrf_token?: string;
};
export default User;
14 changes: 14 additions & 0 deletions frontend/shared/src/utils/localstorage.utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/* eslint-disable scanjs-rules/identifier_localStorage */

const IS_CLIENT = typeof window !== 'undefined';

export const getLocalStorageItem = (key: string): string =>
IS_CLIENT ? localStorage.getItem(key) || '' : '';

export const setLocalStorageItem = (key: string, value: string): void =>
IS_CLIENT && localStorage.setItem(key, value);

export const removeLocalStorageItem = (key: string): void =>
IS_CLIENT && localStorage.removeItem(key);

/* eslint-enable scanjs-rules/identifier_localStorage */
Loading