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

Stop using token authentication in the UI #8289

Merged
merged 1 commit into from
Aug 22, 2024
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
Stop using token authentication in the UI
Currently, the UI authenticates with the server using two parallel methods:

* A cookie set by the `/api/auth/login` endpoint.
* A token returned by the same endpoint.

This is redundant and confusing, and also causes several usability &
security issues:

* If a user creates 2 or more concurrent sessions (e.g. logs in on two
  computers), and then logs out of one of them, it will effectively log
  them out of all other sessions too. This happens because:

  1. The same token is shared between all sessions.
  2. Logging out destroys the token in the DB.
  3. The server tries to authenticate the browser using the token first,
     so if a browser presents a token that's no longer present in the DB,
     the server responds with a 401 (even if the cookie is still valid).

* When a user changes their password, Django invalidates all of that user's
  other sessions... except that doesn't work, because the user's token
  remains valid. This is bad, because if an attacker steals a user's
  password and logs in, the most obvious recourse (changing the password)
  will not work - the attacker will stay logged in.

* Sessions effectively last forever, because, while Django's session data
  expires after `SESSION_COOKIE_AGE`, the token never expires.

* The token is stored in local storage, so it could be stolen in an XSS
  attack. The session cookie is not susceptible to this, because it's marked
  `HttpOnly`.

The common theme in all these problems is the token, so by ceasing to use it
we can fix them all.

Note that this patch does not remove the server-side token creation &
authentication logic, or remove the token from the output of the
`/api/auth/login` endpoint. This is because that would break the
`Client.login` method in the SDK. However, I believe that in the future we
should get rid of the whole "generate token on login" logic, and
let users create API tokens explicitly if (and only if) they wish
to use the SDK.
  • Loading branch information
SpecLad committed Aug 15, 2024
commit cb8babbc2d41432bc6f4049610e41a97d1c75024
15 changes: 15 additions & 0 deletions changelog.d/20240815_143525_roman_no_token_in_ui_2.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
### Fixed

