search

tediousjs / tedious

Node TDS module for connecting to SQL Server databases.
http://tediousjs.github.io/tedious/
MIT License
1.58k stars 439 forks source link

[QUESTION] `azure-active-directory-default` Auto Token Refresh? #1626

Closed elliot-huffman closed 6 months ago

elliot-huffman commented 6 months ago

Question Does Tedious.Js automatically get a new access token when using any of the pre-built @Azure/Identity integrated auth options?

E.g. Access token is short lived, say 1 hour, when this expires, will Tedious get a new token automatically when the current one expires?

arthurschreiber commented 6 months ago

No, we don't support auto-refresh, and I'm not sure tedious is the correct place to put the auto-refresh logic into. I think it should be handled by consumers of tedious (probably another reason to deprecate the existing azure identity options and just replace them with what you proposed in the token credential PR).

elliot-huffman commented 6 months ago

Right now, I have to completely destroy my entire connection and then rebuild it from scratch, it's kind of inefficient and ideally if there is a connection error inside of tedious, it would automatically rerun the get token command if it is in the token credential mode.

A lot of other sdks do this, you pass the token credential and it automatically handles its own authentication lifecycle at that point, a great example is the graph API SDK.

Because of the behavior of how Microsoft recommends implementation of these types of credentials, I do indeed believe it is tedious's responsibility. And I'm willing to write that code for the project.

Because if it's implemented at the tedious level, we can be the most efficient as possible, if it's implemented in the end users level, there's going to be a lot of try catch blocks and a lot of code inefficiency such as re-establishing the entire connection like I have to do currently.

The credential token object has a token cache system already fully implemented, so when we run the get token command, it will most efficiently get a new access token.

Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: Arthur Schreiber @.> Sent: Thursday, May 2, 2024 11:04:24 AM To: tediousjs/tedious @.> Cc: Elliot Huffman @.>; Author @.> Subject: Re: [tediousjs/tedious] [QUESTION] azure-active-directory-default Auto Token Refresh? (Issue #1626)

No, we don't support auto-refresh, and I'm not sure tedious is the correct place to put the auto-refresh logic into. I think it should be handled by consumers of tedious (probably another reason to deprecate the existing azure identity options and just replace them with what you proposed in the token credential PR).

— Reply to this email directly, view it on GitHubhttps://github.com/tediousjs/tedious/issues/1626#issuecomment-2090759895, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGMEWQETRWPFTJ2GPNKH5LZAJIXRAVCNFSM6AAAAABHD3OOX2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJQG42TSOBZGU. You are receiving this because you authored the thread.Message ID: @.***>

elliot-huffman commented 6 months ago

Honestly though, I could be just missing something for my token refresh, what is the best way to inject a new access token without having to completely rebuild the connection state?

Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: Arthur Schreiber @.> Sent: Thursday, May 2, 2024 11:04:24 AM To: tediousjs/tedious @.> Cc: Elliot Huffman @.>; Author @.> Subject: Re: [tediousjs/tedious] [QUESTION] azure-active-directory-default Auto Token Refresh? (Issue #1626)

No, we don't support auto-refresh, and I'm not sure tedious is the correct place to put the auto-refresh logic into. I think it should be handled by consumers of tedious (probably another reason to deprecate the existing azure identity options and just replace them with what you proposed in the token credential PR).

— Reply to this email directly, view it on GitHubhttps://github.com/tediousjs/tedious/issues/1626#issuecomment-2090759895, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGMEWQETRWPFTJ2GPNKH5LZAJIXRAVCNFSM6AAAAABHD3OOX2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJQG42TSOBZGU. You are receiving this because you authored the thread.Message ID: @.***>

arthurschreiber commented 6 months ago

Maybe I don't understand, but when connecting via token authentication to SQL server, the token is only required when the connection is established. Once the connection is established, the token won't be used at any later point in time on that connection.

tedious does not support re-connecting the same connection again, there's too much internal state that's not cleaned up properly to cleanly allow re-connecting. The current way to handle connection errors is to throw away the connection and create a new connection instead.

I believe your PR already solves the "getToken is expensive" issue. azure/identity caches the getToken response per credential object, so if you re-use the same credential object across different connections, we will end up re-using the already previously requested token. Note that this does not work in the current azure-* authentication methods, because those create a new credential object on each connection attempt.

arthurschreiber commented 6 months ago

I believe your PR already solves the "getToken is expensive" issue. azure/identity caches the getToken response per credential object, so if you re-use the same credential object across different connections, we will end up re-using the already previously requested token. Note that this does not work in the current azure-* authentication methods, because those create a new credential object on each connection attempt.

At least this is based on my understanding of how @azure/identity is supposed to have an in-memory cache.

elliot-huffman commented 6 months ago

You are correct on the best practice of using the credential object in my pr, that was my plan also.

I was under the assumption that every request across the live connection needed to have an active access token. I use sequalize as my orm and in the past I believe I had an issue where it timed out and I lost my connection due to the token being dead or something like that.

With the logic you just specified, I do agree that it does not need to be in the SQL driver itself, it does appear that this functionality should reside in the end user's application

Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: Arthur Schreiber @.> Sent: Thursday, May 2, 2024 12:28:58 PM To: tediousjs/tedious @.> Cc: Elliot Huffman @.>; Author @.> Subject: Re: [tediousjs/tedious] [QUESTION] azure-active-directory-default Auto Token Refresh? (Issue #1626)

I believe your PR already solves the "getToken is expensive" issue. azure/identity caches the getToken response per credential object, so if you re-use the same credential object across different connections, we will end up re-using the already previously requested token. Note that this does not work in the current azure-* authentication methods, because those create a new credential object on each connection attempt.

At least this is based on my understanding of how @azure/identity is supposed to have an in-memory cache.

— Reply to this email directly, view it on GitHubhttps://github.com/tediousjs/tedious/issues/1626#issuecomment-2090979310, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGMEWRT64SV4NBASBSMSSDZAJSUVAVCNFSM6AAAAABHD3OOX2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJQHE3TSMZRGA. You are receiving this because you authored the thread.Message ID: @.***>