Esri / arcgis-rest-js

compact, modular JavaScript wrappers for the ArcGIS REST API
https://developers.arcgis.com/arcgis-rest-js/
Apache License 2.0
347 stars 119 forks source link

Handling authentication #5

Closed patrickarlt closed 7 years ago

patrickarlt commented 7 years ago

Authentication is a problem that has been bugging me for awhile about this repo. I'm note entirely sure how to resolve it. As a reference here are some of our design goals for this:

I feel like these goals are at odds with each other. No matter what I seem to always come up with designs where you pass the authentication in or query the authentication object for information.


Design 1

Below is a rehash of my inital design:

import { request } from 'arcgis-rest-core';
import { UserAuthentication, AppAuthentication } from 'arcgis-rest-auth';

const auth = new AppAuthentication({
  clientId: '123'; // required
  clientSecret: '123' // required
  token: '123' // optional, an access token if you have one
  expires: Date.now() // when the provided token expires
  portal: 'https://www.arcgis.com/sharing/rest' // which portal generated this token
});

auth.on('error', (error) => {
  // unable to authenticate a request so get a new token and retry.
  auth.refreshToken(/* ... */).then((token) => {
    error.request.retry(token);
  });
});

request({
  url: '...'
  authentication: auth,
  params: {
    //...
  }
}).then((response) => {
  // Success!
}).catch((error) => {
  // Error! Both HTTP AND server errors will end up here...
});

This approach will work fine, except that most of the interaction this this repo won't be directly with request it will be with help methods that we will write like geocode. So this starts to break down when we do this:

import {request} from 'arcgis-core';

export function geocode (/* ... */) {
  // since request is internal it won't be authenticated. :(
  return request(/* ... */).then(/* ... */);
}

Design 2

import { request } from 'arcgis-rest-core';
import { UserAuthentication, AppAuthentication } from 'arcgis-rest-auth';

const session = new AppAuthentication({
  clientId: '123'; // required
  clientSecret: '123' // required
  token: '123' // optional, an access token if you have one
  expires: Date.now() // when the provided token expires
  portal: 'https://www.arcgis.com/sharing/rest' // which portal generated this token
});

session.on('error', (error) => {
  // unable to authenticate a request so get a new token and retry.
  session.refreshToken(/* ... */).then((token) => {
    error.request.retry(token);
  });
});

// The UserAuthentication and AppAuthentication methods will expose a `authenticate`
// method that accepts a Promise from `request()` it then returns that promise with
// additional error handling.
session.authenticate(request({
  url: '...'
  params: {
    //...
  }
})).then((response) => {
  // Success!
}).catch((error) => {
  // Error! Both HTTP AND server errors will end up here...
});

// this would also work for anything that returned a Promise from `request()` so...
session.authenticate(geocode({/* ... */})).then((response) => {
  // Success!
}).catch((error) => {
  // Error! Both HTTP AND server errors will end up here...
});

This method has 1 major drawback though. The inital request will ALWAYS be unauthenticated, since the token was never passed in the inital params. This sucks but we should be able to recover from the error but it will happen every time whereas the JS API is smart enough to not fail and retry with a token all the time.


Design 3

This design is like the inverse of the one above. We simply expose the authentication option on all methods use request under the hood and teach request how to use the passed authentication object to handle auth failures.

import { request } from 'arcgis-rest-core';
import { UserAuthentication, AppAuthentication } from 'arcgis-rest-auth';

const session = new AppAuthentication({
  clientId: '123'; // required
  clientSecret: '123' // required
  token: '123' // optional, an access token if you have one
  expires: Date.now() // when the provided token expires
  portal: 'https://www.arcgis.com/sharing/rest' // which portal generated this token
});

session.on('error', (error) => {
  // unable to authenticate a request so get a new token and retry.
  session.refreshToken(/* ... */).then((token) => {
    error.request.retry(token);
  });
});