- Logging out of one session will no longer log the user out of all their
other sessions
(<https://github.com/cvat-ai/cvat/pull/8289>)

### Changed

- User sessions now expire after two weeks of inactivity
(<https://github.com/cvat-ai/cvat/pull/8289>)

- A user changing their password will now invalidate all of their sessions
except for the current one
(<https://github.com/cvat-ai/cvat/pull/8289>)

4 changes: 0 additions & 4 deletions cvat-core/src/api-implementation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,6 @@ export default function implementAPI(cvat: CVATCore): CVATCore {
const result = await serverProxy.server.setAuthData(response);
return result;
});
implementationMixin(cvat.server.removeAuthData, async () => {
const result = await serverProxy.server.removeAuthData();
return result;
});
implementationMixin(cvat.server.installedApps, async () => {
const result = await serverProxy.server.installedApps();
return result;
Expand Down
4 changes: 0 additions & 4 deletions cvat-core/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,6 @@ function build(): CVATCore {
const result = await PluginRegistry.apiWrapper(cvat.server.setAuthData, response);
return result;
},
async removeAuthData() {
const result = await PluginRegistry.apiWrapper(cvat.server.removeAuthData);
return result;
},
async installedApps() {
const result = await PluginRegistry.apiWrapper(cvat.server.installedApps);
return result;
Expand Down
1 change: 0 additions & 1 deletion cvat-core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ export default interface CVATCore {
healthCheck: any;
request: any;
setAuthData: any;
removeAuthData: any;
installedApps: any;
apiSchema: typeof serverProxy.server.apiSchema;
};
Expand Down
31 changes: 4 additions & 27 deletions cvat-core/src/server-proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@
return new ServerError(message, 0);
}

function prepareData(details) {

Check warning on line 247 in cvat-core/src/server-proxy.ts

View workflow job for this annotation

GitHub Actions / Linter

Missing return type on function
const data = new FormData();
for (const [key, value] of Object.entries(details)) {
if (Array.isArray(value)) {
Expand Down Expand Up @@ -286,7 +286,7 @@
return requestId++;
}

async function get(url: string, requestConfig) {

Check warning on line 289 in cvat-core/src/server-proxy.ts

View workflow job for this annotation

GitHub Actions / Linter

Missing return type on function
return new Promise((resolve, reject) => {
const newRequestId = getRequestId();
requests[newRequestId] = { resolve, reject };
Expand Down Expand Up @@ -358,10 +358,10 @@
return response;
});

let token = store.get('token');
if (token) {
Axios.defaults.headers.common.Authorization = `Token ${token}`;
}
// Previously, we used to store an additional authentication token in local storage.
// Now we don't, and if the user still has one stored, we'll remove it to prevent
// unnecessary credential exposure.
store.remove('token');

function setAuthData(response: AxiosResponse): void {
if (response.headers['set-cookie']) {
Expand All @@ -370,18 +370,6 @@
const cookies = response.headers['set-cookie'].join(';');
Axios.defaults.headers.common.Cookie = cookies;
}

if (response.data.key) {
token = response.data.key;
store.set('token', token);
Axios.defaults.headers.common.Authorization = `Token ${token}`;
}
}

function removeAuthData(): void {
Axios.defaults.headers.common.Authorization = '';
store.remove('token');
token = null;
}

async function about(): Promise<SerializedAbout> {
Expand Down Expand Up @@ -474,7 +462,6 @@
}

async function login(credential: string, password: string): Promise<void> {
removeAuthData();
let authenticationResponse = null;
try {
authenticationResponse = await Axios.post(`${config.backendAPI}/auth/login`, {
Expand All @@ -491,7 +478,6 @@
async function logout(): Promise<void> {
try {
await Axios.post(`${config.backendAPI}/auth/logout`);
removeAuthData();
} catch (errorData) {
throw generateError(errorData);
}
Expand Down Expand Up @@ -570,17 +556,9 @@

async function authenticated(): Promise<boolean> {
try {
// In CVAT app we use two types of authentication
// At first we check if authentication token is present
// Request in getSelf will provide correct authentication cookies
if (!store.get('token')) {
removeAuthData();
return false;
}
await getSelf();
} catch (serverError) {
if (serverError.code === 401) {
removeAuthData();
return false;
}

Expand Down Expand Up @@ -841,7 +819,7 @@
save_images: saveImages,
};
return new Promise<string | void>((resolve, reject) => {
async function request() {

Check warning on line 822 in cvat-core/src/server-proxy.ts

View workflow job for this annotation

GitHub Actions / Linter

Missing return type on function
Axios.post(baseURL, {}, {
params,
})
Expand Down Expand Up @@ -933,7 +911,7 @@
const url = `${backendAPI}/tasks/${id}/backup/export`;

return new Promise<string | void>((resolve, reject) => {
async function request() {

Check warning on line 914 in cvat-core/src/server-proxy.ts

View workflow job for this annotation

GitHub Actions / Linter

Missing return type on function
try {
const response = await Axios.post(url, {}, {
params,
Expand Down Expand Up @@ -1012,7 +990,7 @@
const url = `${backendAPI}/projects/${id}/backup/export`;

return new Promise<string | void>((resolve, reject) => {
async function request() {

Check warning on line 993 in cvat-core/src/server-proxy.ts

View workflow job for this annotation

GitHub Actions / Linter

Missing return type on function
try {
const response = await Axios.post(url, {}, {
params,
Expand Down Expand Up @@ -1137,7 +1115,7 @@
message: 'CVAT is uploading task data to the server',
}));

async function bulkUpload(taskId, files) {

Check warning on line 1118 in cvat-core/src/server-proxy.ts

View workflow job for this annotation

GitHub Actions / Linter

Missing return type on function
const fileBulks = files.reduce((fileGroups, file) => {
const lastBulk = fileGroups[fileGroups.length - 1];
if (chunkSize - lastBulk.size >= file.size) {
Expand Down Expand Up @@ -2345,7 +2323,6 @@
export default Object.freeze({
server: Object.freeze({
setAuthData,
removeAuthData,
about,
share,
formats,
Expand Down
17 changes: 0 additions & 17 deletions cvat/apps/iam/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
from django.core import signing
from rest_framework import exceptions
from rest_framework.authentication import BaseAuthentication
from rest_framework.authentication import TokenAuthentication
from django.contrib.auth import login
from django.contrib.auth import get_user_model
from furl import furl
import hashlib
Expand Down Expand Up @@ -52,21 +50,6 @@ def unsign(self, signature, url):
except User.DoesNotExist:
raise signing.BadSignature()

# Even with token authentication it is very important to have a valid session id
# in cookies because in some cases we cannot use token authentication (e.g. when
# we redirect to the server in UI using just URL). To overkill that we override
# the class to call `login` method which restores the session id in cookies.
class TokenAuthenticationEx(TokenAuthentication):
def authenticate(self, request):
auth = super().authenticate(request)
# drf_spectacular uses mock requests without session field
session = getattr(request, 'session', None)
if (auth is not None and
session is not None and
(session.session_key is None or (not session.modified and not session.load()))):
login(request, auth[0], 'django.contrib.auth.backends.ModelBackend')
return auth

class SignatureAuthentication(BaseAuthentication):
"""
Authentication backend for signed URLs.
Expand Down
2 changes: 1 addition & 1 deletion cvat/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ def generate_secret_key():
'cvat.apps.iam.permissions.PolicyEnforcer',
],
'DEFAULT_AUTHENTICATION_CLASSES': [
'cvat.apps.iam.authentication.TokenAuthenticationEx',
'rest_framework.authentication.TokenAuthentication',
'cvat.apps.iam.authentication.SignatureAuthentication',
'rest_framework.authentication.SessionAuthentication',
'rest_framework.authentication.BasicAuthentication'
Expand Down

This file was deleted.

Loading