microsoftgraph / msgraph-sdk-powershell

Powershell SDK for Microsoft Graph
https://www.powershellgallery.com/packages/Microsoft.Graph
Other
690 stars 165 forks source link

BUG: ConsistencyLevel: Eventual header should be enabled for advanced queries or all results will be returned regardless #513

Open JustinGrote opened 3 years ago

JustinGrote commented 3 years ago

https://developer.microsoft.com/en-us/identity/blogs/microsoft-graph-advanced-queries-for-directory-objects-are-now-generally-available/ The consistencylevel: eventual header should automatically be set if parameters such as -Search, -Count, -OrderBy, and -Filter if an operation parameter is used.

Workaround

Invoke-GraphRequest

-Headers @{ConsistencyLevel='eventual'}

AutoRest-Generated Commands

-ConsistencyLevel Eventual This should be safe for general profile usage as well in substitute for doing the above: $PSDefaultParameterValues['Get-Mg*:ConsistencyLevel'] = 'Eventual'

AB#7724

JustinGrote commented 3 years ago

Using Fiddler, I did the exact same query with Graph Explorer (which works) and the module (which returns all results, note the body size is smaller).

The URL are identical so it is likely something in the request headers: image

JustinGrote commented 3 years ago

Here are the header differences, going to start stepping through them with Invoke-Restmethod until I find which one is causing the issue image

JustinGrote commented 3 years ago

Found it! The ConsistencyLevel: Eventual header is missing which is required for the advanced queries. https://developer.microsoft.com/en-us/identity/blogs/microsoft-graph-advanced-queries-for-directory-objects-are-now-generally-available/

JustinGrote commented 3 years ago

Also referenced here in the OpenApi Spec: https://github.com/microsoftgraph/msgraph-sdk-powershell/blob/7399ffc1e490a7cfca01392f2f74aad8b488f047/openApiDocs/v1.0/Applications.yml#L16-L25

peombwa commented 3 years ago

@JustinGrote ConsistencyLevel is available via -ConsitencyLevel parameter to all commands that support it. In this case, you should use it like this:

 Get-MgServicePrincipal -Search "displayName:teams" -ConsistencyLevel "eventual"
JustinGrote commented 3 years ago

@peombwa yeah I've expressed that however since this is now GA functionality in the graph, it should be done automatically for these advanced search parameters, especially since it doesn't warn you at all and simply returns all objects. Unless you have a lot of documentation including a massive disclaimer up front in the readme, there's no reason the module shouldn't just silently do this for you.

You're going to have a lot of users doing -Filter and having it returning all results and trying to figure out what is wrong and eventually giving up and saying your module is broken rather than full investigation that I did :)

peombwa commented 3 years ago

I'll discuss this with the service team that owns this API and get back to you on this.

JustinGrote commented 3 years ago

Thanks, I should also note that you basically cannot use these parameters at all anyways unless you specify it, so it is effectively not optional, and is described as such in the GA announcement as well.

Licantrop0 commented 3 years ago

Hi @JustinGrote, I'm the PM for the advanced query capabilities on Directory Objects. I feel your frustration, and it's shared also among many other API developers (not just PowerShell users).

As the Identity APIs (and AAD architecture in general) is based on a distributed and high availability system, we would like to make our developers to consciously choose between Session Consistency (default value for Identity APIs) and Eventual Consistency. The advanced queries capabilities are only eventually consistent because the indexes for those objects are residing on separate servers requiring synchronization (hence, being eventual consistent). There are consequences that may break pre-existing apps if we add that header to all the calls.

Note: we are in discussion of removing the count querystring param requirement after a major architecture work currently in design phase, but the production release for this is few quarters away. After that, we would consider if we can also send the ConsistencyLevel headers automatically for all the queries that are not supported otherwise, and explicitly calling it out in our documentation, but for now please remember to add in your CmdLets: -ConsistencyLevel eventual -Count CountVar

JustinGrote commented 3 years ago

Thanks for the response @Licantrop0

production release for this is few quarters away.

