microsoftgraph / microsoft-graph-toolkit

Authentication Providers and UI components for Microsoft Graph 🦒
https://docs.microsoft.com/graph/toolkit/overview
Other
943 stars 300 forks source link

[BUG] Mgt Tasks component throws error when adding assignee in GCCH tenant #2141

Closed v-prigunasek closed 1 year ago

v-prigunasek commented 1 year ago

The Tasks component is throwing error in GCCH tenant while creating a task with Assignee.

MicrosoftTeams-image

I see the user object id comes in a different format (userObjectID@tenantID) from People API in GCCH tenant , because of that malformed format we are facing issues when assigning user to a task.

MicrosoftTeams-image (1)

MicrosoftTeams-image (2)

ghost commented 1 year ago

Hello v-prigunasek, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

musale commented 1 year ago

@v-prigunasek thank you for trying out the sovereign cloud support! So, the mgt-people component uses the /users/{userId} endpoint when it is used in the mgt-tasks component to render the assignees.

https://github.com/microsoftgraph/microsoft-graph-toolkit/blob/main/packages/mgt-components/src/graph/graph.user.ts#L214

Is that URL supposed to be different when using the GCCH tenant?

v-prigunasek commented 1 year ago

Hi @musale,

Thanks for your response. Yes, the URL is different in GCCH. I have made some code changes to pass the base URL as "https://graph.microsoft.us/" to create a graphclient object. We are using mgt-people picker and mgt-tasks in our application.

The issue with these 2 components were with the user id that comes in a different format from me/people API in GCCH tenant. I had issue in assigning the selected people in the people picker as well, but as a workaround, I was able to truncate the tenant id that comes along with the user id and it worked fine.

selectedUsersArr.push({ displayName: user.displayName.replace(",", ""), userPrincipalName: user.userPrincipalName, id: user.id.includes("@") ? user.id.split("@")[0] : user.id });

Whereas for the people picker inside the mgt-tasks component, I am not able to apply that workaround as we dont have much control over the Add Task element in that mgt-tasks.

image

On click of Add task, it is trying to create a task with the Assignments in a malformed userId and it got failed with the following error. { "error": { "code": "", "message": "The request is invalid:\r\nOne or more property annotations for property '6a284ebc-ee5c-4fa7-b2c9-642af1234a7c' were found in the complex value without the property to annotate. Complex values must only contain property annotations for existing properties.", "innerError": { "date": "2023-03-20T16:48:20", "request-id": "a324f715-e434-484e-9913-06b00fd832eb", "client-request-id": "13f39d5d-f22b-8355-98e6-3bc2f47befec" } } }

image

Please let me know if you need any additional information. Thanks!

sebastienlevert commented 1 year ago

To me, this seems to be on the service side that the versions of the API doesn't match. I would be against having specific code to handle multiple clouds, as this would be super difficult to maintain. Thoughts on this @musale and @gavinbarron? How can we support such scenarios (and I'm also thinking v3 with support for other clouds...) in a sustainable way?

gavinbarron commented 1 year ago

Lots of thoughts, the first is that the id of a user is the id of a user no matter what is in that string value, and we should treat them as opaque values that we never tamper with, with the exception of calling encodeUriComponent when passing them as part of a URL.

The second major thought is how we test these scenarios. we'll need to do some digging internally to get access to a test tenant so that we can test changes and fixes specific to GCCH.

The third main thought is that it appears that the call which is failing here is actually the POST to planner, which suggests a workload level issue.

musale commented 1 year ago

Lots of thoughts, the first is that the id of a user is the id of a user no matter what is in that string value, and we should treat them as opaque values that we never tamper with, with the exception of calling encodeUriComponent when passing them as part of a URL.

The second major thought is how we test these scenarios. we'll need to do some digging internally to get access to a test tenant so that we can test changes and fixes specific to GCCH.

The third main thought is that it appears that the call which is failing here is actually the POST to planner, which suggests a workload level issue.

I agree with Gavin on this. Especially tampering with the IDs that should be a last resort. It'll bring more ideas to cater for special cases like this when we experiment with a tenant presenting us these challenges.