Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move user name comparison to provider #133

Merged
merged 3 commits into from
Sep 12, 2024
Merged

Conversation

adombeck
Copy link
Contributor

@adombeck adombeck commented Sep 3, 2024

The decision whether two usernames are considered equal is provider-specific. For example, some providers have case-insensitive usernames.

UDENG-4414
UDENG-4517

@adombeck adombeck force-pushed the do-not-case-fold-username branch 4 times, most recently from cdc34cb to a29e9b9 Compare September 4, 2024 15:44
@adombeck
Copy link
Contributor Author

adombeck commented Sep 4, 2024

The implementation of the user name check depends on which field we retrieve the username from, so I'll keep this as Draft until https://warthogs.atlassian.net/browse/UDENG-4413 is done.

@adombeck adombeck force-pushed the do-not-case-fold-username branch from a29e9b9 to 8889823 Compare September 4, 2024 16:19
@adombeck
Copy link
Contributor Author

adombeck commented Sep 6, 2024

We decided to go ahead with this and, if needed, change the check after https://warthogs.atlassian.net/browse/UDENG-4413 is done.

@adombeck adombeck marked this pull request as ready for review September 6, 2024 12:59
@adombeck adombeck requested a review from a team as a code owner September 6, 2024 12:59
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

A couple of questions + some formatting tips that we use on the team. Besides that, looking good. Nice work!

internal/broker/broker.go Outdated Show resolved Hide resolved
internal/providers/msentraid/msentraid.go Outdated Show resolved Hide resolved
internal/providers/msentraid/msentraid.go Outdated Show resolved Hide resolved
internal/providers/msentraid/msentraid.go Outdated Show resolved Hide resolved
internal/providers/msentraid/msentraid_test.go Outdated Show resolved Hide resolved
internal/providers/msentraid/msentraid_test.go Outdated Show resolved Hide resolved
@adombeck adombeck force-pushed the do-not-case-fold-username branch 2 times, most recently from 8ff34a8 to bc0d4a4 Compare September 12, 2024 11:48
The decision whether two usernames are considered equal is
provider-specific. For example, some providers have case-insensitive
usernames.

UDENG-4414
@adombeck adombeck force-pushed the do-not-case-fold-username branch from bc0d4a4 to 5a8cafb Compare September 12, 2024 11:54
@adombeck
Copy link
Contributor Author

I added a commit which checks that the usernames comply with the Microsoft Entra username policy (see UDENG-4517).

Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work!

We must avoid that multiple users of the provider map to the same local
user. We currently use `strings.EqualFold` to compare the usernames,
which considers some different Unicode characters the same (for example
the "KELVIN SIGN" (U+212A) and the ASCII "K"). To avoid that, we
considered using a custom `toLowercase` function which only converts
ASCII characters to lowercase. However, we can just check if the
username is valid under the Microsoft Entra username policy.

Microsoft Entra enforces the username policy for the User Principal Name
(UPN). We currently don't use the UPN as the username though, we use the
`preferred_username` field from the ID token. The value of that field is
equal to the UPN in some cases, in other cases it's equal to the user's
email address. It's not clear if the username policy is enforced for all
possible values of the `preferred_username` field.

So to ensure that we don't have to care about the Unicode case-folding
when comparing the usernames, we enforce the username policy ourselves.

UDENG-4517
@adombeck adombeck force-pushed the do-not-case-fold-username branch from 695c4aa to 4b7f8f3 Compare September 12, 2024 12:23
@adombeck adombeck merged commit 44bfe16 into main Sep 12, 2024
4 checks passed
@adombeck adombeck deleted the do-not-case-fold-username branch September 12, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants