django-oauth-toolkit icon indicating copy to clipboard operation
django-oauth-toolkit copied to clipboard

django-oauth-toolkit and keycloack

Open fijemax opened this issue 10 months ago • 10 comments

Describe the bug Token field to short to use with keycloack oidc instropsection endpoint

To Reproduce Create a client than support oidc login on keycloack and use django-oauth-toolkit to valide the query by using introspection endpoint

Version lastest

When I configure django-oauth-toolkit with keycloack and try to validate an Authorization header with a jwt token, it appear than after the introspection validation the token used can't be store in the token field of TokenAccess model because the limit is 255.

value too long for type character varying(255)

is it a normal behavior ? Did I do something wrong ?

fijemax avatar Sep 25 '23 11:09 fijemax

Hi, I faced the same problem, did you manage to solve it?

Leelith avatar Sep 29 '23 05:09 Leelith

Hello, yes to solve my problem I overided django-oauth-toolkit classes with my own classes and I changed the limit of token field in the AccessToken class by overiding it in.

oauth/models.py

from django.db.models import CharField
from oauth2_provider import models


class Application(models.AbstractApplication):
    pass


class Grant(models.AbstractGrant):
    pass


class AccessToken(models.AbstractAccessToken):
    token = CharField(
        max_length=2048,
        unique=True,
        db_index=True,
    )


class RefreshToken(models.AbstractRefreshToken):
    pass


class IDToken(models.AbstractIDToken):
    pass

settings.py


OAUTH2_PROVIDER_APPLICATION_MODEL = "oauth.Application"
OAUTH2_PROVIDER_ACCESS_TOKEN_MODEL = "oauth.AccessToken"
OAUTH2_PROVIDER_REFRESH_TOKEN_MODEL = "oauth.RefreshToken"
OAUTH2_PROVIDER_ID_TOKEN_MODEL = "oauth.IDToken"
OAUTH2_PROVIDER_GRANT_MODEL = "oauth.Grant"

fijemax avatar Sep 29 '23 07:09 fijemax

Hello, yes to solve my problem I overided django-oauth-toolkit classes with my own classes and I changed the limit of token field in the AccessToken class by overiding it in.

oauth/models.py

from django.db.models import CharField
from oauth2_provider import models


class Application(models.AbstractApplication):
    pass


class Grant(models.AbstractGrant):
    pass


class AccessToken(models.AbstractAccessToken):
    token = CharField(
        max_length=2048,
        unique=True,
        db_index=True,
    )


class RefreshToken(models.AbstractRefreshToken):
    pass


class IDToken(models.AbstractIDToken):
    pass

settings.py


OAUTH2_PROVIDER_APPLICATION_MODEL = "oauth.Application"
OAUTH2_PROVIDER_ACCESS_TOKEN_MODEL = "oauth.AccessToken"
OAUTH2_PROVIDER_REFRESH_TOKEN_MODEL = "oauth.RefreshToken"
OAUTH2_PROVIDER_ID_TOKEN_MODEL = "oauth.IDToken"
OAUTH2_PROVIDER_GRANT_MODEL = "oauth.Grant"

Thank you for your help!

Leelith avatar Sep 29 '23 07:09 Leelith

This looks like a reasonable bug. We may in fact want to fully support access token JWTs in the future. We currently only do JWTs for identity tokens and we don't store the compose token for those. Would anyone be open to submitting a PR to switch the token property to a TextField? @n2ygk can you think of any reasons not to make the token a TextField? Would that impact indexing?

dopry avatar Oct 04 '23 15:10 dopry

Hi @dopry, It may depend on the database, but in PostgreSQL, there's no performance difference : https://www.postgresql.org/docs/current/datatype-character.html

tonial avatar Oct 31 '23 13:10 tonial

This looks like a reasonable bug. We may in fact want to fully support access token JWTs in the future. We currently only do JWTs for identity tokens and we don't store the compose token for those. Would anyone be open to submitting a PR to switch the token property to a TextField? @n2ygk can you think of any reasons not to make the token a TextField? Would that impact indexing?

@dopry You'd want to make sure that indexing works for the major supported Django databases. It looks like there is at least some limitation in mysql which I'm not sure is even supported via Django models. There's a documented mysql limitation on uniqueness.

It might be safer to add a separate text attribute.

n2ygk avatar Oct 31 '23 13:10 n2ygk

@n2ygk a separate text field for the full access token?

dopry avatar Oct 31 '23 16:10 dopry

@n2ygk a separate text field for the full access token?

@dopry Yeah for the JWT. I'm not sure why they would ever be needed for an Access Token but I guess that's what keycloak does. Why would one use the introspection endpoint to validate a JWT? It's cryptographically signed. I guess an AT is theoretically any opaque value so can include a JWT. But if it's too big to index then maybe store a cryptographic checksum to facilitate looking it up? mysql's max indexable length is 255.

This feels like something we discussed a while ago w.r.t. JWTs.

n2ygk avatar Oct 31 '23 17:10 n2ygk

I'd like to be certain about spec compliance before we make any decision about whether we shouldn't. But I am inclined to agree with @n2ygk and to not make this change. I'll read up on it next time I have a few cycles for DOT if no one else gets to it first and summarizes it.

I can definitely see the utility for edge use cases where the token may be the only session asset you have... But feel that people could use the id_token in that case and maybe we should lead them in that direction.

dopry avatar Oct 31 '23 17:10 dopry