// we would have to teach request how to use `AppAuthentication` to recover from
// auth failures. Hopefully via an interface so it doesn't bloat the core repo.
request({
  url: '...'
  authentication: session
  params: {
    //...
  }
})).then((response) => {
  // Success!
}).catch((error) => {
  // Error! Both HTTP AND server errors will end up here...
});

// we would have to pass the `authentication` object down throguh the 
geocode({
  authentication: session
})).then((response) => {
  // Success!
}).catch((error) => {
  // Error! Both HTTP AND server errors will end up here...
});

Design 4 - Singleton Authentication Manager

import { request } from 'arcgis-rest-core';
import { AuthenticationManager } from 'arcgis-rest-auth';

// register a client id and client secret
AuthenticationManager.registerOauthCredentials({
  clientId: '123'; // required
  clientSecret: '123' // required
  token: '123' // optional, an access token if you have one
  expires: Date.now() // when the provided token expires
  portal: 'https://www.arcgis.com/sharing/rest' // which portal generated this token
});

// register an existing token or token from user auth
AuthenticationManager.registerToken({
  clientId: '123'; // required
  token: '123' // optional, an access token if you have one
  expires: Date.now() // when the provided token expires
  portal: 'https://www.arcgis.com/sharing/rest' // which portal generated this token
});

AuthenticationManager.on('authentication-required', (error) => {
  // unable to authenticate a request user will need to prompt for auth to a service.
});

// request now talks to the `AuthenticationManager` for all auth decisions.
request({
  url: '...'
  params: {
    //...
  }
})).then((response) => {
  // Success!
}).catch((error) => {
  // Error! Both HTTP AND server errors will end up here...
});

// since geocode will impliment `request` it will also use the `AuthenticationManager` singleton.
geocode({
  authentication: session
})).then((response) => {
  // Success!
}).catch((error) => {
  // Error! Both HTTP AND server errors will end up here...
});

I'm not super liking any of these options here. @jgravois @dbouwman @ajturner @tomwayson.

ajturner commented 7 years ago

Thanks for the thorough walk-through of your designs. I agree that none of them are particularly compelling for accomplishing the priorities you outlined.

Design 5

I'm a fan of the decorator pattern for multiple authentication. Consider that the session is the instance of the concrete object, and that the rest of core operations add functionality (decorate) the class and all instances.

import { request } from 'arcgis-rest-core';
import { UserAuthentication, AppAuthentication } from 'arcgis-rest-auth';

const session = new AppAuthentication({
  clientId: '123'; // required
  clientSecret: '123' // required
  token: '123' // optional, an access token if you have one
  expires: Date.now() // when the provided token expires
  portal: 'https://www.arcgis.com/sharing/rest' // which portal generated this token
});

session.on('error', (error) => {
  // unable to authenticate a request so get a new token and retry.
  session.refreshToken(/* ... */).then((token) => {
    error.request.retry(token);
  });
});

// Use the session instance with geocode method. Geocode has session accessors to use tokens/request methods
session.geocode({/* ... */})).then((response) => {
  // Success!
}).catch((error) => {
  // Error! Both HTTP AND server errors will end up here...
});

// We can create a new session for a different user/portal

const session2 = new AppAuthentication({/* ... */});
session2.geocode({/* ... */}))

Multiple request implementations

We also discussed the necessity/benefit of re-using existing Request implementations provided by frameworks


// Optionally use a different request mechanism
session.setRequester(/*...*/)
dbouwman commented 7 years ago

@ajturner so... I'm not sure I follow that... From that pseudo code, it looks like we'd need to decorate the session with all the other top level API wrapper classes? Thus something like...

session.featureService.create({...}).then(...)

Unless the client app composes the session, this sounds like it would be very difficult to pick-and-choose the parts of the API you want - i.e. just auth + geocoding, vs pulling in everything.

