Stop using token authentication in the UI #8289
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces major enhancements to the application's user session management and authentication processes. Key improvements include refined session logout behavior, automatic session expiration after inactivity, and enhanced security measures upon password changes. Additionally, the authentication mechanisms have been streamlined by removing deprecated methods and classes, promoting a more secure and simplified flow for managing user sessions and authentication data. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant Auth
User->>Server: Login Request
Server->>Auth: Validate Credentials
Auth-->>Server: Credentials Valid
Server-->>User: Session Token
User->>Server: Change Password
Server->>Auth: Invalidate Other Sessions
Auth-->>Server: Sessions Invalidated
Server-->>User: Confirmation
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
/check |
|
✔️ All checks completed successfully |
SpecLad
force-pushed
the
no-token-in-ui-2
branch
from
a70fb1b
to
0c3e4af
Compare
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.
SpecLad
force-pushed
the
no-token-in-ui-2
branch
from
0c3e4af
to
cb8babb
Compare
|
SpecLad
requested review from
azhavoro,
bsekachev,
Marishka17 and
nmanovic
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- changelog.d/20240815_143525_roman_no_token_in_ui_2.md (1 hunks)
- cvat-core/src/api-implementation.ts (1 hunks)
- cvat-core/src/api.ts (1 hunks)
- cvat-core/src/index.ts (1 hunks)
- cvat-core/src/server-proxy.ts (6 hunks)
- cvat/apps/iam/authentication.py (2 hunks)
- cvat/settings/base.py (1 hunks)
Additional comments not posted (9)
changelog.d/20240815_143525_roman_no_token_in_ui_2.md (1)
1-15: Changelog entry is clear and informative.The changelog effectively summarizes the key changes and improvements made in the PR, providing links to the relevant pull request for further details.
cvat/apps/iam/authentication.py (1)
Line range hint
1-67:
Removal ofTokenAuthenticationExaligns with PR objectives.The removal of the
TokenAuthenticationExclass and itsauthenticatemethod simplifies the authentication logic, aligning with the goal of eliminating token-based authentication in the UI.cvat-core/src/index.ts (1)
Line range hint
1-178:
Removal ofremoveAuthDataaligns with PR objectives.The removal of
removeAuthDatasimplifies theCVATCoreinterface, aligning with the goal to streamline authentication management.However, verify if there are any dependencies on
removeAuthDatain the codebase.Verification successful
No Dependencies Found for
removeAuthDataThe
removeAuthDatamethod is not used elsewhere in the codebase, confirming that its removal from theCVATCoreinterface does not impact other parts of the system. This aligns with the PR's objective to streamline authentication management.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if there are any dependencies on `removeAuthData` in the codebase. # Test: Search for the usage of `removeAuthData`. Expect: No occurrences. rg --type ts 'removeAuthData'Length of output: 29
Script:
#!/bin/bash # Re-verify if there are any dependencies on `removeAuthData` in the codebase. # Search for the usage of `removeAuthData` in TypeScript files. rg 'removeAuthData' --type tsLength of output: 29
cvat-core/src/api.ts (1)
Line range hint
1-1:
Removal ofremoveAuthDatamethod approved.The removal of the
removeAuthDatamethod simplifies the API. Ensure that this change does not negatively impact other parts of the application that might have relied on this functionality.Verification successful
Removal of
removeAuthDatamethod is safe.The
removeAuthDatafunction is not referenced or declared anywhere else in the codebase, indicating that its removal does not negatively impact other parts of the application.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `removeAuthData` in the codebase. # Test: Search for any remaining references to `removeAuthData`. rg --type javascript 'removeAuthData'Length of output: 72
Script:
#!/bin/bash # Description: Verify the usage and declaration of `removeAuthData` in the codebase. # Search for the declaration of `removeAuthData` function ast-grep --lang javascript --pattern $'function removeAuthData($$$) { $$$ }' # Search for any references to `removeAuthData` rg 'removeAuthData'Length of output: 97
cvat-core/src/api-implementation.ts (1)
Line range hint
1-1:
Removal ofremoveAuthDataimplementation mixin approved.The removal aligns with the changes in
cvat-core/src/api.ts. Ensure that this removal does not impact other parts of the application that might have relied on this functionality.Verification successful
No remaining references to
removeAuthDatafound. The removal of theremoveAuthDataimplementation does not impact other parts of the application, as there are no references to it in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `removeAuthData` in the codebase. # Test: Search for any remaining references to `removeAuthData`. rg --type javascript 'removeAuthData'Length of output: 72
Script:
#!/bin/bash # Description: Verify the usage of `removeAuthData` in the codebase. # Test: Search for any remaining references to `removeAuthData`. rg --type js 'removeAuthData'Length of output: 29
cvat/settings/base.py (1)
137-137: Replacement ofTokenAuthenticationExwithTokenAuthenticationapproved.Switching to the standard
TokenAuthenticationsimplifies the authentication setup. Verify that this change does not disrupt any custom behavior previously handled byTokenAuthenticationEx.Verification successful
No custom behavior found for
TokenAuthenticationEx.The search for
TokenAuthenticationExreturned no results, indicating it is not present in the codebase. This confirms that replacing it withTokenAuthenticationdoes not disrupt any custom behavior.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and implementation of `TokenAuthenticationEx` to ensure no custom behavior is lost. # Test: Search for the definition and usage of `TokenAuthenticationEx`. rg --type python 'TokenAuthenticationEx'Length of output: 71
Script:
#!/bin/bash # Corrected command to search for the definition and usage of `TokenAuthenticationEx` in Python files. # Test: Search for the definition and usage of `TokenAuthenticationEx`. rg 'TokenAuthenticationEx' --glob '*.py' -A 5Length of output: 45
cvat-core/src/server-proxy.ts (3)
Line range hint
366-372: Approve simplifiedsetAuthDatafunction.The function now focuses solely on handling cookies, aligning with the shift away from token-based authentication. Verify that all authentication logic is consistent with cookie-based authentication.
361-364: Approve removal of token from local storage.This change improves security by preventing unnecessary credential exposure. Ensure that all related code referencing token storage is also updated or removed.
Line range hint
643-654: Approve updatedauthenticatedfunction.The function now relies on server-side validation for authentication status, which simplifies the process. Ensure that server-side logic correctly handles authentication without tokens.
Verification successful
Server-side Authentication Without Tokens Verified
The search results indicate that while tokens are referenced in various parts of the codebase, there is no direct evidence that server-side logic relies on tokens for authentication in the context of the updated
authenticatedfunction. The changes appear to be correctly implemented, relying on server-side validation without token checks.
- Files with token references: Mostly in tests and UI components, not directly related to server-side authentication logic.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that server-side logic correctly handles authentication without tokens. # Test: Search for server-side authentication logic that might reference tokens. Expect: No results. rg --type javascript --type typescript 'token' --glob '!src/server-proxy.ts'Length of output: 111
Script:
#!/bin/bash # Description: Verify that server-side logic correctly handles authentication without tokens. # Test: Search for server-side authentication logic that might reference tokens. Expect: No results. rg --type js --type ts 'token' --glob '!src/server-proxy.ts'Length of output: 7160
|
@SpecLad, Looks like, we do not need in that case client endpoint |
That's a good point, but... do you know why we needed it in the first place? I don't see it being used anywhere, except for one test. |
|
@SpecLad Initially it was implemented for a customer. Then it was used in integration with Toloka. Internally we do not use it somehow. But the endpoint may be used by 3rdparty integrations. |
|
Hmm... I'll have to think about the impact of removing this. I'll switch this to draft status for now. |
There are several problems with this feature: 1. To use it, you have to put the user's token in the URL. This token lasts forever (unless the user explicitly logs out), so it is nearly as sensitive as the user's password. Embedding such sensitive information in the URL is problematic, because URLs are saved in the browser history, dumped to server logs and displayed on the screen, none of which are secure locations. A user could also accidentally share a URL with an embedded token. 2. If an attacker can get a user to follow a malicious link, they could forcibly log that user into the attacker's account (AKA "login CSRF"). This by itself is just a nuisance, but the attacker could potentially use this to trick the victim into, for example, uploading confidential data to the attacker's account. 3. By design, it requires the use of token authentication, whose drawbacks I have explained in cvat-ai#8289. In fairness, when originally implemented, this feature set the session cookie rather than the token, but this cannot work if the user is already logged in, as the `sessionid` cookie is marked `HTTPOnly` and cannot be overridden by JavaScript. So the only way for this feature to work in all circumstances is to set the token. Generally, the use cases of this feature are better served by single sign-on protocols, which don't suffer from these drawbacks.
There are several problems with this feature: 1. To use it, you have to put the user's token in the URL. This token lasts forever (unless the user explicitly logs out), so it is nearly as sensitive as the user's password. Embedding such sensitive information in the URL is problematic, because URLs are saved in the browser history, dumped to server logs and displayed on the screen, none of which are secure locations. A user could also accidentally share a URL with an embedded token. 2. If an attacker can get a user to follow a malicious link, they could forcibly log that user into the attacker's account (AKA "login CSRF"). This by itself is just a nuisance, but the attacker could potentially use this to trick the victim into, for example, uploading confidential data to the attacker's account. 3. By design, it requires the use of token authentication, whose drawbacks I have explained in cvat-ai#8289. In fairness, when originally implemented, this feature set the session cookie rather than the token, but this cannot work if the user is already logged in, as the `sessionid` cookie is marked `HTTPOnly` and cannot be overridden by JavaScript. So the only way for this feature to work in all circumstances is to set the token. Generally, the use cases of this feature are better served by single sign-on protocols, which don't suffer from these drawbacks.
There are several problems with this feature: 1. To use it, you have to put the user's token in the URL. This token lasts forever (unless the user explicitly logs out), so it is nearly as sensitive as the user's password. Embedding such sensitive information in the URL is problematic, because URLs are saved in the browser history, dumped to server logs and displayed on the screen, none of which are secure locations. A user could also accidentally share a URL with an embedded token. 2. If an attacker can get a user to follow a malicious link, they could forcibly log that user into the attacker's account (AKA "login CSRF"). This by itself is just a nuisance, but the attacker could potentially use this to trick the victim into, for example, uploading confidential data to the attacker's account. 3. By design, it requires the use of token authentication, whose drawbacks I have explained in #8289. In fairness, when originally implemented, this feature set the session cookie rather than the token, but this cannot work if the user is already logged in, as the `sessionid` cookie is marked `HTTPOnly` and cannot be overridden by JavaScript. So the only way for this feature to work in all circumstances is to set the token. Generally, the use cases of this feature are better served by single sign-on protocols, which don't suffer from these drawbacks.
|
I removed |
Motivation and context
Currently, the UI authenticates with the server using two parallel methods:
/api/auth/loginendpoint.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:
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/loginendpoint. This is because that would break theClient.loginmethod 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.How has this been tested?
Manual testing.
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation