feat: new UserSession tied to a non-federated ArcGIS Server instance #423

jgravois · 2018-12-20T23:14:52Z

the proposed tweak to UserSession would enable developers to build up a UI of their own for users to enter credentials and access secure content from unfederated instances of ArcGIS Server.

import { request } from "@esri/arcgis-rest-request";
import { UserSession } from "@esri/arcgis-rest-auth";

const authentication = new UserSession({
  username: "user1",
  password: "user1",
  server: "https://sampleserver6.arcgisonline.com/arcgis"
});

request('https://sampleserver6.arcgisonline.com/arcgis/rest/services/Wildfire_secure/MapServer', { authentication })
  .then(token)

AFFECTS PACKAGES:
@esri/arcgis-rest-auth

ISSUES CLOSED: #174

coveralls · 2018-12-20T23:25:31Z

Coverage decreased (-0.1%) to 99.862% when pulling 56d5a24 on f/non-federated into c1c9dd6 on master.

tomwayson

Thanks for taking this on @jgravois!

The API you propose makes sense to me, as does the idea of piggy-backing on trusted servers.

My main suggestions is to add more comments to 'splain this so we don't forget later what this is all about.

tomwayson · 2018-12-20T23:24:28Z

packages/arcgis-rest-auth/src/UserSession.ts

    this.trustedServers = {};
