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

experimental/sso-with-skills Sample Does not handle the Invoke Activity from OAuth Prompt #2658

Closed
HobbyProjects opened this issue Aug 8, 2020 · 15 comments
Assignees
Labels
ExemptFromDailyDRIReport Use this label to exclude the issue from the DRI report. feature-request A request for new functionality or an enhancement to an existing one.

Comments

@HobbyProjects
Copy link

HobbyProjects commented Aug 8, 2020

Github issues for C# /JS / Java/ Python should be used for bugs and feature requests. Use Stack Overflow for general "how-to" questions.

Sample information

  1. Sample type: experimental
  2. Sample language: dotnetcore
  3. Sample name: sso-with-skills

Describe the bug

After the "Sign In" card is displayed and the the login prompt is displayed and completed, we will get an "Invoke" activity which we should send back to MainDialog to complete OAuthPromot login process. The current sample does not handle the Invoke activity and login is never completed. To be precise, this callback needs to be added to TeamsActivityHandler implementation

protected override async Task OnTeamsSigninVerifyStateAsync(ITurnContext<IInvokeActivity> turnContext, CancellationToken cancellationToken)
        {
            // The OAuth Prompt needs to see the Invoke Activity in order to complete the login process.
            await _dialog.RunAsync(turnContext, _conversationState.CreateProperty<DialogState>(nameof(DialogState)), cancellationToken);
        }

To Reproduce

Steps to reproduce the behavior:

  1. Run the sample
  2. Notice the login process never completes

Expected behavior

Give a clear and concise description of what you expected to happen.
Login process completes.

Screenshots

If applicable, add screenshots to help explain your problem.

Additional context

Add any other context about the problem here.

[bug]

@carlosscastro carlosscastro added Bot Services Required for internal Azure reporting. Do not delete. Do not change color. customer-reported Issue is created by anyone that is not a collaborator in the repository. Support labels Aug 10, 2020
@jwiley84
Copy link
Contributor

@dmvtech +Dana

@dmvtech
Copy link
Collaborator

dmvtech commented Aug 13, 2020

I'm checking to see if I can simply reproduce.

@dmvtech
Copy link
Collaborator

dmvtech commented Aug 18, 2020

Still working on repro. Hitting some speedbumps getting it all set up.

@dmvtech dmvtech added the customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. label Aug 18, 2020
@pavolum
Copy link

pavolum commented Aug 31, 2020

@dmvtech any update on this?

@dmvtech
Copy link
Collaborator

dmvtech commented Sep 2, 2020

Still working on it. Having some issues on Teams (my own issues). I hope to have an better update by the end of the day.

@dmvtech
Copy link
Collaborator

dmvtech commented Sep 4, 2020

I can reproduce this, but I wonder if we want to consider this a bug. I know that with samples, we try and be as narrow as possible in the completeness of the sample/example. As we already have an example on how to do Teams auth (That does handle the Invoke Activity), that whether that needs to be added to this sample (for the sake of thoroughness) or not. Not every sample should work in every scenario. They are meant to be small, concise examples that convey small aspects and approaches within SDK.

@pavolum Can you bring this up in triage and see if this is a change/addition/fix that should be implemented?

@HobbyProjects We do accept contributions from the community. Would you be interested in creating a pull request with the fix if this is an accepted approach? (if not, no worries)

@GeoffCoxMSFT
Copy link
Member

@swagatmishra2007 - can you take a look at this?

@tdurnford
Copy link
Contributor

@gabog @clearab What are your thoughts on either updating the sample, leaving the sample as is, or leaving the sample as is and adding a new sample to handle the user's scenario?

@clearab
Copy link
Contributor

clearab commented Sep 23, 2020

@tdurnford Assuming this is the only change required to make SSO work in Teams, I think adding it to this sample should be sufficient. If we want to make it a bit more explicit we could comment it out, and add a "Running in Teams" section to the readme to uncomment the code. A similar treatment may make sense for our other authentication samples (consolidate the Teams one with the "normal" auth sample).

@lzc850612
Copy link

@swagatmishra2007 do you have any update on this issue? Thanks.

@swagatmishra2007
Copy link
Member

I agree with @clearab above, but that increases the scope a bit, including testing. So i would like to discuss this at the triage meeting

@swagatmishra2007 swagatmishra2007 added the feature-request A request for new functionality or an enhancement to an existing one. label Oct 8, 2020
@stevengum
Copy link
Member

@swagatmishra2007 and @clearab can the two of you sync and come up with an actionable plan for this sample?

@chrimc62 chrimc62 added the ExemptFromDailyDRIReport Use this label to exclude the issue from the DRI report. label Oct 30, 2020
@clearab clearab removed ExemptFromDailyDRIReport Use this label to exclude the issue from the DRI report. customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. customer-reported Issue is created by anyone that is not a collaborator in the repository. labels Oct 30, 2020
@cwhitten
Copy link
Member

cwhitten commented Nov 9, 2020

ping @clearab

@clearab clearab removed their assignment Nov 9, 2020
@clearab
Copy link
Contributor

clearab commented Nov 9, 2020

Are there still open questions about this? I'd like to see this get picked up and see the sample updated in-line with the comments above. @johnataylor can you help find the appropriate dev for implementation?

@mrivera-ms mrivera-ms added ExemptFromDailyDRIReport Use this label to exclude the issue from the DRI report. and removed Bot Services Required for internal Azure reporting. Do not delete. Do not change color. Support labels Nov 11, 2020
@gabog
Copy link
Contributor

gabog commented Jan 10, 2022

Closing this one for now, we will address it as part of SSO for teams, there's another PR for SSO with skills here

@gabog gabog closed this as completed Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ExemptFromDailyDRIReport Use this label to exclude the issue from the DRI report. feature-request A request for new functionality or an enhancement to an existing one.
Projects
None yet
Development

No branches or pull requests