support for authentication using sp from auth file #2231

amarzavery · 2017-07-22T04:20:21Z

@balajikris @yugangw-msft - Please review

amarzavery · 2017-07-22T04:21:06Z

amarzavery · 2017-07-24T03:49:05Z

runtime/ms-rest-azure/package.json

@@ -22,11 +34,12 @@
    "@types/node": "^7.0.10"


min dependency for ms-rest should go to ^2.2.2 as the _withAuthFile() function in login.js depends on a function homeDir() added in msRest in this PR. After the CI for first commit is green will update this dep in second commit. until the ms-rest version is not published this PR will fail in the second commit due to this reason.

balajikris · 2017-07-24T17:23:22Z

runtime/ms-rest-azure/test/rpRegistrationFilterTests.js

      rpName.should.equal('Microsoft.Devices');
      done();
    });
    it('should correctly parse the error message from service for an existing subscription where the RP was manually unregistered', (done) => {
-      let msg = "{\"error\":{\"code\":\"MissingSubscriptionRegistration\",\"message\":\"The subscription registration is in 'Unregistered' state. The subscription must be registered to use namespace 'Microsoft.Devices'. See https://aka.ms/rps-not-found for how to register subscriptions.\"}}";
+      var msg = "{\"error\":{\"code\":\"MissingSubscriptionRegistration\",\"message\":\"The subscription registration is in 'Unregistered' state. The subscription must be registered to use namespace 'Microsoft.Devices'. See https://aka.ms/rps-not-found for how to register subscriptions.\"}}";


is there a reason to go from let to var ?

jshint was complaining. So changed it.

balajikris · 2017-07-24T17:24:12Z

runtime/ms-rest-azure/test/credTests.js

+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License. See License.txt in the project root for license information.
+
+var assert = require('assert');


should be ok for now

balajikris · 2017-07-24T17:25:02Z

runtime/ms-rest-azure/test/credTests.js

+      var dummyCreds = {
+      tokenAudience: undefined,
+      environment: {},
+      authorizattionScheme: 'Bearer',


typo. authorizationScheme

thanks for pointing

balajikris · 2017-07-24T17:35:39Z

runtime/ms-rest-azure/test/credTests.js

+      });
+    });
+
+    it('should add custom env to AzureEnvironment and authenticate against it', (done) => {


2 more scenarios come to mind -

authfile with insufficient data (if az-cli encounters a bug while generating auth file)

foreign cloud environments? not sure if this is testable with stubs, so i'll leave it to you to decide.

for 2. what i intend to test here is that: if someone wants to test against dogfood or other env that is not predefined then our code can correctly add that environment and try to authenticate using the sp info

for 1. I have the _validateAuthFileContent that throws an error if something is incorrect.

I can add a test for that scenario.

balajikris · 2017-07-24T17:47:20Z

runtime/ms-rest-azure/lib/login.js

+    content = fs.readFileSync(filePath, {encoding: 'utf8'});
+    credsObj = JSON.parse(content);
+    _validateAuthFileContent(credsObj, filePath);
+    if (!process.env[subscriptionEnvVariableName]) {


move this to outside of the try-catch. The code in its current shape reads like - read and parse a json.. if an environment variable isn't set, then set it and the flow breaks there. which makes me wonder, why read and parse at all if the env var is already set. The answer is few lines below here, where we continue to use credsObj. So, move the setting of subscription to outside the try-catch and it doesn't need a try-catch anyway.

The environment variable that we are setting over here is the env. variable for subscriptionId. It is not the env. var for auth location. Node sdk has a recommended set of environment variables for things like subscriptionId, clientId, username, password, domain, etc.
So in this case, if the user provides his custom environment variable name that is being used in his app then we use that env. variable to set the value of subscriptionId. IF not then we set the subscriptionId to AZURE_SUBSCRIPTION_ID env. variable.
This subscription Id will be the only subscriptionId that the customer can use while instantiating the client since the SP is linked to that subscription. Using any other subscriptionId will be setting him up for failure.

I know this is a bit confusing. But whatever I said above, does that make sense or clear the initial question that you asked?

I think the check is incorrect. It should actually be like this:

//just set the environment variable to correct subscriptionId from the auth file if it is not present. if (process.env[subscriptionEnvVariableName] && process.env[subscriptionEnvVariableName] !== credsObj.subscriptionId) { process.env[subscriptionEnvVariableName] = credsObj.subscriptionId; }

Hope I understand the context correctly, the subscription from the auth file should be precedent the environment variable. You can overwrite the environment variable here, but that should be just an implementation detail being transparent to users. Same with other fields.
If I got it right, then we don't need an if condition, just overwrite it, optionally emit out a warning if you still like users to know.

balajikris · 2017-07-24T17:50:24Z

runtime/ms-rest-azure/lib/login.js

+      break;
+    }
+  }
+  if (envFound.state) {


dont think you need state field. can simply check if name is not null or empty?

amarzavery · 2017-07-24T22:18:20Z

runtime/ms-rest-azure/lib/login.js

+      let msg = `Either provide an absolute file path to the auth file or set/export the environment variable - ${azureConstants.AZURE_AUTH_LOCATION}.`;
+      return callback(new Error(msg));
+    }
+    filePath = authLocation;


if the filePath is not provided then we look for the authLocation (env. var). If that is not provided as well then we throw. If it is provided then we set the value of authlocation env. var to the filePath parameter.

yugangw-msft

LGTM overall

yugangw-msft · 2017-07-25T01:20:54Z

runtime/ms-rest-azure/lib/login.js

+  if (!credsObj.sqlManagementEndpointUrl) {
+    throw new Error(`"sqlManagementEndpointUrl" is missing from the auth file: ${filePath}.`);
+  }
+  if (!credsObj.galleryEndpointUrl) {


I suggest we don't throw on this as this gallery endpoint is not being used by any clients, which we can consider to pulling it out in future

yugangw-msft · 2017-07-25T01:21:27Z

runtime/ms-rest-azure/lib/login.js

+  if (!credsObj.galleryEndpointUrl) {
+    throw new Error(`"galleryEndpointUrl" is missing from the auth file: ${filePath}.`);
+  }
+  if (!credsObj.managementEndpointUrl) {


I suggest we don't error on this as well, rather SDK should default to resourceManagerEndpointUrl for management client

yugangw-msft · 2017-07-28T19:39:36Z

runtime/ms-rest-azure/lib/login.js

+    throw new Error('callback cannot be null or undefined.');
+  }
+
+  let filePath = options.filePath;


in case it helps, will the following code simplify it?

let filePath = options.filePath || process.env[azureConstants.AZURE_AUTH_LOCATION]` if !filepath: return callback(new Error(...))

yugangw-msft · 2017-07-28T19:47:04Z

runtime/ms-rest-azure/lib/login.js

+    content = fs.readFileSync(filePath, {encoding: 'utf8'});
+    credsObj = JSON.parse(content);
+    _validateAuthFileContent(credsObj, filePath);
+    if (!process.env[subscriptionEnvVariableName]) {


Hope I understand the context correctly, the subscription from the auth file should be precedent the environment variable. You can overwrite the environment variable here, but that should be just an implementation detail being transparent to users. Same with other fields.
If I got it right, then we don't need an if condition, just overwrite it, optionally emit out a warning if you still like users to know.

yugangw-msft · 2017-07-28T19:53:42Z

runtime/ms-rest-azure/lib/login.js

+      envParams.activeDirectoryResourceId = credsObj.managementEndpointUrl;
+    }
+    if (!envParams.portalUrl) {
+      envParams.portalUrl = 'https://portal.azure.com';


This is not an environment aware url. If SDK never uses it, which I believe is the case, then maybe we just remove it from the environment

yugangw-msft · 2017-07-28T20:00:03Z

runtime/ms-rest-azure/test/data/auth-customEnv.json

@@ -0,0 +1,12 @@
+{
+  "clientId": "0b868e09-1547-4721-9a57-705a15328409",
+  "clientSecret": "005937fd-3a0f-4cf2-84da-da7388241b9f",


Assume this is a faked secret?

…H with 201 initial response.

amarzavery · 2017-08-25T20:48:11Z

#2245 and #2247

msftclas added the cla-not-required label Jul 22, 2017

amarzavery force-pushed the jsoncreds branch 3 times, most recently from dfa4c13 to 6bbc5da Compare July 24, 2017 03:43

support for authentication using sp from auth file

7f2e1be

amarzavery force-pushed the jsoncreds branch from 6bbc5da to 7f2e1be Compare July 24, 2017 03:46

amarzavery commented Jul 24, 2017

View reviewed changes

min dependency on ms-rest to 2.2.2

fdbafab

balajikris reviewed Jul 24, 2017

View reviewed changes

amarzavery commented Jul 24, 2017

View reviewed changes

yugangw-msft reviewed Jul 28, 2017

View reviewed changes

amarzavery added 3 commits July 28, 2017 17:39

code review feedback

0ec040c

updated documentation

1195841

fixes Azure#2245 and Azure#2247 and adds support for polling LRO PATC…

e2c0c5d

…H with 201 initial response.

amarzavery merged commit 3081f68 into Azure:master Aug 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for authentication using sp from auth file #2231

support for authentication using sp from auth file #2231

amarzavery commented Jul 22, 2017 •

edited

Loading

amarzavery commented Jul 22, 2017

amarzavery Jul 24, 2017

balajikris Jul 24, 2017

amarzavery Jul 24, 2017

balajikris Jul 24, 2017

amarzavery Jul 24, 2017

balajikris Jul 24, 2017

amarzavery Jul 24, 2017

balajikris Jul 24, 2017

amarzavery Jul 24, 2017

amarzavery Jul 24, 2017

balajikris Jul 24, 2017

amarzavery Jul 24, 2017

amarzavery Jul 24, 2017

yugangw-msft Jul 28, 2017 •

edited

Loading

balajikris Jul 24, 2017

amarzavery Jul 24, 2017

yugangw-msft left a comment

yugangw-msft Jul 25, 2017

amarzavery Jul 28, 2017

yugangw-msft Jul 25, 2017

amarzavery Jul 28, 2017

yugangw-msft Jul 28, 2017

yugangw-msft Jul 28, 2017 •

edited

Loading

yugangw-msft Jul 28, 2017

yugangw-msft Jul 28, 2017

amarzavery Jul 28, 2017

amarzavery commented Aug 25, 2017

support for authentication using sp from auth file #2231

support for authentication using sp from auth file #2231

Conversation

amarzavery commented Jul 22, 2017 • edited Loading

amarzavery commented Jul 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarzavery commented Aug 25, 2017

amarzavery commented Jul 22, 2017 •

edited

Loading

yugangw-msft Jul 28, 2017 •

edited

Loading

yugangw-msft Jul 28, 2017 •

edited

Loading