In general, what we've found from building the ember-arcgis-portal-services and torii-arcgis-provider libraries is that they are easier to use in other applications if they are very lean.

Currently they follow a pattern similar to Design 3 above, but with Ember-isms. The consuming application uses Torii (Ember oAuth lib), and the torii-provider-arcgis to fetch / re-hydrate a AGO identity + token that is exposed via a "session" service.

With that in place, the app can then use that "session" service... since this is all at the "ember" level, the ember-arcgis-portal-services uses the Ember dependency injection system to get the session service from the current running context, and then if no portalOptions are passed into the service methods, it will automatically use the credentials from this service. This provides a very clean developer experience, but it is very ember-specific.

Ideally, the arcgis-rest library could be dropped into ember-arcgis-portal-services and torii-arcgis-provider to provide the low-level api access, while still providing an idiomatic ember developer experience. And in success, arcgis-rest should provide the primitives to allow for idiomatic usage in Angular, React, Node etc... or if appropriate, just consume the api directly.

patrickarlt commented 7 years ago

@ajturner thinking about sub-libraries like geocode adding functionality to session is interesting but in your example where does session.geocode() come from? It makes code a little hard to reason about:

import { request } from 'arcgis-rest-core';
import { UserAuthentication, AppAuthentication } from 'arcgis-rest-auth';

const session = new AppAuthentication({
  clientId: '123'; // required
  clientSecret: '123' // required
  token: '123' // optional, an access token if you have one
  expires: Date.now() // when the provided token expires
  portal: 'https://www.arcgis.com/sharing/rest' // which portal generated this token
});

session.on('error', (error) => {
  //handle error, retry ect...
});

session.authenticate(request); // adds a `request` method to session

session.request(/* */);

export session // export the singleton.

in a different file...

// routing-utils.ts
import { geocode } from 'arcgis-rest-geocoding';
import * as routing from 'arcgis-rest-routing';
import session from './arcgis-session';

session.authenticate(geocode); // can handle a functions
session.authenticate(routing); // or an object of functions

session.geocode(/* ... */);

session.calculateDriveTime(/* ... */);

in a third file...

// create-web-map.ts
import { createItem, updateItem, shareItem } from 'arcgis-rest-items';
import session from './arcgis-session';

session.authenticate(createItem, updateItem, shareItem) //

session.geocode(/* */); // works because we decorated session with geocode in routing-utils.ts

I'm also struggling with how to represent this interface in TypeScript. Probally through something like Intersection types or mixins. Since dymanically decorating session would kinda defeat the purpose of using a typed language in the first place, but I'll take a look at it.

patrickarlt commented 7 years ago

I forget that RxJS already does this, you can take a look at See https://medium.com/@OlegVaraksin/modern-way-of-adding-new-functionality-to-typescript-libraries-by-patching-existing-modules-6dcde608de56 for more details.

It would work like this:

// in our app
import { AppAuthentication } from 'arcgis-rest-auth';
import 'arcgis-rest-gecode/authenticated/geocode';

const session = new AppAuthentication({
  clientId: '123',
  clientSecret: 'abc'
});

session.geocode(/* ... */);
// arcgis-rest-gecode/authenticated/geocode.ts
import { AppAuthentication } from "arcgis-rest-auth";
import { geocode } from './geocode';

// open up the "arcgis-rest-auth" module and add a method to AppAuthentication
declare module "arcgis-rest-auth" {
  interface AppAuthentication<T> {
    geocode(): Promise<U>;
  }
}

// now add the method to that prototype
AppAuthentication.prototype.geocode = function (f) {
   return geocode(/* ... */).then(/* ... */).catch(/* ... */);
}

This uses declaration merging in TypeScript to extend existing classes.

patrickarlt commented 7 years ago

@dbouwman @ajturner this gives us the following pattern:

