Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

fix: do not require multiple sign ins for tenant selection #6628

Merged
cwhitten merged 28 commits into from
Apr 2, 2021
Merged

fix: do not require multiple sign ins for tenant selection #6628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
e1c9f55
make dropdown width same as dropdown element
a-b-r-o-w-n Mar 30, 2021
7ba4ad9
do not require sign in every time fetching tenants
a-b-r-o-w-n Mar 30, 2021
ac57019
open dev tools after the app is ready
a-b-r-o-w-n Mar 30, 2021
8979a4c
do not populate dropdowns if token is null
a-b-r-o-w-n Mar 30, 2021
5b7a379
hide tenant selection when tokens are user provided
a-b-r-o-w-n Mar 30, 2021
0961076
rename method for clarity
a-b-r-o-w-n Mar 30, 2021
b4190cd
refactor how form data is used
a-b-r-o-w-n Mar 31, 2021
312ccf8
make seeding form data more resiliant
a-b-r-o-w-n Mar 31, 2021
704e3e6
fix issue where page was not set correctly
a-b-r-o-w-n Mar 31, 2021
fbe66ec
allow publishing from web
a-b-r-o-w-n Mar 31, 2021
6f95947
store tenant id in publish profiles for token based users
a-b-r-o-w-n Mar 31, 2021
8842806
add test for using cached tenant token
a-b-r-o-w-n Mar 31, 2021
db4e69f
Merge branch 'main' into abrown/select-tenant
a-b-r-o-w-n Mar 31, 2021
eafc1f1
disable resource group downdop for existing profiles
a-b-r-o-w-n Mar 31, 2021
b6c4217
Merge branch 'main' into abrown/select-tenant
a-b-r-o-w-n Mar 31, 2021
5cfbdd8
remove useless if statement
a-b-r-o-w-n Mar 31, 2021
1bea1eb
Merge branch 'main' into abrown/select-tenant
beyackle Mar 31, 2021
cd9028c
Merge branch 'main' into abrown/select-tenant
a-b-r-o-w-n Mar 31, 2021
9520595
Merge branch 'main' into abrown/select-tenant
a-b-r-o-w-n Apr 1, 2021
04ee40d
Merge branch 'main' into abrown/select-tenant
a-b-r-o-w-n Apr 1, 2021
0c27568
Fix function name
cwhitten Apr 1, 2021
9ee8f55
Merge branch 'main' into abrown/select-tenant
a-b-r-o-w-n Apr 2, 2021
00d0010
update name for clarity
a-b-r-o-w-n Apr 2, 2021
b903767
remove default tenant id
a-b-r-o-w-n Apr 2, 2021
7246381
add isGetTokensFromUser for compatibility with 3p extensions
a-b-r-o-w-n Apr 2, 2021
fad2d40
revert lint-staged config
a-b-r-o-w-n Apr 2, 2021
4a37dfe
Merge branch 'main' into abrown/select-tenant
cwhitten Apr 2, 2021
7abde0a
Merge branch 'main' into abrown/select-tenant
cwhitten Apr 2, 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
2 changes: 1 addition & 1 deletion Composer/packages/client/__tests__/pages/botProjectsSettings/PublishTarget.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { isShowAuthDialog } from '../../../src/utils/auth';
jest.mock('../../../src/utils/auth', () => ({
isShowAuthDialog: jest.fn(),
getTokenFromCache: jest.fn(),
isGetTokenFromUser: jest.fn(),
userShouldProvideTokens: jest.fn(),
}));

