Skip to content

Commit

Permalink
[C#] refactor: fix warnings and issues in code (#944)
Browse files Browse the repository at this point in the history
## Linked issues

closes: #861 

## Details

1. Fix warnings in the code
2. Add cancellation token parameter to interface
3. Mark bot/message extension/adaptive card base class as internal - we
can change them to public in the future if we want to developers to
leverage them. For now, they should be internal implementation.
4. Add `Async` to function names
5. Remove `TeamsSsoAdaptiveCardAuthentication` class as it's not
supported temporary
6. Fix a bug that cannot read token from MSAL OBO token cache by using
the `ILongRunningWebApi` interface
7. Fix a bug that token exchange route selector may throw exception

## Attestation Checklist

- [x] My code follows the style guidelines of this project

- I have checked for/fixed spelling, linting, and other errors
- I have commented my code for clarity
- I have made corresponding changes to the documentation (we use
[TypeDoc](https://typedoc.org/) to document our code)
- My changes generate no new warnings
- I have added tests that validates my changes, and provides sufficient
test coverage. I have tested with:
  - Local testing
  - E2E testing in Teams
- New and existing unit tests pass locally with my changes
  • Loading branch information
blackchoey authored Nov 30, 2023
1 parent 035e49e commit 1f7b921
Show file tree
Hide file tree
Showing 19 changed files with 127 additions and 100 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public async void Test_SignIn_DefaultHandler()
var turnState = await TurnStateConfig.GetTurnStateWithConversationStateAsync(turnContext);

// act
var response = await authManager.SignUserIn(turnContext, turnState);
var response = await authManager.SignUserInAsync(turnContext, turnState);

// assert
Assert.Equal(SignInStatus.Complete, response.Status);
Expand All @@ -48,7 +48,7 @@ public async void Test_SignIn_SpecificHandler()
var turnState = await TurnStateConfig.GetTurnStateWithConversationStateAsync(turnContext);

// act
var response = await authManager.SignUserIn(turnContext, turnState, "sharepoint");
var response = await authManager.SignUserInAsync(turnContext, turnState, "sharepoint");

// assert
Assert.Equal(SignInStatus.Complete, response.Status);
Expand All @@ -72,7 +72,7 @@ public async void Test_SignIn_Pending()
var turnState = await TurnStateConfig.GetTurnStateWithConversationStateAsync(turnContext);

// act
var response = await authManager.SignUserIn(turnContext, turnState);
var response = await authManager.SignUserInAsync(turnContext, turnState);

// assert
Assert.Equal(SignInStatus.Pending, response.Status);
Expand All @@ -99,7 +99,7 @@ public async void Test_SignOut_DefaultHandler()
};

// act
await authManager.SignOutUser(turnContext, turnState);
await authManager.SignOutUserAsync(turnContext, turnState);

// assert
Assert.False(turnState.Temp.AuthTokens.ContainsKey("graph"));
Expand All @@ -126,7 +126,7 @@ public async void Test_SignOut_SpecificHandler()
};

// act
await authManager.SignOutUser(turnContext, turnState, "sharepoint");
await authManager.SignOutUserAsync(turnContext, turnState, "sharepoint");

// assert
Assert.False(turnState.Temp.AuthTokens.ContainsKey("sharepoint"));
Expand All @@ -147,7 +147,7 @@ public async void Test_IsValidActivity_DefaultHandler()
var turnContext = MockTurnContext();

// act
var validActivity = await authManager.IsValidActivity(turnContext);
var validActivity = await authManager.IsValidActivityAsync(turnContext);

// assert
Assert.True(validActivity);
Expand All @@ -167,7 +167,7 @@ public async void Test_IsValidActivity_SpecificHandler()
var turnContext = MockTurnContext();

// act
var validActivity = await authManager.IsValidActivity(turnContext, "sharepoint");
var validActivity = await authManager.IsValidActivityAsync(turnContext, "sharepoint");

// assert
Assert.True(validActivity);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,16 @@ public MockedBotAuthentication(Application<TState> app, string name, List<Dialog
_throwExceptionWhenContinue = throwExceptionWhenContinue;
}

public override Task<DialogTurnResult> ContinueDialog(ITurnContext context, TState state, string dialogStateProperty)
public override Task<DialogTurnResult> ContinueDialog(ITurnContext context, TState state, string dialogStateProperty, CancellationToken cancellationToken = default)
{
if (_throwExceptionWhenContinue)
{
throw new Exception("mocked error");
throw new TeamsAIAuthException("mocked error");
}
return Task.FromResult(_continueDialogResult);
}

public override Task<DialogTurnResult> RunDialog(ITurnContext context, TState state, string dialogStateProperty)
public override Task<DialogTurnResult> RunDialog(ITurnContext context, TState state, string dialogStateProperty, CancellationToken cancellationToken = default)
{
var result = _runDialogResult.FirstOrDefault();
if (result == null)
Expand Down Expand Up @@ -113,7 +113,7 @@ public async void Test_Authenticate_Complete()
{
// arrange
var app = new Application<TurnState>(new ApplicationOptions<TurnState>());
var botAuth = new MockedBotAuthentication<TurnState>(app, "test", runDialogResult: new List<DialogTurnResult>() { new DialogTurnResult(DialogTurnStatus.Complete, new TokenResponse(token: "test token")) });
var botAuth = new MockedBotAuthentication<TurnState>(app, "test", runDialogResult: new List<DialogTurnResult>() { new(DialogTurnStatus.Complete, new TokenResponse(token: "test token")) });
var context = MockTurnContext();
var state = await TurnStateConfig.GetTurnStateWithConversationStateAsync(context);

Expand All @@ -130,7 +130,7 @@ public async void Test_Authenticate_CompleteWithoutToken()
{
// arrange
var app = new Application<TurnState>(new ApplicationOptions<TurnState>());
var botAuth = new MockedBotAuthentication<TurnState>(app, "test", runDialogResult: new List<DialogTurnResult>() { new DialogTurnResult(DialogTurnStatus.Complete) });
var botAuth = new MockedBotAuthentication<TurnState>(app, "test", runDialogResult: new List<DialogTurnResult>() { new(DialogTurnStatus.Complete) });
var context = MockTurnContext();
var state = await TurnStateConfig.GetTurnStateWithConversationStateAsync(context);

Expand Down Expand Up @@ -161,7 +161,7 @@ public async void Test_HandleSignInActivity_Complete()
messageText = context.Activity.Text;
return Task.CompletedTask;
});
botAuth.OnUserSignInFailure((context, state, exception) => { throw new Exception("sign in failure handler should not be called"); });
botAuth.OnUserSignInFailure((context, state, exception) => { throw new TeamsAIAuthException("sign in failure handler should not be called"); });

// act
await botAuth.HandleSignInActivity(context, state, new CancellationToken());
Expand All @@ -180,7 +180,7 @@ public async void Test_HandleSignInActivity_CompleteWithoutToken()
var context = MockTurnContext();
var state = await TurnStateConfig.GetTurnStateWithConversationStateAsync(context);
TeamsAIAuthException? authException = null;
botAuth.OnUserSignInSuccess((context, state) => { throw new Exception("sign in success handler should not be called"); });
botAuth.OnUserSignInSuccess((context, state) => { throw new TeamsAIAuthException("sign in success handler should not be called"); });
botAuth.OnUserSignInFailure((context, state, exception) => { authException = exception; return Task.CompletedTask; });

// act
Expand All @@ -200,7 +200,7 @@ public async void Test_HandleSignInActivity_ThrowException()
var context = MockTurnContext();
var state = await TurnStateConfig.GetTurnStateWithConversationStateAsync(context);
TeamsAIAuthException? authException = null;
botAuth.OnUserSignInSuccess((context, state) => { throw new Exception("sign in success handler should not be called"); });
botAuth.OnUserSignInSuccess((context, state) => { throw new TeamsAIAuthException("sign in success handler should not be called"); });
botAuth.OnUserSignInFailure((context, state, exception) => { authException = exception; return Task.CompletedTask; });

// act
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using Microsoft.Bot.Schema;
using Microsoft.Bot.Schema.Teams;
using Microsoft.Teams.AI.Tests.TestUtils;
using Microsoft.Teams.AI.Exceptions;

namespace Microsoft.Teams.AI.Tests.Application.Authentication.MessageExtensions
{
Expand All @@ -26,7 +27,7 @@ public override Task<TokenResponse> HandleUserSignIn(ITurnContext context, strin
{
if (_signInResponse == null)
{
throw new Exception("HandlerUserSignIn failed");
throw new TeamsAIAuthException("HandlerUserSignIn failed");
}
return Task.FromResult(_signInResponse);
}
Expand All @@ -35,7 +36,7 @@ public override Task<TokenResponse> HandleSsoTokenExchange(ITurnContext context)
{
if (_tokenExchangeResponse == null)
{
throw new Exception("HandleSsoTokenExchange failed");
throw new TeamsAIAuthException("HandleSsoTokenExchange failed");
}
return Task.FromResult(_tokenExchangeResponse);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public void Initialize(Application<TState> app, string name, IStorage? storage =
return;
}

public Task<bool> IsValidActivity(ITurnContext context)
public Task<bool> IsValidActivityAsync(ITurnContext context)
{
return Task.FromResult(_validActivity);
}
Expand All @@ -38,7 +38,7 @@ public IAuthentication<TState> OnUserSignInSuccess(Func<ITurnContext, TState, Ta
return this;
}

public Task<SignInResponse> SignInUser(ITurnContext context, TState state)
public Task<SignInResponse> SignInUserAsync(ITurnContext context, TState state, CancellationToken cancellationToken = default)
{
var result = new SignInResponse(_mockedStatus);
if (_mockedStatus == SignInStatus.Complete)
Expand All @@ -48,7 +48,7 @@ public Task<SignInResponse> SignInUser(ITurnContext context, TState state)
return Task.FromResult(result);
}

public Task SignOutUser(ITurnContext context, TState state)
public Task SignOutUserAsync(ITurnContext context, TState state, CancellationToken cancellationToken = default)
{
return Task.CompletedTask;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -846,9 +846,9 @@ private async Task _OnTurnAsync(ITurnContext turnContext, CancellationToken canc
if (Authentication != null && _startSignIn != null && await _startSignIn(turnContext, cancellationToken))
{
// Should skip activity that does not support sign-in
if (await Authentication.IsValidActivity(turnContext))
if (await Authentication.IsValidActivityAsync(turnContext))
{
SignInResponse response = await Authentication.SignUserIn(turnContext, turnState);
SignInResponse response = await Authentication.SignUserInAsync(turnContext, turnState);
if (response.Status == SignInStatus.Pending)
{
// Requires user action, save state and stop processing current activity
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace Microsoft.Teams.AI
/// <summary>
/// Base class for adaptive card authentication that handles common logic
/// </summary>
public abstract class AdaptiveCardsAuthenticationBase
internal abstract class AdaptiveCardsAuthenticationBase
{
/// <summary>
/// Authenticate current user
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
/// <summary>
/// Handles authentication for Adaptive Cards in Teams using OAuth Connection.
/// </summary>
public class OAuthAdaptiveCardsAuthentication : AdaptiveCardsAuthenticationBase
internal class OAuthAdaptiveCardsAuthentication : AdaptiveCardsAuthenticationBase
{
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -38,16 +38,17 @@ public AuthenticationManager(AuthenticationOptions<TState> options)
/// <param name="context">The turn context</param>
/// <param name="state">The turn state</param>
/// <param name="handlerName">Optional. The name of the authentication handler to use. If not specified, the default handler name is used.</param>
/// <param name="cancellationToken">The cancellation token</param>
/// <returns>The sign in response</returns>
public async Task<SignInResponse> SignUserIn(ITurnContext context, TState state, string? handlerName = null)
public async Task<SignInResponse> SignUserInAsync(ITurnContext context, TState state, string? handlerName = null, CancellationToken cancellationToken = default)
{
if (handlerName == null)
{
handlerName = _default;
}

IAuthentication<TState> auth = Get(handlerName);
SignInResponse response = await auth.SignInUser(context, state);
SignInResponse response = await auth.SignInUserAsync(context, state, cancellationToken);
if (response.Status == SignInStatus.Complete)
{
AuthUtilities.SetTokenInState(state, handlerName, response.Token!);
Expand All @@ -61,15 +62,16 @@ public async Task<SignInResponse> SignUserIn(ITurnContext context, TState state,
/// <param name="context">The turn context</param>
/// <param name="state">The turn state</param>
/// <param name="handlerName">Optional. The name of the authentication handler to use. If not specified, the default handler name is used.</param>
public async Task SignOutUser(ITurnContext context, TState state, string? handlerName = null)
/// <param name="cancellationToken">The cancellation token</param>
public async Task SignOutUserAsync(ITurnContext context, TState state, string? handlerName = null, CancellationToken cancellationToken = default)
{
if (handlerName == null)
{
handlerName = _default;
}

IAuthentication<TState> auth = Get(handlerName);
await auth.SignOutUser(context, state);
await auth.SignOutUserAsync(context, state, cancellationToken);
AuthUtilities.DeleteTokenFromState(state, handlerName);
}

Expand All @@ -79,15 +81,15 @@ public async Task SignOutUser(ITurnContext context, TState state, string? handle
/// <param name="context">Current turn context.</param>
/// <param name="handlerName">Optional. The name of the authentication handler to use. If not specified, the default handler name is used.</param>
/// <returns>True if current activity supports authentication. Otherwise, false.</returns>
public async Task<bool> IsValidActivity(ITurnContext context, string? handlerName = null)
public async Task<bool> IsValidActivityAsync(ITurnContext context, string? handlerName = null)
{
if (handlerName == null)
{
handlerName = _default;
}

IAuthentication<TState> auth = Get(handlerName);
return await auth.IsValidActivity(context);
return await auth.IsValidActivityAsync(context);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace Microsoft.Teams.AI
/// <summary>
/// Base class for bot authentication that handles common logic
/// </summary>
public abstract class BotAuthenticationBase<TState>
internal abstract class BotAuthenticationBase<TState>
where TState : TurnState, new()
{
/// <summary>
Expand Down Expand Up @@ -47,21 +47,22 @@ public BotAuthenticationBase(Application<TState> app, string name, IStorage? sto
app.AddRoute(this.VerifyStateRouteSelector, async (context, state, cancellationToken) =>
{
await this.HandleSignInActivity(context, state, cancellationToken);
});
}, true);

app.AddRoute(this.TokenExchangeRouteSelector, async (context, state, cancellationToken) =>
{
await this.HandleSignInActivity(context, state, cancellationToken);
});
}, true);
}

/// <summary>
/// Authenticate current user
/// </summary>
/// <param name="context">The turn context</param>
/// <param name="state">The turn state</param>
/// <param name="cancellationToken">The cancellation token</param>
/// <returns>The sign in response</returns>
public async Task<SignInResponse> AuthenticateAsync(ITurnContext context, TState state)
public async Task<SignInResponse> AuthenticateAsync(ITurnContext context, TState state, CancellationToken cancellationToken = default)
{
// Get property names to use
string userAuthStatePropertyName = GetUserAuthStatePropertyName(context);
Expand All @@ -76,7 +77,7 @@ public async Task<SignInResponse> AuthenticateAsync(ITurnContext context, TState
});
}

DialogTurnResult result = await RunDialog(context, state, userDialogStatePropertyName);
DialogTurnResult result = await RunDialog(context, state, userDialogStatePropertyName, cancellationToken);
if (result.Status == DialogTurnStatus.Complete)
{
// Delete user auth state
Expand All @@ -87,7 +88,7 @@ public async Task<SignInResponse> AuthenticateAsync(ITurnContext context, TState
// Completed dialog without a token.
// This could mean the user declined the consent prompt in the previous turn.
// Retry authentication flow again.
return await AuthenticateAsync(context, state);
return await AuthenticateAsync(context, state, cancellationToken);
}
else
{
Expand Down Expand Up @@ -125,7 +126,7 @@ public async Task HandleSignInActivity(ITurnContext context, TState state, Cance
try
{
string userDialogStatePropertyName = GetUserDialogStatePropertyName(context);
DialogTurnResult result = await ContinueDialog(context, state, userDialogStatePropertyName);
DialogTurnResult result = await ContinueDialog(context, state, userDialogStatePropertyName, cancellationToken);

if (result.Status == DialogTurnStatus.Complete)
{
Expand Down Expand Up @@ -259,16 +260,18 @@ private void DeleteAuthFlowState(ITurnContext context, TState state)
/// <param name="context">The turn context</param>
/// <param name="state">The turn state</param>
/// <param name="dialogStateProperty">The property name for storing dialog state.</param>
/// <param name="cancellationToken">The cancellation token</param>
/// <returns>Dialog turn result that contains token if sign in success</returns>
public abstract Task<DialogTurnResult> RunDialog(ITurnContext context, TState state, string dialogStateProperty);
public abstract Task<DialogTurnResult> RunDialog(ITurnContext context, TState state, string dialogStateProperty, CancellationToken cancellationToken = default);

/// <summary>
/// Continue the authentication dialog.
/// </summary>
/// <param name="context">The turn context</param>
/// <param name="state">The turn state</param>
/// <param name="dialogStateProperty">The property name for storing dialog state.</param>
/// <param name="cancellationToken">The cancellation token</param>
/// <returns>Dialog turn result that contains token if sign in success</returns>
public abstract Task<DialogTurnResult> ContinueDialog(ITurnContext context, TState state, string dialogStateProperty);
public abstract Task<DialogTurnResult> ContinueDialog(ITurnContext context, TState state, string dialogStateProperty, CancellationToken cancellationToken = default);
}
}
Loading

0 comments on commit 1f7b921

Please sign in to comment.