server-auth icon indicating copy to clipboard operation
server-auth copied to clipboard

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

Open Ricardoalso opened this issue 1 year ago • 0 comments

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

Ricardoalso avatar Apr 30 '24 14:04 Ricardoalso