arabold / serverless-sentry-lib

MIT License
32 stars 15 forks source link

Missing user context when using Cognito, lambda-proxy integration, and Cognito Authorizer #6

Closed TomSeldon closed 4 years ago

TomSeldon commented 6 years ago

When using the default lambda-proxy integration, and the built-in Cognito authorizer, the user context (Cognito user pool, username, etc.) are all undefined.

Looking at the source, this seems to be because you're checking in event.requestContext.identity (if it's set).

In my case, that object exists but the properties are mostly null. The user information is actually within event.requestContext.authorizer (though information like user pool ID doesn't exist).

Example request context object, with redacted data:

{  
   "path":"[REDACTED]",
   "accountId":"[REDACTED]",
   "resourceId":"[REDACTED]",
   "stage":"prod",
   "authorizer":{  
      "claims":{  
         "sub":"[REDACTED]",
         "email_verified":"true",
         "iss":"https://cognito-idp.eu-west-1.amazonaws.com/[REDACTED]",
         "phone_number_verified":"false",
         "cognito:username":"[REDACTED]",
         "given_name":"[REDACTED]",
         "aud":"[REDACTED]",
         "token_use":"id",
         "auth_time":"1509610756",
         "phone_number":"[REDACTED]",
         "exp":"Thu Nov 02 09:19:16 UTC 2017",
         "iat":"Thu Nov 02 08:19:16 UTC 2017",
         "family_name":"[REDACTED]",
         "email":"[REDACTED]"
      }
   },
   "requestId":"[REDACTED]",
   "identity":{  
      "cognitoIdentityPoolId":null,
      "accountId":null,
      "cognitoIdentityId":null,
      "caller":null,
      "apiKey":"",
      "sourceIp":"[REDACTED]",
      "accessKey":null,
      "cognitoAuthenticationType":null,
      "cognitoAuthenticationProvider":null,
      "userArn":null,
      "userAgent":"PostmanRuntime/6.1.6",
      "user":null
   },
   "resourcePath":"[REDACTED]",
   "httpMethod":"GET",
   "apiId":"[REDACTED]"
}

As you can see, the identity object exists but doesn't contain anything that useful.

Result in Sentry:

screen shot 2017-11-02 at 09 45 08

I can put a PR in for this, whereby I pull in user data from claims data, but wanted to check if there's anything you can think of that I'm missing?

Proposal:

Pull in claims['cognito:username'] if it's set, from event.requestContext.authorizer.claims and use that for the username property.

arabold commented 6 years ago

Thank you for your bug report, @TomSeldon . Indeed, I think this isn't handled properly in the current version as I've never used authorizers. But your proposal makes total sense.

Sentry itself defines a set of common user properties: https://docs.sentry.io/learn/context/: id -Your internal identifier for the user. username -The username. Generally used as a better label than the internal ID. email - An alternative to a username (or addition). Sentry is aware of email addresses and can show things like Gravatars, unlock messaging capabilities, and more. ip_address - The IP address of the user. If the user is unauthenticated providing the IP address will suggest that this is unique to that IP. We will attempt to pull this from HTTP request data if available.

These should map very well to the data you receive from the authorizer.claims.

If you have a PR I'm happy to merge it in. Otherwise I can hopefully add this in the next couple of days myself though I would need help with testing it.

TomSeldon commented 6 years ago

Thanks :)

I'll have a closer look at this in the morning and try to get a PR in, but it may have to wait until the weekend.

On 2 Nov 2017 13:44, "Andre Rabold" notifications@github.com wrote:

Thank you for your bug report, @TomSeldon https://github.com/tomseldon . Indeed, I think this isn't handled properly in the current version as I've never used authorizers. But your proposal makes total sense.

Sentry itself defines a set of common user properties: https://docs.sentry.io/learn/context/: id -Your internal identifier for the user. username -The username. Generally used as a better label than the internal ID. email - An alternative to a username (or addition). Sentry is aware of email addresses and can show things like Gravatars, unlock messaging capabilities, and more. ip_address - The IP address of the user. If the user is unauthenticated providing the IP address will suggest that this is unique to that IP. We will attempt to pull this from HTTP request data if available.

These should map very well to the data you receive from the authorizer.claims.

If you have a PR I'm happy to merge it in. Otherwise I can hopefully add this in the next couple of days myself though I would need help with testing it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/arabold/serverless-sentry-lib/issues/6#issuecomment-341425024, or mute the thread https://github.com/notifications/unsubscribe-auth/ADQxzA4RDA_v_ATvyIl4n9ERKlkmUlKXks5sycdUgaJpZM4QPV1J .

TomSeldon commented 6 years ago

Just a note on this, in case you end up picking this up before I get a chance to... (I'm planning on having a look this weekend)

It's a little bit more complicated.

Depending on the token type used (ID token vs. Access token), different information is available, and the username (Congito username) has a different field name.

If it's an access token, then the username is available as username. If it's an ID token, it's available as 'cognito:username'. The ID token also includes additional claims vs. the access token, which may or not be relevant.

So the way I'm probably going to tackle this is refactor out the creation of the user context object and move it to a function/class that takes the event or request context and build up the user context depending on the token type (if that info is available), what details are available, etc. Moving it out to a separate function or class also makes it testable (I can add some test cases and feed it different event objects and assert the correct user context is generated).

arabold commented 4 years ago

Closing as outdated. v2.0.0 has been released. If this is still a problem please create a new issue.