fix(auth): Bind with specified credentials for login function if cred…

[Cornelicorn]

…entials are specified

Allow users to opt-in to the old behaviour by setting user_bind_lookups to True, but default to binding with the specified credentials, if they are given

Needed in order to solve rackslab/Slurm-web#587

[Cornelicorn]

Tested to not break when upgrading just RFL and to use the new defaults when slurm-web is upgraded.

[rezib]

Ok thanks for the update! I think the logic and naming are fine now. This new code deserves unit tests to:

• Cover the lookup_as_user attribute assignment logic,
• Check login() calls _bind() or not, depending on value of look_up_user attribute.

Can you consider adding these tests?

[Cornelicorn]

• Cover the lookup_as_user attribute assignment logic,
• Check login() calls _bind() or not, depending on value of look_up_user attribute.

Can you consider adding these tests?

I don't really think these would bring a benefit. look_up_user is boolean and the test would essentially check if python correctly interprets boolean values, and I can't really imagine how the code for this test should actually look.

And testing the attribute assignment logic would essentially be a copy of the block in the __init__ function, so just the same lines of code again. I can add a test for this if you want, but I don't see a benefit in it.

[rezib]

I don't really think these would bring a benefit. look_up_user is boolean and the test would essentially check if python correctly interprets boolean values, and I can't really imagine how the code for this test should actually look.

And testing the attribute assignment logic would essentially be a copy of the block in the __init__ function, so just the same lines of code again. I can add a test for this if you want, but I don't see a benefit in it.

The purpose of these tests is not really to validate the code as today, it is more to check absence of regression with future changes, or at least detect potential impacts in predictable way. Can you please add tests to cover assignment logic?

If you don't figure out how to check _bind() calls, I will add these tests by myself.

[Cornelicorn]

PTAL

[rezib]

LGTM. I will just slightly reword the code and commits messages, and update changelog before merging.

[rezib]

It's merged, thank you @Cornelicorn for your contribution! FYI, I plan to release 1.5.0 with this feature on friday 27/06.

[Cornelicorn]

Just noticed, src/authentication/rfl/tests/test_ldap.py.orig probably shouldn't have been merged?

[rezib]

🤦 Fixed in 44982fa, thanks for letting me known.