Page MenuHomePhabricator

[L] Update iOS Authentication management to support temporary accounts
Closed, ResolvedPublic5 Estimated Story Points

Description

It was found during T335425 that:

  • background processes relating to reading list syncing were receiving 401 errors and automatically logging the user out.
  • temporary accounts were not being recognised by the app as being logged in
  • app does not attempt to fetch notifications

Tasks

  • Update iOS authentication management to support temporary accounts
  • Audit app functionality in relation to temporary accounts, testing via test.wikimedia.org
  • Engineering only fixes to features to support or at minimum fail gracefully when handling temp account authentication

Engineering Details

  1. Clone cookies after edit posts

I do not expect our newer networking stack calls (WMFBasicService) to return any responses with the cookie set. As of today those calls are fundraising config calls, Apple Pay payment calls, and fetching the article summary. No MediaWiki editing calls go through here. So WMFData and by extension WMFBasicService can be left untouched.

What that does mean is that we need to make some changes to the legacy networking class in Fetcher.swift. For any POST calls there, in the response completion block we need to call self.session.cloneCentralAuthCookies(), that way any new cookies that spin up from an edit POST are propagated to the other wiki domains.

  1. (Cleanup) Remove Session.isAuthenticated

Session should not care about "authenticated" states, instead it should only be aware of cookies (if there are valid ones, to clone cookies, or to remove cookies). WMFAuthenticationManager should be the only class that modifies and is aware of an "authenticated" state.

  1. Add convenience isTemporaryAccount public Boolean property to WMFAuthenticationManager

Check WMFAuthenticationManager's loggedInUsername format and set to true if temporary account username pattern matches.

  1. Audit related calls throughout the app and consider if action should be taken if isTemporaryAccount = true. If not, wrap up logic in a (if !authenticationManager.isTemporaryAccount) conditional. Maybe reference T361765 for this

getLoggedInUser completions
getLoggedInUserCache calls
loggedInUsername calls
isLoggedIn calls

We should also hash out what we send for analytics. isAnon=?

This is hopefully enough to patch up the underlying bugs.

Additional things to test (Reference T361765 for this):
When in a temporary account state, terminate the app then relaunch. The app attempts to login the user again when relaunched, and it's unclear what will happen here. Do an edit, is the temporary account retained?

Test editing on pilot and non-pilot wikis. We want to be sure any of our cleanup methods (resetLocalUserLoginSettings, session.removeAllCookies()) do not clear out pilot wiki cookies as we edit back and forth.

Next level cleanup:

There are several methods and states that reference a "logged in" user. Upon first glance these appear to be users that fetched the MediaWiki API userinfo API (which returns the authenticated user info). It seems likely temporary accounts will have a valid userinfo response from that API. We should rename these methods to reflect this is nonIP users, perhaps, since now these methods could reference both logged in users AND temporary account users.

Add a very clear enum of state in WMFAuthenticationManager, instead of "isLoggedIn" & "isTemporaryAccount":

