Skip to content

Commit

Permalink
Minor updates before removing auth env variables (#415)
Browse files Browse the repository at this point in the history
### Motivation and Context

<!-- Thank you for your contribution to the chat-copilot repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->
this PR makes small changes which will be required in a follow-up PR
that removes the AAD env variables from the frontend.

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->
- [X] The code builds clean without any errors or warnings
- [X] The PR follows the [Contribution
Guidelines](https://github.com/microsoft/chat-copilot/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/chat-copilot/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [X] All unit tests pass, and I have added new tests where possible
- [X] I didn't break anyone 😄
  • Loading branch information
dehoward committed Sep 28, 2023
1 parent e0934b5 commit 5cd6ac0
Show file tree
Hide file tree
Showing 24 changed files with 84 additions and 49 deletions.
4 changes: 2 additions & 2 deletions webapi/Controllers/ServiceInfoController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ public IActionResult GetAuthConfig()
{
AuthType = this._chatAuthenticationOptions.Type.ToString(),
AadAuthority = authorityUriString,
AadClient = this._frontendOptions.AadClientId,
AadApiScope = $"api://{this._chatAuthenticationOptions.AzureAd!.ClientId}/{this._chatAuthenticationOptions.AzureAd!.Scopes}"
AadClientId = this._frontendOptions.AadClientId,
AadApiScope = $"api://{this._chatAuthenticationOptions.AzureAd!.ClientId}/{this._chatAuthenticationOptions.AzureAd!.Scopes}",
};

return this.Ok(config);
Expand Down
2 changes: 1 addition & 1 deletion webapi/Extensions/ServiceExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ internal static IServiceCollection AddUtilities(this IServiceCollection services
return services.AddScoped<AskConverter>();
}

internal static IServiceCollection AddMainetnanceServices(this IServiceCollection services)
internal static IServiceCollection AddMaintenanceServices(this IServiceCollection services)
{
// Inject migration services
services.AddSingleton<IChatMigrationMonitor, ChatMigrationMonitor>();
Expand Down
4 changes: 2 additions & 2 deletions webapi/Models/Response/FrontendAuthConfig.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ public class FrontendAuthConfig
/// <summary>
/// Azure Active Directory client ID the frontend is to use.
/// </summary>
[JsonPropertyName("aadClient")]
public string AadClient { get; set; } = string.Empty;
[JsonPropertyName("aadClientId")]
public string AadClientId { get; set; } = string.Empty;

/// <summary>
/// Azure Active Directory scope the frontend should request.
Expand Down
2 changes: 1 addition & 1 deletion webapi/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public static async Task Main(string[] args)

// Add in the rest of the services.
builder.Services
.AddMainetnanceServices()
.AddMaintenanceServices()
.AddEndpointsApiExplorer()
.AddSwaggerGen()
.AddCorsPolicy(builder.Configuration)
Expand Down
6 changes: 3 additions & 3 deletions webapp/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ const App: FC = () => {
}
}

if ((isAuthenticated || !AuthHelper.IsAuthAAD) && appState === AppState.LoadingChats) {
if ((isAuthenticated || !AuthHelper.isAuthAAD()) && appState === AppState.LoadingChats) {
void Promise.all([
// Load all chats from memory
chat.loadChats().then((succeeded) => {
Expand Down Expand Up @@ -139,7 +139,7 @@ const App: FC = () => {
className="app-container"
theme={features[FeatureKeys.DarkMode].enabled ? semanticKernelDarkTheme : semanticKernelLightTheme}
>
{AuthHelper.IsAuthAAD ? (
{AuthHelper.isAuthAAD() ? (
<>
<UnauthenticatedTemplate>
<div className={classes.container}>
Expand Down Expand Up @@ -191,7 +191,7 @@ const Chat = ({
<BackendProbe
uri={process.env.REACT_APP_BACKEND_URI as string}
onBackendFound={() => {
if (AuthHelper.IsAuthAAD) {
if (AuthHelper.isAuthAAD()) {
setAppState(AppState.SettingUserInfo);
} else {
setAppState(AppState.LoadingChats);
Expand Down
5 changes: 3 additions & 2 deletions webapp/src/components/chat/ChatInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { RootState } from '../../redux/app/store';
import { addAlert } from '../../redux/features/app/appSlice';
import { editConversationInput, updateBotResponseStatus } from '../../redux/features/conversations/conversationsSlice';
import { Alerts } from '../shared/Alerts';
import { getErrorDetails } from '../utils/TextUtils';
import { SpeechService } from './../../libs/services/SpeechService';
import { updateUserIsTyping } from './../../redux/features/conversations/conversationsSlice';
import { ChatStatus } from './ChatStatus';
Expand Down Expand Up @@ -97,7 +98,7 @@ export const ChatInput: React.FC<ChatInputProps> = ({ isDraggingOver, onDragLeav

React.useEffect(() => {
async function initSpeechRecognizer() {
const speechService = new SpeechService(process.env.REACT_APP_BACKEND_URI as string);
const speechService = new SpeechService();
const response = await speechService.getSpeechTokenAsync(
await AuthHelper.getSKaaSAccessToken(instance, inProgress),
);
Expand All @@ -108,7 +109,7 @@ export const ChatInput: React.FC<ChatInputProps> = ({ isDraggingOver, onDragLeav
}

initSpeechRecognizer().catch((e) => {
const errorDetails = e instanceof Error ? e.message : String(e);
const errorDetails = getErrorDetails(e);
const errorMessage = `Unable to initialize speech recognizer. Details: ${errorDetails}`;
dispatch(addAlert({ message: errorMessage, type: AlertType.Error }));
});
Expand Down
5 changes: 2 additions & 3 deletions webapp/src/components/chat/shared/EditChatName.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { addAlert } from '../../../redux/features/app/appSlice';
import { editConversationTitle } from '../../../redux/features/conversations/conversationsSlice';
import { Breakpoints } from '../../../styles';
import { Checkmark20, Dismiss20 } from '../../shared/BundledIcons';
import { getErrorDetails } from '../../utils/TextUtils';

const useClasses = makeStyles({
root: {
Expand Down Expand Up @@ -67,9 +68,7 @@ export const EditChatName: React.FC<IEditChatNameProps> = ({ name, chatId, exitE

const handleSave = () => {
onSaveTitleChange().catch((e: any) => {
const errorMessage = `Unable to retrieve chat to change title. Details: ${
e instanceof Error ? e.message : String(e)
}`;
const errorMessage = `Unable to retrieve chat to change title. Details: ${getErrorDetails(e)}`;
dispatch(addAlert({ message: errorMessage, type: AlertType.Error }));
});
};
Expand Down
2 changes: 1 addition & 1 deletion webapp/src/components/header/UserSettingsMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export const UserSettingsMenu: FC<IUserSettingsProps> = ({ setLoadingState }) =>

return (
<>
{AuthHelper.IsAuthAAD ? (
{AuthHelper.isAuthAAD() ? (
<Menu>
<MenuTrigger disableButtonEnhancement>
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Divider, Switch, Text, makeStyles, shorthands, tokens } from '@fluentui/react-components';
import { useCallback } from 'react';
import { AuthHelper } from '../../../libs/auth/AuthHelper';
import { useAppDispatch, useAppSelector } from '../../../redux/app/hooks';
import { RootState } from '../../../redux/app/store';
import { FeatureKeys, Setting } from '../../../redux/features/app/AppState';
Expand Down Expand Up @@ -56,7 +57,9 @@ export const SettingSection: React.FC<ISettingsSectionProps> = ({ setting, conte
<Switch
label={feature.label}
checked={feature.enabled}
disabled={feature.inactive}
disabled={
!!feature.inactive || (key === FeatureKeys.MultiUserChat && !AuthHelper.isAuthAAD())
}
onChange={() => {
onFeatureChange(key);
}}
Expand Down
12 changes: 7 additions & 5 deletions webapp/src/components/open-api-plugins/PluginConnector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@ import {
} from '@fluentui/react-components';
import { Dismiss20Regular } from '@fluentui/react-icons';
import { FormEvent, useState } from 'react';
import { AuthHelper } from '../../libs/auth/AuthHelper';
import { TokenHelper } from '../../libs/auth/TokenHelper';
import { useAppDispatch } from '../../redux/app/hooks';
import { AdditionalApiProperties, Plugin, PluginAuthRequirements } from '../../redux/features/plugins/PluginsState';
import { AdditionalApiProperties, PluginAuthRequirements } from '../../redux/features/plugins/PluginsState';
import { connectPlugin } from '../../redux/features/plugins/pluginsSlice';

const useClasses = makeStyles({
Expand Down Expand Up @@ -54,7 +55,6 @@ interface PluginConnectorProps {
publisher: string;
authRequirements: PluginAuthRequirements;
apiProperties?: AdditionalApiProperties;
inactive?: Plugin['inactive'];
}

export const PluginConnector: React.FC<PluginConnectorProps> = ({
Expand All @@ -63,7 +63,6 @@ export const PluginConnector: React.FC<PluginConnectorProps> = ({
publisher,
authRequirements,
apiProperties,
inactive,
}) => {
const classes = useClasses();

Expand Down Expand Up @@ -121,6 +120,9 @@ export const PluginConnector: React.FC<PluginConnectorProps> = ({
setOpen(false);
};

const inactive = msalRequired && !AuthHelper.isAuthAAD();
const inactiveReason = 'Only available when using Azure AD authorization.';

return (
<Dialog
open={open}
Expand All @@ -135,8 +137,8 @@ export const PluginConnector: React.FC<PluginConnectorProps> = ({
data-testid="openPluginDialogButton"
aria-label="Enable plugin"
appearance="primary"
disabled={!!inactive}
title={inactive}
disabled={inactive}
title={inactive ? inactiveReason : ''}
>
Enable
</Button>
Expand Down
3 changes: 1 addition & 2 deletions webapp/src/components/open-api-plugins/cards/PluginCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ interface PluginCardProps {
}

export const PluginCard: React.FC<PluginCardProps> = ({ plugin }) => {
const { name, publisher, enabled, authRequirements, apiProperties, icon, description, inactive } = plugin;
const { name, publisher, enabled, authRequirements, apiProperties, icon, description } = plugin;
const dispatch = useAppDispatch();

const onDisconnectClick = (event: FormEvent) => {
Expand Down Expand Up @@ -42,7 +42,6 @@ export const PluginCard: React.FC<PluginCardProps> = ({ plugin }) => {
publisher={publisher}
authRequirements={authRequirements}
apiProperties={apiProperties}
inactive={inactive}
/>
)
}
Expand Down
7 changes: 7 additions & 0 deletions webapp/src/components/utils/TextUtils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,10 @@ export function replaceCitationLinksWithIndices(formattedMessageContent: string,

return formattedMessageContent;
}

/**
* Gets message of error
*/
export function getErrorDetails(error: unknown) {
return error instanceof Error ? error.message : String(error);
}
5 changes: 4 additions & 1 deletion webapp/src/components/views/Login.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Body1, Button, Image, Title3 } from '@fluentui/react-components';
import React from 'react';
import signInLogo from '../../ms-symbollockup_signin_light.svg';
import { useSharedClasses } from '../../styles';
import { getErrorDetails } from '../utils/TextUtils';

export const Login: React.FC = () => {
const { instance } = useMsal();
Expand All @@ -24,7 +25,9 @@ export const Login: React.FC = () => {
style={{ padding: 0 }}
appearance="transparent"
onClick={() => {
instance.loginRedirect().catch(() => {});
instance.loginRedirect().catch((e: unknown) => {
alert(`Error signing in: ${getErrorDetails(e)}`);
});
}}
data-testid="signinButton"
>
Expand Down
6 changes: 4 additions & 2 deletions webapp/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ document.addEventListener('DOMContentLoaded', () => {

const missingEnvVariables = getMissingEnvVariables();
const validEnvFile = missingEnvVariables.length === 0;
const shouldUseMsal = validEnvFile && AuthHelper.IsAuthAAD;
const shouldUseMsal = validEnvFile && AuthHelper.isAuthAAD();

let msalInstance: IPublicClientApplication | null = null;
if (shouldUseMsal) {
Expand Down Expand Up @@ -58,7 +58,9 @@ document.addEventListener('DOMContentLoaded', () => {
{/* eslint-enable @typescript-eslint/no-non-null-assertion */}
</ReduxProvider>
) : (
<MissingEnvVariablesError missingVariables={missingEnvVariables} />
<FluentProvider className="app-container" theme={semanticKernelLightTheme}>
<MissingEnvVariablesError missingVariables={missingEnvVariables} />
</FluentProvider>
)}
</React.StrictMode>,
);
Expand Down
6 changes: 3 additions & 3 deletions webapp/src/libs/auth/AuthHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,12 @@ const logoutAsync = (instance: IPublicClientApplication) => {
/**
* Determines if the app is configured to use Azure AD for authorization
*/
const IsAuthAAD = process.env.REACT_APP_AUTH_TYPE === AuthType.AAD;
const isAuthAAD = () => process.env.REACT_APP_AUTH_TYPE === AuthType.AAD;

// SKaaS = Semantic Kernel as a Service
// Gets token with scopes to authorize SKaaS specifically
const getSKaaSAccessToken = async (instance: IPublicClientApplication, inProgress: InteractionStatus) => {
return IsAuthAAD
return isAuthAAD()
? await TokenHelper.getAccessTokenUsingMsal(inProgress, instance, Constants.msal.semanticKernelScopes)
: '';
};
Expand All @@ -126,5 +126,5 @@ export const AuthHelper = {
ssoSilentRequest,
loginAsync,
logoutAsync,
IsAuthAAD,
isAuthAAD,
};
6 changes: 3 additions & 3 deletions webapp/src/libs/hooks/useChat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ export const useChat = () => {
const { conversations } = useAppSelector((state: RootState) => state.conversations);
const { activeUserInfo, features } = useAppSelector((state: RootState) => state.app);

const botService = new BotService(process.env.REACT_APP_BACKEND_URI as string);
const chatService = new ChatService(process.env.REACT_APP_BACKEND_URI as string);
const documentImportService = new DocumentImportService(process.env.REACT_APP_BACKEND_URI as string);
const botService = new BotService();
const chatService = new ChatService();
const documentImportService = new DocumentImportService();

const botProfilePictures: string[] = [botIcon1, botIcon2, botIcon3, botIcon4, botIcon5];

Expand Down
2 changes: 1 addition & 1 deletion webapp/src/libs/hooks/useFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export const useFile = () => {
const { instance, inProgress } = useMsal();

const chat = useChat();
const documentImportService = new DocumentImportService(process.env.REACT_APP_BACKEND_URI as string);
const documentImportService = new DocumentImportService();

async function loadFile<T>(file: File, loadCallBack: (data: T) => Promise<void>): Promise<T> {
return await new Promise((resolve, reject) => {
Expand Down
2 changes: 1 addition & 1 deletion webapp/src/libs/hooks/useGraph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export const useGraph = () => {
const graphService = new GraphService();

const loadUsers = async (userIds: string[]) => {
if (!AuthHelper.IsAuthAAD) {
if (!AuthHelper.isAuthAAD()) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion webapp/src/libs/hooks/usePlugins.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { getErrorDetails } from './useChat';
export const usePlugins = () => {
const dispatch = useAppDispatch();
const { instance, inProgress } = useMsal();
const chatService = React.useMemo(() => new ChatService(process.env.REACT_APP_BACKEND_URI as string), []);
const chatService = React.useMemo(() => new ChatService(), []);

const addCustomPlugin = (manifest: PluginManifest, manifestDomain: string) => {
const newPlugin: Plugin = {
Expand Down
17 changes: 11 additions & 6 deletions webapp/src/libs/services/BaseService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,26 @@ interface ServiceRequest {

const noResponseBodyStatusCodes = [202, 204];

export const BackendServiceUrl = process.env.REACT_APP_BACKEND_URI ?? window.origin;

export class BaseService {
// eslint-disable-next-line @typescript-eslint/space-before-function-paren
constructor(protected readonly serviceUrl: string) {}
constructor(protected readonly serviceUrl: string = BackendServiceUrl) {}

protected readonly getResponseAsync = async <T>(
request: ServiceRequest,
accessToken: string,
accessToken?: string,
enabledPlugins?: Plugin[],
): Promise<T> => {
const { commandPath, method, body, query } = request;
const isFormData = body instanceof FormData;

const headers = new Headers({
Authorization: `Bearer ${accessToken}`,
});
const headers = new Headers(
accessToken
? {
Authorization: `Bearer ${accessToken}`,
}
: undefined,
);

if (!isFormData) {
headers.append('Content-Type', 'application/json');
Expand Down
22 changes: 19 additions & 3 deletions webapp/src/redux/features/app/AppState.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,27 @@
// Copyright (c) Microsoft. All rights reserved.

import { AuthHelper, DefaultActiveUserInfo } from '../../../libs/auth/AuthHelper';
import { AuthHelper } from '../../../libs/auth/AuthHelper';
import { AlertType } from '../../../libs/models/AlertType';
import { IChatUser } from '../../../libs/models/ChatUser';
import { ServiceOptions } from '../../../libs/models/ServiceOptions';
import { TokenUsage } from '../../../libs/models/TokenUsage';

// This is the default user information when authentication is set to 'None'.
// It must match what is defined in PassthroughAuthenticationHandler.cs on the backend.
export const DefaultChatUser: IChatUser = {
id: 'c05c61eb-65e4-4223-915a-fe72b0c9ece1',
emailAddress: 'user@contoso.com',
fullName: 'Default User',
online: true,
isTyping: false,
};

export const DefaultActiveUserInfo: ActiveUserInfo = {
id: DefaultChatUser.id,
email: DefaultChatUser.emailAddress,
username: DefaultChatUser.fullName,
};

export interface ActiveUserInfo {
id: string;
email: string;
Expand Down Expand Up @@ -86,7 +103,6 @@ export const Features = {
enabled: false,
label: 'Live Chat Session Sharing',
description: 'Enable multi-user chat sessions. Not available when authorization is disabled.',
inactive: !AuthHelper.IsAuthAAD,
},
[FeatureKeys.RLHF]: {
enabled: false,
Expand Down Expand Up @@ -122,7 +138,7 @@ export const Settings = [

export const initialState: AppState = {
alerts: [],
activeUserInfo: AuthHelper.IsAuthAAD ? undefined : DefaultActiveUserInfo,
activeUserInfo: AuthHelper.isAuthAAD() ? undefined : DefaultActiveUserInfo,
tokenUsage: {},
features: Features,
settings: Settings,
Expand Down
Loading

0 comments on commit 5cd6ac0

Please sign in to comment.