microsoft / azure-devops-extension-sdk

Client SDK for developing Azure DevOps extensions
MIT License
123 stars 38 forks source link

In *some* cases - 'nameid' claim does not contain user id in getAppToken() call #16

Open jochenvanwylick opened 5 years ago

jochenvanwylick commented 5 years ago

Hi,

We're using the method getAppToken() (https://docs.microsoft.com/en-us/javascript/api/azure-devops-extension-sdk/#getapptoken--) for authenticating to our backend, as described in this document: https://docs.microsoft.com/en-us/azure/devops/extend/develop/auth?view=azure-devops#authenticating-requests-to-your-service. When validating the generated token in our backend, we get a claim nameid that contains a Guid. In most cases, this is the user Id of the logged in user. However, in approximately 1 out of 10 cases, we see that the nameid claim from the token does not correspond to the user Id of the user. Instead, it is some other Guid (we don't know what).

We haven't been able to find a pattern in cases where it does / does not work correctly :( can you provide guidance how to pinpoint the issue ?

Here's a link to the referenced method: https://github.com/microsoft/azure-devops-extension-sdk/blob/4f44715f582e173e6fd35ebcf97ed33d50e64dec/src/SDK.ts#L341

coliveiragrupomult commented 4 years ago

Hi,

I have the same problem, did you solve it?

mpseidel commented 4 years ago

Hey there - I had the same problem. In short - there are different identifiers that you can get depending on where you look.

I recommend reading about descriptors and storage keys https://docs.microsoft.com/en-us/rest/api/azure/devops/graph/?view=azure-devops-rest-5.1

If no one from the team responds here I'll try to summarize what I learned when I get around to it.

yasen-yankov commented 4 years ago

Our team had the same issue while developing a custom extension. We contacted MS support and managed to solve the problem. It turns out that the "nameid" claim is not meant to store the Azure DevOps user Id. It is only a coincidence that it does for a lot of users.

The way to get the current user Id is to call the "https://dev.azure.com/{{orgname}}/_apis/connectionData" endpoint. In our extension, we send the AppToken and AccessToken to the server-side function, which validates the AppToken and uses the AcessToken to call the "connectionData" endpoint. We also have AAD integration and do a follow up call to "https://vssps.dev.azure.com/{{orgname}}/_apis/graph/users/{{subjectDescriptor}}" to get the descriptor for the user and acquire the AAD user Id.

sandeepiiit commented 3 years ago

This discussion is so helpful. The documentation misleadingly says:

getAppToken()
Fetch an token which can be used to identify the current user

However, AccessToken + connectData API is what lets you actually identify the current user, not the app token.

pelizza commented 2 years ago

Just sharing a related issue open on the community forums: https://developercommunity.visualstudio.com/t/user-id-from-sdkgetapptoken-doesnt-match-the-user/1610412

This information is really great, thanks a lot! 🎉

I would argue though that the appToken should indeed store the user ID. Having the "correct" user ID in the token is essential for any authorization checks on the backend side for extensions. One can always call a second API to then get the "correct" user ID and match against any information coming from the frontend that requires validation, but this adds a lot of overhead and is not a standard security design.

So, although the proposed workarounds work, I'd argue that this issue should be left open until we hear from Microsoft.

Additionally, the proposed workarounds won't work for Azure DevOps Server, given there is no way for an extension's backend to call that API...

Ebbelink commented 3 months ago

Is there anything going on on this issue in the past years? Two year later and the documentation has not been updated while the nameid also does not properly reflect the user id? I would love to get some pointers on what the recommended way and location of solving this would be. Because as mentioned before having a backend do this for every request is not really a known standard practice