[17.0][IMP] auth_saml: only write value that changes #726
Conversation
vincent-hatakeyama
force-pushed
the
feature/17.0/auth-saml/write-only-necessary-fields
branch
2 times, most recently
from
b1dc23b to
2468d5e
Compare
vincent-hatakeyama
force-pushed
the
feature/17.0/auth-saml/write-only-necessary-fields
branch
from
2468d5e to
d481a04
Compare
|
@OCA/tools-maintainers Any chance for a review? It needs to be ported in 16 and 18 and I’d rather have a review first. |
|
@vincent-hatakeyama If you can try to do review trades with other PR authors, that would be preferable, and then can ping us PSC's for a merge. |
auth_saml/models/res_users.py
Outdated
| # when login/email changes (from mail module) | ||
| vals = {} | ||
| for key, value in validation.get("mapped_attrs", {}).items(): | ||
| if not isinstance(value, str) or getattr(user, key) != value: |
There was a problem hiding this comment.
| if not isinstance(value, str) or getattr(user, key) != value: | |
| if not isinstance(value, str) or user[key] != value: |
is it granted that value will be compatible w/ the field type? Eg: what if you have a boolean field and you compare False with 0?
There was a problem hiding this comment.
The mapping as implemented match attributes in the SAML response. I do not believe it can be a type different than string. In keycloak, that does not seem possible.
It’s possible to map one of the attribute to a boolean field. It might be strange to the users that any value is considered as True ("0", "1", "False", etc.) and only "" would put False in the field.
Other type of fields can also be written, but in most cases, that will fail. Writing a many2one with a string fails for example.
To make the code more robust, I’m going to only compare if the value is a string. That will avoid rewriting login/email and any string or selection field which is probably the most often written field when the feature is used.
There was a problem hiding this comment.
Thanks for the feedback. Can you please report this comment in the code so that we'll have clear where we come from? :)
There was a problem hiding this comment.
I’ve already adjusted code and added a comment.
When using mapping, not writing the value systematically avoids getting security mail on login/email changes when there is no change. Also use SQL for blanking passwords avoids the security update mails.
vincent-hatakeyama
force-pushed
the
feature/17.0/auth-saml/write-only-necessary-fields
branch
from
d481a04 to
5b078f1
Compare
When using mapping, not writing the value systematically avoids getting security mail on login/email changes when there is no change. Also use SQL for blanking passwords avoids the security update mails.