Add missing tenantid documentid

[sonalideshpandemsft]

This PR ensures that tenant ID and document ID are captured and logged even in cases where the server times out or returns a 499 error. Previously, these values were missing as shown in the below query

https://dataexplorer.azure.com/clusters/frspme.westus2/databases/FRS?query=H4sIAAAAAAAAA1WOQYvCQAyF7%2F6K0JOCIHsV6mVB9OIu1j8QZ1I70JnpJpmKsj9%2BOy2uekrIe9972R6rnWp3pJ9EojL7hWtDTEA9BT05PxzRd7ApAS9x%2FmEX744DeoKyhOIlpAAMFjxpE%2B2ofX9Vp%2BKfE%2BLemYnCtmayE0ChdxyDH1JHreNok1EXw5PtUJtPVLpEvo2mlY0mZURWa6WAQff2pSoZQyJ1arO5xlboqSlqkjxY5eq0gaIaPiPOuCTvkd2dwMQUdL6A8w0e%2BUt4lOZ9yvkDp%2FRrfEcBAAA%3D

FRSHttpRequests
| where eventTimestamp >= ago(1d)
| where eventName == "HttpRequest" and method == "POST"
| where service == "alfred" and environment == "production"
| where pathCategory == "/documents/:tenantId"
| where successful == false
| where status startswith "Server"
| summarize count() by tenantId, documentId, status

[tianzhu007]

How did you get this logic? Thanks. :)

[sonalideshpandemsft]

Here is the function: https://github.com/microsoft/FluidFramework/blob/main/server/routerlicious/packages/services-client/src/historian.ts#L42 :)

[manishgargd]

On service side, this file is used for token validation logic

FluidFramework/server/routerlicious/packages/services-utils/src/auth.ts

Line 188 in 62554e4

export function extractTokenFromHeader(authorizationHeader: string): string {

[yunho-microsoft]

As I mentioned in below comment, alfred and historian have different token format. So it could break existing code.

[manishgargd]

In addition to "basic", it can also be "Bearer"

[znewton]

A few perf related concerns here, especially with regards to a recent testing result that revealed performance degradations potentially caused by parsing tokens in this way.

[znewton]

I've recently been looking at this code with @shubhi1092 with regards to slow downs caused by parsing tokens with large documentIds. One of the theories is that the greedy regexs here could be to blame. Alternative code was proposed that avoided regexs:

export function parseToken(
 tenantId: string,
 authorization: string | undefined,
): string | undefined {
 let token: string | undefined;
 if (authorization) {
  if (!authorization.startsWith("Basic ")) {
   throw new NetworkError(403, "Malformed authorization token");
  }
  const base64TokenMatch = authorization.replace("Basic ", "");
  const decoded = Buffer.from(base64TokenMatch[1], "base64").toString();
  const tokenMatch = decoded.split(":");
  if (tokenMatch.length !== 2 || tenantId !== tokenMatch[0]) {
   throw new NetworkError(403, "Malformed authorization token");
  }
  token = tokenMatch[1];
 }
 return token;
}

I'd suggest that we use this instead.

[znewton]

I'm not sure we should be throwing 403's here in the "Logging middleware," mostly out of principle. I.e. the telemetry code shouldn't also be enforcing auth.

[znewton]

Parsing and decoding is not free, and we'll now be doing this twice on every request instead of once. It would be ideal if we could instead simply add a property to the response object where we are already doing parse/decode. That can be read on request end before logging.

[znewton]

Misclicked approve. Requesting changes to reset

[yunho-microsoft]

I remember tokens for Alfred and Historian are in a little different format. Alfred token is just the plain string (Bearer tokenString) while historian tokens is first encoded in base64, which means you need to decode first. It means the parsing logic will be different. If this logic is shared for both services, please test it locally. I am not sure if we can to change it to the same format since it is hard to change client behavior.
There is a tokenRevocationTestTool.ts in FRS repo you can check for reference about how to manually send the request (different token format). Please help the confirm if behavior is still the same.

[znewton]

I had this same comment, then deleted it because I noticed that this is in an if block and the Alfred parsing is below it 😆

[yunho-microsoft]

@znewton I guess you are talking about this line "if (authorization.startsWith("Basic "))" and "if (authorization.startsWith("Bearer "))"
I just did a clicker test, at least clicker shows alfred request header is like
"Authorization: Basic <JWT token>

And Historian token is like
Authorization: Basic <base64 encoded string>
And after decode, it will be this format: <tenantId>:<JWT token>

So both alfred and historian token parsing will enter the same block (startWith Basic) for parsing and alfred token parsing will fail. I do remember we use "Bearer " for token but not sure why it is now basic.