benbaran / adal-angular4

Angular 4/5/6/7 ADAL Wrapper
MIT License
86 stars 104 forks source link

Automatic renewal of login token #71

Closed geerzo closed 5 years ago

geerzo commented 6 years ago

Right now there is no automatic renewal of the login token which will make adal-angular4/adal.js think you aren't authenticated when you still have a valid login session. To fix this I've added a refresh that does two things: 1) If the login token has expired during init of adal-angular4 then it will refresh the login token and then reload the page. I don't like the reload but it was required for the new token to take affect. 2) As long as there is a valid token there is now a background timer that silently renews the token before it expires.

I created a PR #70 for my change. Please take a look and see if it helps in your systems before I merge it in. So far my testing has been very positive.

geerzo commented 6 years ago

Looks like @benbaran merged the PR so review master and let me know if anyone sees any issues.

SIlver-- commented 6 years ago

As per #70

Can this be added into version 2.0(for angular 5) as well? I wouldn't want to change angular version to get this very important change. Thank you

ghost commented 6 years ago

@geerzo I have checked the documentation of Azure AD and adal JS.

Please correct me if I am wrong, but based on below https://docs.microsoft.com/en-us/azure/active-directory/active-directory-configurable-token-lifetimes#single-sign-on-session-tokens

if you make an interaction with the AD Endpoint the SSO session will be renewed and I think this is not a desired behaviour as this enhancement eliminates the session instead of achieving the desired goal.

I have experienced also a side effect of the timer in Chrome Developer Tools as it was collapsing about every couple of minutes and it does it since I have this update. If you turn logging on like

((window) as any).Logging = {
                level: 3,
                log: function (message) {

                    console.log(`ADALJS: ${message}`);
                }
           };

You can see how often the timer runs, though I might missed some config to set it.

The third thing is mainly a question to you https://docs.microsoft.com/en-gb/azure/active-directory/develop/authentication-scenarios#token-expiration-1

based on the token expiration handling of adal js I think this should be done automatically in the background, when you are trying to use a expired login token. Have you experienced differently?

@benbaran have I mistaken on above? Are you planing to release a version where the timed renewal could be turned off if above is correct or any other action?

benbaran commented 6 years ago

I have not had much time to devote to this project lately, so I’ll defer to others on the answer. I’ll try to take a look to see if I can think of a better solution.

Sent from my iPhone

On Aug 25, 2018, at 11:19 AM, RDevai notifications@github.com wrote:

@geerzo I have checked the documentation of Azure AD and adal JS.

Please correct me if I am wrong, but based on below https://docs.microsoft.com/en-us/azure/active-directory/active-directory-configurable-token-lifetimes#single-sign-on-session-tokens

if you make an interaction with the AD Endpoint the SSO session will be renewed and I think this is not a desired behaviour as this enhancement eliminates the session instead of achieving the desired goal.

I have experienced also a side effect of the timer in Chrome Developer Tools as it was collapsing about every couple of minutes and it does it since I have this update. If you turn logging on like ((window) as any).Logging = { level: 3, log: function (message) {

            console.log(`ADALJS: ${message}`);
        }
   };

You can see how often the timer runs, though I might missed some config to set it.

The third thing is mainly a question to you https://docs.microsoft.com/en-gb/azure/active-directory/develop/authentication-scenarios#token-expiration-1

based on the token expiration handling of adal js I think this should be done automatically in the background, when you are trying to use a expired login token. Have you experienced differently?

@benbaran have I mistaken on above? Are you planing to release a version where the timed renewal could be turned off if above is correct or any other action?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

geerzo commented 6 years ago

@RDevai all the timer does is refresh the id token (login access token) before it expires. This action is actually very important because it's the only way to validate the user session is still valid with the identity provider. If you can successfully refresh the token, the user is still good. If not, the user has been logged out of the identity provider and should also be logged out here.

Currently the timer is set to run 10 minutes before the id token is set to expire. It would actually be more secure to run it more frequently while the user is active. It would probably be good to add some feature to detect inactivity and stop the timer after a certain inactivity time.

We can add a flag to turn off the auto refresh if that helps. Can you submit a pull request?

ghost commented 6 years ago

@benbaran Thanks, I think this is a great work, helped me a lot understanding adal from angular.

@geerzo Thanks for the the detailed answare. I think still the this enhancement is not desired because refresment should happen on interception by the interceptor on user interaction with the site. Otherwise the session will kept alive by the timer. I have a short session liftime configured. which should get invalidated after lets say 15 minutes user inactivity.

To assign resources to the interceptor we should assign resources to endpoints in the following way (adal js config, passed to init service) https://github.com/AzureAD/azure-activedirectory-library-for-js/issues/649 I think the site where the SPA operates is the only assigned resurece by default but not with full url.

I think the session only should be reset on user interaction and the isAuthenticated will kept on a fresh state with every interaction.

I have worked with different technoloogies than adal, lets say backend cookie based session. And usually the session time should be reset only when a user interacts with the backend.

Let's say there is a story where a user is imputing data and leave the us when he/she completed half of the way and leaves it there. If the user tries to continue the work after 15 minutes and hit submit he won't get notified while comleeting the interaction and when hits 'submit' finds out only he/she is loged out.

