search

ubuntu / authd

Authentication daemon for external Brokers
GNU Lesser General Public License v3.0
110 stars 9 forks source link

Ignore case when comparing the user name #511

Closed adombeck closed 1 month ago

adombeck commented 2 months ago

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

Closes #496

UDENG-4334

codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

adombeck commented 2 months ago

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

adombeck commented 2 months ago

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 commented 2 months ago

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

adombeck commented 2 months ago

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 commented 1 month ago

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! :)

adombeck commented 1 month ago

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.