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

Change WindowsProvider permission scope separator to space #193

Merged
merged 1 commit into from
Dec 15, 2022

Conversation

Richasy
Copy link
Contributor

@Richasy Richasy commented Jun 17, 2022

Fixes #192

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Failed when trying to grant AAD multiple permissions

What is the new behavior?

Grant AAD account multiple permissions normally

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented Jun 17, 2022

Thanks Richasy for opening a Pull Request! The reviewers will test the PR and highlight if there is any merge conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@Richasy Richasy changed the title Change permission scope separator to space Change WindowsProvider permission scope separator to space Jun 17, 2022
@shweaver-MSFT
Copy link
Member

@Richasy can you share some more details about the failure/error you are seeing? It seems like you are fixing a bug, but it's hard to tell if this could be breaking functionality. I think understanding the error will help us figure out which it is.

@Richasy
Copy link
Contributor Author

Richasy commented Jun 22, 2022

@shweaver-MSFT Here are the error message:

Login failed: Failed
Login failed: 3399614466: AADSTS65002: Consent between first party application '{my-app-client-id}' and first party resource '00000003-0000-0000-c000-000000000000' must be configured via preauthorization - applications owned and operated by Microsoft must get approval from the API owner before requesting tokens for that API.
Trace ID: 72841998-32b2-46ec-ad49-e804e9d22000
Correlation ID: 9bb2e503-2bbe-4cb2-acb2-3b854f60cbe0
Timestamp: 2022-06-22 02:49:19Z

Code here:

// string[] RequestScopes = { "User.Read", "Tasks.ReadWrite" };
var webAccountProviderConfig = new WebAccountProviderConfig(WebAccountProviderType.Any, Constants.GraphClientId);
var authProvider = new WindowsProvider(RequestScopes, webAccountProviderConfig);

I hide the ClientId.
It seems that AAD provider and MSA provider treat scope a little differently. We expected two permissions, A and B, but after connecting with a comma, AAD provider recognized it as one permission, namely A,B, obviously we do not have this permission, so the error message returned is not pre-authorized

@michael-hawker michael-hawker added this to the 7.1.3 milestone Aug 24, 2022
@michael-hawker michael-hawker added bug 🐛 Something isn't working Area: Providers labels Aug 24, 2022
@michael-hawker michael-hawker self-assigned this Oct 11, 2022
Copy link
Member

@Arlodotexe Arlodotexe left a comment

Choose a reason for hiding this comment

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

Did some digging through the source code for MSAL. They mostly deal with scopes in an IEnumerable<string>, but
this extension method is used to create the scopes string for many of the requests types:

image

@michael-hawker
Copy link
Member

image

Posting the image of that function here as well for full context, nice find @Arlodotexe, still would be good to know where that's called out in the docs. Following up with the Graph doc team.

@michael-hawker michael-hawker merged commit fce6356 into CommunityToolkit:main Dec 15, 2022
@michael-hawker
Copy link
Member

Ah, got pointed to the doc finally here: https://learn.microsoft.com/en-us/azure/active-directory/develop/v2-protocols-oidc#send-the-sign-in-request

A space-separated list of scopes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Providers bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WindowsProvider uses deprecated range separator
4 participants