search

TheM4hd1 / SwiftyInsta

Instagram Private API Swift
MIT License
225 stars 51 forks source link

Save info doesn't work after sign in. #203

Closed mycroftcanner closed 4 years ago

mycroftcanner commented 4 years ago

@sbertix There should be a setting to allow the web view to save the cookies. I look into and submit a PR if it is possible.

sbertix commented 4 years ago

What are you referring to? 🤔\ We already have a cache for authentication responses and custom LoginWebView support (without LoginWebViewController)…

Check out KeychainSwift persistency implemented in Authentication.

mycroftcanner commented 4 years ago

I am referring to LoginWebViewController. after you sign in it offer to save info. However we don't seem to save the cookies.

sbertix commented 4 years ago

What cookies should we save?\ We already save all .instagram.com-session related cookies: that's exactly how we can then use them to call the endpoints…

You should persist them once they're returned and then fetch them instead of presenting LoginWebViewController if they're still valid.\ Please check out the README.md with the implementation details.

mycroftcanner commented 4 years ago

@sbertix I do persist them...

There are two types of cookies, cookies that survive the session or not (when you close the browser session cookies are deleted.)

I had the impression that we are not saving the session cookies but not the persistent cookies (the ones with expiration date.)

Because even if I tap Save Info and reopen the login view controller it doesn't seem to matter. I have to take a closer look.

Maybe we just need a way to load back the cookies in the login web view controller.

sbertix commented 4 years ago

LoginWebViewController is provided out of the box and for security reason it should never be provided with stored cookies.\ We store all cookies returned by the browser needed for the API to operate. Cookies related to browsing itself, e.g. the language selected in LoginWebViewController, are not saved, as they only pertain to LoginWebViewController.

The authentication flow is pretty solid:

If the user logs out from your app and is presented LoginWebViewController again, it should be empty.\ There's no reason for LoginWebViewController to be filled with data, if it is, it means you're doing something wrong as Authentication.Responses are persisted in the Keychain.

mycroftcanner commented 4 years ago

I double-checked... if you login on Instagram from a web browser and tap Save Info after signing in... then if you sign out and try to sign in again you don't need a password for that account to log back in.

image
mycroftcanner commented 4 years ago

The issue is that the end user of a SwiftyInsta app will expect the above behaviour after tapping Save Info. I did and one of my friends did too.

image
sbertix commented 4 years ago

I'm saying it again… you're friend should only be presented with LoginWebViewController after they log out, meaning their Authentication.Response gets unpersisted.\ In that case, there's no saving your login info that matters. Cause you're log out, and they wouldn't be saved either way: you just deleted them (that's what happens from the browser too).

So again, if your friend get shown the LoginWebViewController and requires that feature it means that they haven't implemented this properly: for all intents and purposes LoginWebViewController should only be presented once, when you first load the app, and then forgotten about it.

mycroftcanner commented 4 years ago

They are only presented LoginWebViewController after they log out already.

However when they were presented the LoginWebViewController, instagram is offering to "Save Info" which means that you won't need to type your password next time you are presented with that sign in page.

People who are familiar with the web version of instagram are familiar with this feature as I have shown in the screenshots above and it is natural for them to expect not to have to type their password again.

Cause you're log out, and they wouldn't be saved either way: you just deleted them (that's what happens from the browser too).

@sbertix that's not true at all: on the web version of Instagram... if you tap Save Info after login in and then log out, you will not be prompted for a password:

It works also that in the Instagram app and the Facebook app and web. They do that because people forget their passwords all the time and they lose usage when this happen.

sbertix commented 4 years ago

You won't be asked to log in again, but if you decide to log out, then it will pop again after logging back in.\ Same thing here, as LoginVebViewController should only be displayed after logging out.

Furthermore, if we were to actually implement that, as you're suggesting, you'd need two "logout" methods: one for the API, presenting the login web view on completion, and one for the web view cause this time, according to your flow, you'd be automatically logged back in...

mycroftcanner commented 4 years ago

It works this way in the official instagram app and web... to delete the save info you have to log out and then click for Remove account on the login page. I can have 'Remove account' in my settings page to make it clear to the user.

image
mycroftcanner commented 4 years ago

Please actually try it for yourself in both www.instagram.com and the official app.

mycroftcanner commented 4 years ago

Or if you insist on not allowing that... then the login web view controller shouldn't show the Save Info page because it won't make any sense to the user.

sbertix commented 4 years ago

You're right, I meant to say "when you delete your cookies", not just "logging out", cause that's technically what we do.\ Genuinely bringing this to SwiftyInsta seems to me more like we would be adding one more step instead of removing one (multi account support? You still need to present them that "continue as..." but they would just skip it, etc).\ It makes sense for Insta, the way the people use the website, definitely not for an API client.

Anyway, I'm not here to fight hahaha.\ I'm not convinced it's a good idea but you're more than welcome to implement it and push a PR for it, and I'd be more than happy to merge it.