Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take magic code detection out of OAuth BeginDialog #1850

Merged
merged 6 commits into from
Jun 6, 2019

Conversation

mdrichardson
Copy link
Contributor

@mdrichardson mdrichardson commented May 1, 2019

Fixes #1596

Also related: https://stackoverflow.com/questions/55970661/ms-bot-framework-doesnt-remember-authentication/55975228#55975228 - this seems like a pretty easy issue to run into.

Issue Summary

OAuthPrompt has to be called to for certain messages. If that message contains a string that has a 6-digit number, OAuthPrompt used that number as the magic code, which was invalid, so it re-prompted for Login. The JS SDK doesn't have this issue because it doesn't check for the magic code in beginDialog()
.

Changes

  • Removes attempt to detect magic code in BeginDialogAsync()
  • Removes some other checks in GetUserTokenAsync() that are unnecessary because of the above change that are handled elsewhere

Important Notes

  • I couldn't really get the Auth Sample (18) on master to work well with or without this change. The samples-work-in-progress sample works fine now.
  • Neither sample worked in Teams -- I'm not sure if they ever did. the Login button pops up and then nothing happens after clicking it.
  • I rarely use Auth, so a review to make sure this wouldn't break something I'm missing would be much appreciated
  • As you might gather by looking through my commits, I think some code duplication can be avoided by making more calls to OAuthPrompt.GetUserTokenAsync(). @fuselabs was throwing errors when I attempted to do this, however, since it required adding , string code = null to the GetUserTokenAsync() arguments (which would make it match the Node version).

Testing

image

These are using the samples-work-in-progress branch:

Before

image

After

image

@microsoft microsoft deleted a comment from fuselabs May 1, 2019
@microsoft microsoft deleted a comment from fuselabs May 1, 2019
@fuselabs
Copy link
Collaborator

fuselabs commented May 1, 2019

No issues found in Microsoft.Bot.Builder.dll
No issues found in Microsoft.Bot.Builder.AI.Luis.dll
No issues found in Microsoft.Bot.Builder.AI.QnA.dll
No issues found in Microsoft.Bot.Builder.ApplicationInsights.dll
No issues found in Microsoft.Bot.Builder.Azure.dll
No issues found in Microsoft.Bot.Builder.Dialogs.dll
No issues found in Microsoft.Bot.Builder.TemplateManager.dll
No issues found in Microsoft.Bot.Configuration.dll
No issues found in Microsoft.Bot.Connector.dll
No issues found in Microsoft.Bot.Schema.dll

@tomlm tomlm requested review from Jeffders and removed request for Jeffders May 3, 2019 00:53
Copy link
Contributor

@cleemullins cleemullins left a comment

Choose a reason for hiding this comment

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

I'm not commenting on the code correctness, as I don't have context.

... but a code change like this needs a corresponding test change. Please add a Unit Test.

@mdrichardson
Copy link
Contributor Author

mdrichardson commented May 17, 2019

FYI: Still working on writing tests. This was more difficult than anticipated as it took me awhile to discover that TestAdapter returns a valid token regardless of the magic code (if the userId, channelId, and connectionName are the same). This is pretty key for testing this and behaves very differently than production.

@mdrichardson
Copy link
Contributor Author

mdrichardson commented May 20, 2019

@cleemullins Tests complete.

With new OAuthPrompt:

image

With old OAuthPrompt:

image

@coveralls
Copy link
Collaborator

coveralls commented May 20, 2019

Pull Request Test Coverage Report for Build 63129

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+5.0%) to 71.155%

Totals Coverage Status
Change from base Build 56745: 5.0%
Covered Lines: 4344
Relevant Lines: 6105

💛 - Coveralls

@fuselabs
Copy link
Collaborator

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.Luis.dll

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.QnA.dll

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.ApplicationInsights.dll

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Azure.dll

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Dialogs.dll

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.TemplateManager.dll

✔️ No Binary Compatibility issues for Microsoft.Bot.Configuration.dll

✔️ No Binary Compatibility issues for Microsoft.Bot.Connector.dll

✔️ No Binary Compatibility issues for Microsoft.Bot.Schema.dll

Copy link
Member

@Jeffders Jeffders left a comment

Choose a reason for hiding this comment

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

Looks right to me. No magic code should be sent on BeginDialog. Thank you!

@mdrichardson
Copy link
Contributor Author

Note: I believe this actually bring it to parity with JS, since JS doesn't do magic code detection in BeginDialog

@fuselabs
Copy link
Collaborator

fuselabs commented Jun 6, 2019

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.Luis.dll
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.QnA.dll
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.ApplicationInsights.dll
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Azure.dll
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Dialogs.dll
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.TemplateManager.dll
✔️ No Binary Compatibility issues for Microsoft.Bot.Configuration.dll
✔️ No Binary Compatibility issues for Microsoft.Bot.Connector.dll
✔️ No Binary Compatibility issues for Microsoft.Bot.Schema.dll

@cleemullins cleemullins merged commit 7641c92 into master Jun 6, 2019
@cleemullins cleemullins deleted the issue1596OAuthFix branch June 6, 2019 18:34
@mdrichardson
Copy link
Contributor Author

mdrichardson commented Jun 6, 2019

Confirmed: JS SDK does not detect for magic code in beginDialog, which calls its own getUserToken, which also does not attempt to detect magic codes.

oauth_prompt does not exist in Python SDK. "Parity" would be making it exist. Related: microsoft/botbuilder-python#183

ShYuPe pushed a commit to ShYuPe/botbuilder-dotnet that referenced this pull request Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OAuthPrompt requires user credentials when message contains guid or 6-digit number
6 participants