// Export single `ApplicationSession` and `UserSession` could both extend from a single `Session` base class.
// We would then extend the `Session` class with new methods.
import { ApplicationSession, UserSession } from 'arcgis-rest-auth';
import 'arcgis-rest-gecode/authenticated/geocode';
import 'arcgis-rest-routing/authenticated/serviceArea';
import 'arcgis-rest-items/authenticated/createItem';

const session = new UserSession({
  clientId: '123',
  token: 'zyx',
  expires: new Date(),
  portal: 'https://www.arcgis.com/sharing/rest'
});

// im not 100% sure how this would work but I'll figure it out...
session.on('authentication-error', (e) => {
  // the session couldn't figure out how to access a resource
  // prompt user for auth and try again with a new token:
  e.retry(newToken);
});

session.geocode(/* ... */);
session.serviceArea(/* ... */);
session.createItem(/* ... */);

This does mean a few things:

  1. More work in each package, since all packages have to export versions of their methods that work with arcgis-rest-auth. We could probally make some utils to make this easier.
  2. We have to be careful about naming things. Since we are always extending Session we cannot namespace things like session.featurelayer.create(/* ... */)

This pattern does meet all of our goals however:

  1. Can maintain multiple sessions simultaneously.
  2. Each session only needs to be setup once.
  3. We avoid singletons like IdentityManager
  4. Proper federation as long as you use session
  5. Developers can still opt out of using arcgis-rest-auth and pass token in the params.
dbouwman commented 7 years ago

Since the consuming application will have be best understanding of the context in which calls are made, and this is becoming a complicated solution, with a variety of trade-offs, I'd like to suggest that we side-step the "authManager" aspect of arcgis-rest, and start building out the sub-modules/packages.

If this is to work in node and browsers, we can't have the library itself "prompt for creds", so that has to be the concern of the consumer... and even if it was purely browser based, I think we have all shaken our fists at the JSAPI popping up a UI we don't control, so we'd still want the consumer to handle that aspect so we can present a nice prompt, in the correct context of our UI.

Thus - it seems that the consumer needs to do a few things:

If the consuming app needs to talk to multiple portal instances, it should send the correct set of creds into the arcgis-rest calls. Yes it's an additional param on all calls, but it keeps everything simple and consistent, which should not be underestimated as a value.

patrickarlt commented 7 years ago

@dbouwman it sounds like you are still most in favor of Design 3 which is pretty much like this:

import { request } from 'arcgis-rest-core';
import { UserAuthentication, AppAuthentication } from 'arcgis-rest-auth';

const session = new AppAuthentication(/* ... */);

// listen for auth errors so you can prompt the user for auth
session.on('authentication-error', (error) => {/* ... */});

// we would have to teach request how to use `AppAuthentication` to recover from
// auth failures and use auth. Hopefully via an interface so it doesn't bloat the core repo.
request(url, params, {
  authentication: session
))
  .then(/* ... */)
  .catch(/* ... */);

// we would have to pass the `authentication` object down to `request` internally
geocode(url, params, {
  authentication: session
})) 
  .then(/* ... */)
  .catch(/* ... */);;

I'm generally still fine with Design 3 but it does mean a few things:

I still think there are advantages to Design 5:

import { ApplicationSession, UserSession } from 'arcgis-rest-auth';
import 'arcgis-rest-gecode/authenticated/geocode';
import 'arcgis-rest-routing/authenticated/serviceArea';
import 'arcgis-rest-items/authenticated/createItem';

const session = new UserSession({
  clientId: '123',
  token: 'zyx',
  expires: new Date(),
  portal: 'https://www.arcgis.com/sharing/rest'
});

// listen for auth errors so you can prompt the user for auth
session.on('authentication-error', (error) => {/* ... */});

// these methods are authenticated with `session`
session.geocode(/* ... */);
session.serviceArea(/* ... */);
session.createItem(/* ... */);

Either way I still think there are some valid concerns for worrying about authentication and "auth manager" experience this early.

dbouwman commented 7 years ago

