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

Refresh token exists but user is still presented with login pop up for Safari #52

Closed bandhavivinay90 closed 5 years ago

bandhavivinay90 commented 6 years ago

img_2549 We were testing the iOS app today and got into a situation where in the user was logged out and isAuthorised() returned us false when we tried to get into the app again. What we do in such scenario when isAuthorised() is false, is we try to call the requestAccess() method and that takes care of presenting a pop up (login page) or internally refreshing the token and taking us inside the app. The screen shot shows the pop up and I expect the login screen to be presented after I tap "Continue" in the pop up. But instead, what happened was we directly got into the app after hitting Continue and the tokens were refreshed and generated automatically. This is a scenario we are not aware of. Can this be a possibility or do you see this as a bug?

jorunfa commented 6 years ago

What might be happening here is the Safari browsers SSO cookie sees that the user is signed in. I believe you can test it by opening https://connect.staging.telenordigital.com or https://connect.telenordigital.com (staging/not staging) and see if the user is signed in.

bandhavivinay90 commented 6 years ago

I believe we never share any cookie with the browser in iOS. Apple still has that as bug. Also, when I am logged in and I go to the browser, I can see not being logged in there.

jorunfa commented 6 years ago
  1. Are you using LoA 1 or 2?
  2. On wifi or just mobile data?
  3. It's possible you're just being automatically signed in with header enrichment?
  4. Is webView in config true or false?

You can send me some details about the phone number the device is using and which environment, and I can see if I can find the session to see what is happening.

bandhavivinay90 commented 6 years ago

Here goes the detail:

  1. We use "acr_values": "2", that is LoA 2
  2. Happens in either case.
  3. Not sure about this. But maybe header enrichment comes into place if we set some specific parameter in the configuration.
  4. webview is set to false.

Its random and occur in any environment. But as far as we remember, we have been mostly testing in the Production environment and not staging.

jorunfa commented 6 years ago

I suggest testing in staging in general. Please send me the phone number at jorund.fagerjord@telenordigital.com and the given time when this happens and I will see if I can find the sessions.

jorunfa commented 6 years ago

I think I understand the problem now. It's been encountered it before.

Some cut and paste from an earlier Jira discussion:

The isAuthorized() method in the SDK of the OAuth2Module checks if the user has an access token that is still valid: https://github.com/telenordigital/connect-ios-sdk/blob/master/TDConnectIosSdk/OAuth2Module.swift#L624

While this makes sense for an Http authorization module, because you must have a valid access token there to get access to a protected resource, it is not the best indication that a user is signed in. A better indication for that is if there is an unexpired refresh token present.

You can do that by checking oauth2Module.oauth2Session.refreshToken != nil (we don't provide refresh token expiration time from our api's, but they last a long time). If that is true, then the user is signed in. And then isAuthorized is a better check to see if the user's current access token is valid.

I'll see if I can find some time to add docs for this to avoid future problems with this.

bandhavivinay90 commented 6 years ago

Yes, that definitely makes sense. Maybe relying upon the refresh token is a safer idea.

But one thing which is a bit strange is when we receive false for isAuthorised(), we try to check for if the access token exists and is not empty, but we get back an empty access token from the SDK, which is not correct in this scenario. In this case, access token still exists but is not valid, so when SDK returns an empty access token, we assume that there is no session created, with no access or refresh token and so we log the user out.

bandhavivinay90 commented 6 years ago

I tried the solution of relying upon refreshTokenIsNotExpired() of oauth2Session, but looks like it returns true even when access token and refresh token are empty. I think the sdk should return false here when there is no active session.

jorunfa commented 6 years ago

As mentioned we don't return expire time on refresh token, so refreshTokenIsNotExpired always returns true. You should just check if the refresh token is not nil: oauth2Module.oauth2Session.refreshToken != nil

I might agree it should return false if there are no tokens.

jorunfa commented 5 years ago

So in conclusion I think we might update the SDK and/or update the documentation to avoid this problem in the future.

bandhavivinay90 commented 5 years ago

Yes, SDK would need some fixes. Access token returned to us is still empty at times, which should not happen. But for now, we will spend some time in testing with the alternative solution proposed by you and then get back to you with the updates.

jorunfa commented 5 years ago

Access token being empty sounds like a bug I haven't encountered before. Please create a new issue with some recreation details. I don't have a lot of time to work on SDKs at the moment due to a cluster fuck of priorities. If you feel like it, please consider making a pull request to suggest changes.

jorunfa commented 5 years ago

@bandhavivinay90: Does #56 make refreshTokenIsNotExpired() conform to your expectations?

bandhavivinay90 commented 5 years ago

Is there a way I can log the refresh Token Expiration Date. I always get it nil in the code. It's important that we know how much time does the app have before it would need a new refresh token. I always assumed refresh token comes with a validity of a year long. Is that true?

jorunfa commented 5 years ago

Did you miss this reply, by any chance? 👉https://github.com/telenordigital/connect-ios-sdk/issues/53#issuecomment-422734725

And as mentioned a few times now, refresh token expiration will always be nil, since you will never get that from out APIs.

The pull request #56 aims to fix refreshTokenIsNotExpired the bug where it returns true when there are no tokens. I can merge it if you think it's fine.

bandhavivinay90 commented 5 years ago

Yes, please go ahead