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

Panic while iterating over members of an AAD Group #455

Closed djuneja-uptycs closed 1 year ago

djuneja-uptycs commented 1 year ago

Description:

We are trying to fetch the members of a give AAD Group but we are getting the following issue but it is crashing in the iteration step. Below is the code which we are using:

func (c *clientImpl) GetMemberOfGroup(ctx context.Context, groupId string) ([]models.DirectoryObjectable, error) {
    ctx = azure.CtxWithOperationName(ctx, "GetMemberOfGroup")
    result, err := c.client.GroupsById(groupId).Members().Get(ctx, nil)

    if err != nil {
        return nil, err
    }

    appList := make([]models.DirectoryObjectable, 0)

    pageIterator, err := msgraphcore.NewPageIterator(result, c.adapter, models.CreateDirectoryObjectFromDiscriminatorValue)
    if err != nil {
        return nil, err
    }
    err = pageIterator.Iterate(ctx, func(pageItem interface{}) bool {
        app := pageItem.(models.DirectoryObjectable)
        appList = append(appList, app)
        return true
    })
    if err != nil {
        return nil, err
    }
    return appList, nil
}

We are getting below panic logs:

panic: reflect: call of reflect.Value.IsNil on zero Value

goroutine 6657 [running]:
reflect.Value.IsNil(...)
    /home/ubuntu/.gvm/gos/go1.19.3/src/reflect/value.go:1554
github.com/microsoftgraph/msgraph-sdk-go-core.convertToPage({0xe725480, 0xc005f5dbe0})
    /home/ubuntu/cloudquery/go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go-core@v0.34.1/page_iterator.go:202 +0x44f
github.com/microsoftgraph/msgraph-sdk-go-core.(*PageIterator).next(0xc00100bf20?, {0x100ee8a8?, 0xc0080a4de0?})
    /home/ubuntu/cloudquery/go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go-core@v0.34.1/page_iterator.go:124 +0x4e
github.com/microsoftgraph/msgraph-sdk-go-core.(*PageIterator).Iterate(0xc00100bf20, {0x100ee8a8, 0xc0080a4de0}, 0xc004944500?)
    /home/ubuntu/cloudquery/go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go-core@v0.34.1/page_iterator.go:90 +0x5e
github.com/Uptycs/uptycs-cloudquery/extension/azure/active_directory.(*clientImpl).GetMemberOfGroup(0xc00257d4d0, {0x100ee8a8, 0xc00257cf90}, {0xc003b050b0, 0x24})
    /home/ubuntu/workspace/release-build/extension/azure/active_directory/client.go:677 +0x511

On further inspection, it seems that the panic is coming from the page iterator itself:

func convertToPage(response interface{}) (PageResult, error) {
    var page PageResult

    if response == nil {
        return page, errors.New("response cannot be nil")
    }

    method := reflect.ValueOf(response).MethodByName("GetValue")
    if method.IsNil() {
        return page, errors.New("value property missing in response object")
    }
    value := method.Call(nil)[0]

    // Collect all entities in the value slice.
    // This converts a graph slice ie []graph.User to a dynamic slice []interface{}
    collected := make([]interface{}, 0)
    for i := 0; i < value.Len(); i++ {
        collected = append(collected, value.Index(i).Interface())
    }

    parsablePage, ok := response.(PageWithOdataNextLink)
    if !ok {
        return page, errors.New("response does not have next link accessor")
    }

    page.oDataNextLink = parsablePage.GetOdataNextLink()
    page.value = collected

    return page, nil
}

Graph dependencies we are using:

github.com/microsoft/kiota-authentication-azure-go v0.6.0
github.com/microsoft/kiota-http-go v0.16.0
github.com/microsoft/kiota-serialization-json-go v0.8.2
github.com/microsoftgraph/msgraph-beta-sdk-go v0.54.0
github.com/microsoftgraph/msgraph-sdk-go v0.56.0
github.com/microsoftgraph/msgraph-sdk-go-core v0.34.1
djuneja-uptycs commented 1 year ago

Found the issue, I was giving incorrect object type when creating iterator. I made the below change:

Initial Code:

pageIterator, err := msgraphcore.NewPageIterator(result, c.adapter, models.CreateDirectoryObjectFromDiscriminatorValue)

Changed Code:

pageIterator, err := msgraphcore.NewPageIterator(result, c.adapter, models.CreateDirectoryObjectCollectionResponseFromDiscriminatorValue)

this did the trick, we need to make sure we are giving the correct object to the iterator. Nevertheless, it shouldn't crash IMO. But thanks!