Azure / ms-rest-nodeauth

node.js based authentication library for Azure with type definitions
MIT License
33 stars 33 forks source link

[Issue 89] Changing withAuthFileWithAuthResponse to re-authenticate for each tenant to return all the subscriptions #90

Closed sadasant closed 4 years ago

sadasant commented 4 years ago

Issue 89 shows that interactiveLoginWithAuthResponse is not returning any subscription for personal accounts with one or more tenants. This issue is not perceivable until the code explicitly calls to _getSubscriptions, which returns an empty array for those cases.

Upon investigation, a possible solution was to call interactiveLoginWithAuthResponse again with one of the received tenants from a previous execution of the same function as the domain parameter of this new call.

Another alternative seemed to set organizations as the domain, which should work in the v2.0 of the OAuth2 authentication workflow, as described in:

The {tenant} value in the path of the request can be used to control who can sign into the application. The allowed values are common, organizations, consumers, and tenant identifiers. For more detail, see protocol basics. (Source)

However, passing in organizations as the domain causes the network requests to answer with status code 400. This is likely because ms-rest-nodeauth is using v1.0 of the API (adal-node specifically uses version 1.0 by default: https://github.com/AzureAD/azure-activedirectory-library-for-nodejs/blob/dev/lib/oauth2client.js#L73).

Assuming a change of OAuth API version is out of the table, the repeated authentication approach could be narrowed down to a modification of withInteractiveWithAuthResponse, in which after retrieving the tenants, we could re-generate the credentials from adal-node to retrieve the subscriptions. For example: in lib/login.ts, in withInteractiveWithAuthResponse:

  let creds = await new Promise<DeviceTokenCredentials>((resolve, reject) => {
    // ...
      try {
        creds = new DeviceTokenCredentials(
          // ...
          interactiveOptions.domain, // <-- What we want to change
          // ...
        );
      } catch (err) {
    // ...
  });

  const tenants = await buildTenantList(creds);

  creds = await new Promise<DeviceTokenCredentials>((resolve, reject) => {
    // ...
      try {
        creds = new DeviceTokenCredentials(
          // ...
          tenants[0], // <-- What we changed
          // ...
        );
      } catch (err) {
    // ...
  });

That change in the domain parameter could also be done by altering the DeviceTokenCredentials's parent class, TokenCredentialsBase to allow setting this domain internally, by extracting this functionality from the constructor into a public function and changing _authContext to be a private property:

export abstract class TokenCredentialsBase implements TokenClientCredentials {
  private _authContext?: AuthenticationContext;

  public constructor(/* ... */) {
    // ...
    this.setDomain(domain);
  }

  get authContext(): AuthenticationContext {
    return this._authContext!;
  }

  public setDomain(domain: string): void {
    this.domain = domain;
    const authorityUrl = this.environment.activeDirectoryEndpointUrl + this.domain;
    this._authContext = new AuthenticationContext(authorityUrl, this.environment.validateAuthority, this.tokenCache);
  }

With that in hand, and to overcome the limit of just one tenant, we decided to loop over the tenants, fetch the organizations and concatenate them into a single array, with the following code at the end of withInteractiveWithAuthResponse:

  const tenants = await buildTenantList(creds);
  let subscriptions: LinkedSubscription[] = [];

  for (const tenant of tenants) {
    creds.setDomain(tenant);
    subscriptions = [
      ...subscriptions,
      ...await _getSubscriptions(creds, tenants, interactiveOptions.tokenAudience)
    ];
  }

  return { credentials: creds, subscriptions: subscriptions };

This code makes ADAL do a new request to refresh the token for each tenant we work with. The logs from ADAL can be seen here:

These logs were produced by using adal-node's Logging:

Logging.setLoggingOptions({
  log: function(level, message, error) {
    console.log({ level, message, error });
  },
  level: 3,
  loggingWithPII: false  // Whether to log personal identification information.
});

If after feedback this PR happens to get approved, the following is true:

Fixes #89

sadasant commented 4 years ago

Closing on favor of: https://github.com/Azure/ms-rest-nodeauth/pull/91