WMFAuthenticationManager {
enum LoginState {
     .anonymous
     .registered //(permanent)
     .nonregistered //(temporary)
}

let var loginState: LoginState?
QA Notes

These are the test steps we followed for Development testing:

  1. Launch app in a logged out state.
  2. Edit an article on Test Language and publish. After editing, go to revision history and confirm you see the temporary account name.
  3. Terminate the app. Run again, then edit an article on Test 2 Language and publish. After editing, go to revision history and confirm you see same temporary account name as you did in step 2.
  4. Now edit something on EN Wikipedia. You can edit your own user talk page if you don't want to edit an article (search User_talk:TSevener (WMF) for example). After editing, go to revision history and confirm you see an IP address as your edit.
  5. Go back and edit a Test Language article. Go to article history, confirm you see the same temporary account name as you did in step 2.
  6. Log into the app. Go back to Test, Test 2, and EN Wiki articles (or talk pages) that you edited earlier. Edit them again. Go to article history and confirm you see your username as the edit.
  7. As you click around the app, note if anything feels like a regression with authentication (unexpected logged out panels, Settings displaying an unexpected logged in our out state, etc.).

Event Timeline

From: T335425

Findings so far

The major bug I see right away is that a new account is created each time I edit anonymously. After doing a bit of digging, the reason is that our ReadingListSyncOperation, which is constantly running, assumes valid auth tokens = logged in user, and begins attempting to sync to the reading lists endpoint located at https://en.wikipedia.org/api/rest_v1/data/lists/changes/since/2023-04-27T21:22:13Z.

This in turn is returning a 401 error, and the app is automatically logging out the user before I attempt to edit again. I think this is likely just a problem with our inconsistent pointing to the beta cluster, as you may have noticed its trying to sync reading lists to EN Wiki. My hope is that once I fix that, a call to https://de.wikipedia.beta.wmflabs.org/api/rest_v1/data/lists/changes/since/2023-04-27T21:22:13Z with the temporary account cookies will not return a 401, which would stop this automatic logging out between edits.

That being said, it highlights how the app assumes valid cookies = logged in user throughout the app. We shouldn't even be attempting to hit that reading lists sync endpoint if the user has a temporary account, in my opinion, because I think that should be only a feature for permanent account users.

Problematic methods:

These will or may be true or populated for temporary accounts, which may not be what we want depending on the context we're using it in.
Session > hasValidCentralAuthCookies: https://github.com/wikimedia/wikipedia-ios/blob/main/Wikipedia/Code/Session.swift#L167
Session > isAuthenticated: https://github.com/wikimedia/wikipedia-ios/blob/main/Wikipedia/Code/Session.swift#L186 (calls hasValidCentralAuthCookies)
WMFAuthenticationManager > isLoggedIn: https://github.com/wikimedia/wikipedia-ios/blob/main/Wikipedia/Code/WMFAuthenticationManager.swift#L101
WMFAuthenticationManager > loggedInUsername: https://github.com/wikimedia/wikipedia-ios/blob/main/Wikipedia/Code/WMFAuthenticationManager.swift#L101

This is called explicitly when a user logs in to copy cookies over to wikidata & commons endpoints. We may want to be sure it gets called when a temporary account status appears as well.
Session > cloneCentralAuthCookies: https://github.com/wikimedia/wikipedia-ios/blob/main/Wikipedia/Code/Session.swift#L93

Update

After fixing the EN prod > DE beta labs mixup for syncing saved reading lists, it's acting better. User account # is retained between edits and app sessions. I do not see a logged in username under app settings, it still appears as if the user is logged out. I also don't see the app attempting to download their notifications, but I do see it attempting to sync their saved articles, which now returns with this error:

{
  "type": "https://mediawiki.org/wiki/HyperSwitch/errors/bad_request",
  "title": "readinglists-db-error-not-set-up",
  "method": "get",
  "detail": "Reading lists have not been set up for this user.",
  "uri": "/de.wikipedia.beta.wmflabs.org/v1/data/lists/changes/since/2023-05-01T14%3A40%3A57Z"
}

So we should probably prevent that syncing in this state.

If I log into an account, that account is attributed to the next edit. If I then log out and edit again, a new temporary account number is attributed to it.

I was unable to test cross-language or cross-site editing (for example, are we able to edit a Wikidata article description with these temporary accounts?). I expect it won't work currently - we'll need to add a cross-site cookie copying call whenever these temporary accounts are spun up after an edit, but I'm not sure. I also am unable to test cookie expiration - the app won't launch when I change the device date to next year. Maybe we could temporarily decrease it server-side to 1 week to test this scenario?

LGoto triaged this task as High priority.Jun 27 2023, 6:15 PM
LGoto moved this task from Needs Triage to Engineering Backlog on the Wikipedia-iOS-App-Backlog board.

@Seddon I am assuming this task is dependent on deployment to test.wiki but correct me if I'm wrong.

@Seddon I am assuming this task is dependent on deployment to test.wiki but correct me if I'm wrong.

Correct!

Seddon renamed this task from Audit iOS app and update iOS Authentication management to support temporary accounts to Update iOS Authentication management to support temporary accounts.Aug 15 2024, 3:03 PM
Seddon moved this task from Up next to Needs Triage on the Wikipedia-iOS-App-Backlog board.
Seddon moved this task from Needs Triage to Up next on the Wikipedia-iOS-App-Backlog board.
Tsevener updated the task description. (Show Details)
Tsevener updated the task description. (Show Details)
Tsevener renamed this task from Update iOS Authentication management to support temporary accounts to [L] Update iOS Authentication management to support temporary accounts.Aug 28 2024, 7:08 PM
Seddon set the point value for this task to 5.Aug 29 2024, 12:31 PM

PR: https://github.com/wikimedia/wikipedia-ios/pull/4959

With this work, weird bugs found in https://phabricator.wikimedia.org/T361712#10097437 should be fixed. When user is in temporary account state, the app should appear as if the user is logged out.

No temporary account UI changes will be displayed yet. That work will be done when we tackle the UI subtasks under https://phabricator.wikimedia.org/T373067.

ABorbaWMF subscribed.

This appears to be working on 7.5.9 (4078)

I had one issue with Step 4 in the QA area of the tickets description. In this case, I think an IP Block prevented me from editing a user talk page on en.wiki. I was seeing this error after encountering IP block message and attempting to add a reply to a talk page. I was able to add talk page messages on Test and Test2 wikis using a temp account.

IMG_0709.PNG (2×2 px, 284 KB)

I did Steps 1-4, and my IP address appeared as expected when I edited a talk page on en.wiki for step 4!