feat(datasource/middleauth): Add terms of service handling

[chrisj]

Added an optional errorHandler method on CredentialsProvider

I also want to retry with the same credentials after a user agrees to the terms of service, for now I kept that logic in middleauth but I would be a little cleaner if errorHandler could return "refresh" or "retry"

[jbms]

Would it be possible to do the TOS check as part of obtaining the credentials in the first place?

[chrisj]

The main reason why we have avoided that is that our terms of service are per dataset and a user may be given access to datasets they never intend to use so we only request terms of service when they attempt to access the dataset. So here we have the server informing the client which terms of service is missing.

For some dataset specific deploments we do a combined login+tos flow.

[chrisj]

I'm asking our team if that is necessary

[jbms]

Since the MiddleAuthCredentialsProvider is also given the requested URL, I think it would in principle work for it to also query if the TOS has been accepted for that particular dataset URL, when the credentials are requested.

However, that would result in an additional request per dataset URL even if the TOS were already accepted, or there are multiple datasources that all share the same TOS.

One tricky thing with CredentialsProvider is that it gets proxied across the main thread/worker thread boundary, and also when using the Python integration, it also gets proxied across the Python/Neuroglancer client boundary (not currently supported for middleauth but might be useful).

That means that having an additional errorHandler method on CredentialsProvider would be a bit tricky since it would have to get proxied in those two places as well.

If all of the API calls that need a custom error handler are within the graphene datasource anyway, then you could instead just supply the custom error handler when the API call is made rather than tying it to the CredentialsProvider -- that would avoid the complications of having to proxy it.

The issue with the error handler is that it does need to communicate back information that a specific TOS needs to be accepted. If we do need something like your current approach, of waiting for an error regarding the TOS rather than checking it when the credentials are first obtained, then one option could be to allow an additional value associated with the "refresh" request to be passed back by the error handler (the type would be a type parameter of the CredentialsProvider). This would be relatively easy to proxy back.

[chrisj]

We needed a bit of time before we were able to discuss this. We would like to keep graphene and middleauth decoupled.

Could you explain how the additional value would work? The singular oauth2 error handler would be able to return a value along with "refresh" but you said it could be a type parameter of the CredentialsProvider, so how would the CredentialsProvider let the error handler know what to pull out? (in our case, parsing the response out of the HTTPError)

Once we get the relevant value, would fetchWithCredentials be able to pass that to the CredentialsProvider? Perhaps to the next call to credentialsProvider.get(credentials, cancellationToken, NEW_VALUE)?

[jbms]

Regarding decoupling of graphene and middleauth, I suppose that middleauth already can be used for generic HTTP requests independent of graphene so my proposed solution of specifying a custom error handler in graphene API calls doesn't really work.

I suppose having the credentials provider handle the errors is then what is required. However, because of the various forms of proxying of the CredentialsProvider, we would need to also proxy the error information back. I think the easiest solution would be to always send back the http status code, headers, and body (perhaps only if json). This will be ignored by non-middleauth credentialsproviders but since this only occurs in the case of an error the small overhead does not matter.