microsoft / mssql-jdbc

The Microsoft JDBC Driver for SQL Server is a Type 4 JDBC driver that provides database connectivity with SQL Server through the standard JDBC application program interfaces (APIs).
MIT License
1.06k stars 424 forks source link

MFA authentication opens multiple browser tabs if connection fails #2237

Closed Destrolaric closed 5 months ago

Destrolaric commented 1 year ago

Driver version

Microsoft JDBC Driver 9.2 for SQL Server 9.2.00

SQL Server version

SQL Server 12.00.5290

Client Operating System

Windows 10

JAVA/JVM version

java.runtime.name=OpenJDK Runtime Environment java.runtime.version=17.0.6+10

Table schema

Irrelevant to the issue

Problem description

Authentication using ActiveDirectoryInteractive creates multiple browser tabs when encountering any issue during connection(for example, IP not being allowed by the firewall).

Expected behavior

SSO requests should be sent only once and not be requested during retries

Actual behavior

Multiple SSO requests are being sent, leading to multiple browser tabs being opened

Error message/stack trace

No error was produced during retries.

Any other details that can be helpful

Video demonstrating the issue could be provided if needed Original issue could be found here https://github.com/dbeaver/dbeaver/issues/21417

Jeffery-Wasty commented 12 months ago

Hi @Destrolaric,

We had a similar issue previously with DataGrip, this was resolved by making AAD username caching case-insensitive. The fix was merged for version 12.2.0. Can you try driver version 12.2.0, or later, and let us know if this solves your issue?

Destrolaric commented 12 months ago

@Jeffery-Wasty Hi, I have tried with the 12.4.1.jre11 version, and it still leads to the same behavior

Jeffery-Wasty commented 12 months ago

Okay we'll look into it.

Jeffery-Wasty commented 12 months ago

Hi @Destrolaric,

Can I ask you to try one more thing? I'm thinking these issues may have been resolved by synchronizing our calls to MSAL. This change is not available in a release yet, but I've attached it below, if you're able to test this out. We'll continue to try to replicate this on our end in the meantime. Thanks!

mssql-jdbc-12.5.0-SNAPSHOT.jre11-preview.zip

Destrolaric commented 12 months ago

@Jeffery-Wasty Hi, I have tried to use the 12.5.0 it still leads to the same issue.

Jeffery-Wasty commented 11 months ago

We're looking into this further. For now, we're treating this as something to fix, and are looking into if we can make ActiveDirectoryInteracitve a special case. We'll add this as a todo to look at in this or upcoming sprints.

I have a question about how the multiple tabs come up, is it multiple tabs per connection attempt, or multiple tabs since they are so many connection attempts. In other words, for connection property connectRetryCount=0 how many tabs are popping up on a single connection attempt?

Destrolaric commented 11 months ago

@Jeffery-Wasty I Tried with mentioned parameter, and around ten tabs were shown

Jeffery-Wasty commented 11 months ago

~10 tabs on driver version 9.2, 12.4.1, or 12.5?

Destrolaric commented 11 months ago

@Jeffery-Wasty On version 12.5

Destrolaric commented 11 months ago

@Jeffery-Wasty Hi, It would be great to get information about the current status of the issue? Let me know if there's anything I can do to help.

Jeffery-Wasty commented 11 months ago

Hi @Destrolaric,

We're currently rewriting our retry behavior, which could affect several open issues, including this one. If the issue still persists after we re-write that part of the driver, we'll look into additional steps that we can take to resolve the above issue.

sblackstone commented 8 months ago

Hi @Destrolaric,

We're currently rewriting our retry behavior, which could affect several open issues, including this one. If the issue still persists after we re-write that part of the driver, we'll look into additional steps that we can take to resolve the above issue.

Hi..

I was wondering if this has been addressed in a latest release of the driver or if its still pending?

Thanks

Jeffery-Wasty commented 8 months ago

This is still pending. We still believe this may be related to our retry behavior, and are working on resolving that first. There is no planned date for when the changes to retry behavior will be released.

lukasheinz92 commented 8 months ago

Hi, I am facing the same issue. After a few minutes of inactivity it requires me to sign in again and in addition it often opens two tabs consecutively (in the first tab I sign in and after a few seconds it requires to sign in again).

tkyc commented 8 months ago

@lukasheinz92 This is on our radar. As Jeff said we are reworking our retry logic to resolve this.

Jeffery-Wasty commented 5 months ago

After looking through this again, I'm confused how this is not resolved by the existing changes to caching that we do for MFA authentication. @Destrolaric @lukasheinz92 If either of you are still experiencing this with the latest driver version, 12.7.0, can you also enable logging so we can see where this repeat is happening?

Also please share the connection properties or connection string you are using, as we have seen correlation between these properties and repeats.

Finally, please try setting connectRetryCount to 0. We have had recent changes to connectRetryCount which I believe might help in this situation.

Destrolaric commented 5 months ago

@Jeffery-Wasty Greetings, just tried with 12.7.0, now it is a little better. If the retrycount = 0, issue is not noticeable. But 3 tabs are created during connection fail if retrycount = 1

Jeffery-Wasty commented 5 months ago

@Destrolaric Thank you. It's good to hear that there is a partial solution with connectRetryCount = 0, but it should work with all cases. Would you be able to reproduce this issue while logging is enabled? Also, what other connection properties are you using? We've noticed the value of loginTimeout can have an effect from similar, previous, issues.

Jeffery-Wasty commented 5 months ago

Adding on to the previous comment. We're aware there are still issues with connectRetryCount > 0, but for your use case, would setting connectRetryCount = 0 work? If so, we can move forward with closing this issue as there is now a resolution for the issue. We'll update this thread when the additional changes to the connection retry logic are completed.

Jeffery-Wasty commented 5 months ago

Closed for above reasons, use connectRetryCount=0. Support for >0 is being worked on.