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

Inconsistent return types in CardActionContext.getSignInUrl() #3439

Closed
stevengum opened this issue Aug 27, 2020 · 2 comments · Fixed by #3459
Closed

Inconsistent return types in CardActionContext.getSignInUrl() #3439

stevengum opened this issue Aug 27, 2020 · 2 comments · Fixed by #3459
Assignees
Labels
bug Indicates an unexpected problem or an unintended behavior.
Milestone

Comments

@stevengum
Copy link
Member

Line 118 returns a Promise, but line 125 returns a string or undefined:

getSignInUrl:
cardAction.type === 'signin'
? () => {
const { value } = cardAction;
if (directLine.getSessionId) {
// TODO: [P3] We should change this one to async/await.
// This is the first place in this project to use async.
// Thus, we need to add @babel/plugin-transform-runtime and @babel/runtime.
return observableToPromise(directLine.getSessionId()).then(
sessionId => `${value}${encodeURIComponent(`&code_challenge=${sessionId}`)}`
);
}
console.warn('botframework-webchat: OAuth is not supported on this Direct Line adapter.');
return value;

This causes an exception in createDefaultCardActionMiddleware() where it expects getSignInUrl to return a Promise:

const popup = window.open();
getSignInUrl().then(url => {
popup.location.href = url;
});

This issue was discovered while trying to use OAuth + Streaming (specifically DL ASE), though directLine.getSessionId may be falsey, the return values should have the same type.


Possible fix:

One solution is to return Promise.resolve(value); instead of return value;

image

FYI @jiruss, @DDEfromOR, @ckkashyap, @anusharavindrar

@stevengum
Copy link
Member Author

As an aside, using this resulted in a partially successful OAuthPrompt flow when using Direct Line ASE. (The signInUrl link worked as expected)

However, attempts to use the magic code post-successful login through the OAuth provider failed.

@stevengum stevengum self-assigned this Sep 10, 2020
@stevengum stevengum added R11 and removed R11 labels Sep 10, 2020
@stevengum stevengum added this to the R11 milestone Sep 10, 2020
@cwhitten cwhitten added R11 bug Indicates an unexpected problem or an unintended behavior. labels Sep 11, 2020
@stevengum
Copy link
Member Author

Update: I've successfully tested this fix along with a fix for microsoft/botbuilder-dotnet#4673 and the workaround mentioned in microsoft/botbuilder-dotnet#4674 with Direct Line ASE and Direct Line Speech. The OAuth flow works as expected.

Note: No magic-code OAuth flows are not supported outside of regular Direct Line with Enhanced Authentication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or an unintended behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants