labd / django-cognito-jwt

An Authentication backend for Django Rest Framework for AWS Cognito JWT tokens
MIT License
177 stars 59 forks source link

USER_MODEL.objects.get_or_create_for_cognito implementation? #3

Closed anyeone closed 5 years ago

anyeone commented 6 years ago

In the backend.py JSONWebTokenAuthentication.authenticate() method, you call UserModel.objects.get_or_create_for_cognito() but this obviously is not a built-in method of the UserManager.

The documentation should include that this needs to be implemented and also provide guidance on how it needs to work. From what I can gather the Cognito APIs require an access token (which isn't part of your default method signature, which passes only the jwt_payload to get_or_create_from_cognito) to retrieve the user itself from Cognito but if I try to use the JWT token that was passed in, it fails validation with an error about needing a string not a byte string but even converting it back, it fails:

An error occurred (InvalidParameterException) when calling the GetUser operation: 1 validation error detected: Value at 'accessToken' failed to satisfy constraint: Member must satisfy regular expression pattern: [A-Za-z0-9-_=.]+

I think adding a sample implementation of this method would be extremely helpful.

mvantellingen commented 6 years ago

Hi @anyeone, you are correct that the idea was that this should be implemented by the project. But we can definitely add an example.

We use the following:

    def get_or_create_for_cognito(self, payload):
        cognito_id = payload['sub']

        try:
            return self.get(cognito_id=cognito_id)
        except self.model.DoesNotExist:
            pass

        try:
            first_name = payload.get('given_name', None)
            last_names = [
                payload.get('middle_name', None),
                payload.get('family_name', None)
            ]
            last_name = ' '.join(filter(None, last_names)) or None

            user = self.create(
                cognito_id=cognito_id,
                email=payload['email'],
                is_active=True,
                first_name=first_name,
                last_name=last_name)
        except IntegrityError:
            user = self.get(cognito_id=cognito_id)

        return user
joshkersey commented 5 years ago

@mvantellingen How are you getting the payload values in your example method? The call to get_or_create_for_cognito is sending the jwt_payload which is the access token from Cognito. We need the ID token to get the claims about the authenticated user.

joshkersey commented 5 years ago

Realized that the ID token is what's expected in the auth header. I've added a PR #9 to make that more clear.

aurashn commented 5 years ago

Hi @anyeone, you are correct that the idea was that this should be implemented by the project. But we can definitely add an example.

We use the following:

    def get_or_create_for_cognito(self, payload):
        cognito_id = payload['sub']

        try:
            return self.get(cognito_id=cognito_id)
        except self.model.DoesNotExist:
            pass

        try:
            first_name = payload.get('given_name', None)
            last_names = [
                payload.get('middle_name', None),
                payload.get('family_name', None)
            ]
            last_name = ' '.join(filter(None, last_names)) or None

            user = self.create(
                cognito_id=cognito_id,
                email=payload['email'],
                is_active=True,
                first_name=first_name,
                last_name=last_name)
        except IntegrityError:
            user = self.get(cognito_id=cognito_id)

        return user

Where do I add this definition?

mikedebock commented 5 years ago

Hi @aurashn,

You should add this to the UserManager of your UserModel.

from django.contrib.auth.models import AbstractBaseUser, PermissionsMixin
from django.contrib.auth.models import UserManager as _UserManager

class UserManager(_UserManager):
    def get_or_create_for_cognito(self, payload):
        cognito_id = payload['sub']

        try:
            return self.get(cognito_id=cognito_id)
        except self.model.DoesNotExist:
            pass

        try:
            user = self.create(
                cognito_id=cognito_id,
                email=payload['email'],
                is_active=True)
        except IntegrityError:
            user = self.get(cognito_id=cognito_id)

        return user

class User(AbstractBaseUser, PermissionsMixin):
  cognito_id = models.CharField(max_length=128, blank=True)

  objects = UserManager()
rollue commented 5 years ago

@mikedebock Hi. I'm trying to understand how this library works to see if we should move our auth-related apis from DRF to AWS cognito. If I understand correctly, the auth workflow is as follows(correct me if I'm wrong).

  1. Client app requests cognito for auth, which returns tokens including id_token in JWT format.
  2. Client app then uses the JWT id_token to call the DRF backend. The get_or_create_for_cognito() is called, meaning cognito_id is mapped to the specific user in database.

If looks like here we save the cognito_id in the database, which baffles me. I thought the whole point of using JWT instead of the default DRF token was NOT to save the token on db & remove the necessity to query database on every request - and this should be better for scalability.

Am I understanding this right? If so, could you explain the logic behind your implementation?

ps: Additional question - do we have to add a custom cognito_id field to the user table, if we decide to use the library?

mikedebock commented 5 years ago

Hi @mhoonjeon, you're right that a additional cognito_id field should be added to the UserModel, I'll add that to the example above.

The difference between this JWT implementation and the DRF token is that we don't store the (JWT) token in the db, but only the Cognito user id (in the column cognito_id).

The payload argument in the get_or_create_for_cognito is a dict which contains user data (see: https://docs.aws.amazon.com/cognito/latest/developerguide/amazon-cognito-user-pools-using-tokens-with-identity-providers.html#user-pool-id-token-payload). We use the value of sub which contains the UUID (Cognito's user-id) to create a user.

The django_cognito_jwt.JSONWebTokenAuthentication queries every time the UserModel to populate request.user so it is available for the DRF views.

rollue commented 5 years ago

@mikedebock Thanks for the detailed explanation.

In the meantime, it seems cognito_id(which is in uuid4) is used for querying the user database; I would perhaps change the code a little more efficient in fetching the user object. You may or may not make cognito_id the primary key, but I think it should at least be indexed.

import uuid

from django.contrib.auth.models import AbstractBaseUser, PermissionsMixin
from django.contrib.auth.models import UserManager as _UserManager
from django.db import models

class UserManager(_UserManager):
    def get_or_create_for_cognito(self, payload):
        cognito_id = payload['sub']

        try:
            return self.get(cognito_id=cognito_id)
        except self.model.DoesNotExist:
            pass

        try:
            user = self.create(
                cognito_id=cognito_id,
                email=payload['email'],
                is_active=True)
        except IntegrityError:
            user = self.get(cognito_id=cognito_id)

        return user

class User(AbstractBaseUser, PermissionsMixin):
  cognito_id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)

  objects = UserManager()