One thing we need (and have built) in Hub is a way to manage two (or more) distinct sets of creds (UserSession from above) - one set for the Enterprise Org, and one for the Community Org.

While this is an uncommon flow, it is foundational for the Community aspects of the Hub.

We also allow authenticated users to make requests w/o auth - as that is useful in some scenarios.

I guess my main issue with Design 5 is that it means we are hanging all the methods off "Instances" of the UserSession object, as opposed to having an instance of the UserSession object, and passing that into stateless methods, all of which use a centralized .request(...) which would handle federation / white-listing etc

This latter pattern keeps most of arcgis-rest-* stateless, keeps things decoupled and would allow for an app to manage multiple sets of creds, passing in the one they want to use at any given time.

This is pseudo code for how this would work in Ember... I assume other frameworks would be somewhat similar

// user-identity-service
// Holds a set of identities for use in AGO / Portal calls
import Ember from 'ember';
import { UserSession } from 'arcgis-rest-auth';
export default Ember.Service.extend({
  identities: {},
  addIdentity (key, creds) {
    let session = new UserSession(creds);
    this.get('identities').set(key, session);
  }
})

During the auth process, we inject things into the identity service...

Currently we have "services" for the specific entity types - items, groups, users etc etc...

// items-service
// Exposes methods for manipulating items
import Ember from 'ember';
import { items } from 'arcgis-rest-portal';
export default Ember.Service.extend({
  // these are wrapper methods over the `arcgis-rest-*` calls 
  // doing anything that is frameworks specific
  getById (itemId, creds) {
      return items.getById(itemId, creds)
       .then((response) => {
         // any ember specific things... ideally none...
       })
       .catch((ex) => {
        // any ember specific things... ideally none...
       })
  }
}

And in the application code... in this case a route in Ember

import Ember from 'ember';
export default Ember.Route.extend({
  itemsService: Ember.inject.service('items-service'),
  identityStore: Ember.inject.service('user-identity-store'),
  // model hook - fetch the model for the {id} param in the route
  model (params) {
    let userSession = this.get('identityStore.identities.current');
    return this.get('itemsService').getById(params.id, userSession)
      .then((model) => {
        // usually we do more here... but keepin it simple
        return model;
      })
      .catch((ex) => {
        // example where we'd redirect to sign-in route
        // ideally we'd use an Enum here vs magic string
        if (ex.type === 'invalidToken') {
          this.redirectTo('sign-in');
        }
      })
  }
});

Ideally we drop the items-service and just do this in the application code...

import Ember from 'ember';
import { items } from 'arcgis-rest-portal';
export default Ember.Route.extend({
  identityStore: Ember.inject.service('user-identity-store'),
  // model hook - fetch the model for the {id} param in the route
  model (params) {
    let userSession = this.get('identityStore.identities.current');
    return items.getById(params.id, userSession)
      .then((model) => {
        // usually we do more here... but keepin it simple
        return model;
      })
      .catch((ex) => {
        // example where we'd redirect to sign-in route
        // ideally we'd use an Enum here vs magic string
        if (ex.type === 'invalidToken') {
          this.redirectTo('sign-in');
        }
      })
  }
});
mjuniper commented 7 years ago

I don't have a lot to add but I like what Dave is suggesting. Passing session into stateless methods allows for maximum flexibility and isn't too onerous (we're doing something similar on open data/hub now). I'm coming at this mostly from my perspective of having to manage multiple sets of credentials and make requests using any of them. That is probably not a concern for most people though.

patrickarlt commented 7 years ago

@dbouwman I edited your comment to add highlighting.

@dbouwman @mjuniper If everyone else is feeling like a stateless request method that relies on getting passed a stateful authentication option is best I'll work on implementing it.

I'm going to open a new issue to work on the design of this based on Design 3.

patrickarlt commented 7 years ago

@dbouwman @mjuniper I've opened up #10 with a design based on Design 3 but with a few improvements.