So consider for your module users that they are using the switches like -Search, -ExpandProperty, -OrderBy, -Count which all require consistencylevel Eventual to work otherwise no filtering occurs. If you can present me a scenario where this is not the case I'd like to see it, but that's not how the GA advanced search announcement reads.

If you want to make it explicit then if these switches are not specified with-consistencylevel eventual, you should display an error or at least a warning that the searches will be ineffective and return all results without consistencylevel Eventual. Even better, you should make them into a separate parameterset, and if that parameterset is called, consistencylevel eventual is a mandatory switch.

As-Is, a user who does the reasonable thing of specifying the filters is going to get non-filtered results without any kind of notification or indication what the problem is, and don't you think that is

far more dangerous

than potentially getting non-eventually consistent results? At best they get frustrated and give up using your module if they don't find the note about eventual consistency buried deep in some docs or announcement somewhere. At worst they go to production not knowing their filters arent really filtering and causing havoc.

IF you still insist on preserving the existing behavior, you should at least update the help for every one of these parameters to say that consistencyLevel eventual is mandatory for those parameters to actually work, but IMHO that's pretty dumb UI design as long as it is mandatory anyways for the parameters to work. IF that changes and filters will work with a different consistency level, then that makes sense, but right now it does not.

darrelmiller commented 3 years ago

@JustinGrote The Microsoft Graph API review team insisted that Eventual consistency had to be opted into. It would be unacceptable to allow teams to introduce different levels of consistency across Microsoft Graph APIs as the default. I understand that having to always specify the header is annoying, but it is the lesser of the evils.

I would love to see the various capabilities that are enabled via eventual consistency, supported regardless of the level of consistency. Ideally, the consistency header should only control the consistency not what features are available.

I do think that the PowerShell SDK should consider supporting a global default, much like your extension has done. That would mean you could set the consistency value once and not for each command.

JustinGrote commented 3 years ago

@darrelmiller I then strongly suggest that you add a check if -Search, -ExpandProperty, -OrderBy, -Count are in use and eventualconsistency is not specified, that you throw an error or at the very least a warning. Otherwise you're going to have a lot of very unhappy users who have no idea why their searches aren't working.

darrelmiller commented 3 years ago

@JustinGrote That would be a decision the API team would need to make. PowerShell SDK is not going to introduce new behavior here.

JustinGrote commented 3 years ago

@darrelmiller I understand that you're trying to keep it as transparent to the API as possible, but you don't even think introducing a write-warning here which won't actually impact the command from running wouldn't be prudent? Maybe even write-host if you're trying to be as "low touch" as possible? Both of which are easily suppressed.

Last point I'll make on this and I'll respect your decision, thank you for engaging in the discussion.

jhoneill commented 3 years ago

The problem here is auto generation produces an SDK (note the name of this repo) but a "blank project" from that SDK is shipped as a PowerShell module. The resulting commands are "developer-ready" not "user-ready".

