microsoftgraph / msgraph-sdk-java

Microsoft Graph SDK for Java
https://docs.microsoft.com/en-us/graph/sdks/sdks-overview
MIT License
370 stars 125 forks source link

Change notification (subscription lifecycle events) with an `organizationId` not fully parsed #1551

Open jonathan-grs opened 12 months ago

jonathan-grs commented 12 months ago

Expected behavior

Subscription Lifecycle events (and possibly others) that contain an organizationId property instead of a tenantId property should be fully parsed.

Actual behavior

When a subscription lifecycle notification such as the following is sent, organizationId property not being parsed (does not exist in the ChangeNotification class):

{
    "value": [
        {
            "lifecycleEvent": "reauthorizationRequired",
            "subscriptionId": "subscription-id-uuid",
            "resource": "",
            "clientState": "client-state",
            "sequence": null,
            "resourceData": {
                "@odata.type": "#microsoft.graph.subscription",
                "@odata.id": "subscriptions/subscription-id-uuid",
                "id": "resource-id-uuid"
            },
            "encryptedContent": null,
            "organizationId": "organization-id-uuid",
            "subscriptionExpirationDateTime": "2023-09-05T01:03:01.889254-07:00"
        }
    ],
    "validationTokens": [
        "validation-toke-value"
    ]
}

Steps to reproduce the behavior

As it is unclear (to me at least) when change notifications are sent with the organizationId property instead of the tenantId property, there is no consistent reproduction scenario.

ramsessanchez commented 11 months ago

Hi @jonathan-grs , do you have some specific examples as to where you are running into this scenario where a changeNotification is being returned with the organizationId property? As far as I can see there isn't any entity type in our metadata that has an OrganizationId property. The documentation (here)[https://learn.microsoft.com/en-us/graph/webhooks-lifecycle?tabs=http#structure-of-a-lifecycle-notification] states that the structure includes a tenantId not an organizationId. If you could please share where you are seeing this scenario occur that would allow me to better uncover what is going on. Thanks!

jonathan-grs commented 10 months ago

Hi @ramsessanchez, the only thing I have is a specific change notification we've got, it does not happen frequently and I am aware that the docs barely has any info about it, but here is one reference in the change notification context.

Here's the notification (quoted in the description as well):

{
    "value": [
        {
            "lifecycleEvent": "reauthorizationRequired",
            "subscriptionId": "0ba8361d-38de-4090-8c55-1eb48eea3755",
            "resource": "",
            "clientState": "80160887-75d5-4e9b-874a-c3e5d82cc918",
            "sequence": null,
            "resourceData": {
                "@odata.type": "#microsoft.graph.subscription",
                "@odata.id": "subscriptions/0ba8361d-38de-4090-8c55-1eb48eea3755",
                "id": "0ba8361d-38de-4090-8c55-1eb48eea3755"
            },
            "encryptedContent": null,
            "organizationId": "2533157c-209b-41fb-b5e1-679ebe8ff0e5",
            "subscriptionExpirationDateTime": "2023-09-05T01:03:01.889254-07:00"
        }
    ],
    "validationTokens": [
        "eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsIng1dCI6Ii1LSTNROW5OUjdiUm9meG1lWm9YcWJIWkdldyIsImtpZCI6Ii1LSTNROW5OUjdiUm9meG1lWm9YcWJIWkdldyJ9.eyJhdWQiOiI4MjI3MDMxNC1lYTQ0LTRiNzAtYmVmNS0zOGRkYzE5YjA1MTciLCJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC8yNTMzMTU3Yy0yMDliLTQxZmItYjVlMS02NzllYmU4ZmYwZTUvIiwiaWF0IjoxNjkzODk5NzkwLCJuYmYiOjE2OTM4OTk3OTAsImV4cCI6MTY5Mzk4NjQ5MCwiYWlvIjoiRTJGZ1lGZ3VzZG01M2xqTlZhVG9TRVBoaXNXZkFRPT0iLCJhcHBpZCI6IjBiZjMwZjNiLTRhNTItNDhkZi05YTgyLTIzNDkxMGM0YTA4NiIsImFwcGlkYWNyIjoiMiIsImlkcCI6Imh0dHBzOi8vc3RzLndpbmRvd3MubmV0LzI1MzMxNTdjLTIwOWItNDFmYi1iNWUxLTY3OWViZThmZjBlNS8iLCJvaWQiOiJiMTMwZjE5My04NDZhLTQ4ZjItOGUxNi1mZDNmYzVjNTAzMTIiLCJyaCI6IjAuQVgwQWZCVXpKWnNnLTBHMTRXZWV2b193NVJRREo0SkU2bkJMdnZVNDNjR2JCUmVhQUFBLiIsInN1YiI6ImIxMzBmMTkzLTg0NmEtNDhmMi04ZTE2LWZkM2ZjNWM1MDMxMiIsInRpZCI6IjI1MzMxNTdjLTIwOWItNDFmYi1iNWUxLTY3OWViZThmZjBlNSIsInV0aSI6IjRYZ3RWdFlJemt1ZWJBX0loeEktQUEiLCJ2ZXIiOiIxLjAifQ.c3nb_Pc78jvsE5jgAQkJ9vCjIE9pAkCBXiY_B7U21HioyYj2-29gDZbNRY78KbTdPEf3nj43FSvYgVScP-D08_XMFvvw2sss0UuBC_sEQoo2KHH1xVa0CqVb00dJ5NfNn43Jq9SnQqXSy7Y9P0HbsZib0196kTj_sjPgTsMB6Ml-Rr9ZUHDW4roX5dw4teReox6Dj0rK9CybLtZRX_egfUodz66_OZdL9rA41E12N8rO7gm3jVHFvQxrPA3eKJxarBA3Z6Ih9xOaHQv_CiQOa3UiCMfUi70JxhNC0bQCqpYbjDtQA6v-swNOi9F4HjJpgqsSGY0k08i6wM-cq6EoeA"
    ]
}
ramsessanchez commented 10 months ago

Hi @jonathan-grs, I see what you mean. This documentation seems to be quite confusing as both organization and tenant id property have the example value "{Organization/Tenant id}" which is not clear. This is likely a documentation issue, I believe [this] (https://learn.microsoft.com/en-us/graph/api/resources/changenotification?view=graph-rest-1.0) is the documentation which is more consistent with the sdk metadata. When information is included that isn't specified/isn't in the class it should appear in the additionalData hashmap which is a part of all entities in the SDK.

jonathan-grs commented 10 months ago

Hi @ramsessanchez - Thanks for the link, I understand what the configuration says, but we actually got a notification with the organizationId field instead of the tenantId field which I pasted above. The SDK does not parse these type of notifications and we need to intercept all incoming notifications and identify whether we got one or the other before letting the SDK continue the parsing.

BGourlayHerring commented 7 months ago

Hi, is there any update on the above? I too am confused by the missing tenantId but inclusion of an organizationId. E.g. this lifecycle notification I received:

{
    "value": [
        {
            "lifecycleEvent": "reauthorizationRequired",
            "subscriptionId": "<subscription_guid>",
            "resource": "Subscriptions/<subscription_guid>",
            "clientState": "<secretstate>",
            "sequence": null,
            "resourceData": {
                "@odata.type": "#microsoft.graph.subscription",
                "@odata.id": "subscriptions/<subscription_guid>",
                "id": "<subscription_guid>"
            },
            "encryptedContent": null,
            "organizationId": <tenant_id_guid>,
            "subscriptionExpirationDateTime": "2024-02-08T04:18:58.0463343-08:00"
        }
    ]
}

This documentation states we shoiuld be getting a tenantId: https://learn.microsoft.com/en-us/graph/change-notifications-lifecycle-events?tabs=http#structure-of-a-lifecycle-notification

We're now writing validation against the organizationId so would be good to know which way this is going to go in future, update docs or update notification payload?

baywet commented 7 months ago

Note: with the latest major version this type is being trimmed. @andrueastman do you want to look into restoring it for Java like you did for dotnet. Everyone, thanks for your patience on the matter, we were focusing on the new major release.