add oauth support and an oauth prompt #520
Conversation
| @@ -0,0 +1,95 @@ | |||
| using System; | |||
There was a problem hiding this comment.
Needs copyright(s) at the top. #Resolved
|
|
||
| namespace Microsoft.Bot.Builder.Dialogs | ||
| { | ||
| public class OAuthPromptSettingsWithTimeout : prompts.OAuthPromptSettings |
There was a problem hiding this comment.
XML Code Comments are needed on all public classes and all public methods. #Resolved
| @@ -0,0 +1,95 @@ | |||
| using System; | |||
| using prompts = Microsoft.Bot.Builder.Prompts; | |||
There was a problem hiding this comment.
Why the import like this? Just do a standard using and avoid the "prompts." everywhere. #Resolved
| using static Microsoft.Bot.Builder.Prompts.PromptValidatorEx; | ||
| using Microsoft.Bot.Schema; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.Bot.Builder.Prompts; |
There was a problem hiding this comment.
Sort the usings. Be sure the "System" usings are at the top, and custom usings are last. There's a VS Setting for this. #Resolved
| public int? Timeout { get; set; } | ||
| } | ||
|
|
||
| public class OAuthPromptState : PromptOptions |
There was a problem hiding this comment.
XML Comment.s #Resolved
| throw new ValidationException(ValidationRules.CannotBeNull, "userId"); | ||
| } | ||
|
|
||
| bool _shouldTrace = ServiceClientTracing.IsEnabled; |
There was a problem hiding this comment.
same comments around "_" variables. #Resolved
| { | ||
| if (connectionName == null) | ||
| { | ||
| throw new ValidationException(ValidationRules.CannotBeNull, "connectionName"); |
There was a problem hiding this comment.
Why isn't this just "ArgumentNullException()" ? #Resolved
| return String.Empty; | ||
| } | ||
|
|
||
| public async Task SendEmulateOAuthCardsAsync(bool emulateOAuthCards) |
There was a problem hiding this comment.
More of the same comments. #Resolved
| /// OAuthCard ContentType value | ||
| public partial class OAuthCard | ||
| { | ||
| public const string ContentType = "application/vnd.microsoft.card.oauth"; |
There was a problem hiding this comment.
Useful code comments. :) #Resolved
There was a problem hiding this comment.
this is good enough though right? it's pretty much just defining a constant (contenttype) for this class).
In reply to: 184823915 [](ancestors = 184823915)
| @@ -0,0 +1,34 @@ | |||
| using System; | |||
There was a problem hiding this comment.
//Copyright #Resolved
|
|
||
| var cards = activity.Attachments.Where(a => a.Content is OAuthCard); | ||
| if (cards.Count() == 0) | ||
| throw new InvalidOperationException("OAuthPrompt.Prompt(): atleast one of the cards should be an oauth card"); |
There was a problem hiding this comment.
"atleast" => "at least"
| /// <returns></returns> | ||
| public async Task Prompt(ITurnContext context, string text, string speak = null) | ||
| { | ||
| if (context == null) |
There was a problem hiding this comment.
There is a BotAssert. Method that will do this for you. Applies through for context==null checks.
| { | ||
| if (context == null) | ||
| throw new ArgumentNullException(nameof(context)); | ||
| if (string.IsNullOrEmpty(text)) |
There was a problem hiding this comment.
You probably mean IsNullOrWhitespace()
|
|
||
| await context.SendActivity(text, speak).ConfigureAwait(false); | ||
|
|
||
| var adapter = context.Adapter as BotFrameworkAdapter; |
There was a problem hiding this comment.
This (and the similar check above) is going to make Testing really hard.
Because we test via the "TestAdapter" I suspect you'll want to factor the necessary bits into an interface "IOAuthAdapter" and allow both the Bot Framework adapter and the Test adapter to implement.
| { | ||
| Uri uriResult; | ||
| if (!(Uri.TryCreate(uri, UriKind.Absolute, out uriResult) && uriResult.Scheme == Uri.UriSchemeHttps)) | ||
| throw new InvalidOperationException("Please supply a valid https uri"); |
| ServiceClientTracing.Enter(invocationId, this, "GetUserTokenAsync", tracingParameters); | ||
| } | ||
| // Construct URL | ||
| var tokenUrl = new Uri(new Uri(uri + (uri.EndsWith("/") ? "" : "/")), "api/usertoken/GetToken?userId={userId}&connectionName={connectionName}{magicCodeParam}").ToString(); |
There was a problem hiding this comment.
Please move all scary magic strings into constants with detailed comments on them.
There was a problem hiding this comment.
i want to keep this logic as such for now, since it's in sync wtih v3 sdk and node as well (v3 and v4) . i will update all sdk's to use a proper uribuilder for this logic in another review.
| return tokenResponse; | ||
| } | ||
| catch (JsonException) | ||
| { |
There was a problem hiding this comment.
This exception handling seems... incorrect. If it's right, please add comments in here.
| } | ||
| else if (statusCode == HttpStatusCode.NotFound) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
I can imagine tracing here would be very handy for debugging.
| /// <returns></returns> | ||
| public async Task<bool> SignOutUserAsync(string userId, string connectionName, CancellationToken cancellationToken = default(CancellationToken)) | ||
| { | ||
| if (string.IsNullOrEmpty(userId)) |
There was a problem hiding this comment.
You've got these all throughout the code. Consider adding:
private static void AssertValidUserId(string userId)
{
if (string.isNullOrWhitespace(userId)) throw new ArgumentNullExcpetion(nameof(userId))
}
There was a problem hiding this comment.
it's in just a couple of places and more descriptive, so i dont think it's worth encapsulating in anohter method.
|
|
||
| namespace Microsoft.Bot.Schema | ||
| { | ||
| public class TokenExchangeState |
|
Please make sure to rebase against master, as there's been drift in Prompts / Dialog classes since you started. |
add oauth support and an oauth prompt
* exporting teams from botbuilder schema * black:exporting teams from botbuilder schema
the following changes are part of this PR -