The PowerShell commands should be a wrapper around the API, which turns it into something fit-for-users. ( @darrelmiller - the API team don't specify the user interface for things which call their API - like a Get-ServicePrincipal command)

 [                      user                        ] 
===== User Interface  - command & parameters  ===== 
[Application program to present/manipulate objects ] 
====== Application Program[ming] interface  ======
[         External facing service                  ]
========== Internal interfaces ===================
[ Low-level internal services (storage, auth etc)  ] 

In fact "application program" is split in two, there are private cmdlets, and public proxy functions, but instead of those functions doing the things a user would expect - building filters, adding headers etc - they are just a short cut for someone who knows how to call the API to call it it. Imagine if git needed you to understand it's protocol / api in-order to switch to a new branch, or granting permissions to a local file on your C: drive needed you to know the SID of the user or group, or sending an email required an understanding of SMTP.

The problem (per the conversation @darrelmiller , @JustinGrote and I were having on twitter) is that a module is going out into the world which is totally API centric, it's taking the API spec and presenting it without doing any of the things a user would expect - it's the job of a programmer put knowledge of the API into a program users run so the user doesn't need it - missing out the "what would a user of this command expect", (UX) specification stage and the programmer input results in this kind of thing, and putting it right requires significant effort. In the same way the notes in the API spec are not user-ready-help. It needs text which is appropriate to the audience.

darrelmiller commented 3 years ago

I'm going to say something that you probably are not going to like, but I figure you will forgive me for being honest about how I feel. PowerShell users are not users. They are developers. You can call them IT admins, or IT Pros, I don't care, they provide commands to instruct computers to perform actions. They batch them into scripts, which are programs. There's no difference in what a developer using another programing language does. PowerShell developers don't require an easier programming interface because somehow they aren't as smart as developers who call an HTTP API. The warning that Justin is suggesting should be provided, also should be provided to the dev calling the HTTP API. PowerShell developers don't need special treatment. All developers should get that assistance.

The PowerShell SDK should try and present the API using conventions that feel natural to PowerShell developers. It should try and enable PowerShell capabilities like pipelining. It should provide default list formats. But higher level scenario cmdlets is not something that is a special requirement for PowerShell. Scenario based tasks are something that we have architected for in other programming languages too. Every programming language wants to have a better experience than they currently have, and they deserve it. We are going to work towards that.

PowerShell is a first class SDK for my team. But it isn't going to get extra special treatment over other programming languages because of the suggestion that PowerShell devs somehow are not as capable as other devs. That's just not true in my experience.

I respect the fact that PowerShell developers are vocal advocates for better DX. We will continue trying to bring better DX, but if we can, we will do it at the API level so that it can benefit all the developers of Graph, regardless of the chosen programming language.

jhoneill commented 3 years ago

I'm going to say something that you probably are not going to like, but I figure you will forgive me for being honest about how I feel. PowerShell users are not users. They are developers. You can call them IT admins, or IT Pros, I don't care, they provide commands to instruct computers to perform actions. They batch them into scripts, which are programs. There's no difference in what a developer using another programing language does.

It's not a matter of like or dislike. People who work with PowerShell are not a homogenous group but a spectrum,. I've worked with people who see themselves as developers and wish PowerShell was more like C# , and with people who who will never write a .PS1 file and just want to open a window, and paste in commands rather than follow what-to-click instructions in the GUI. This second group don't care if the command is running python (like the AZ commands), is a compiled .exe, or a PowerShell script. They want a command to issue at a prompt and do not care if the prompt is cmd, bash, windows PowerShell, Powershell core or even something like netsh.

I know my bias is to think of the world as predominantly [non programming] IT Pros - for the first four years of PowerShell's life I was working at Microsoft as evangelist to the IT pro audience. I expect someone with a job title like "API architect" to think of the world as predominantly people who call APIs. Accepting that there is a spectrum and we're each looking at different parts of it will make for a better discussion. We talked about this before on twitter.

@darrel_miller Oct 7, 2020 It is absolutely true that my team are not experienced PowerShell users. Just like we are not Objective-C, PHP, Python or Ruby users. But we care about all of our users, and we will listen to those of you who are experts and do what we can to fix issues.

Is it unfair to say that not having anyone on the team representing PowerShell users, you're assuming the part of the spectrum which looks like yourselves is all there is ? I think I'm qualified to talk about the rest of the spectrum: when server 2008 shipped Hyper-V without PowerShell support I wrote it and got 250,000 downloads from codeplex. The ImportExcel module is pushing 700K downloads with a run-rate of about 25,000 a month and that's got more of my code than anyone else's. I've contributed chapters to books on making PowerShell usable, talk at conferences on it etc..

Outside of intune and planner, people aren't downloading the Graph modules. There's a choice between your module for users (say) and Az.Resources one , the Teams one, or or AzureAD one (all of which are pre-loaded in cloud shell), the Msonline one and various community ones - yours isn't being chosen or recommended (even by your colleagues inside MSFT).

PowerShell developers don't require an easier programming interface because somehow they aren't as smart as developers who call an HTTP API.

Their requirement comes from using PowerShell to work with things which are not their primary domains of expertise. I work with VMs, Network components (like firewalls), SQL databases, PowerApp deployments, Selenium scripts, unit tests, user and group accounts, OS patches (and 101 other bits of app and OS configuration), scheduled tasks, X509 certificates, work-items in Azure Devops (both content and specification of types), Excel sheets, not to mention remote-control of my camera and analysing meta data in pictures. The job of the PowerShell modules is to be the shoulders for me to stand on - embodying the intelligence turn an "I want to" like "Find users named smith" to the necessary API calls, do the processing of results and give me an answer.

The PowerShell SDK should try and present the API using conventions that feel natural to PowerShell developers.

Yes.

It should try and enable PowerShell capabilities like pipelining.

No What ? Have I gone mad ? It is not the SDK's job to implement pipelines. That's the job of the command. And where you are going wrong is that you a shipping a module of commands which are don't start with "As a user I want to" and do work the to make the API call , and make sense of what comes back. You're starting with "The API offers this, so that's what what command will do" the job of the command is to embody intelligence over and above the API. It's the commands which say "Oh we got a bunch of things from the pipeline, make 10 API calls", it's the commands which say "He's given me 'name'" so I'll run

("`$filter=startswith(displayName,'{0}') or startswith(givenName,'{0}') or startswith(surname,'{0}') or startswith(mail,'{0}') or startswith(userPrincipalName,'{0}')" -f $Name )

And add the result to the API call I'm using. (Yes I actually wrote that here https://github.com/jhoneill/MsftGraph/blob/master/User.ps1 nearly two years ago)

[the sdk] should provide default list formats.

yes and classes which describe the different kinds of objects which get returned (which it does). As an SDK it's good. As a module ... not so much.

But higher level scenario cmdlets is not something that is a special requirement for PowerShell. Scenario based tasks are something that we have architected for in other programming languages too. Every programming language wants to have a better experience than they currently have, and they deserve it. We are going to work towards that.

Ah. The job of a module is to do the scenario based tasks. The job of the SDK is to let someone write the module. Somewhere SDK started delivering scenario based tasks (PowerShell commands in a module on the gallery) which didn't align with the scenarios users need.

PowerShell is a first class SDK for my team. But it isn't going to get extra special treatment over other programming languages because of the suggestion that PowerShell devs somehow are not as capable as other devs. That's just not true in my experience.

Like I said you're only considering a subset of PowerShell users, writing for those who see themselves as devs who will spend all day every day working with your API. You also need to look at the commands you're putting on the gallery. If you were running Get-mgServicePrincipal in cmd or in bash (ok it would be gmgsp in bash :-) would it be acceptable to ask the user to enter -consitency "eventual" when they specify -orderby - of course not. It's fine for your private cmdlet to demand it from the command the user interacts with.

ToddKlindt commented 3 years ago

I agree with @jhoneill and @JustinGrote. I spend of my days doing stuff in PowerShell against M365. It's my bread and butter. I want to love the Graph API PowerShell, but it's just too damned hard. Like James said, in every situation where I need to do something if any other module can do it I'll do it there before I'll try the Graph API. In all of the other modules I can send a beginner to a blog post that shows how to get a task done, and with one, maybe two cmdlets, they can get the task done and get on with their day. That would never be the case with the Graph API. I was super excited when it came out, it was going to open up all kinds of opportunities for me to get things done inside of M365. But it's just so much different than every other PowerShell module that it just takes too much time. I give up. PowerShell users are not developers. On a near daily basis I help Admins do their jobs by giving them one and two cmdlet fixes for issues they have. Most of them couldn't tell you what API stands for, or be able to explain it off the top of their head. They don't know JS, or Python or any other structured language. In their heads PowerShell is like CMD. You want to perform a task, you run the command that performs that task. That's it. Some of us have the luxury of moving past that and writing scripts, functions, modules, etc, but that is a small subset of the PowerShell using community as I see it. @darrelmiller , you said PowerShell users don't deserve special treatment. I don't feel like we want special treatment. We want the Graph API to work like the other PowerShell modules that Microsoft and the community create. We want our existing PowerShell skills to be applicable to the Graph API so we can do our jobs.

JustinGrote commented 3 years ago

I mean clearly the compromise is to treat this module purely as an SDK (though I don't see a ton of value it offers over just importing the C# types and using those in Powershell for "developers") and that there should be overlay modules that makes things more friendly. I assume most people will want a version of the AzureAD module (e.g. MgAzureAD, MSGraphAD, whatever), but this could be used as the base for a Outlook Module, a new Sharepoint module, etc.

However without those, Powershell users are left with major adoption/usage gaps

Obviously that is an ambitious scope that takes a lot of work however.

I would expect at least the AzureAD team to produce an equivalent module since their existing module/API has been deprecated. The remaining ones ideally would come from the respective teams, though community efforts could exist there as well.

jhoneill commented 3 years ago

@justin.

I mean clearly the compromise is to treat this module purely as an SDK (though I don't see a ton of value it offers over just importing the C# types and using those in Powershell for "developers"

I think most of the value is in the those types, but if I want to my own "Get-GraphUser" command it is easier to wrap one or more of the private cmdlets used by Get-MgUser than it is build the URLs and call invoke-MgGraphRequest (which in turn saves all the trouble of managing sessions authentication etc compared with calling Invoke-RestMethod).

there should be overlay modules that makes things more friendly. I assume most people will want a version of the AzureAD module (e.g. MgAzureAD, MSGraphAD, whatever), but this could be used as the base for a Outlook Module, a new Sharepoint module, etc.

However without those, Powershell users are left with major adoption/usage gaps

Yes!!. I've (obviously) been quite vocal about the gaps, overlay modules are the way to close them.

Obviously that is an ambitious scope that takes a lot of work however.

Doing it for every single call would be a massive task. But the need isn't uniform. Some things don't need an overlay (Get-MgOrganization for example just wants better help, but functionally it's fine.) Some things are very rarely called and we wouldn't bother writing a command - auto-generation adds them. The ones that are crying out to be done will tend to have people wanting to do them. MSFT folk can go it alone, work with the community, or just leave it to the community entirely. The AzureAD team should do theirs but if they don't the community will (probably) plug the gap. Since this project owns the modules on the PSGallery, really it means groups in MSFT and members of the community both bring their overlay parts here. That's how it has worked with https://github.com/MethodsAndPractices/vsteam - another sprawling REST API , but there commands are only being added as Scenario-based-tasks, not API calls. The effort to curate the contributions is still significant (as that project has found). Whether @darrelmiller 's team has the capacity to do the curation job is another question - maybe they should hire someone for that ? :-)

JustinGrote commented 2 years ago

Just as a follow-up, at least maybe handle the errors and make a suggestion for the consistencylevel/count. This is the most user-hostile UI I've ever seen, how are you supposed to figure this out if you don't already know? I'm sure people are wasting hours on this every day. image

fume commented 2 years ago

i just noticed that not all the cmdlet supports the -ConsistencyLevel parameter. For example, i'm just trying to use the following Get-MgServicePrincipalOauth2PermissionGrant -ServicePrincipalId $spId -Filter "consentType eq 'Principal'" -CountVariable myCount -ConsistencyLevel eventual and i got the error Get-MgServicePrincipalOauth2PermissionGrant: A parameter cannot be found that matches parameter name 'ConsistencyLevel'.

while, as an example, i was able to run the following Get-MgServicePrincipal -All -Filter "appOwnerOrganizationId ne $TenantId" -CountVariable spCount -ConsistencyLevel eventual

Using this workaround doesn't work, since the cmdlet doesn't have the ConsistencyLevel parameter at all $PSDefaultParameterValues['Get-Mg*:ConsistencyLevel'] = 'Eventual' so i had to use the Invoke-GraphRequest with -Headers parameter.

I'm not asking to add the header automagically to every call, but at least having the -ConsistencyLevel param for all the cmdlets so we can leverage the cmdlets instead of creating requests manually with Invoke-GraphRequest.

peombwa commented 2 years ago

@fume, -ConsistencyLevel is missing on Get-MgServicePrincipalOauth2PermissionGrant and other related commands due to the header missing in the CSDL metadata.

We will address the missing parameter in https://github.com/microsoftgraph/msgraph-metadata/issues/108.

fume commented 2 years ago

@fume, -ConsistencyLevel is missing on Get-MgServicePrincipalOauth2PermissionGrant and other related commands due to the header missing in the CSDL metadata.

We will address the missing parameter in microsoftgraph/msgraph-metadata#108.

Thanks @peombwa, i was suspecting this was related to some sort of metadata. I see the PR https://github.com/microsoftgraph/msgraph-metadata/pull/165 is already merged. Do you know when the MsGraph Powershell module will be updated accordingly?

thanks a lot!

JustinGrote commented 10 months ago

As an update, since I filed this issue, the graph API actually now spots several (but not all) of these situations and returns errors https://learn.microsoft.com/en-us/graph/aad-advanced-queries?tabs=http#error-handling-for-advanced-queries-on-directory-objects

However the errors are very graph specific and do not guide a user to what their problem is. I would recommend the Microsoft Graph Module intercept these error types and rewrite them with better information specific to Microsoft PowerShell.

In Lieu of that, PowerShell 7.4 has the new feedback provider feature, it would be feasible to have a feedback provider also produce this information.

jhoneill commented 10 months ago

As an update, since I filed this issue, the graph API actually now spots several (but not all) of these situations and returns errors https://learn.microsoft.com/en-us/graph/aad-advanced-queries?tabs=http#error-handling-for-advanced-queries-on-directory-objects

However the errors are very graph specific and do not guide a user to what their problem is. I would recommend the Microsoft Graph Module intercept these error types and rewrite them with better information specific to Microsoft PowerShell.

After doing some work for a new client in recent weeks I've found graph's behaviour is as maddening as ever.

This particular client has in excess of 100K users (but less than 1M) and a similar number of groups, and hoped they could do the equivalent of Select Name, Azure_Guid, Employee_ID, Mail, Member_Of From Users where Mail is not null.

Attempts to do this produced a variety of errors. TBH I think trying to anticipate them - especially, as you say the set evolves - can at best catch the most common.

zaaj commented 8 months ago

I have to say, it's gratifying to hear others have similar frustrations with the Microsoft.Graph PowerShell module(s) (ie, I'm not alone)

My vote would be for the consistency level=eventual option to be automatically added when parameters that require it are present. In fact, to one commenter's point about there being different first class languages accessing the APIs, how about modify the API to automatically assume the user wants the consistency level to be set to eventual if they presented options which require it?
Are there scenarios where one can run an API call with and without this option, and get different results? If not, why ever present the option to an API consumer?

My own use of PowerShell is mostly doing admin work at the command line. I rarely ever open ADUC anymore, or the DHCP or DNS RSAT tools, finding the power of the shell compelling, most of the time.

The PowerShell modules for these areas do a great job at allowing the objects returned by a get- command be used as a working InputObject by a Set- or Remove-* command.

This is in stark contrast to the -MgUser and -MgGroup commands, which take a -UserId or -GroupId parameter but spit out objects with no properties with those name, using "Id" instead

Sure, we can use a foreach-object loop and add a "-UserID $_.Id" in the Process scriptblock, but none of the other PowerShell modules require that. Sure I can write scripts to do these types of things, but the vast majority of my work is one-off one-liners, not worth writing a script for.

I DO appreciate the module helping to handle authentication and sessions, but even there, I find it disturbing from a security standpoint. With AzureAD, if I exit my terminal tab, I'm no longer authenticated to EntraID with my admin account, but if I exit after Connect-MgGraph, I can even fully reboot, and my next Connect-MgGraph win not prompt for credentials and use the last-used credentials (yet somehow -Mg commands still need me to connect-mggraph first?)

Also, why do so many graph modules work one Connector-MgGraph has been issued, but Intune needs Connect-MsGraph instead?

Sigh.

JustinGrote commented 8 months ago

I have created a feedback provider that detects most of the known situations where this occurs: https://github.com/JustinGrote/MgAqDetect image