Added reset password dialog upon SQL Server expired password error

[smartguest]

This PR adds in a simple reset dialog that pops up whenever a password expired error is encountered while connecting to a profile.

The user can press OK to change the password (and connect to the server and save the password immediately if enabled).

If an error is returned from STS when attempting to change the password, users will need to try connecting again to bring the dialog again so they can enter a different password.

Redesigned password reset dialog:

Removed unnecessary dialog in error:

First connect for an expired password (initial unexpected error happens on SSMS as well):

Error for an invalid password (error from STS):

Loading spinner and successful connect for a valid password:

This PR fixes #605

Companion PRS:

microsoft/sqlops-dataprotocolclient#71

microsoft/sqltoolsservice#1771

[smartguest]

Added updated GIFs after changes

[cheenamalhotra]

My concern here is that you're adding stuff to the public extensibility layer that other extensions won't be able to use (since this is currently only done for MSSQL).

I think 'Change Password' on server request/error number is not an uncommon scenario and can be extended beyond MSSQL provider. e.g. MySQL supports Password expiration which is something MySQL provider can make use of in future.

[Charles-Gagnon]

Definitely, I wasn't saying that it wouldn't be useful. Just that it currently isn't doing that (we specifically check for MSSQL and only our specific error code).

And if you're looking to make this extensible I would very much suggest looking at a much more generic way of doing this - for example having a generic way for providers to register a callback that is called when an error occurs during a connection. This would allow them to do whatever they needed without us needing to cover every single possible scenario in core ourselves.

Example flow :

1. Provider is registered with an "onConnectionError" callback
2. User attempts a connection through that provider
3. Core calls the provider extension to make the connection
4. Provider connection call returns an error
5. Core calls the provider onConnectionError callback (if one exists, otherwise falls back to default of just showing error)
6. Provider does whatever it wants, such as showing a dialog or automatically trying to fix the issue and then returns the result (handled or not)
7. If result is handled core then re-attempts connection, otherwise it shows the error

We could then still provide simple ways for users to accomplish common tasks - such as showing the firewall rule dialog or change password dialog (so that each extension doesn't have to re-implement it themselves). This could be done either through the extension API or by a package we provide that they can reference separately.

[alanrenmsft]

@smartguest let's talk about the feedback in our meeting today.

[cheenamalhotra]

Definitely, I wasn't saying that it wouldn't be useful. Just that it currently isn't doing that (we specifically check for MSSQL and only our specific error code).

And if you're looking to make this extensible I would very much suggest looking at a much more generic way of doing this - for example having a generic way for providers to register a callback that is called when an error occurs during a connection. This would allow them to do whatever they needed without us needing to cover every single possible scenario in core ourselves.

Example flow :

1. Provider is registered with an "onConnectionError" callback
2. User attempts a connection through that provider
3. Core calls the provider extension to make the connection
4. Provider connection call returns an error
5. Core calls the provider onConnectionError callback (if one exists, otherwise falls back to default of just showing error)
6. Provider does whatever it wants, such as showing a dialog or automatically trying to fix the issue and then returns the result (handled or not)
7. If result is handled core then re-attempts connection, otherwise it shows the error

We could then still provide simple ways for users to accomplish common tasks - such as showing the firewall rule dialog or change password dialog (so that each extension doesn't have to re-implement it themselves). This could be done either through the extension API or by a package we provide that they can reference separately.

@Charles-Gagnon as discussed offline, we would take this feedback as a follow-up investigation activity as its the initial design limitation and not specific to the proposed changes. It would be ideal to address it altogether with other error codes too.

[smartguest]

Added updated dialog (shown in screenshot) and addressed feedback. Please review for merge this release (Additional functionality to dataprotocol API will be added as part of the next phase.)

[erinstellato-ms]

@smartguest I have a question about this comment in the updated screenshots (thank you for those): "First connect for an expired password (initial unexpected error happens on SSMS as well)" Do we know why that fails and can we handle it better?

[erinstellato-ms]

For the password cannot be changed error... Do we have to include the "Changed database context to 'master'" and "Changed language setting to us_english" text? It doesn't seem relevant to the error, and is not helpful for users.

[cheenamalhotra]

Other than some minor changes, looks good.

[smartguest]

@erinstellato-ms The first connect error happens when you attempt to connect to a server using credentials for the first time, the exact error is:

"A connection was successfully established with the server, but then an error occurred during the login process. (provider: Shared Memory Provider, error: 0 - No process is on the other end of the pipe.)"

This will have to be a separate issue to be looked into as it also happens with SSMS so it's not really part of this PR.

Also those messages are part of the SqlClient error message as it is. I added a filter to remove them

[Charles-Gagnon]

@smartguest This PR depends on both the STS changes and (to a slightly lesser extent) the dataprocol-client changes.

You typically should be including all required changes together in a single PR so we don't have broken features in ADS - which normally means waiting until a build of STS is published that contain the required changes. (and a new release of dataprotocol-client should be done as well)

[sirio3mil]

It is merged but has it already been included at least in the insider version? Yesterday it expired and I did not get the pop up although it is possible that I did not have the latest build, now I have version 1.42.0-insider, yesterday I may have had the previous one

[rick-syncbak]

I'm still seeing this in version 1.48.0 in 2024. Every time I create a new user who uses Azure Data Studio with "User must change password at next login" , we have to have them use a desktop w/ SSMS to change their password.

[erinstellato]

@smartguest can you re-test this in ADS 1.48 when you have a minute? Thanks.

@kisantia for awareness.

[smartguest]

@erinstellato It works for error 18488 (The one for user must change password at next login and it should work for error 18487, which is the password expired error as addressed in this PR: #22916).

Can you get the exact error details from the "Copy Details" button when it happens? @rick-syncbak and paste it here? I need to identify to see if the error you are encountering matches the error code for expired password that we currently check for). Also is this a cloud instance or a local SQL Server instance? Currently password reset is only supported on non-cloud SQL Server instances.

If the error is a "no process at the other end of the pipe" error, that is a temporary error that is unrelated and can be retried a couple of times to get it working.

[smartguest]

Here is a GIF of release 1.48 testing: showing me creating the login on a non cloud SQL Server Instance with SQL Login, with password expired for next login, and then logging in with successful results. Currently as it is, I am not able to reproduce the issue with no password reset dialog:

[rick-syncbak]

I was able to test successfully on a "must change password on first login" scenario. Thank you!

…

On Tue, Mar 26, 2024 at 5:03 PM Alex Ma ***@***.***> wrote: @erinstellato <https://github.com/erinstellato> It works for error 18488 (The one that states password needs to be changed and it should work for error 18487, which is the password expired error as addressed in this PR): #22916 <#22916>). Can you get the exact error from the "Copy Details" button when it happens? @rick-syncbak <https://github.com/rick-syncbak> and paste it here? I need to identify to see if the error you are encountering matches the error code that we currently check for). — Reply to this email directly, view it on GitHub <#21295 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABRRCEWTMCTKV5KJDTGXFXDY2HWDFAVCNFSM6AAAAAASIMCY32VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRRGU2DQMRWHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[erinstellato-ms]

@rick-syncbak Thanks for following up, glad it's working for you!

Thanks @smartguest for validating and sharing the video!