Skip to content

Conversation

@andreboscatto
Copy link
Contributor

@andreboscatto andreboscatto commented Mar 15, 2025

Added 5 test cases to cover AD and LDAP access_filter conditions using the new testing framework

@andreboscatto andreboscatto marked this pull request as draft March 15, 2025 14:04
@andreboscatto
Copy link
Contributor Author

andreboscatto commented Mar 15, 2025

Due to my lack of knowledge I am struggling with the filters. I created the entire scaffolding but I really need help from either @danlavu or @pbrezina

test_access_filter__single_ldap_attribute_permits_user_login

  • (ad) PASSED
  • (ldap) PASSED
  • (samba) PASSED

test_access_filter__group_attributes_permits_user_login

  • (ad) PASSED
  • (ldap) FAILED
  • (samba) PASSED

test_access_filter__ldap_query_with_wildcard_permits_user_login

  • (ad) PASSED
  • (ldap) PASSED
  • (samba) PASSED

test_access_filter__ldap_query_with_and_or_not_permits_user_login

  • (ad) PASSED
  • (ldap) PASSED
  • (samba) PASSED

test_access_filter__ldap_attributes_approximately_greater_and_less_than_permits_user_login

  • (ad) PASSED
  • (ldap) PASSED
  • (samba) PASSED

Miscellaneous

  • Remove pudb references and back traces
  • Understand how I can perform ldap searches against master.ldap.test (I tried multiple times but I wasn't able to)

@andreboscatto andreboscatto force-pushed the testAccessControlLDAPFilter branch from 8b7d42f to 3a603ed Compare March 22, 2025 14:05
@andreboscatto andreboscatto added Help wanted Extra attention is needed and removed backport-to-sssd-2-9 labels Mar 22, 2025
@andreboscatto andreboscatto force-pushed the testAccessControlLDAPFilter branch from 3a603ed to ddc08d4 Compare March 24, 2025 09:41
@andreboscatto andreboscatto force-pushed the testAccessControlLDAPFilter branch 2 times, most recently from 2619ae6 to 99c0d0e Compare March 26, 2025 15:36
@andreboscatto
Copy link
Contributor Author

Some of the mypy coding failures are related to SSSD/sssd-test-framework#161, so once it is merged it should get rid of the warnings.

@andreboscatto
Copy link
Contributor Author

So, for test_access_filter__group_attributes_permits_user_login (LDAP) the test is failing due to no group membership being established in LDAP.

Here is user1 structure

dn: cn=user1,ou=users,dc=ldap,dc=test
objectClass: posixAccount
objectClass: top
cn: user1

and here is group1 structure

dn: cn=group1,ou=groups,dc=ldap,dc=test
objectClass: posixGroup
objectClass: top
gidNumber: 33001
cn: group1
memberUid: user1
gidNumber: 23001
homeDirectory: /home/user1
uid: user1
uidNumber: 23001
userPassword:: e1NIQTI1Nn1MdEJuWm5sZFdLVHlMVkVhWnk4Z3ByQ1cwLzViVnE4NmRFWjRxY
 U5XL1lJPQ==

Based on a quick research:

  • I can't setup a filter based on memberUid because it is in the group, not in the user;
  • Either we remove this test or change the schema to RFC2307bis (including memberOf or equivalent);
  • Does any customer use this kind of setup? Or they rely on simple_allow_groups for such thing?

@andreboscatto andreboscatto force-pushed the testAccessControlLDAPFilter branch from 99c0d0e to 981f776 Compare March 26, 2025 18:28
@andreboscatto andreboscatto force-pushed the testAccessControlLDAPFilter branch from cc7cce6 to a6e3545 Compare April 24, 2025 08:37
@andreboscatto
Copy link
Contributor Author

andreboscatto commented Apr 24, 2025

  • there is a conflict, please rebase and resolve a conflict,

Done

What branches does this target?

I'll defer it to @danlavu

@andreboscatto
Copy link
Contributor Author

CI failures are not related to this PR.

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I prefer approach with reusing object instead of copying username around

user1 = provider.user("user1").add()
...
assert client.auth.ssh.password(user1.name, "Secret123"), f"`{user1.name}` should be able to login!"

But if you consider your approach more readable I'm fine with that. ACK

@thalman
Copy link
Contributor

thalman commented May 27, 2025

It looks to me that CI errors are not in the test

@andreboscatto
Copy link
Contributor Author

@danlavu I don't know which labels should be set when it comes to back porting or not, can you please handle that?

@danlavu
Copy link

danlavu commented May 29, 2025

@andreboscatto, sorry for the delay, done.

@alexey-tikhonov
Copy link
Member

@andreboscatto, please rebase.

Added 5 test cases to cover AD and LDAP access_filter conditions using
the new testing framework
@andreboscatto andreboscatto force-pushed the testAccessControlLDAPFilter branch from a081eff to b3b1f3a Compare May 30, 2025 10:38
@alexey-tikhonov alexey-tikhonov added Ready to push Ready to push and removed Ready to push Ready to push backport-to-sssd-2-9-4 Corresponds to C8S labels May 31, 2025
@alexey-tikhonov
Copy link
Member

Pushed PR: #7880

  • master
    • 41a0df2 - TESTS: Add tests to cover access control access_filter (AD/LDAP)
  • sssd-2-10
    • 55e6b6f - TESTS: Add tests to cover access control access_filter (AD/LDAP)
  • sssd-2-9
    • 4a3157b - TESTS: Add tests to cover access control access_filter (AD/LDAP)

@alexey-tikhonov
Copy link
Member

There was a conflict in sssd-2-9-4. Please open explicit backport PR if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants