microsoftgraph / msgraph-sdk-go

Microsoft Graph SDK for Go
https://docs.microsoft.com/en-us/graph/sdks/sdks-overview
MIT License
245 stars 38 forks source link

querying ServicePrincipals with a filter #40

Closed joelddiaz closed 2 years ago

joelddiaz commented 3 years ago

I've been kicking the tires with this new (new to me) graph package, and I've managed to get some bits working, but I'm failing to be able to list ServicePrincipals with filters defined. Following the same pattern I used for creating an Application, when I try it with a ServicePrincipal I get no results (and an error if you dig into the actual response variable returned).

func ensureAppRegistrationExists(name string) (*graph.Application, error) {
    filter := fmt.Sprintf("displayname eq '%s'", name)
    listQuery := &applications.ApplicationsRequestBuilderGetOptions{
        Q: &applications.ApplicationsRequestBuilderGetQueryParameters{
            Filter: &filter,
        },
    }

    resp, err := graphClient.Applications().Get(listQuery)
    if err != nil {
        log.Fatalf("Error listing Apps: %+v\n", err)
    }

    switch len(resp.GetValue()) {
    case 0:
        log.Printf("No existing App found, will create one...")
        newApp := graph.NewApplication()
        newApp.SetDisplayName(&name)
        postOpts := &applications.ApplicationsRequestBuilderPostOptions{
            Body: newApp,
        }

        app, err := graphClient.Applications().Post(postOpts)
        if err != nil {
            log.Fatalf("Error creating app: %+v\n", err)
        }
        fmt.Printf("Created App: %+v\n", *app.GetAppId())
        return app, nil
    case 1:
        app := resp.GetValue()[0]
        log.Printf("Found single App with displayname %s: %s", name, *app.GetAppId())
        return &app, nil
    default:
        for _, app := range resp.GetValue() {
            fmt.Printf("APP: %+v\tNAME: %+v\n", *app.GetAppId(), *app.GetDisplayName())
        }
        return nil, fmt.Errorf("unexpect number of apps found: %d", len(resp.GetValue()))
    }
}

func ensureServicePricipalExists(name string) error {
    appReg, err := ensureAppRegistrationExists(name)
    if err != nil {
        log.Fatalf("FAIL :%+v\n", err)
    }

    filter := fmt.Sprintf("appId eq '%s'", *appReg.GetAppId())
    listQuery := &serviceprincipals.ServicePrincipalsRequestBuilderGetOptions{
        Q: &serviceprincipals.ServicePrincipalsRequestBuilderGetQueryParameters{
            Filter: &filter,
        },
    }
    resp, err := graphClient.ServicePrincipals().Get(listQuery)
    if err != nil {
        log.Fatalf("failed to list ServicePrincipals: %+v\n", err)
    }

    switch len(resp.GetValue()) {
    case 0:
        newSP := graph.NewServicePrincipal()
        newSP.SetAppId(appReg.GetAppId())
        newSP.SetDisplayName(appReg.GetDisplayName())
        query := &serviceprincipals.ServicePrincipalsRequestBuilderPostOptions{
            Body: newSP,
        }
        sp, err := graphClient.ServicePrincipals().Post(query)
        if err != nil {
            log.Fatalf("fialed to create SP: %+v\n", err)
        }
        fmt.Printf("Created new SesrvicePrincipal: %+v\n", sp)
        return nil
    case 1:
        sp := resp.GetValue()[0]
        log.Printf("Found single ServicePrincipal with displayname %s: %s", *sp.GetDisplayName(), *sp.GetAppId())
        return nil
    default:
        return fmt.Errorf("Unexpected number of serviceprincipals found: %d", len(resp.GetValue()))

    }
}

When digging into the resp variable returned from the ServicePrincipals().Get(), I see an error code of Request_BadRequest and a more detailed message of "Unrecognized query argument specified: 'filter'."

Because there is no error returned, it just tries to re-create a new ServicePrincipal, and that also doesn't error, but the HTTP response is a 4xx b/c the ServicePrincipal already exists from previous runs through this code.

Based on my reading of the Service Principal section of https://docs.microsoft.com/en-us/graph/aad-advanced-queries , I believe that a query of appId eq '<VALUE>' should "just work".

I stepped down through the raw http.Request that gets built by the graphsdk, and it all appears correct to my eyes.

Here are the versions of the modules I'm currently using:

[jdiaz@minigoomba test (master *)]$ cat go.mod | grep -E 'graph-sdk|kiota'
        github.com/microsoft/kiota/authentication/go/azure v0.0.0-20211130101617-a4871ba0f35f
        github.com/microsoftgraph/msgraph-sdk-go v0.3.0
        github.com/microsoft/kiota/abstractions/go v0.0.0-20211129093841-858bd540489b // indirect
        github.com/microsoft/kiota/http/go/nethttp v0.0.0-20211112084539-17ac73ffdc7c // indirect
        github.com/microsoft/kiota/serialization/go/json v0.0.0-20211112084539-17ac73ffdc7c // indirect
        github.com/microsoftgraph/msgraph-sdk-go-core v0.0.2 // indirect
