OCA / server-auth

https://odoo-community.org/psc-teams/tools-30
GNU Affero General Public License v3.0
146 stars 399 forks source link

[FIX][14.0] auth_saml: Fix logic of SELECT FOR UDPDATE to only lock records that will be updated #642

Closed Ricardoalso closed 1 month ago

Ricardoalso commented 3 months ago

Code before the PR

self.env.cr.execute(
    "SELECT id FROM auth_saml_provider WHERE id in %s FOR UPDATE",
    (tuple(providers.ids),),
)

This line of code locks the records of the SAML providers specified in providers.ids to avoid concurrent update conflicts. The locking is performed at the database level using the FOR UPDATE clause in the SQL query.

After the lock query, a loop is performed over each locked SAML provider.

for provider in providers:

The provider's metadata is downloaded from the provider.idp_metadata_url. If the download fails (status code different from 200), an error is raised.

document = requests.get(provider.idp_metadata_url)
if document.status_code != 200:
    raise UserError(
        f"Unable to download the metadata for {provider.name}: {document.reason}"
    )

If the content of the downloaded metadata (document.text) is different from the current provider's metadata (provider.idp_metadata), the metadata is updated with the new content. A log message is also recorded to indicate that the metadata has been updated.

if document.text != provider.idp_metadata:
    provider.idp_metadata = document.text
    _logger.info("Updated provider metadata for %s", provider.name)
    updated = True

Identified Problem The raised problem is that, although the code locks the provider records, it does not commit the changes made to the records that were not updated (those that fall into the else). This means that the locking is not released for these records, which can potentially lead to performance issues or locks waiting forever.

Improvement Suggestion To resolve this issue, we only lock records matching document.text != provider.idp_metadata

thomaspaulb commented 1 month ago

/ocabot merge patch

OCA-git-bot commented 1 month ago

This PR looks fantastic, let's merge it! Prepared branch 14.0-ocabot-merge-pr-642-by-thomaspaulb-bump-patch, awaiting test results.

OCA-git-bot commented 1 month ago

Congratulations, your PR was merged at bb9e38878b7e4df780d67e388bd44db45088c398. Thanks a lot for contributing to OCA. ❤️