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

port:[#3906] Port Managed Identity (MSI) + Single Tenant from DotNet #3923

Merged
merged 33 commits into from
Sep 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
2253145
Port MSI and SingleTenant support
ceciliaavila Sep 2, 2021
189f9d2
Add unit tests for botframework-connector
ceciliaavila Sep 2, 2021
d88c616
Restore yarn.lock
ceciliaavila Sep 3, 2021
7c45a2b
Test nohoist for azure/identity
ceciliaavila Sep 3, 2021
542a7ab
Fix dependencies issues
sw-joelmut Sep 7, 2021
f3dd2c3
Connect ManagedIdentityAuthenticator.getToken
sw-joelmut Sep 7, 2021
86f0f2d
Add remaining tests and fix the already added
sw-joelmut Sep 7, 2021
e0a233a
Update yarn.lock
sw-joelmut Sep 7, 2021
171733b
Fixing depcheck errors in PR
MartinLuccanera Sep 9, 2021
875b0d3
Fixing depcheck, adjusting versions and ignores
MartinLuccanera Sep 9, 2021
288765f
Fix parity port with DotNet causing SingleTenant to fail at authentic…
sw-joelmut Sep 9, 2021
0c4a9a6
Merge branch 'southworks/add/port-msi-single-tenant-support' of https…
sw-joelmut Sep 9, 2021
135522d
Apppliying feedback
MartinLuccanera Sep 10, 2021
14386eb
Merge branch 'southworks/add/port-msi-single-tenant-support' of githu…
MartinLuccanera Sep 10, 2021
afebb4d
Applying fixes for tests, lint and zod related feedback
MartinLuccanera Sep 13, 2021
bc25476
Fixing asserts import and extra parameter
MartinLuccanera Sep 14, 2021
799842f
Merge branch 'southworks/add/port-msi-single-tenant-support' of githu…
MartinLuccanera Sep 14, 2021
a6132b6
Fix tests failing due to wrong link and fixing case-wise comparison f…
MartinLuccanera Sep 15, 2021
5c95709
Fixing lint
MartinLuccanera Sep 15, 2021
4d54c25
Merge branch 'main' into southworks/add/port-msi-single-tenant-support
ceciliaavila Sep 15, 2021
12fe13f
Add ignore-casing to MicrosoftAppType based on DotNet code and add un…
sw-joelmut Sep 16, 2021
c78713a
Improve issuer array assignation
sw-joelmut Sep 16, 2021
7bbf539
Merge remote-tracking branch 'upstream/southworks/add/port-msi-single…
sw-joelmut Sep 16, 2021
0e207e5
fix: export ms-rest-js type and remove added dep
Sep 17, 2021
9f75226
Improve issuers, assertions, unit tests and many other small changes
sw-joelmut Sep 20, 2021
27fbdf0
fix: skillValidation validTokenIssuers
sw-joelmut Sep 20, 2021
1fa5179
Merge branch 'main' into southworks/add/port-msi-single-tenant-support
sw-joelmut Sep 21, 2021
a8a8277
Update @azure/identity to 2.0.0-beta.6
sw-joelmut Sep 21, 2021
9c9f8d9
Change logical operator for nullable operator
sw-joelmut Sep 21, 2021
2f88d99
Merge branch 'main' into southworks/add/port-msi-single-tenant-support
sw-joelmut Sep 24, 2021
3245f18
Improve SingleTenant and MSI implementation from feedback
sw-joelmut Sep 24, 2021
47d891f
fix: IJwtTokenProviderFactory
Sep 27, 2021
5f66d89
fix: reintroduced typo
Sep 27, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions libraries/botbuilder-core/etc/botbuilder-core.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { O365ConnectorCard } from 'botframework-schema';
import { PasswordServiceClientCredentialFactory } from 'botframework-connector';
import { ReceiptCard } from 'botframework-schema';
import { ResourceResponse } from 'botframework-schema';
import { ServiceClientCredentials } from 'botframework-connector';
import { ServiceClientCredentialsFactory } from 'botframework-connector';
import { ServiceCollection } from 'botbuilder-dialogs-adaptive-runtime-core';
import { SignInUrlResponse } from 'botframework-schema';
Expand Down Expand Up @@ -353,6 +354,12 @@ export type ConfigurationBotFrameworkAuthenticationOptions = z.infer<typeof Type
// @public
export class ConfigurationServiceClientCredentialFactory extends PasswordServiceClientCredentialFactory {
constructor(factoryOptions?: ConfigurationServiceClientCredentialFactoryOptions);
// (undocumented)
createCredentials(microsoftAppId: string, audience: string, loginEndpoint: string, validateAuthority: boolean): Promise<ServiceClientCredentials>;
// (undocumented)
isAuthenticationDisabled(): Promise<boolean>;
// (undocumented)
isValidAppId(microsoftAppId: string): Promise<boolean>;
}

// Warning: (ae-forgotten-export) The symbol "TypedConfig" needs to be exported by the entry point index.d.ts
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,19 @@
// Licensed under the MIT License.

import * as z from 'zod';
import { ok } from 'assert';
import { Configuration } from 'botbuilder-dialogs-adaptive-runtime-core';
import { PasswordServiceClientCredentialFactory } from 'botframework-connector';
import {
JwtTokenProviderFactory,
ManagedIdentityServiceClientCredentialsFactory,
PasswordServiceClientCredentialFactory,
ServiceClientCredentials,
ServiceClientCredentialsFactory,
} from 'botframework-connector';

const MultiTenant = 'MultiTenant';
const SingleTenant = 'SingleTenant';
const UserAssignedMsi = 'UserAssignedMsi';

const TypedConfig = z
.object({
Expand All @@ -16,6 +27,16 @@ const TypedConfig = z
* The password assigned to your bot in the [Bot Framework Portal](https://dev.botframework.com/).
*/
MicrosoftAppPassword: z.string(),

/**
* The type of app id assigned to your bot in the [Bot Framework Portal](https://dev.botframework.com/).
*/
MicrosoftAppType: z.string(),
MartinLuccanera marked this conversation as resolved.
Show resolved Hide resolved

/**
* The tenant id assigned to your bot in the [Bot Framework Portal](https://dev.botframework.com/).
*/
MicrosoftAppTenantId: z.string(),
})
.partial();

Expand All @@ -28,14 +49,77 @@ export type ConfigurationServiceClientCredentialFactoryOptions = z.infer<typeof
* ServiceClientCredentialsFactory that uses a [ConfigurationServiceClientCredentialFactoryOptions](xref:botbuilder-core.ConfigurationServiceClientCredentialFactoryOptions) or a [Configuration](xref:botbuilder-dialogs-adaptive-runtime-core.Configuration) instance to build ServiceClientCredentials with an AppId and App Password.
*/
export class ConfigurationServiceClientCredentialFactory extends PasswordServiceClientCredentialFactory {
private readonly inner: ServiceClientCredentialsFactory;
stevengum marked this conversation as resolved.
Show resolved Hide resolved

/**
* Initializes a new instance of the [ConfigurationServiceClientCredentialFactory](xref:botbuilder-core.ConfigurationServiceClientCredentialFactory) class.
*
* @param factoryOptions A [ConfigurationServiceClientCredentialFactoryOptions](xref:botbuilder-core.ConfigurationServiceClientCredentialFactoryOptions) object.
*/
constructor(factoryOptions: ConfigurationServiceClientCredentialFactoryOptions = {}) {
const { MicrosoftAppId = null, MicrosoftAppPassword = null } = TypedConfig.nonstrict().parse(factoryOptions);
super(MicrosoftAppId, MicrosoftAppPassword);
const {
MicrosoftAppId = null,
MicrosoftAppPassword = null,
MicrosoftAppType = null,
MicrosoftAppTenantId = null,
} = TypedConfig.nonstrict().parse(factoryOptions);
super(MicrosoftAppId, MicrosoftAppPassword, MicrosoftAppTenantId);

const appType = MicrosoftAppType?.trim() ?? MultiTenant;

switch (appType.toLocaleLowerCase()) {
case UserAssignedMsi.toLocaleLowerCase():
ok(MicrosoftAppId?.trim(), 'MicrosoftAppId is required for MSI in configuration.');
ok(MicrosoftAppTenantId?.trim(), 'MicrosoftAppTenantId is required for MSI in configuration.');
ok(!MicrosoftAppPassword?.trim(), 'MicrosoftAppPassword must not be set for MSI in configuration.');

this.inner = new ManagedIdentityServiceClientCredentialsFactory(
MicrosoftAppId,
new JwtTokenProviderFactory()
);
break;
case SingleTenant.toLocaleLowerCase():
ok(MicrosoftAppId?.trim(), 'MicrosoftAppId is required for SingleTenant in configuration.');
ok(MicrosoftAppPassword?.trim(), 'MicrosoftAppPassword is required for SingleTenant in configuration.');
ok(MicrosoftAppTenantId?.trim(), 'MicrosoftAppTenantId is required for SingleTenant in configuration.');

this.inner = new PasswordServiceClientCredentialFactory(
MicrosoftAppId,
MicrosoftAppPassword,
MicrosoftAppTenantId
);
break;
default:
//MultiTenant
this.inner = new PasswordServiceClientCredentialFactory(MicrosoftAppId, MicrosoftAppPassword, '');
break;
}
}

/**
* @inheritdoc
*/
isValidAppId(microsoftAppId: string): Promise<boolean> {
return this.inner.isValidAppId(microsoftAppId);
}

/**
* @inheritdoc
*/
isAuthenticationDisabled(): Promise<boolean> {
return this.inner.isAuthenticationDisabled();
}

/**
* @inheritdoc
*/
createCredentials(
microsoftAppId: string,
audience: string,
loginEndpoint: string,
validateAuthority: boolean
): Promise<ServiceClientCredentials> {
return this.inner.createCredentials(microsoftAppId, audience, loginEndpoint, validateAuthority);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ describe('ConfigurationServiceClientCredentialFactory', function () {
static DefaultConfig = {
MicrosoftAppId: 'appId',
MicrosoftAppPassword: 'appPassword',
MicrosoftAppType: undefined,
MicrosoftAppTenantId: undefined,
};

constructor(config = {}) {
Expand All @@ -25,20 +27,34 @@ describe('ConfigurationServiceClientCredentialFactory', function () {
set(_path, _val) {}
}

it('constructor should work', function () {
const SingleTenantConfig = {
MicrosoftAppId: 'singleAppId',
MicrosoftAppPassword: 'singleAppPassword',
MicrosoftAppType: 'SingleTenant',
MicrosoftAppTenantId: 'singleAppTenantId',
};

const MSIConfig = {
MicrosoftAppId: 'msiAppId',
MicrosoftAppType: 'UserAssignedMsi',
MicrosoftAppTenantId: 'msiAppTenantId',
MicrosoftAppPassword: undefined,
};

it('constructor() should work', function () {
const bfAuth = new ConfigurationServiceClientCredentialFactory(TestConfiguration.DefaultConfig);
assert.strictEqual(bfAuth.appId, TestConfiguration.DefaultConfig.MicrosoftAppId);
assert.strictEqual(bfAuth.password, TestConfiguration.DefaultConfig.MicrosoftAppPassword);
});

it('createServiceClientCredentialFactoryFromConfiguration should work', function () {
it('createServiceClientCredentialFactoryFromConfiguration() should work', function () {
const config = new TestConfiguration();
const bfAuth = createServiceClientCredentialFactoryFromConfiguration(config);
assert.strictEqual(bfAuth.appId, TestConfiguration.DefaultConfig.MicrosoftAppId);
assert.strictEqual(bfAuth.password, TestConfiguration.DefaultConfig.MicrosoftAppPassword);
});

it('undefined or null configuration values should result in null values', function () {
it('createServiceClientCredentialFactoryFromConfiguration() with undefined or null configuration values should result in null values', function () {
const config = new TestConfiguration({
MicrosoftAppId: undefined,
MicrosoftAppPassword: undefined,
Expand All @@ -48,7 +64,7 @@ describe('ConfigurationServiceClientCredentialFactory', function () {
assert.strictEqual(bfAuth.password, null);
});

it('non-string values should fail', function () {
it('createServiceClientCredentialFactoryFromConfiguration() with non-string values should fail', function () {
const config = new TestConfiguration({
MicrosoftAppId: 1,
MicrosoftAppPassword: true,
Expand All @@ -63,4 +79,96 @@ describe('ConfigurationServiceClientCredentialFactory', function () {
}
);
});

it('createServiceClientCredentialFactoryFromConfiguration() with non-defined MicrosoftAppType configuration should work', function () {
const config = new TestConfiguration();

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { MicrosoftAppType, ...configuration } = config.configuration;
config.configuration = configuration;

createServiceClientCredentialFactoryFromConfiguration(config);
});

it('createServiceClientCredentialFactoryFromConfiguration() with empty-string MicrosoftAppType configuration should work', function () {
const config = new TestConfiguration({ MicrosoftAppType: '' });

createServiceClientCredentialFactoryFromConfiguration(config);
});

it('createServiceClientCredentialFactoryFromConfiguration() with casing-string MicrosoftAppType configuration should work', function () {
const config = new TestConfiguration({ ...SingleTenantConfig, MicrosoftAppType: 'singletenant' });

createServiceClientCredentialFactoryFromConfiguration(config);
});

it('createServiceClientCredentialFactoryFromConfiguration() singleTenant with config should work', function () {
const config = new TestConfiguration(SingleTenantConfig);
const bfAuth = createServiceClientCredentialFactoryFromConfiguration(config);
assert.strictEqual(bfAuth.appId, config.configuration.MicrosoftAppId);
assert.strictEqual(bfAuth.password, config.configuration.MicrosoftAppPassword);
assert.strictEqual(bfAuth.tenantId, config.configuration.MicrosoftAppTenantId);
});

it('createServiceClientCredentialFactoryFromConfiguration() singleTenant without tenantId should throw', function () {
const config = new TestConfiguration({ ...SingleTenantConfig, MicrosoftAppTenantId: undefined });

assert.throws(() => createServiceClientCredentialFactoryFromConfiguration(config), {
name: 'AssertionError',
message: 'MicrosoftAppTenantId is required for SingleTenant in configuration.',
});
});

it('createServiceClientCredentialFactoryFromConfiguration() singleTenant without appId should throw', function () {
const config = new TestConfiguration({ ...SingleTenantConfig, MicrosoftAppId: undefined });

assert.throws(() => createServiceClientCredentialFactoryFromConfiguration(config), {
name: 'AssertionError',
message: 'MicrosoftAppId is required for SingleTenant in configuration.',
});
});

it('createServiceClientCredentialFactoryFromConfiguration() singleTenant without appPassword should throw', function () {
const config = new TestConfiguration({ ...SingleTenantConfig, MicrosoftAppPassword: undefined });

assert.throws(() => createServiceClientCredentialFactoryFromConfiguration(config), {
name: 'AssertionError',
message: 'MicrosoftAppPassword is required for SingleTenant in configuration.',
});
});

it('createServiceClientCredentialFactoryFromConfiguration() manageIdentityApp with config should work', function () {
const config = new TestConfiguration(MSIConfig);
const bfAuth = createServiceClientCredentialFactoryFromConfiguration(config);

assert.strictEqual(bfAuth.appId, config.configuration.MicrosoftAppId);
assert.strictEqual(bfAuth.tenantId, config.configuration.MicrosoftAppTenantId);
});

it('createServiceClientCredentialFactoryFromConfiguration() manageIdentityApp without tenantId should throw', function () {
const config = new TestConfiguration({ ...MSIConfig, MicrosoftAppTenantId: undefined });

assert.throws(() => createServiceClientCredentialFactoryFromConfiguration(config), {
name: 'AssertionError',
message: 'MicrosoftAppTenantId is required for MSI in configuration.',
});
});

it('createServiceClientCredentialFactoryFromConfiguration() manageIdentityApp without appId should throw', function () {
const config = new TestConfiguration({ ...MSIConfig, MicrosoftAppId: undefined });

assert.throws(() => createServiceClientCredentialFactoryFromConfiguration(config), {
name: 'AssertionError',
message: 'MicrosoftAppId is required for MSI in configuration.',
});
});

it('createServiceClientCredentialFactoryFromConfiguration() manageIdentityApp with appPassword should throw', function () {
const config = new TestConfiguration({ ...MSIConfig, MicrosoftAppPassword: 'msiAppPassword' });

assert.throws(() => createServiceClientCredentialFactoryFromConfiguration(config), {
name: 'AssertionError',
message: 'MicrosoftAppPassword must not be set for MSI in configuration.',
});
});
});
21 changes: 19 additions & 2 deletions libraries/botbuilder-dialogs-adaptive-runtime/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { ServiceCollection } from 'botbuilder-dialogs-adaptive-runtime-core';

import {
AuthenticationConfiguration,
AuthenticationConstants,
allowedCallersClaimsValidator,
BotFrameworkAuthentication,
ServiceClientCredentialsFactory,
Expand Down Expand Up @@ -226,6 +227,20 @@ function addSkills(services: ServiceCollection, configuration: Configuration): v
({ storage }) => new SkillConversationIdFactory(storage)
);

// If TenantId is specified in config, add the tenant as a valid JWT token issuer for Bot to Skill conversation.
// The token issuer for MSI and single tenant scenarios will be the tenant where the bot is registered.
const validTokenIssuers: string[] = [];
const tenantId = configuration.type(['runtimeSettings', 'MicrosoftAppTenantIdKey'], z.string()) ?? '';

if (tenantId) {
// For SingleTenant/MSI auth, the JWT tokens will be issued from the bot's home tenant.
// So, these issuers need to be added to the list of valid token issuers for authenticating activity requests.
validTokenIssuers.push(`${AuthenticationConstants.ValidTokenIssuerUrlTemplateV1}${tenantId}/`);
validTokenIssuers.push(`${AuthenticationConstants.ValidTokenIssuerUrlTemplateV2}${tenantId}/v2.0`);
validTokenIssuers.push(`${AuthenticationConstants.ValidGovernmentTokenIssuerUrlTemplateV1}${tenantId}/`);
validTokenIssuers.push(`${AuthenticationConstants.ValidGovernmentTokenIssuerUrlTemplateV2}${tenantId}/v2.0`);
}

services.addFactory<AuthenticationConfiguration>('authenticationConfiguration', () => {
const allowedCallers =
configuration.type(['runtimeSettings', 'skills', 'allowedCallers'], z.array(z.string())) ?? [];
Expand All @@ -248,14 +263,16 @@ function addSkills(services: ServiceCollection, configuration: Configuration): v
// runtimeSettings.sills are ignored
return new AuthenticationConfiguration(
undefined,
allowedCallersClaimsValidator(skills.map((skill) => skill.msAppId))
allowedCallersClaimsValidator(skills.map((skill) => skill.msAppId)),
validTokenIssuers
);
} else {
// If the config entry for runtimeSettings.skills.allowedCallers contains entries, then we are a skill and
// we validate caller against this list
return new AuthenticationConfiguration(
undefined,
allowedCallers.length ? allowedCallersClaimsValidator(allowedCallers) : undefined
allowedCallers.length ? allowedCallersClaimsValidator(allowedCallers) : undefined,
validTokenIssuers
);
}
});
Expand Down
3 changes: 2 additions & 1 deletion libraries/botframework-connector/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
}
},
"dependencies": {
"@azure/identity": "2.0.0-beta.6",
"@azure/ms-rest-js": "1.9.1",
"@types/jsonwebtoken": "7.2.8",
"@types/node": "^10.17.27",
Expand Down Expand Up @@ -54,7 +55,7 @@
"build:browserify:run": "browserify -s BFC --debug lib/browser.js | exorcist lib/browser.js.map | sponge lib/browser.js",
"build:downlevel-dts": "downlevel-dts lib _ts3.4/lib --checksum",
"clean": "rimraf _ts3.4 lib tsconfig.tsbuildinfo",
"depcheck": "depcheck --config ../../.depcheckrc --ignores azure",
"depcheck": "depcheck --config ../../.depcheckrc --ignores azure,sinon",
"lint": "eslint . --ext .js,.ts",
"postbuild": "npm-run-all -p build:browserify build:downlevel-dts",
"test": "yarn build && yarn test:mocha",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ export class AuthenticationConfiguration {
* @param {string[]} requiredEndorsements An array of JWT endorsements.
* @param {(claims: Claim[]) => Promise<void>} validateClaims Function that validates a list of Claims
* and should throw an exception if the validation fails.
* @param {string[]} validTokenIssuers An array of valid JWT token issuers.
*/
constructor(public requiredEndorsements: string[] = [], public validateClaims?: ValidateClaims) {}
constructor(
public requiredEndorsements: string[] = [],
public validateClaims?: ValidateClaims,
public validTokenIssuers?: string[]
) {}
}
Loading