replace github.com/microsoft/kiota/http/go/nethttp => github.com/microsoft/kiota/http/go/nethttp v0.0.0-20211130101617-a4871ba0f35f

Have I messed something up, or is there something in the graphsdk (or its dependencies) that is keeping this from working?

baywet commented 3 years ago

Hi @joelddiaz Thanks for reaching out and for trying the SDK. The code you've shared so far looks correct.

Let's run through a few tests to understand what's going on here:

Can you confirm the URL of the request is https://graph.microsoft.com/v1.0/servicePrincipals?filter=appId eq '<id>' ? In which case it should be https://graph.microsoft.com/v1.0/servicePrincipals?$filter=appId eq '<id>' (see the dollar sign)

The dollar sign is added by a custom handler and I believe it's missing in your application pipeline's configuration, it's usually added by default.

Can you share how you setup the client/request adapter (without sharing any secret) please?

Lastly, can you run go get -u at the root of the project to make sure all the dependencies are up to date and run the code again please? Thanks!

joelddiaz commented 3 years ago

go get -u did upgrade a number of packages, but still the issue persists

go get: upgraded github.com/microsoft/kiota/abstractions/go v0.0.0-20211129093841-858bd540489b => v0.0.0-20211201125630-3501743a5dc5
go get: upgraded github.com/microsoft/kiota/authentication/go/azure v0.0.0-20211130101617-a4871ba0f35f => v0.0.0-20211201125630-3501743a5dc5
go get: upgraded github.com/microsoft/kiota/http/go/nethttp v0.0.0-20211112084539-17ac73ffdc7c => v0.0.0-20211201125630-3501743a5dc5
go get: upgraded github.com/microsoft/kiota/serialization/go/json v0.0.0-20211112084539-17ac73ffdc7c => v0.0.0-20211201125630-3501743a5dc5
go get: upgraded github.com/microsoftgraph/msgraph-sdk-go-core v0.0.2 => v0.0.4
go get: upgraded github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4 => v0.0.0-20210911075715-681adbf594b8
go get: upgraded golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2 => v0.0.0-20211117183948-ae814b36b871
go get: upgraded golang.org/x/net v0.0.0-20211123203042-d83791d6bcd9 => v0.0.0-20211201190559-0a0e4e1bb54c
go get: upgraded golang.org/x/sys v0.0.0-20211025201205-69cdffdb9359 => v0.0.0-20211124211545-fe61309f8881

I put some breakpoints on those handlers responsible for adding the $, and it only triggered while performing the Applications().Get() query, but not on the ServicePrincipals().Get() query.

I'm attaching the smallest bit of fully buildable code that should show the issue I'm running into.

main.go.txt

joelddiaz commented 3 years ago

I think I see what's going on. Those custom handlers only run once because the index that iterates through the list of middleware interceptors never gets reset to zero. So that's why my first query succeeds, but all subsequent queries fail. This got things working:

diff --git a/vendor/github.com/microsoft/kiota/http/go/nethttp/pipeline.go b/vendor/github.com/microsoft/kiota/http/go/nethttp/pipeline.go
index 6129829..beb20c4 100644
--- a/vendor/github.com/microsoft/kiota/http/go/nethttp/pipeline.go
+++ b/vendor/github.com/microsoft/kiota/http/go/nethttp/pipeline.go
@@ -37,8 +37,13 @@ func (pipeline *middlewarePipeline) incrementMiddlewareIndex() {
        pipeline.middlewareIndex++
 }

+func (pipeline *middlewarePipeline) resetMiddlewareIndex() {
+       pipeline.middlewareIndex = 0
+}
+
 // Next moves the request object through middlewares in the pipeline
 func (pipeline *middlewarePipeline) Next(req *nethttp.Request) (*nethttp.Response, error) {
+       defer pipeline.resetMiddlewareIndex()
        if pipeline.middlewareIndex < len(pipeline.middlewares) {
                middleware := pipeline.middlewares[pipeline.middlewareIndex]

Whether or not that's the "proper" fix is something I suppose will be hashed out in that repo.

baywet commented 2 years ago

Thanks for the deep investigation on this issue and for starting a PR over kiota. I have made additional contributions to your PR and authored a corresponding PR in the go core repo. Once that second PR gets merged, it'll close this issue.

baywet commented 2 years ago

FYI the updates have been merged and released, please run go get -u at the root of your project to get the latest version of the packages.