[16.0][FIX] users_ldap_groups: safe LDAP decode

[oh2fih]

The group mapping in query mode fails if LDAP returns binary data in any of the fields. This adds a function that handles such situation by base64 encoding it.

[oh2fih]

This same bug affects all other versions, too.

I'm about to give up writing the test coverage for this change because codecov keeps complaining about everything I try. (Especially I didn't understand why it demands test coverage for the tests, too.) My idea is to add binary data to the LDAP response, e.g., a minimal GIF image as the LDAP attribute thumbnailPhoto, as this is a common case where LDAP returns binary data.

{
    "dc=users_ldap_groups,dc=example,dc=com": {
        "cn": [b"User Name"],
        "name": [b"hello", b"hello2"],
        "thumbnailPhoto": [
            b"GIF89a\x01\x00\x01\x00\x00\xff\x00,"
            b"\x00\x00\x00\x00\x01\x00\x01\x00\x00\x02\x00;"
        ],
    }
}

[oh2fih]

Tested with Odoo 16.0.20240102 on Ubuntu 22.04.

[sbidoul]

This decode-that-encodes-in-base64 looks a bit weird to me. Why is the decode necessary in the first place?

Perhaps, to move faster, you could do the security fix in a PR, so I can merge this, and do the decode improvement in another PR and work on that one with folks who are more familiar with this module?

[oh2fih]

This decode-that-encodes-in-base64 looks a bit weird to me. Why is the decode necessary in the first place?

This simply replaces the original ldap_entry[1][attr][0].decode() doing the fix with the least possible modifications to the module. The Str.decode() was added in ad15e3aa96d7d6f04aba840cd1499ad2c3af78ea when migrating the module from Odoo 10 to Odoo 12 and seems quite necessary, because it did not work without it on the first attempts to address this bug. The contains & equals uses the Str.decode() (in x.decode()), too, so not using it here does not seem coherent, either. Although not explained in the commit, I guess the root cause might be support for non-ASCII names.

So, why not use this safe_ldap_decode() in contains & equals then? Because only the requested LDAP attributes are compared and they should never contain binary data. This is a special case for the query operator for which LDAP returns the entire Active Directory object with all the attributes.

Does this satisfactorily explain the situation here?

[oh2fih]

Perhaps, to move faster, you could do the security fix in a PR

@sbidoul: There's now another PR https://github.com/OCA/server-auth/pull/659 that only contains the commit fixing the vulnerability. After merging that this PR is only the second commit ahead of upstream 16.0 branch.

[oh2fih]

Rebased to fix the checks that didn't pass; the problem was in another module.

Could someone please review & merge this already?