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

Fixed Azure Active Directory user name cache matching to be case insensitive #1923

Merged
merged 2 commits into from
Jan 24, 2023

Conversation

kassak
Copy link
Contributor

@kassak kassak commented Sep 22, 2022

See https://youtrack.jetbrains.com/issue/DBE-13085
The problem is that server reply is cached and user input is searched
We've been able to track the situation where user entered JohnnyCash@folsom.org and cache contained johnnycash@folsom.org -> cache miss
I've done the quickfix by using case-insensitive comparison.
I'm not aware if the logins are truly case-insensitive. Feel free to make the fix of your own :)

See https://youtrack.jetbrains.com/issue/DBE-13085
The problem is that server reply is cached and user input is searched
@Jeffery-Wasty
Copy link
Contributor

Jeffery-Wasty commented Sep 22, 2022

Thank you, we'll take a look into this.

@Jeffery-Wasty
Copy link
Contributor

Hi @kassak,

An update. We're not 100% sure on the behavior of MSAL4J and case-sensitivity. Depending on the answer we receive from the team, this may or may not be a JDBC issue. We'll keep you updated on the status of the issue.

@kassak
Copy link
Contributor Author

kassak commented Oct 3, 2022

I'd like to note that the problem may lay deeper:

  1. You provide the email with some casing
  2. Server responds with the token, with the email with another casing
    So the server may do case-insensitive matching or transform the case in the response

@Jeffery-Wasty
Copy link
Contributor

The reply from MSAL is that the items cached are case-sensitive, and so we shouldn't be looking up with equalsIgnoreCase. You explicitly describe an issue where you looked up JohnnyCash@folsom.org and cache contained johnnycash@folsom.org. How do you know this is the problem? Reading the thread, I don't see where you mention this is the problem.

@Jeffery-Wasty
Copy link
Contributor

If there is some sort of case mismatch between driver and msal, we can look into it more. But for now (1) it doesn't appear to be an MSAL problem, and (2) we need to confirm this caused the original issue mentioned in the thread [MFA Azure support causes multiple browser tabs to open]

@Jeffery-Wasty Jeffery-Wasty added the Waiting for Response Waiting for a reply from the original poster, or affiliated party label Oct 6, 2022
@kassak
Copy link
Contributor Author

kassak commented Oct 11, 2022

While investigating the issue, I've created a patched jar with extended logging (also in my patch)
Got this output (user email is replaced with, dummy, but the case is preserved)
First time login:

Accounts in cache = , size = 0, user = user@example.com

Further attempts

Accounts in cache = User@example.com, size = 1, user = user@example.com

So the cached e-mail, thus the one returned by server is User@example.com and the one provided by user is user@example.com

Can I help you with anything else?

@Jeffery-Wasty
Copy link
Contributor

We haven't been able to look at this during the last week. We'll try looking at this as soon as we can. I'm still a bit confused on how this links to the original issue you posted. Is it that, because of the casing, users are not being authenticated properly, and this causes a window pop up asking them to reauthenticate?

@kassak
Copy link
Contributor Author

kassak commented Oct 21, 2022

Nope, on the contrary. The user authenticates ok, using CamelCase login, but the reply contains it in lower case. So on the next connection that reply is not found in cache and authentication is requested once again. Making cache case insensitive helped our users

@Jeffery-Wasty
Copy link
Contributor

Okay, thank you for clearing that up.

@Jeffery-Wasty
Copy link
Contributor

I wasn't able to do this on my end, can you remove the logging? Just the casing fix would be all that is needed here.

@lilgreenbird
Copy link
Contributor

hi @kassak

we can not have this change in the driver as usernames are case sensitive in linux/mac they could be different users. I have confirmed this is a bug (see AzureAD/microsoft-authentication-library-for-java#578) in the MSAL library we will wait for this to be fixed there.

@lilgreenbird
Copy link
Contributor

re-opening PR as Azure ActiveDirectory user names are not case sensitive and MSAL library converts to lower case

@lilgreenbird lilgreenbird added this to the 12.2.0 milestone Jan 17, 2023
@lilgreenbird
Copy link
Contributor

/azp run public-mssql-jdbc.linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lilgreenbird lilgreenbird merged commit 65c8cf9 into microsoft:main Jan 24, 2023
@tkyc tkyc mentioned this pull request Jan 26, 2023
@lilgreenbird lilgreenbird changed the title Fix cache account name casing issue Fixed Azure Active Directory user name cache matching to be case insensitive Jan 30, 2023
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.

MFA Azure support causes multiple browser tabs to open
5 participants