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

[17.0][IMP] auth_oauth_multi_token: make it compatible with odoo.sh "login as" #764

Open
wants to merge 1 commit into
base: 17.0
Choose a base branch
from

Conversation

CRogos
Copy link
Contributor

@CRogos CRogos commented Feb 17, 2025

Odoo.sh provides an option to login-in as a specified user.
When you install this OCA module auth_oauth_multi_token , the "connect" or "login as" of odoo.sh is not working anymore, but this is essential for the Odoo support.

image
image

This is caused by this change of the module:

def _get_session_token_fields(self):
res = super()._get_session_token_fields()
res.remove("oauth_access_token")
return res | {"oauth_master_uuid"}

This causes, that the session_token (I assume generated by odoo.sh code) is not valid and the check here causes an automatic logout.
https://github.com/odoo/odoo/blob/7d3931bf36996cf5b51533e4a0a788ad8f3eb6c0/odoo/service/security.py#L11-L13C17

https://github.com/odoo/odoo/blob/7d3931bf36996cf5b51533e4a0a788ad8f3eb6c0/odoo/addons/base/models/res_users.py#L924-L941

My assumption is, that odoo.sh does not use the changes done by the _get_session_token_fields method, when creating the login session, but because the odoo.sh code is not public I cannot validate this assumption.

Meanwhile I also received and answer from Odoo and they are not willing/able to improve their implementation.

This PR bypasses the change of def _get_session_token_fields and therefore stays compatible with Odoo.sh

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

I think you need to write migration script for moving the values in oauth_master_uuid to oauth_access_token, isn't it?

oauth_master_uuid = fields.Char(

# use the oauth_access_token field as oauth_master_uuid
oauth_access_token = fields.Char(
string="Master UUID",
Copy link
Member

Choose a reason for hiding this comment

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

You only need to define the default, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The origninal definition is this:
oauth_access_token = fields.Char(string='OAuth Access Token', readonly=True, copy=False, prefetch=False)

Copy link
Member

Choose a reason for hiding this comment

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

The redefinition of labels cause a mix of terms across the system sometimes, so it's not the best option. And the required=True will provoke troubles everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am also not very happy for reusing this field. We could leave the string value as is and just add an comment that this field is used as Master UUID instead of access_token.
The required=True I just took from the oauth_master_uuid field, but I am not sure if this is needed to force the default creation? It shouldn't be empty or the same value as another user, to make sure that the access_token is not used by a different user.

@CRogos CRogos force-pushed the 17.0-imp-auth_oauth_multi_token-odoo.sh branch 3 times, most recently from 024bcf6 to 890d7f7 Compare February 21, 2025 16:09
@CRogos CRogos force-pushed the 17.0-imp-auth_oauth_multi_token-odoo.sh branch from 890d7f7 to 6dfc1d7 Compare February 21, 2025 16:39
@CRogos
Copy link
Contributor Author

CRogos commented Feb 21, 2025

I think you need to write migration script for moving the values in oauth_master_uuid to oauth_access_token, isn't it?

I've added a migration script.

I've also added a test and found out, that not the oldest but the second oldest token was removed when reaching max token number.

@CRogos
Copy link
Contributor Author

CRogos commented Mar 4, 2025

@pedrobaeza could you have a look on the last update?

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.

3 participants