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

T1486 can not log in 2fa activated #1638

Open
wants to merge 59 commits into
base: 14.0
Choose a base branch
from

Conversation

clementcharmillot
Copy link
Contributor

@clementcharmillot clementcharmillot commented Jul 31, 2024

This is a draft to support token-based authentication on the Translation Platform.

Currently self-contained in its own module for simplicity purpose.

TODO before merging

  • Write unit / integration tests to verify the security of the implementation
  • Fix points marked "TODO" in the code
  • Improve documentation

Related PR to modify the frontend

CompassionCH/translation-platform-web#41
Note: The current PR must be merged/deployed before the related frontend PR. Otherwise, the frontend will make requests that the backend doesn't understand.

Copy link

sonarcloud bot commented Jul 31, 2024

@nlachat-compassion
Copy link
Contributor

I tested this PR locally.
Authentication for users with 2FA enabled seems to work with the modified frontend ( CompassionCH/translation-platform-web#41 ).
Looking at the code, the implementation seems secure. However, I would like to write unit/integration tests to increase certainty, as this is very sensitive code.

@nlachat-compassion
Copy link
Contributor

In the process of trying to store the tokens in HttpOnly cookies, I discovered that the XmlRpcClient library used by the frontend (import { XmlRpcClient } from '@foxglove/xmlrpc';) does not support the withCredentials property (https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/withCredentials). This means that the access token is not being sent with XmlRpc requests, and thus the server rejects them with AccessDenied. It would require substantial efforts to use another library or to re-implement the library with the required withCredentials property.
For now, I will revert the changes made in order to use cookies for the tokens. Maybe in the future, the transition to a new XmlRpc library can be made, and thus the tokens could be stored in cookies, which is more secure than the current implementation using localStorage.

@nlachat-compassion
Copy link
Contributor

Quality Gate Failed Quality Gate failed

Failed conditions 5 Security Hotspots

See analysis details on SonarCloud

These are false positives.

Copy link

sonarcloud bot commented Oct 29, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
5 Security Hotspots

See analysis details on SonarCloud

@nlachat-compassion
Copy link
Contributor

Pre-commit is failing because of setup files:

setup/account_reconcile_checkout/odoo/addons/account_reconcile_checkout
setup/account_reconcile_checkout/setup.py
setup/auth_external/odoo/addons/auth_external
setup/auth_external/setup.py
setup/crm_switzerland/odoo/addons/crm_switzerland
setup/crm_switzerland/setup.py

I don't know what they're used for. Should they be committed?

@nlachat-compassion nlachat-compassion marked this pull request as ready for review October 29, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants