Skip to content

[IMP] session_db: add method delete_from_identifiers#3415

Open
maq-adhoc wants to merge 1 commit into
OCA:18.0from
adhoc-dev:18.0-H-101032-MAQ
Open

[IMP] session_db: add method delete_from_identifiers#3415
maq-adhoc wants to merge 1 commit into
OCA:18.0from
adhoc-dev:18.0-H-101032-MAQ

Conversation

@maq-adhoc

Copy link
Copy Markdown

Added the delete_from_identifiers method to PGSessionStore.
This method allows bulk deletion of session records from the http_sessions table based on a list of session identifiers (sid). It is triggered from the backend via the "Revoke" button

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hi @sbidoul,
some modules you are maintaining are being modified, check this out!

@sbidoul

sbidoul commented Oct 14, 2025

Copy link
Copy Markdown
Member

Ah I had not noticed delete_from_identifiers was there in 18.0 too.

If I'm not mistaken, the identifiers provided to that method are the first 42 characters of the sid.

It handled that in the 19.0 migration. You may want to review #3413 and then extract the relevant part for 18.

@maq-adhoc maq-adhoc force-pushed the 18.0-H-101032-MAQ branch 2 times, most recently from 0fc95f6 to a529c9d Compare October 20, 2025 15:36
@maq-adhoc

Copy link
Copy Markdown
Author

@sbidoul Perfect, I backported 19. Regards

        Added the delete_from_identifiers method to PGSessionStore.
        This method allows bulk deletion of session records from the http_sessions table
        based on a list of session identifiers (sid). It is triggered from the backend
        via the "Revoke" button
@sbidoul sbidoul changed the title [IMP] Add method delete_from_identifiers [IMP] session_db: add method delete_from_identifiers Nov 15, 2025
@with_cursor
def delete_from_identifiers(self, identifiers: list[str]) -> None:
for identifier in identifiers:
if not sessions._sha1_re.match(identifier):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

_sha1_re is not the same regex as _session_identifier_re. Why do you change this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

hi @sbidoul
The module imports the sessions class from odoo.tools._vendor import sessions.

The generate_key method it calls is this one: https://github.com/odoo/odoo/blob/18.0/odoo/tools/_vendor/sessions.py#L33-L37 which creates a SHA (40 characters) and not a 42-character identifier.

I understand that we could modify the FilesystemSessionStore class to inherit from FilesystemSessionStore (https://github.com/odoo/odoo/blob/18.0/odoo/http.py#L911) and ensure consistent behavior. However, we would have to remove the validations to avoid errors with existing sessions.

Since we are on a stable version with many productive installations, I prefer to be conservative with the changes. I look forward to hearing your opinion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I see. And in 19, we don't have that problem because we use the generate_key method from FileSystemSessionStore here:

generate_key = http.FilesystemSessionStore.generate_key

Is that right?

So maybe we should do the same here, and support both regexes in delete_from_identifiers?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@tarteo

tarteo commented Jun 24, 2026

Copy link
Copy Markdown
Member

@maq-adhoc Can you take a look at #3662 and review if you have time?
This PR solves multiple issues, and the implementation it consistent with 19.0.
I resused a snippet (sessions._sha1_re.match(identifier):) from this PR, so I will add you to the contributors if you like.
It also includes a few additional tests covering these methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants