Ignore case when comparing the user name

[adombeck]

Microsoft Entra user names are case insensitive, so we should ignore the case when comparing the user name.

Closes #496

UDENG-4334

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.74%. Comparing base (924b003) to head (c69735d).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #511      +/-   ##
==========================================
- Coverage   84.84%   84.74%   -0.10%     
==========================================
  Files          79       79              
  Lines        6942     6937       -5     
  Branches       75       75              
==========================================
- Hits         5890     5879      -11     
- Misses        736      739       +3     
- Partials      316      319       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[adombeck]

We're having some real flakiness issues in github.com/ubuntu/authd/pam/integration-tests. Took four attempts to get them to pass here.

[didrocks]

The rationale itself looks good to me.

However, as everything we do, this requires a companion tests for that use case.

[adombeck]

What I want to avoid is that a user can log in as another user on the same Entra instance because strings.EqualFold considers different Entra user names equal.

I'll create a PR against the brokers repo to use strings.ToLower instead of strings.EqualFold.

[didrocks]

forgot to push some commits or is my browser broken? :)

[adombeck]

forgot to push some commits or is my browser broken? :)

Right, my pre-push hook failed because of those weird "directive is unused for linter" errors from golangci-lint which only I get locally :/

Pushed with --no-verify now.

[didrocks]

I am unsure if you noticed that the tests are failing (please always check CI after pushing).

If they would not anyway, I would have asked for a test illustrating that we don’t check the user name matching anyway, but we already have them to cover that functionality of the code! :)

[didrocks]

Feel free to revert to "request for review" once the tests are fixed, or do not hesitate if you need any help fixing them!

I think now that we moved the logic broker side, we can probably remove the pam service and integration tests, and only rely on the broker package tests to demonstrate that functionality, as it’s not widespread in the end to end contract term (we expect the external broker to do this work).

[adombeck]

I am unsure if you noticed that the tests are failing (please always check CI after pushing).

I did notice that, but at the bottom of the logs of the CI job I only saw the pam integration tests failing, which are known to be flaky, so I just retried the tests a couple of times. After taking a closer look now, I see that the broker tests are also failing. I will address that.

[didrocks]

Thanks a lot for the changes! Everything looks good from my end :)