msgraph-sdk-dotnet-core icon indicating copy to clipboard operation
msgraph-sdk-dotnet-core copied to clipboard

ChangeNotificationCollection.AreTokensValid fails with v2 tokens

Open jasonjoh opened this issue 4 years ago • 0 comments

Please provide the following (and please check them off the list with [x]) before submitting this issue:

  • [x] Expected behavior. Please provide links to the specific Microsoft Graph documentation you used to determine the expected behavior.
  • [x] Actual behavior. Provide error codes, stack information, and a Fiddler capture of the request and response (please remove personally identifiable information before posting).
  • [x] Steps to reproduce the behavior. Include your code, IDE versions, client library versions, and any other information that might be helpful to understand your scenario.

Expected behavior

ChangeNotificationCollection.AreTokensValid should succeed when notifications are received from valid Microsoft Graph senders.

When failing validation, ChangeNotificationCollection.AreTokensValid should return false, not throw an exception.

Actual behavior

Receiving a notification for /teams/getAllMessages with encrypted content, the token sent by Microsoft Graph has an issuer like https://login.microsoftonline.com/tenant-id/v2.0. The existing code expects the issuer to be https://sts.windows.net/tenant-id/. You can override the domain portion, but even so, it omits the /v2.0 bit on the end, so issuer validation still fails.

https://github.com/microsoftgraph/msgraph-sdk-dotnet-core/blob/2e43863e349b4b3ebe2e166c26e3afcc4a974365/src/Microsoft.Graph.Core/Extensions/ITokenValidableExtension.cs#L33-L48

Exception:

Microsoft.IdentityModel.Tokens.SecurityTokenInvalidIssuerException: IDX10205: Issuer validation failed. Issuer: 'System.String'. Did not match: validationParameters.ValidIssuer: 'System.String' or validationParameters.ValidIssuers: 'System.String'.
         at Microsoft.IdentityModel.Tokens.Validators.ValidateIssuer(String issuer, SecurityToken securityToken, TokenValidationParameters validationParameters)
         at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.ValidateIssuer(String issuer, JwtSecurityToken jwtToken, TokenValidationParameters validationParameters)
         at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.ValidateTokenPayload(JwtSecurityToken jwtToken, TokenValidationParameters validationParameters)
         at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.ValidateToken(String token, TokenValidationParameters validationParameters, SecurityToken& validatedToken)

Steps to reproduce the behavior

bool areTokensValid = await notifications.AreTokensValid(_tenantIds, _appIds);
bool areTokensValid = await notifications.AreTokensValid(_tenantIds, _appIds, issuerPrefix: "https://login.microsoftonline.com/");

Recommended fix

  • To maintain compatibility with v1 tokens, add both https://sts.windows.net/tenant-id/ and https://login.microsoftonline.com/tenant-id/v2.0 to the issuersToValidate (by default).
  • Allow callers to pass an array of issuer prefixes.
  • Wrap call to ValidateToken in try/catch and return false if exception is raised. Or, if bubbling the exception to the caller is desired, change this method from async Task<bool> AreTokensValid to something like async Task ValidateTokens, with the intent that the method completes if the are valid, throws if not.

jasonjoh avatar Sep 13 '21 15:09 jasonjoh