+    if (options.server) {
+      this.trustedServers[options.server] = null;


I think I know why you're doing this, but probably a good idea to leave a comment here.

tomwayson · 2018-12-20T23:26:29Z

packages/arcgis-rest-auth/src/UserSession.ts

@@ -760,7 +790,8 @@ export class UserSession implements IAuthenticationManager {
            params: {
              username: this.username,
              password: this.password,
-              expiration: this.tokenDuration
+              expiration: this.tokenDuration,
+              client: "referer"


Do we always want to send client=referer? Do we need to add that to above call to generateToken() too?

it was only necessary in the case that i tweaked, but there's no harm in being consistent.

tomwayson · 2018-12-20T23:30:37Z

packages/arcgis-rest-auth/src/UserSession.ts

+            );
+          } else {
+            return request(
+              `${response.owningSystemUrl}/sharing/rest/info`,


Maybe add a comment here to 'splain what this case is?

tomwayson · 2018-12-20T23:31:59Z

packages/arcgis-rest-auth/src/UserSession.ts

+           * if its a stand-alone instance of ArcGIS Server that doesn't advertise
+           * federation at all and the root url is recognized, use its built in token endpoint.
+           */
+          return new Promise((resolve, reject) => {


Maybe just return Promise.resolve({ authInfo: response.authInfo });

tomwayson · 2018-12-21T00:15:24Z

@jgravois

I believe we'd also need to make further changes if we wanted to support @dbouwman's idea

where the consuming app has the token (from a cookie, localstorage, some other auth flow managed by the app), but not the username/password?

Maybe set the token here?

#423 (diff)

jgravois · 2018-12-21T19:08:16Z

where the consuming app has the token (from a cookie, localstorage, some other auth flow managed by the app), but not the username/password?

i've added another small tweak to support the use case you mentioned. this is what needs to be provided to invoke it:

const authentication = new UserSession({
  server: "https://sampleserver6.arcgisonline.com/arcgis",
  token: "SOSlV3v1wOOSN75azMS..",
  tokenExpires: new Date(1545415669763)
});

to do:

get @patrickarlt's blessing
refactor the trusted server check to compare only root urls
add a few more tests.

(i'll wait on the first to do the second and third)

jgravois · 2019-01-07T20:00:14Z

@patrickarlt given that you chimed in on #422 but not this PR i'll assume you aren't fundamentally opposed to providing a mechanism to support making requests to non-federated, authenticated services.

i know you're busy with https://developers.arcgis.com tasks so if i'm wrong even a quick "hold your horses, i haven't had time to look at this yet and i'd like to", sometime this week would be welcome.

patrickarlt · 2019-01-16T22:57:13Z

@jgravois I realized that you asked about this again ago but it all looks good to me. I figured at some point someone would ask for this, I just didn't want to do it.

jgravois · 2019-01-17T22:45:18Z

thanks @patrickarlt!

i've rebased and pushed up some more changes to ensure test coverage and tie up the remaining loose ends. i'm happy with this if @tomwayson is.

… services AFFECTS PACKAGES: @esri/arcgis-rest-auth ISSUES CLOSED: #174

codecov · 2019-02-12T23:44:21Z

Codecov Report

Merging #423 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #423   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          94     94           
  Lines        1192   1202   +10     
  Branches      210    216    +6     
=====================================
+ Hits         1192   1202   +10

Impacted Files	Coverage Δ
packages/arcgis-rest-auth/src/UserSession.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbdac40...fc2f06b. Read the comment docs.

jgravois requested review from tomwayson and patrickarlt December 20, 2018 23:14

tomwayson reviewed Dec 20, 2018

View reviewed changes

tomwayson mentioned this pull request Dec 20, 2018

How should apps handle multiple credentials #422

Closed

jgravois changed the title ~~feat: add support for creating a UserSession tied to a non-federated ArcGIS Server instance~~ feat: new UserSession tied to a non-federated ArcGIS Server instance Dec 21, 2018

jgravois mentioned this pull request Jan 14, 2019

BUG: serverInfo.owningSystemUrl may be undefined Esri/ember-arcgis-server-services#48

Closed

patrickarlt approved these changes Jan 16, 2019

View reviewed changes

jgravois force-pushed the f/non-federated branch from 56d5a24 to 34acd33 Compare January 17, 2019 22:44

feat(:snowman:): add basic support for accessing secure non-federated…

fc2f06b

… services AFFECTS PACKAGES: @esri/arcgis-rest-auth ISSUES CLOSED: #174

jgravois force-pushed the f/non-federated branch from 34acd33 to fc2f06b Compare February 12, 2019 23:44

jgravois merged commit cd6e95e into master Feb 12, 2019

jgravois deleted the f/non-federated branch February 12, 2019 23:55

jgravois mentioned this pull request Mar 13, 2019

error when UserSession created with portal token attempts to exchange it for a server token #485

Closed

jgravois mentioned this pull request May 4, 2019

fix(try): try an anonymous request before throwing an auth error #560

Merged

jgravois mentioned this pull request Jul 24, 2019

Using with non-federated (standalone) servers not associated to portal #604

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new UserSession tied to a non-federated ArcGIS Server instance #423

feat: new UserSession tied to a non-federated ArcGIS Server instance #423

jgravois commented Dec 20, 2018 •

edited

Loading

coveralls commented Dec 20, 2018 •

edited

Loading

tomwayson left a comment

tomwayson Dec 20, 2018

tomwayson Dec 20, 2018

jgravois Dec 21, 2018

tomwayson Dec 20, 2018

tomwayson Dec 20, 2018

jgravois Dec 21, 2018

tomwayson commented Dec 21, 2018

jgravois commented Dec 21, 2018 •

edited

Loading

jgravois commented Jan 7, 2019

patrickarlt commented Jan 16, 2019

jgravois commented Jan 17, 2019

codecov bot commented Feb 12, 2019 •

edited

Loading

feat: new UserSession tied to a non-federated ArcGIS Server instance #423

feat: new UserSession tied to a non-federated ArcGIS Server instance #423

Conversation

jgravois commented Dec 20, 2018 • edited Loading

coveralls commented Dec 20, 2018 • edited Loading

tomwayson left a comment

Choose a reason for hiding this comment

tomwayson Dec 20, 2018

Choose a reason for hiding this comment

tomwayson Dec 20, 2018

Choose a reason for hiding this comment

jgravois Dec 21, 2018

Choose a reason for hiding this comment

tomwayson Dec 20, 2018

Choose a reason for hiding this comment

tomwayson Dec 20, 2018

Choose a reason for hiding this comment

jgravois Dec 21, 2018

Choose a reason for hiding this comment

tomwayson commented Dec 21, 2018

jgravois commented Dec 21, 2018 • edited Loading

jgravois commented Jan 7, 2019

patrickarlt commented Jan 16, 2019

jgravois commented Jan 17, 2019

codecov bot commented Feb 12, 2019 • edited Loading

Codecov Report

jgravois commented Dec 20, 2018 •

edited

Loading

coveralls commented Dec 20, 2018 •

edited

Loading

jgravois commented Dec 21, 2018 •

edited

Loading

codecov bot commented Feb 12, 2019 •

edited

Loading