I am aware of this situation as well but I think this is how it works, and you should not try to work this around because your app will keep the session alive otherwise insted of the interactions of the user.

Update: To continue the above example if we can not acquire tokens silently anymore because the session expired we should begin a sign in process again. It could happen in a pop up or in a separate tab, if we would like to preserve already inputed data on the page where the user is.

geerzo commented 6 years ago

@RDevai as I mentioned, I'm all for having an option to disable the timer. If someone wants to submit a pull request it will get done faster. For my use case, and others, this solution helps us even though the session is being extended. At least in my scenario, our sessions have a max life anyway so they eventually time out and require re-auth regardless of activity.

I also agree that we should add some sort of activity monitor and disable the timer if there isn't activity...again, a pull request will speed it up.

The endpoints config is used so I'm not sure I understand the point you are trying to make with the link. Can you clarify?

You mentioned that activity should be refreshed during interception, and it is for resource tokens, but that's not a fail safe way for the id token. Let's use the example project as the use case where you have a single button to show your Graph API user data. When you log in, the id token gets populated with a 1 hour expiration. When I hit the "Profile" button, the Graph API access token get refreshed with a 1 hour expiration. Now I can hit that "Profile" button a lot over the next couple hours and that Graph API access token will get refreshed as required through the interception process but at the same time the id token will expire because nothing is explicitly accessing the login server.

Taking a slightly different view of the problem from above. Let's say that right after you get your Graph API access token you go to AD and log out. Then when you come back to your application you'll still be able to use that Graph API access token till it expires (up to an hour). Your app doesn't know you no longer have a valid session with AD because it's not checking.

Technically the timer helps with both. In the first scenario it keeps your id token active. In the second scenario it detects the logout has happened and logs you out of this app as well. The side effect right now is that it keeps your session active even if you aren't active, which we can fix.

So I think the timer has a perfectly valid place as a tool here but I also agree there are a lot more features we can add to make it better.

ghost commented 6 years ago

@geerzo Thanks for the detailed example. I think we do not have an id token here as adal js is not capable by design to getting one. You can make a work around like below, but have you done this? https://github.com/AzureAD/azure-activedirectory-library-for-js/issues/525

I think it should intentionally not be there because the session should be handled based on the access_token renewals (and the SPA has an acccess token as well by default) with cookies. I think we do not need an explicit id token as Graph shoud be used for user information retrieval.

Sorry if I was not clean enough, I may correct my above example, but I need to take some rest after my long week.

So lets introduce a log in and log off button to your example.

When you log out from the application you should lose the session cookies. If you have a persisted session and hit log out than that should be handled by the AD endpoint (deleting the session cookie, no matter which type you have) and when you come back you have to log in again to gain access to the app and the Graph API access token handled by adal, so without the session the Graph token can not be used in the app as there is no session (and by the way no valid SPA access token) .

If we have a persisted session cookie and just click on close browser, when we come back as we have a session we should be able to use the Graph Token just as you mentioned. This case should look like I see I am not logged into the app when I get to the landing page, but I can get an acces token, if I hit log in on the app without entering credentials.

If you have a non persisted session cookie and close the browser you need to sign in again, when you navigate to the app again as the non persisted session cookie is deleted by the browser.

The persisted/non persisted part handled by the user selection of stay logged in.

So the SPA access token should not have role in the users logged in state and the log in operation should request a new access token for the SPA if we are navigating back to the application. I think the difference comes from tokens are for Authorization but Authentication should be handled by the AD endpoint all the time base on sessions.

The role of SPA access token is to communicate with the backend of the SPA application.

Please correct me if I am wrong about this. Sorry for not giving a helping hand but I really need some rest.

geerzo commented 6 years ago

@RDevai adal.js absolutely supports an id token, it's key to the success of the platform. Go look at your local session storage and you'll find "adal.idtoken" as one of the attributes. That's where it gets the username from. Go through the adal.js code, you'll find it all over the place.

BTW - the idtoken is just a special access token and matches the access token for the clientid resource. As you state, the session cookie shouldn't be used for login state, the idtoken is what determines valid login state and why it's important to keep it recent.

If all we were discussing was a single app, then your example works just fine. What I was describing was a SSO scenario where the identity session isn't tied to a single application but a broader identity context. If your identity is tied to a single app it's simpler but AAD is not scoped to a single app, it's a general identity provider with session independent of any app. Remember, the session cookie the documentation talks about is with AAD (i.e. login.microsoft.com), not with the application using AAD. So logging out of AAD may happen outside your app, but you app needs to react to it happening outside it's context.

ghost commented 6 years ago

@geerzo You are right about AAD, the only thing, when the user logs out AAD invalidates the session tokens for the user (all for the user). So when adal requests a new token for any registered service silently, it should fail from all other apps. And this is decided based on the SSO session tokens by AAD and this is almost invisible for us but should be handled as a log out by all the apps.

Access and Id tokens are not revokeable so the app will only recognise the user logged out when it requests a new token by user interaction (without timed token retrieval I mean). If the user uses the same browser (application and not window) I think this is not a problem as cookies are assigned to the same provider (i.e. login.microsoft.com domain) and not for individual applications so it will kept alive.

Thanks for your time.