const state = {
Expand Down
4 changes: 2 additions & 2 deletions Composer/packages/client/src/components/ManageLuis/ManageLuis.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { ProvisionHandoff } from '@bfc/ui-shared';
import { AuthClient } from '../../utils/authClient';
import { AuthDialog } from '../../components/Auth/AuthDialog';
import { armScopes } from '../../constants';
import { getTokenFromCache, isShowAuthDialog, isGetTokenFromUser } from '../../utils/auth';
import { getTokenFromCache, isShowAuthDialog, userShouldProvideTokens } from '../../utils/auth';
import { LUIS_REGIONS } from '../../constants';
import { dispatcherState } from '../../recoilModel/atoms';

Expand Down Expand Up @@ -94,7 +94,7 @@ export const ManageLuis = (props: ManageLuisProps) => {

const hasAuth = async () => {
let newtoken = '';
if (isGetTokenFromUser()) {
if (userShouldProvideTokens()) {
if (isShowAuthDialog(false)) {
setShowAuthDialog(true);
}
Expand Down
4 changes: 2 additions & 2 deletions Composer/packages/client/src/components/ManageQNA/ManageQNA.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { ProvisionHandoff } from '@bfc/ui-shared';
import { AuthClient } from '../../utils/authClient';
import { AuthDialog } from '../../components/Auth/AuthDialog';
import { armScopes } from '../../constants';
import { getTokenFromCache, isShowAuthDialog, isGetTokenFromUser } from '../../utils/auth';
import { getTokenFromCache, isShowAuthDialog, userShouldProvideTokens } from '../../utils/auth';
import { dispatcherState } from '../../recoilModel/atoms';

type ManageQNAProps = {
Expand Down Expand Up @@ -102,7 +102,7 @@ export const ManageQNA = (props: ManageQNAProps) => {

const hasAuth = async () => {
let newtoken = '';
if (isGetTokenFromUser()) {
if (userShouldProvideTokens()) {
if (isShowAuthDialog(false)) {
setShowAuthDialog(true);
}
Expand Down
6 changes: 3 additions & 3 deletions Composer/packages/client/src/pages/botProject/adapters/ABSChannels.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { settingsState } from '../../../recoilModel';
import { AuthClient } from '../../../utils/authClient';
import { AuthDialog } from '../../../components/Auth/AuthDialog';
import { armScopes } from '../../../constants';
import { getTokenFromCache, isShowAuthDialog, isGetTokenFromUser } from '../../../utils/auth';
import { getTokenFromCache, isShowAuthDialog, userShouldProvideTokens } from '../../../utils/auth';
import httpClient from '../../../utils/httpUtil';
import { dispatcherState } from '../../../recoilModel';
import {
Expand Down Expand Up @@ -119,7 +119,7 @@ export const ABSChannels: React.FC<RuntimeSettingsProps> = (props) => {
navigateTo(`/bot/${projectId}/botProjectsSettings/#addNewPublishProfile`);
} else {
let newtoken = '';
if (isGetTokenFromUser()) {
if (userShouldProvideTokens()) {
if (isShowAuthDialog(false)) {
setShowAuthDialog(true);
}
Expand Down Expand Up @@ -315,7 +315,7 @@ export const ABSChannels: React.FC<RuntimeSettingsProps> = (props) => {

const hasAuth = async () => {
let newtoken = '';
if (isGetTokenFromUser()) {
if (userShouldProvideTokens()) {
if (isShowAuthDialog(false)) {
setShowAuthDialog(true);
}
Expand Down
10 changes: 7 additions & 3 deletions ...oser/packages/client/src/pages/botProject/create-publish-profile/PublishProfileDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { Dialog } from 'office-ui-fabric-react/lib/Dialog';
import { Link } from 'office-ui-fabric-react/lib/Link';
import { useRecoilValue } from 'recoil';

import { getTokenFromCache, isGetTokenFromUser, setTenantId, getTenantIdFromCache } from '../../../utils/auth';
import { getTokenFromCache, userShouldProvideTokens, setTenantId, getTenantIdFromCache } from '../../../utils/auth';
import { PublishType } from '../../../recoilModel/types';
import { PluginAPI } from '../../../plugins/api';
import { PluginHost } from '../../../components/PluginHost/PluginHost';
Expand Down Expand Up @@ -91,8 +91,12 @@ export const PublishProfileDialog: React.FC<PublishProfileDialogProps> = (props)
graphToken: getTokenFromCache('graphToken'),
};
};
/** @deprecated use `userShouldProvideTokens` instead */
PluginAPI.publish.isGetTokenFromUser = () => {
return isGetTokenFromUser();
return userShouldProvideTokens();
};
PluginAPI.publish.userShouldProvideTokens = () => {
return userShouldProvideTokens();
};
PluginAPI.publish.setTitle = (value) => {
setTitle(value);
Expand Down Expand Up @@ -160,7 +164,7 @@ export const PublishProfileDialog: React.FC<PublishProfileDialogProps> = (props)
const fullConfig = { ...config, name: name, type: targetType };

let arm, graph;
if (!isGetTokenFromUser()) {
if (!userShouldProvideTokens()) {
const tenantId = getTenantIdFromCache();
// require tenant id to be set by plugin (handles multiple tenant scenario)
if (!tenantId) {
Expand Down
10 changes: 5 additions & 5 deletions Composer/packages/client/src/pages/publish/Publish.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { getSensitiveProperties } from '../../recoilModel/dispatchers/utils/proj
import {
getTokenFromCache,
isShowAuthDialog,
isGetTokenFromUser,
userShouldProvideTokens,
setTenantId,
getTenantIdFromCache,
} from '../../utils/auth';
Expand Down Expand Up @@ -246,13 +246,13 @@ const Publish: React.FC<RouteComponentProps<{ projectId: string; targetName?: st
if (target && isPublishingToAzure(target)) {
const { tenantId } = JSON.parse(target.configuration);

if (tenantId) {
if (userShouldProvideTokens()) {
token = getTokenFromCache('accessToken');
} else if (tenantId) {
token = tenantTokenMap.get(tenantId) ?? (await AuthClient.getARMTokenForTenant(tenantId));
tenantTokenMap.set(tenantId, token);
} else if (isGetTokenFromUser()) {
token = getTokenFromCache('accessToken');
// old publish profile without tenant id
} else {
// old publish profile without tenant id
let tenant = getTenantIdFromCache();
let tenants;
if (!tenant) {
Expand Down
3 changes: 3 additions & 0 deletions Composer/packages/client/src/plugins/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ interface PublishAPI {
getName?: () => string;
savePublishConfig?: (config: PublishConfig) => void;
getTokenFromCache?: () => { accessToken: string; graphToken: string };
/** @deprecated use `userShouldProvideTokens` instead */
isGetTokenFromUser?: () => boolean;
userShouldProvideTokens?: () => boolean;
getTenantIdFromCache?: () => string;
setTenantId?: (value: string) => void;
}
Expand All @@ -62,6 +64,7 @@ class API implements IAPI {
savePublishConfig: undefined,
getTokenFromCache: undefined,
isGetTokenFromUser: undefined,
userShouldProvideTokens: undefined,
getTenantIdFromCache: undefined,
setTenantId: undefined,
};
Expand Down
2 changes: 1 addition & 1 deletion Composer/packages/client/src/utils/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ export function isShowAuthDialog(needGraph: boolean): boolean {
}
}

export function isGetTokenFromUser(): boolean {
export function userShouldProvideTokens(): boolean {
if (isElectron()) {
return false;
} else if (authConfig.clientId && authConfig.redirectUrl && authConfig.tenantId) {
Expand Down
101 changes: 73 additions & 28 deletions Composer/packages/electron-server/__tests__/auth/oneAuthService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ describe('OneAuth Serivce', () => {
beforeEach(() => {
jest.resetModules();
oneAuthService = new OneAuthInstance();
// eslint-disable-next-line no-underscore-dangle
(oneAuthService as any)._oneAuth = mockOneAuth;
mockOneAuth.acquireCredentialInteractively.mockClear();
mockOneAuth.acquireCredentialSilently.mockClear();
Expand Down Expand Up @@ -169,38 +170,82 @@ describe('OneAuth Serivce', () => {
const result = await service.getAccessToken({});

expect(result).toEqual({ accessToken: '', acquiredAt: 0, expiryTime: 99999999999 });
// reset node env
process.env.NODE_ENV = 'test';
});

it('should get a list of tenants', async () => {
const mockTenants = [
{
tenantId: 'tenant1',
},
{
tenantId: 'tenant2',
},
{
tenantId: 'tenant3',
},
];
mockFetch.mockResolvedValueOnce({
json: jest.fn().mockResolvedValue({ value: mockTenants }),
describe('#getTenants', () => {
it('should get a list of tenants', async () => {
const mockTenants = [
{
tenantId: 'tenant1',
},
{
tenantId: 'tenant2',
},
{
tenantId: 'tenant3',
},
];
mockFetch.mockResolvedValueOnce({
json: jest.fn().mockResolvedValue({ value: mockTenants }),
});
const tenants = await oneAuthService.getTenants();

// it should have initialized
expect(mockOneAuth.setLogPiiEnabled).toHaveBeenCalled();
expect(mockOneAuth.setLogCallback).toHaveBeenCalled();
expect(mockOneAuth.initialize).toHaveBeenCalled();

// it should have signed in
expect(mockOneAuth.signInInteractively).toHaveBeenCalled();
expect((oneAuthService as any).signedInARMAccount).toEqual(mockAccount);

// it should have called the tenants API
expect(mockFetch).toHaveBeenCalledWith('https://management.azure.com/tenants?api-version=2020-01-01', {
headers: {
Authorization: 'Bearer someToken',
},
});

expect(tenants).toBe(mockTenants);
});
const tenants = await oneAuthService.getTenants();

// it should have initialized
expect(mockOneAuth.setLogPiiEnabled).toHaveBeenCalled();
expect(mockOneAuth.setLogCallback).toHaveBeenCalled();
expect(mockOneAuth.initialize).toHaveBeenCalled();

// it should have signed in
expect(mockOneAuth.signInInteractively).toHaveBeenCalled();
expect((oneAuthService as any).signedInARMAccount).toEqual(mockAccount);

// it should have called the tenants API
expect(mockFetch.mock.calls[0][0]).toBe('https://management.azure.com/tenants?api-version=2020-01-01');

expect(tenants).toBe(mockTenants);
it('should not attempt to sign in if token already fetched', async () => {
const mockTenants = [
{
tenantId: 'tenant1',
},
{
tenantId: 'tenant2',
},
{
tenantId: 'tenant3',
},
];
mockFetch.mockResolvedValueOnce({
json: jest.fn().mockResolvedValue({ value: mockTenants }),
});
(oneAuthService as any).signedInARMAccount = { some: 'account' };
(oneAuthService as any).tenantToken = 'cached-token';
const tenants = await oneAuthService.getTenants();

// it should have initialized
expect(mockOneAuth.setLogPiiEnabled).toHaveBeenCalled();
expect(mockOneAuth.setLogCallback).toHaveBeenCalled();
expect(mockOneAuth.initialize).toHaveBeenCalled();

expect(mockOneAuth.signInInteractively).not.toHaveBeenCalled();

// it should have called the tenants API
expect(mockFetch).toHaveBeenCalledWith('https://management.azure.com/tenants?api-version=2020-01-01', {
headers: {
Authorization: 'Bearer cached-token',
},
});

expect(tenants).toBe(mockTenants);
});
});

it('should throw an error if something goes wrong while getting a list of tenants', async () => {
Expand Down
3 changes: 2 additions & 1 deletion Composer/packages/electron-server/__tests__/setupTests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
process.env.DEBUG = 'composer*';
Copy link
Contributor Author

@a-b-r-o-w-n a-b-r-o-w-n Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tonyanziano I removed this. Was it there for a good reason? Just trying to cut down on the noise in the test output!

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

jest.mock('electron', () => ({
app: {
Expand Down
20 changes: 12 additions & 8 deletions Composer/packages/electron-server/src/auth/oneAuthService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ export class OneAuthInstance extends OneAuthBase {
private _oneAuth: typeof OneAuth | null = null; //eslint-disable-line
private signedInAccount: OneAuth.Account | undefined;
private signedInARMAccount: OneAuth.Account | undefined;
/** Token solely used to fetch tenants */
private tenantToken: string | undefined;

constructor() {
super();
Expand Down Expand Up @@ -189,17 +191,19 @@ export class OneAuthInstance extends OneAuthBase {
if (!this.initialized) {
this.initialize();
}
// log the user into the infrastructure tenant to get a token that can be used on the "tenants" API
log('Logging user into ARM...');
this.signedInARMAccount = undefined;
const signInParams = new this.oneAuth.AuthParameters(DEFAULT_AUTH_SCHEME, ARM_AUTHORITY, ARM_RESOURCE, '', '');
const result: OneAuth.AuthResult = await this.oneAuth.signInInteractively('', signInParams, '');
this.signedInARMAccount = result.account;
const token = result.credential.value;

if (!this.signedInARMAccount || !this.tenantToken) {
// log the user into the infrastructure tenant to get a token that can be used on the "tenants" API
log('Logging user into ARM...');
const signInParams = new this.oneAuth.AuthParameters(DEFAULT_AUTH_SCHEME, ARM_AUTHORITY, ARM_RESOURCE, '', '');
const result: OneAuth.AuthResult = await this.oneAuth.signInInteractively('', signInParams, '');
this.signedInARMAccount = result.account;
this.tenantToken = result.credential.value;
}

// call the tenants API
const tenantsResult = await fetch('https://management.azure.com/tenants?api-version=2020-01-01', {
headers: { Authorization: `Bearer ${token}` },
headers: { Authorization: `Bearer ${this.tenantToken}` },
});
const tenants = (await tenantsResult.json()) as GetTenantsResult;
log('Got Azure tenants for user: %O', tenants.value);
Expand Down
8 changes: 4 additions & 4 deletions Composer/packages/electron-server/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,6 @@ async function main(show = false) {
const mainWindow = ElectronWindow.getInstance().browserWindow;
initAppMenu(mainWindow);
if (mainWindow) {
if (process.env.COMPOSER_DEV_TOOLS) {
mainWindow.webContents.openDevTools();
}

await mainWindow.loadURL(getBaseUrl());

if (show) {
Expand Down Expand Up @@ -296,6 +292,10 @@ async function run() {

const mainWindow = getMainWindow();
mainWindow?.webContents.send('session-update', 'session-started');

if (process.env.COMPOSER_DEV_TOOLS) {
mainWindow?.webContents.openDevTools();
}
});

// Quit when all windows are closed.
Expand Down
7 changes: 6 additions & 1 deletion Composer/packages/extension-client/src/hooks/usePublishApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,12 @@ export function usePublishApi() {
function setTenantId(value): void {
window[ComposerGlobalName].setTenantId(value);
}
function userShouldProvideTokens(): boolean {
return window[ComposerGlobalName].userShouldProvideTokens();
}
/** @deprecated use `userShouldProvideTokens` instead */
function isGetTokenFromUser(): boolean {
return window[ComposerGlobalName].isGetTokenFromUser();
return window[ComposerGlobalName].userShouldProvideTokens();
}
return {
publishConfig: getPublishConfig(),
Expand All @@ -71,6 +75,7 @@ export function usePublishApi() {
savePublishConfig,
getTokenFromCache,
isGetTokenFromUser,
userShouldProvideTokens,
getTenantIdFromCache,
setTenantId,
};
Expand Down
2 changes: 1 addition & 1 deletion Composer/packages/server/src/locales/en-US.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4025,4 +4025,4 @@
"your_template_requires_qna_maker_to_access_content_a4ca6f76": {
"message": "Your template requires QnA Maker to access content for your bot."
}
}
}
4 changes: 3 additions & 1 deletion extensions/azurePublish/src/components/ResourceGroupPicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ type ResourceGroupItemChoice = {
};

type Props = {
disabled: boolean;
/**
* The resource groups to choose from.
* Set to undefined to disable this picker.
Expand Down Expand Up @@ -93,6 +94,7 @@ const onRenderLabel = (props) => {
};

export const ResourceGroupPicker = ({
disabled,
resourceGroupNames,
selectedResourceGroupName: controlledSelectedName,
newResourceGroupName: controlledNewName,
Expand Down Expand Up @@ -169,7 +171,7 @@ export const ResourceGroupPicker = ({
ariaLabel={formatMessage(
'A resource group is a collection of resources that share the same lifecycle, permissions, and policies'
)}
disabled={loading}
disabled={loading || disabled}
label={formatMessage('Resource group')}
options={options}
placeholder={formatMessage('Select one')}
Expand Down
Loading