telenordigital / connect-ios-sdk

Docs 📒👉
https://telenordigital.github.io/id-docs.telenordigital.com/integrate-ios-sdk.html
Apache License 2.0
9 stars 8 forks source link

refreshTokenExpirationDate and accessTokenExpirationDate is nil #53

Closed bandhavivinay90 closed 5 years ago

bandhavivinay90 commented 5 years ago

We have an extension to our app (widget) where we manually need to pass in the active session's details like accessToken, refreshToken, jwt, token expiration date etc in order to continue using the extension in the same way as the app. While trying to do that, we realised that these two members of the class OAuth2Session, refreshTokenExpirationDate and accessTokenExpirationDate is always nil. Do you think there can be a way to fix this?

We see this code: self.oauth2Session.save(accessToken: accessToken, refreshToken: refreshToken, accessTokenExpiration: exp, refreshTokenExpiration: nil, idToken: nil) in method refreshAccessToken in class OAuth2Module. Here we always set the token's expiration date to nil.

jorunfa commented 5 years ago

Hi. It's been very hectic lately. Will try to give you a proper reply to this issue later today.

jorunfa commented 5 years ago

You're referring to this line? https://github.com/telenordigital/connect-ios-sdk/blob/master/TDConnectIosSdk/OAuth2Module.swift#L400

https://github.com/telenordigital/connect-ios-sdk/issues/52#issuecomment-418273729:

As mentioned we don't return expire time on refresh token[...]

The API call for refreshing never returns refresh token expiration time. Thus we can't save it, as it doesn't exist. The expiration time for a refresh token is long. Something like a month.

If you have a refresh token you should assume it is valid, and try refreshing with it as per normal. If that fails the tokens will be cleared and the completion handler will be called (400 response): https://github.com/telenordigital/connect-ios-sdk/blob/master/TDConnectIosSdk/OAuth2Module.swift#L379

The accessTokenExpirationDate shouldn't be null, I would need some more information to understand why that is happening.

bandhavivinay90 commented 5 years ago

I tried reproducing accessTokenExpirationDate being nil but since it doesn't happen so frequently, it is tough to recreate the details. But yes I understand why does SDK set refreshTokenExpirationDate to nil in the refresh call because the response doesn't return the value for it. But do you think we can make it a private property or remove it completely if we don't use it since it is nil? Also, I have another question with the validity of refreshToken. I was assuming it comes with one year of validity. But since you mentioned a month, can we get a confirmation on that? Any way of printing the refresh token expiration date, just for us to know about it.

jorunfa commented 5 years ago

But do you think we can make it a private property or remove it completely if we don't use it since it is nil?

I see your point. The (historical) reason it's there is because this project is forked of a generic oauth 2 project. I won't be prioritizing to remove it, but when I find the time the plan is to implement the change to refreshTokenIsNotExpired as mentioned here: https://github.com/telenordigital/connect-ios-sdk/issues/52#issuecomment-418273729.

Meaning after the change refreshTokenIsNotExpired will return false if there are no tokens, and true if there is a refresh token that is not expired or does not have expiration time. Still, if you have more time than me, feel free to create a pull request for that 👼.

jorunfa commented 5 years ago

Quote from our backend dev:

I don't think the services need to know the exact values, as they should (according to the protocols) just try to use the token until it does not work any more. A refresh token will stop working in logout and revoke situations, in addition to the situation that it times out. They should know that every time they refresh, the token is consumed and a new one is issued [note from me: this is handled by the SDK]. We cannot give an exact number because the expiry time varies with application type, clients, and user state. Also, we should be free to change the refresh token timeout defaults at any time without having any implicit or explicit obligation to tell services.

Also:

I would say that it typically times out in the order of months. One month is not correct. You can also tell them that the timeout is longer for native clients than for web clients

jorunfa commented 5 years ago

I'll close this for now, until you can reproduce the accessTokenExpirationDate nil error. I'll try to fix the refreshTokenIsNotExpired bug and close #52 when it's done.