Esri / data-collection-ios

Mobile data collection app using the iOS Runtime SDK.
https://developers.arcgis.com/
Apache License 2.0
25 stars 26 forks source link

Use credentialCache.removeAndRevokeAllCredentials when signing out. #248

Closed mhdostal closed 4 years ago

mhdostal commented 4 years ago

Data Collection to support clearing and revoking credential - As of 100.6, Runtime introduced a method to clear and revoke credentials. This introduction came after the initial general release of Data Collection. Calling this method revokes the portal user's credential on the server side.

Let's retroactively revisit Data Collection to ensure this method is called when a user requests to log out.

Additional considerations should be made to ensure that services that have been supplied a previously valid credential will issue a challenge again after that credential is revoked.

esreli commented 4 years ago

Quick note here, please rebase onto branch v.next-1.2.2

mhdostal commented 4 years ago

Are we sure the order of operations are correct here?

The first method performed clears credentials locally and attempts to revoked them on the server. This method is async. Should we perform appAddressLocator.removeCredentialsFromServices() in the callback? Should we set the new portal portal = AGSPortal.configuredPortal(loginRequired: false) in the callback?

I'm not exactly sure, though that does seem correct... at least to me.

I don't think it matters. The reason the removeCredentialsFromServices() is async is it's going over the network. That may/may not take a while and I think it's important for the workflow that once the user "signs out", the UI updates quickly, which wouldn't happen if we did it in the async handler. The operations are independent of each other, so I don't think they need to move to the async completion handler.

mhdostal commented 4 years ago

@esreli How about we move the sign-in/out and potential PortalSessionManager updates/fixes to a future release? That way we can get this out the door, with the 100.9 Toolkit update and 100.9.0 SDK certification. The proposed portal sign-in/out changes are more involved.

esreli commented 4 years ago

The operations are independent of each other, so I don't think they need to move to the async completion handler.

@mhdostal

Sounds good! Last consideration, what if the user is disconnected? Would it be that the server isn't aware the user is trying to sign out but because the credentials are wiped locally, it doesn't matter if the server persists them because they will never again be used? Just trying to spitball some different cases here...

@esreli How about we move the sign-in/out and potential PortalSessionManager updates/fixes to a future release?

@mhdostal

Sounds good, i've created an issue to capture this work down the road.

mhdostal commented 4 years ago

FYI, here is the test code I used:

This produces a “Credential Required” error the second time fetchContent is called:

fetchOp = portal.user?.fetchContent(completion: { [weak self] (items, folders, error) in
    print("fetchContent error = \(String(describing: error?.localizedDescription))")

    AGSAuthenticationManager.shared().credentialCache.removeAndRevokeAllCredentials { [weak self] (results) in
        if !results.isEmpty {
            print("Could not revoke the following credentials:")
            results.forEach { (credential, error) in
                print("  \(credential.username ?? ""); error: \(error.localizedDescription)")
            }
        }

        //Test and make sure credentials are revoked
        self?.fetchOp = self?.portal.user?.fetchContent(completion: { (items, folders, error) in
            print("post-signOut fetchContent error = \(String(describing: error?.localizedDescription))")
        })
    }
})

The output I get is:

fetchContent error = nil
post-signOut fetchContent error = Optional("Credential Required")
mhdostal commented 4 years ago

Sounds good! Last consideration, what if the user is disconnected? Would it be that the server isn't aware the user is trying to sign out but because the credentials are wiped locally, it doesn't matter if the server persists them because they will never again be used? Just trying to spitball some different cases here...

@njarecha Can you answer @esreli question? Essentially, would the behavior be the same as the pre-credentialCache.removeAndRevokeAllCredentials (meaning only clearing the local credential cache) behavior if the user was disconnected when credentialCache.removeAndRevokeAllCredentials was called?

njarecha commented 4 years ago

@mhdostal @esreli

@njarecha Can you answer @esreli question? Essentially, would the behavior be the same as the pre-credentialCache.removeAndRevokeAllCredentials (meaning only clearing the local credential cache) behavior if the user was disconnected when credentialCache.removeAndRevokeAllCredentials was called?

Yes, in case of any error (including network error) while while calling removeAndRevokeAllCredentials behavior same as before this method. I would suggest dismissing view controller after SignOut and setting portal object to nil are new instance for everything to work correctly.

mhdostal commented 4 years ago

Yes, in case of any error (including network error) while while calling removeAndRevokeAllCredentials behavior same as before this method. I would suggest dismissing view controller after SignOut and setting portal object to nil are new instance for everything to work correctly.

Thanks @njarecha . We are already setting the portal to a new instance, so we should be good.