Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

USER_MODEL.objects.get_or_create_for_cognito implementation? #3

Closed
anyeone opened this issue May 1, 2018 · 8 comments
Closed

USER_MODEL.objects.get_or_create_for_cognito implementation? #3

anyeone opened this issue May 1, 2018 · 8 comments
Labels
good first issue Good for newcomers

Comments

@anyeone
Copy link

anyeone commented May 1, 2018

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
Copy link
Member

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

@mvantellingen mvantellingen added the good first issue Good for newcomers label May 3, 2018
@joshkersey
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link

aurashn commented Apr 30, 2019

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
Copy link
Member

mikedebock commented May 1, 2019

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
Copy link

rollue commented May 2, 2019

@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
Copy link
Member

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
Copy link

rollue commented May 2, 2019

@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()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

6 participants