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

Handling authentication #5

Closed
patrickarlt opened this issue Aug 3, 2017 · 11 comments
Closed

Handling authentication #5

patrickarlt opened this issue Aug 3, 2017 · 11 comments

Comments

@patrickarlt
Copy link
Contributor

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:

  • Avoid use of a singleton like IdentityManager
  • Make it easy for a developer to configure authentication once and forget about it no matter what library they are using.
  • We want to properly federate requests (a la JS API) as much as possible with as minimal overhead as possible.

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
Copy link
Member

ajturner commented Aug 3, 2017

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
Copy link
Member

dbouwman commented Aug 3, 2017

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Member

dbouwman commented Aug 3, 2017

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:

  • handle initial and subsequent authentication "interactions"
    • use ENV vars for un/pwd for node apps, make calls to portal.getToken(...)
    • oAuth via iframe / pop-out (the preferred path for webapps)
    • un/pwd form in the app (makes calls to portal.getToken(...) in the app)
  • cred/token persistence / re-hydration (cookie, localStorage, file, whatever)
  • get notification of a request that failed b/c of invalid token, so it can take appropriate action

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
Copy link
Contributor Author

patrickarlt commented Aug 4, 2017

@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:

  • The session object becomes a lot more visible and developers will have to manage passing it around their app and make sure it gets passed into all their calls.
  • The request method has to know how to use the session objects which means that we are no longer talking about core and auth being 2 separate modules.

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(/* ... */);
  • This approach keeps the core and auth modules separate since each library and provide their own handlers.
  • We can start building out packages right now and worry about this later since it lays over the top of existing packages.
  • Developers still have to keep track of their session object but everything "just works".

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

  • We will get no internal traction outside our own teams without it since we don't federate requests in a secure way (sessions could send portal tokens to unknown servers)
  • No buy-in from JS API/Online team (see above)
  • Would be helpful for building browser based experiences without the JS API like the developers site and getting helpers for federation and auth.

@dbouwman
Copy link
Member

dbouwman commented Aug 4, 2017

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
Copy link
Member

mjuniper commented Aug 4, 2017

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@tomwayson tomwayson mentioned this issue Aug 5, 2017
patrickarlt pushed a commit that referenced this issue Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants