[arnaudlayec]

Sometimes, it can be useful to store attachments either in filestore or database, on a per-attachment basis, not following the global Odoo setting.
One example : with fs_attachment one can store some attachments in the DB and some other in the filestore.
This PR allows to be compatible with such scenarios.

Note: I can port this change in newer versions if validated

[OCA-git-bot]

Hi @vincent-hatakeyama,
some modules you are maintaining are being modified, check this out!

[arnaudlayec]

Hello @vincent-hatakeyama
I submitted this PR for limited changes but as an opened discussion I would like to bring 2 suggestions.

1. What about reverting to attachment=False instead of True for sp_pem_public and sp_pem_private? This would store them in DB instead of filestore. They are light files, in a small table, I think this is reasonable. The reason behind: I am thinking of CI/CD pipelines, which may copy PROD DB to CI DB but not PROD filestore to CI filestore => the SSO is then broken on the CI because the certificates were stored in the filestore. This issue does not exist with auth_oidc because there is no file, so this is a drawback I think for auth_saml.

2. Make auth_saml depends of server_environment so the certificates are encrypted in DB or filestore (or whatever location they are stored).
   Dependancy: this PR github.com/[16.0][IMP] server_environment_data_encryption: support binary fields server-env#313
   Alternative: a glue module auth_saml_server_environment with:

class AuthSamlProvider(models.Model):
    _name = "auth.saml.provider"
    _inherit = ["server.env.mixin", "auth.saml.provider"]

    sp_pem_public = fields.Binary(attachment=False)
    sp_pem_private = fields.Binary(attachment=False)

[vincent-hatakeyama]

1. Storing directly in database would prevent compatibility with fs-storage. I’m not in favor of that.
2. No direct dependency to server_environment should be added in this module. There’s alternative to server_environment and this would make it incompatible. A separate addon would be possible thought. I believe there was an addon or a PR for such an addon in the past.

[vincent-hatakeyama]

This version might need a pin on some library version like I did in 7a3d40c (but with the python version compatible with 16).

[arnaudlayec]

1. Storing directly in database would prevent compatibility with fs-storage. I’m not in favor of that.

   2. No direct dependency to server_environment should be added in this module. There’s alternative to server_environment and this would make it incompatible. A separate addon would be possible thought. I believe there was an addon or a PR for such an addon in the

Right, I understand. Thanks for the feedback.
For pysaml2, this is cherry-picked.

[vincent-hatakeyama]

/ocabot merge minor

[OCA-git-bot]

Sorry @vincent-hatakeyama you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

[vincent-hatakeyama]

@OCA/tools-maintainers Hi, can someone merge this?

[thomaspaulb]

/ocabot merge minor

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1043-by-thomaspaulb-bump-minor, awaiting test results.

[OCA-git-bot]

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