remove user token after account is deleted

[mojganii]

This PR introduces a crucial improvement to our system. After an account is deleted, it is essential to remove the associated user token from memory. This enhancement ensures that user data remains secure and reduces the risk of unauthorized access.

---

This change is 

[linear]

IOS-309 REST: evict access token for deleted account

REST.AccessTokenManager holds an in-memory dictionary with key being account number and the value being access token associated with it.

We should make sure to evict the associated access token after deleting the account from backend.

[pinkisemils]

As per the updated issue description, we should do this when the user logs out too. It's slightly less relevant there, as logging in will reset the token anyway, but there's no reason to keep the old auth token around if the user has logged out.

Reviewable status: 0 of 3 files reviewed, all discussions resolved

[pronebird]

AccessTokenProvider is bound by accountNumber so technically you don't have to accept accountNumber as input, you could just use self.accountNumber

[mojganii]

indeed!

[pronebird]

Looks good but REST.AccessTokenProvider can be improved a bit as per comment.

[pronebird]

Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

[pronebird]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @mojganii)

---

ios/MullvadREST/RESTDevicesProxy.swift line 163 at r3 (raw file):

        /// The completion handler will receive `true` if device is successfully removed,
        /// otherwise `false` if device is not found or already removed.
        public func deleteDevice(

I am afraid that removing device does not qualify as "log out", because we do that from other places too. It would be better to release all access tokens from SetAccountOperation when .unset action is given and perhaps at the very end after we delete a device.

In essence REST.ProxyFactory is the one creating REST.AccessTokenManager and then it holds it in REST.ProxyFactory.configuration.accessTokenManager property.

We could perhaps pass REST.AccessTokenManager all the way from AppDelegate into TunnelManager and then to SetAccountOperation so that we could tell it directly to remove all tokens right after logging out.

Note that the task card specifies that we should delete all tokens on logout.

[pronebird]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)

[buggmagnet]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii and @pronebird)

---

ios/MullvadREST/RESTDevicesProxy.swift line 163 at r3 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

I am afraid that removing device does not qualify as "log out", because we do that from other places too. It would be better to release all access tokens from SetAccountOperation when .unset action is given and perhaps at the very end after we delete a device.

In essence REST.ProxyFactory is the one creating REST.AccessTokenManager and then it holds it in REST.ProxyFactory.configuration.accessTokenManager property.

We could perhaps pass REST.AccessTokenManager all the way from AppDelegate into TunnelManager and then to SetAccountOperation so that we could tell it directly to remove all tokens right after logging out.

Note that the task card specifies that we should delete all tokens on logout.

I agree.
I would suggest to revert the current PR, and go with @pronebird 's suggestion

---

ios/MullvadREST/RESTDevicesProxy.swift line 169 at r3 (raw file):

            completion: @escaping CompletionHandler<Bool>
        ) -> Cancellable {
            let accessTokenProvider = createAuthorizationProvider(accountNumber: accountNumber)

This shouldn't be needed here.

[rablador]

Reviewed 1 of 3 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii and @pronebird)

[pronebird]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadREST/RESTDevicesProxy.swift line 169 at r3 (raw file):

Previously, buggmagnet wrote…

This shouldn't be needed here.

It's a matter of preference and technically REST can invalidate tokens for accounts that it removes, because it knows when it does that.

It's just that there is no separate call for logout so in that regard certainly tunnel manager is the best equipped to clean all tokens after the fact.

[mojganii]

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

---

ios/MullvadREST/RESTDevicesProxy.swift line 163 at r3 (raw file):

Previously, buggmagnet wrote…

I agree.
I would suggest to revert the current PR, and go with @pronebird 's suggestion

Done.

---

ios/MullvadREST/RESTDevicesProxy.swift line 169 at r3 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

It's a matter of preference and technically REST can invalidate tokens for accounts that it removes, because it knows when it does that.

It's just that there is no separate call for logout so in that regard certainly tunnel manager is the best equipped to clean all tokens after the fact.

Done.

[pronebird]

Reviewed 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)