This repository has been archived by the owner on Jun 30, 2022. It is now read-only.
Multiple Authentication Providers #243
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…to lamil/multi_auth
ryanisgrig
reviewed
Nov 6, 2018
ryanisgrig
reviewed
Nov 6, 2018
solutions/Virtual-Assistant/src/csharp/microsoft.bot.solutions/Authentication/OAuthProvider.cs
Show resolved
Hide resolved
lzc850612
reviewed
Nov 6, 2018
...rtual-Assistant/src/csharp/microsoft.bot.solutions/Authentication/MultiProviderAuthDialog.cs
Outdated
Show resolved
Hide resolved
|
Changed to used connectionName to drive choices. This allows for the bot dev to create more meaningful names for the connections, and allows user to have multiple connections of the same type (i.e. personal Microsoft account and corporate 0365 account)
|
lzc850612
reviewed
Nov 6, 2018
| AddDialog(new WaterfallDialog(nameof(MultiProviderAuthDialog), auth)); | ||
| AddDialog(new ChoicePrompt(DialogIds.ProviderPrompt) { Style = ListStyle.SuggestedAction }); | ||
|
|
||
| foreach (var connection in skillConfiguration.AuthenticationConnections) |
There was a problem hiding this comment.
is it possible skillConfiguration.AuthenticationConnections to be null?
There was a problem hiding this comment.
Added null checks in all the SkillDialogs
lzc850612
reviewed
Nov 6, 2018
|
|
||
| private async Task<DialogTurnResult> PromptForProvider(WaterfallStepContext stepContext, CancellationToken cancellationToken) | ||
| { | ||
| if (_skillConfiguration.AuthenticationConnections.Count() == 1) |
There was a problem hiding this comment.
i think i'd check AuthenticationConnections is null before using it
lzc850612
reviewed
Nov 6, 2018
| catch | ||
| { | ||
| throw; | ||
| } |
There was a problem hiding this comment.
catch block useless if you're just gonna throw
lzc850612
reviewed
Nov 6, 2018
| //await a.SignOutUserAsync(dc.Context, skillConfiguration.AuthConnectionName, dc.Context.Activity.From.Id, default(CancellationToken)); | ||
| if (!_useCachedTokens) | ||
| { | ||
| var a = dc.Context.Adapter as BotFrameworkAdapter; |
There was a problem hiding this comment.
'a' is definitely not the worst variable name ever :)
lzc850612
reviewed
Nov 6, 2018
| @@ -17,7 +19,7 @@ public class CalendarService : ICalendar | |||
| /// <param name="token">the access token.</param> | |||
| /// <param name="source">the calendar provider.</param> | |||
| /// <param name="timeZoneInfo">the user timezone info.</param> | |||
| public CalendarService(string token, EventSource source) | |||
| public CalendarService(string token, EventSource source, GoogleClient config) | |||
There was a problem hiding this comment.
let's think about how to do this in a better way, in a different PR
There was a problem hiding this comment.
Created an issue to track #244
lzc850612
reviewed
Nov 6, 2018
| @@ -18,6 +18,16 @@ | |||
| <CodeAnalysisRuleSet>..\..\VirtualAssistant.ruleset</CodeAnalysisRuleSet> | |||
| </PropertyGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <None Remove="ServiceClients\WindowsIanaMapping" /> | |||
There was a problem hiding this comment.
is this generated by changing the output policy to 'Always'? feel like it's not needed
lzc850612
approved these changes
Nov 7, 2018
darrenj
approved these changes
Nov 7, 2018
lauren-mills
added a commit
that referenced
this pull request
Apr 18, 2019
* consolidated skill case statements * va multi-auth handling * started skill update for multi auth * fixed indentation * updated skill authentication dialog * updated va authentication to include provider in token response * updated logout method * multi auth in skills * updated calendar skill to pull google config from appsettings * updated to use t4 template responses * consolidated skill case statements * va multi-auth handling * started skill update for multi auth * fixed indentation * updated skill authentication dialog * updated va authentication to include provider in token response * updated logout method * multi auth in skills * updated calendar skill to pull google config from appsettings * updated to use t4 template responses * updated appsettings for va * turned useCachedTokens on * Added validator for multi-auth prompts * Added todo service provider back to appsettings * change to use the oauth ConnectionName to drive buttons * stylecop fixes * reverting trivial appSettings change * review fixes * updated email skill test
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This change adds support for multiple authentication providers in the virtual assistant and skills.
Expected Behavior
Related Issue
fixes #19Testing Steps