Skip to content

Change token field type from CharField to TextField in AbstractRefres… #1558

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

Closed

Conversation

hb-joel
Copy link

@hb-joel hb-joel commented Mar 13, 2025

Fixes #1557

Description of the Change

This pull request changes the AbstractRefreshToken model in django-oauth-toolkit by updating the token field from a CharField to a TextField. This modification allows for greater storage capacity and prevents the django.db.utils.DataError caused by exceeding the character limit (255) for the CharField.

By switching to TextField, we ensure that the refresh token can accommodate larger values, which may be needed as token sizes or encoding formats evolve.

Checklist

  • PR only contains one change
  • Documentation updated to reflect changes made

This PR should resolve the token size issue. Let me know if you need any further adjustments to the description or additional information!

hb-joel and others added 2 commits March 13, 2025 12:12
@n2ygk
Copy link
Contributor

n2ygk commented Mar 13, 2025

TextFields are not searchable for certain database backends. This feels like something recently discussed in another PR.

@n2ygk
Copy link
Contributor

n2ygk commented Mar 13, 2025

See #1447

@komarov
Copy link

komarov commented Apr 29, 2025

As another user of DOT, I would like to kindly challenge the approach of changing these base models: both AbstractRefreshToken (in this PR) and AbstractAccessToken (in #1447, which in hindsight I would even view as a mistake).

My perception may be incorrect, but I see the swappable models as a better solution to these exceptional cases of requiring larger storage for token payloads. This keeps the project core code more suitable for a broader audience, but still accommodates more specific use cases.

Bringing in support for every [marginal] use case into the project core leads to unfortunate situations when a token checksum becomes larger than the original payload, the storage of the token payload moves out of the db page (engine-dependent), and it is impossible to opt-out from this overhead because the base code now requires a checksum to exist. And my guess would be that most users of the project don't benefit from this choice.

To support these custom models we would need to have a flexible mechanism (e.g. a method override) for a token lookup, allowing the core code to delegate the storage choice and the lookup implementation to the custom swappable model.

I don't know if it's too late to come up with a plan for AbstractAccessToken, but I think it would be good to consider a different way forward for AbstractRefreshToken and not proceed with this PR as it is now.

@ivancarrancho
Copy link

so, at this time, what do yo think how should we avoid these error? (the best way)

@dopry
Copy link
Member

dopry commented Aug 12, 2025

I agree with @komarov with regard to the refresh token, but not the access token. Best practices indicate using opaque tokens for both refresh tokens and access tokens. WRT Access tokens there are common and use cases for JWTs with embedded scopes/permission particularly where performance and scale are concerned and you want to save a call to an authorization server that are acceptable exceptions to the best practice for implementers who understand the trade off. Refresh tokens shouldn't be shared with 3rd parties in the same way and there aren't any strong use cases that I know of that would warrant using a JWT over an opaque string for a refresh token.

@hb-joel I appreciate your effort and contribution here. Unfortunately, until we can reproduce a bug with the core generating oversized refresh tokens or someone can provide a compelling use case for larger Refresh tokens supported by current OAuth or OIDC specification we're not going to accept this change. I would like to continue the discussion in the issue and explore your use case more.

@dopry dopry closed this Aug 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when creating refresh token: value too long for type character varying(255)
5 participants