-
Notifications
You must be signed in to change notification settings - Fork 437
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
azure-active-directory-access-token REFRESH conn automatically on expiring #1144
Comments
Hi @panbhatt I'll take a look and see if there's a way for tedious to do that! |
Thanks @IanChokS : What i think can be done is that besides taking the token details, we can ask for a function (or a object having a function) that can be called once the duration of the token got expired. However, i would leave it up to the community to decide what best can be done. |
I think a good solution could be that we have a boolean flag in the configuration object used at the start to signal if we want to auto refresh or not. I'm sure there is somebody out there who is happy with the session expiring. |
Any update on this, team. |
@panbhatt I think that's not really an issue in |
the only caveat with this issue is all the ORM's has to change their structure just for this. i.e. they have to implement this functionality, which i think its not required, as the problem is present at TEDIOUS JS level, if in the dialect options they take a function (since they are the only who is providing the functionality for token validation etc) and refresh their connection details, so that whenever a new connection is required, it always used the fresh token. |
@panbhatt I understand. How do you feel about allowing to pass in an async function as Would you be willing to open a PR that implements this? |
i can try working on that, however it seems like it would be hard for me to test it without an availability of azure sql server and azure auth. let me know, if can be provided an env, i can try it. |
After playing with this a bit in the recent week, I actually now also agree that this is not up to Tedious to deal with. Mainly, since the token is already created before you even start with Tedious. You are just passing the token through. It should be up to the application or the library doing the authentication in the first place to ensure it has an always valid token. |
Actually it could be nice to allow us to provide tedious with a callback if it throws an error due to an expired token. That way, we can feed it the a function which will get a new token for it, if it encountered an expiry. Then tedious doesnt have to actually do any token authentication work, but it can help to provide a seamless experience controlled by the application. |
just wanted to know will it be a good practice:
|
Maybe I'm wrong here, but the token is only checked once, when you open the connection. As long as the connection is kept open, the token is not re-checked. Only when the token is used when a new connection is established by the connection pool after the token has expired, the authentication fails. This could e.g. happen due to the idle time out of the connection pool you mentioned. This is why I suggested we allow the token option to be a (async) function - it'd be invoked by tedious only when a connection is established, and the function would be responsible for returning a token that is valid at that moment. The function could decide itself how it handles token caching and expiration, and the connection pool does not have to know about any of this. Idle timeout of the connection pool would also not be an issue. |
I tried to work on this and need your feedback on this as just trying to make sure (without actually testing this), what we are proposing is right. extra: {
authentication: {
type: "azure-active-directory-access-token",
options: {
encrypt: true,
token: "",
tokenGenerator,
},
},
}
async function tokenGenerator(): Promise<String> {
// This function will get the token remotely and will use some function E.g. await getPresentToken()
return "abc";
} in connection.ts (line no 235) interface AzureActiveDirectoryAccessTokenAuthentication {
type: 'azure-active-directory-access-token';
options: {
/**
* A user need to provide `token` which they retrieved else where
* to forming the connection.
*/
token: string;
tokenGenerator() : Promise<String>;
};
} in line no 2531: in function sendLogin7Packet() case 'azure-active-directory-access-token':
payload.fedAuth = {
type: 'SECURITYTOKEN',
echo: this.fedAuthRequired,
fedAuthToken: await authentication.options.tokenGenerator()
};
break; the problem is this function sendLogin7Packet is not async() so we can't use await here. If we have to then we need to make sure async is replicated back to the top level. |
Another way to achieve this can be since ASYNC would force a lot of things to change is:
setInterval(5000, ()=> {
(async()=> { await tokenGenerator() } )();
} this will make sure whenever the token is about to expire a new token is generated prior to it. we can look in the expiry of individual connection too so that we dont have any pending connections that are expired. |
@panbhatt - did you get this to work? I'm facing the same problem with tokens expiring. |
Hey @mathew-pigram : There is no way you can achieve it as of now via using this driver. I have talked to the Microsoft guys and got an answer to use another method which uses Service Principal assigned to the App Service. That's how i had overcome this issue. This issue is not on the focus of MS team as of now. |
@panbhatt Thanks for the quick reply. I assume your connection now looks similar to this?
Did you encounter any issues when the app is not executing inside an App Service - e.g. when developing on your PC? |
Yes, that's correct. I used that one. In our organization we are not being allowed to test this on our PC, we are working against SQL Server on our local PC, but in order to test this we deploy our APP to Azure and use the kudu console to make the changes. but this indeed worked. |
@panbhatt Thanks for your help, much appreciated. |
Regarding all discussions, I'm on a way to implement token generator functionality. It would allow removing deprecated But instead of implementing That would make tedious code a little bit cleaner, without unnecessary dependencies, that could be deprecated anytime. I'll create 3 pull requests:
So keep your fingers crossed and just wait for updates. |
I made a PR that should help with this issue: This PR will allow you to configure your auth system the way you need and just pass the credential object. This should remove the requirement to get a new access token manually as the credential object can now generate them via the standards maintained by MS at @Azure/Identity |
I am working on application, where we are connecting to the SQL Server database and trying to create the connection pool (with TYPEORM and Sequelize). As per the org policies our token expires every 24 hours, that means all the connections in the pool are getting expired, and we have to restart the app to make sure every object /pool is created again.
Do we have an internal functionality to refresh the token automatically when it expires. It seems so obvious that the solution to restart the application wont' fly. Is there any way in the app we can do it, so that we can save restarts.
Any help would be appreciated.
The text was updated successfully, but these errors were encountered: