Add ssl flag #332

skitterm · 2018-09-20T17:32:45Z

Addresses #295.

Features

Adds ssl field to UserSession class and FetchToken response
Reads/writes to that field in to/fromCredential, deserialize and toJSON
Reads oauth-callback hash for ssl property in completeOAuth2
Reads oauth2/token response for ssl property in exchangeAuthorizationCode

Future work needed (not included in this PR)

Ideally arcgis-rest-request methods (and other requests when authenticated) should look for ssl property and force the URL to always be HTTPS in that case
Does ApplicationSession need this ssl property as well?
Does UserSession.getToken method need to read ssl value?

…completeOAuth2 and used in to/fromCredential.

coveralls · 2018-09-20T17:41:10Z

Coverage remained the same at 100.0% when pulling f80f4f8 on skitterm:add-ssl-flag into 30fadcb on Esri:master.

tomwayson · 2018-09-20T17:42:50Z

packages/arcgis-rest-auth/src/fetch-token.ts

@@ -12,13 +12,15 @@ interface IFetchTokenRawResponse {
  access_token: string;
  expires_in: number;
  username: string;
+  ssl: boolean;


I wonder if this should be ssl?: boolean?

tomwayson · 2018-09-20T17:45:50Z

Nice work @skitterm, thanks for the contribution!

jgravois · 2018-09-20T17:41:53Z

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

@@ -165,6 +165,11 @@ export interface IUserSessionOptions {
   */
  portal?: string;

+  /**
+   * Whether requests should be made exlusively over HTTPS.


exclusively 😈

jgravois · 2018-09-20T17:41:57Z

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

@@ -456,10 +469,14 @@ export class UserSession implements IAuthenticationManager {
      Date.now() + parseInt(match[2], 10) * 1000 - 60 * 1000
    );
    const username = decodeURIComponent(match[3]);
+    const ssl =
+      win.location.href.indexOf("&ssl=true") !== -1 ||


this may just be me, but .indexOf("&ssl=true") > -1 feels more readable than a double-negative.

packages/arcgis-rest-auth/src/fetch-token.ts

jgravois · 2018-09-20T17:46:11Z

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

@@ -19,6 +19,7 @@ describe("UserSession", () => {
    const session = new UserSession({
      clientId: "clientId",
      redirectUri: "https://example-app.com/redirect-uri",
+      ssl: false,


unless i'm missing something, there's nothing a user can do in the UserSession constructor to dicate what the server is going to respond with so there's no point in including ssl as an option.

is that correct? if so, we just need to update our examples and make that clear in the API reference.

My thinking is that ssl is an attribute of a user session, and something that would be needed by arcgis-rest-request's request methods (since they take in a UserSession/ApplicationSession as a param) to determine whether or not to force HTTPS for a request.

I think the main risk w/ the approach in this PR is that a user can set ssl before beginOAuth() which may overwrite whatever they set.

One alternative would be that we don't let consuming apps set this and we have some kind of read-only property/fn like sslOrg that always returns what was in the redirect url parameter.

Given that we haven't thought through how request() is going to use this parameter yet, it may be safer to do the latter. That would be a much more useful signal to request(). For example, it could so it could only force SSL for requests that are made to that org.

I'm open to suggestions though.

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

…urned or present.

jgravois

i pushed up one more commit to make it a little clearer in the API reference that the property set internally and remove the last instance where it was set manually in a UserSession constructor.

jgravois · 2018-09-20T20:16:01Z

awesome contribution. thank you @skitterm!

skitterm · 2018-09-20T20:17:09Z

Should we then remove the this.ssl = options.ssl from the constructor itself? Though if we do, I feel like we'd still want to be able to set ssl property via a setter.

jgravois · 2018-09-20T20:26:53Z

Should we then remove the this.ssl = options.ssl from the constructor itself?

i didn't spend long with it, but the way things are set up now its necessary in order to be able to set the property internally later.

skitterm added 3 commits September 19, 2018 17:07

Esri#295: SSL property added to UserSession class. SSL calculated in …

a350f76

…completeOAuth2 and used in to/fromCredential.

Merge remote-tracking branch 'upstream/master' into add-ssl-flag

65c0b82

Esri#295: fetchToken returns ssl property with response.

1dba15d

skitterm mentioned this pull request Sep 20, 2018

Unable to determine if a user session should be SSL-only #295

Closed

jgravois self-requested a review September 20, 2018 17:37

tomwayson approved these changes Sep 20, 2018

View reviewed changes

jgravois reviewed Sep 20, 2018

View reviewed changes

skitterm and others added 3 commits September 20, 2018 11:01

Esri#295: Syntax fixes.

eb96e2b

Esri#295: Test cases added for when ssl property/parameter is not ret…

584b0e5

…urned or present.

make it clearer in doc that ssl shouldnt be set in constructor

f80f4f8

jgravois approved these changes Sep 20, 2018

View reviewed changes

jgravois merged commit e0d57d9 into Esri:master Sep 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ssl flag #332

Add ssl flag #332

skitterm commented Sep 20, 2018 •

edited

Loading

coveralls commented Sep 20, 2018 •

edited

Loading

tomwayson Sep 20, 2018

tomwayson commented Sep 20, 2018

jgravois Sep 20, 2018

jgravois Sep 20, 2018

jgravois Sep 20, 2018

skitterm Sep 20, 2018

tomwayson Sep 20, 2018

jgravois left a comment

jgravois commented Sep 20, 2018

skitterm commented Sep 20, 2018

jgravois commented Sep 20, 2018 •

edited

Loading

Add ssl flag #332

Add ssl flag #332

Conversation

skitterm commented Sep 20, 2018 • edited Loading

Features

Future work needed (not included in this PR)

coveralls commented Sep 20, 2018 • edited Loading

tomwayson Sep 20, 2018

Choose a reason for hiding this comment

tomwayson commented Sep 20, 2018

jgravois Sep 20, 2018

Choose a reason for hiding this comment

jgravois Sep 20, 2018

Choose a reason for hiding this comment

jgravois Sep 20, 2018

Choose a reason for hiding this comment

skitterm Sep 20, 2018

Choose a reason for hiding this comment

tomwayson Sep 20, 2018

Choose a reason for hiding this comment

jgravois left a comment

Choose a reason for hiding this comment

jgravois commented Sep 20, 2018

skitterm commented Sep 20, 2018

jgravois commented Sep 20, 2018 • edited Loading

skitterm commented Sep 20, 2018 •

edited

Loading

coveralls commented Sep 20, 2018 •

edited

Loading

jgravois commented Sep 20, 2018 •

edited

Loading