Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getService, getLayer should automatically retry failed requests sans authentication #920

Open
drewdaemon opened this issue Sep 23, 2021 · 6 comments

Comments

@drewdaemon
Copy link
Contributor

drewdaemon commented Sep 23, 2021

Background

ArcGIS Server tries to validate any token passed in a request whether or not authentication is needed for that request. This can lead to a scenario such as the following:

Bob creates a Feature Service item on ArcGIS Enterprise for a public feature service on AGOL. The consuming application sends an ArcGIS Enterprise authentication token in requests to the AGOL service. The service tries to validate the token with AGOL and fails, preventing the request from succeeding. If a token is not sent, the request is successful since the service is public.

Solution

A simple and bullet-proof approach to solving this would be to

  • Make the request with authentication
  • If it fails with an ArcGIS Auth Error, make it again without authentication

cc @rgwozdz @tomwayson

@tomwayson
Copy link
Member

I thought we already did this, maybe down at the request level. I looked at that code, but it's really hard to follow.

So you're saying this definitely does not happen today?

@drewdaemon
Copy link
Contributor Author

drewdaemon commented Sep 23, 2021

@rgwozdz was under the same impression. The most we could find was the retry method on the ArcGISAuthError class which is helpful, but doesn't get us all the way there.

Simply put, when I call getService and provide authentication it does not automatically try it without authentication.

@drewdaemon drewdaemon self-assigned this Sep 23, 2021
@patrickarlt
Copy link
Contributor

We support this but not automatically. See #503. We allow calling authError.retry() without a session to retry a request without auth. I'm hesitant to do this automatically especially for only a few requests because I think the auth flow should be as predictable across methods as possible without any changes for specific methods. For example why only getService() and getLayer() and not queryFeatures() or getItem()?

There are also a few other solutions to this which are reasonable to implement in the application:

  1. If you know a service is public you shouldn't pass authentication avoiding this issue entirely.
  2. If you aren't sure if a service is public don't pass authentication` for the initial request and retry with authentication for the subsequent request.

The other reason I'm hesitant to do this automatically is that we cannot know if the AuthError was caused by the token being expired OR is the token isn't needed. For example consider this:

getService({
  url: "some unknown service",
  authentication: someUserSession
}).catch(err => {
  if (err.name === 'ArcGISAuthError') {
    // Did the token expire and we need to refresh it? Or is this a public service that the session isn't good for?
    // It is difficult for us to know.
    err.retry(() => Promise.resolve(undefined))
      .then(response)
  }
})

This ties into something larger that I wanted to discuss though which is, what is our default refresh behavior? This example consider the following scenario:

You implement oAuth 2.0 on the server with a refresh token on the server and short lived tokens on the client. Ideally you would want want to define a single authErrorRetryHandler for all of REST JS to fetch a new token from your server whenever a request with a token fails on the client. Since tokens on the client are short lived you will probably run into this frequently and having to define a custom error handler for every request to handle token refreshes is a lot of work. It would probably be easier if as the developer I could define how I wanted by UserSession to refresh itself in cases where I cannot automatically refresh it. However this runs counter to the "retry without auth pattern" we are discussing here.

@drewdaemon
Copy link
Contributor Author

@patrickarlt those strike me as fair arguments and I like the idea of focusing on broad solutions, though I think @tomwayson would probably have more to say in that realm.

I did notice and mention the retry method in my comment above. My initial impulse was to push this behavior as low in our stack as possible since I would call this an idiosyncrasy of ArcGIS Server and I see smoothing such things as a big value-add of this library. That said, I hear you about the expiring token concern and I'm comfortable keeping this logic in our consuming application as you suggest.

For example why only getService() and getLayer() and not queryFeatures() or getItem()

I think the key is that I've only observed this behavior with requests to ArcGIS Server. Have you observed this behavior in the context of the Portal API as well?

@patrickarlt
Copy link
Contributor

My initial impulse was to push this behavior as low in our stack as possible since I would call this an idiosyncrasy of ArcGIS Server and I see smoothing such things as a big value-add of this library.

I do agree that we should handle this, it is just a question of how. I agree it should be low level probally as low as request itself. The JS API works like this:

  1. When a request to an unknown service is made make the request without the token first.
  2. If that fails find the owning portal and lookup the correct token in IdentityManager and try to federate by having the Portal generate a token for the service. The token is used for subsequent requests.
  3. If you cannot federate (because you don't have a token for the owning portal) or the service has no owning portal popup the auth dialog.

REST JS is fundamentally different because:

  1. We don't have a global identity manager
  2. We don't have a UI or auth popups
  3. Auth is passed directly to the request

There are 2 use cases I would ideally like to handle:

  1. Retrying the request without a token (this use case)
  2. Refresh the token (if possible) and retry the request with the fresh token

I think we handle this like this:

  • Add an anonymousRequest option to request. Defaults to false. If true will try the request without authentication first (matching the JS API), methods like getLayer(), getService() and queryFeatures() can internally set this to true while most others can leave this as false.
  • If the object passed to authentication supports refreshing the session on auth errors we should automatically refresh the token and retry BEFORE sending the error down the chain.
  • Add a new refreshToken option in UserSession which if a function that if set will be tried instead of the internal refreshSession this allows for my use case of allowing the app to set a custom refresh function for split client/server auth.

@drewdaemon drewdaemon removed their assignment Sep 29, 2021
@patrickarlt
Copy link
Contributor

@tomwayson @gavinr @jwasilgeo and I met on this. We can't decide on the best way forward. There isn't a good consensus on HOW we should resolve this issue since it is complex and the auth flow is already rather complex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants