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

providers/oauth2: fix refresh_token grant returning incorrect id_token #9275

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

BeryJu
Copy link
Member

@BeryJu BeryJu commented Apr 15, 2024

Details

#4191


Checklist

  • Local tests pass (ak test authentik/)
  • The code has been formatted (make lint-fix)

If an API change has been made

  • The API schema has been updated (make gen-build)

If changes to the frontend have been made

  • The code has been formatted (make web)

If applicable

  • The documentation has been updated
  • The documentation has been formatted (make website)

@BeryJu BeryJu requested a review from a team as a code owner April 15, 2024 21:55
Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for authentik-docs canceled.

Name Link
🔨 Latest commit bb7599f
🔍 Latest deploy log https://app.netlify.com/sites/authentik-docs/deploys/661da24ca713f70008f32f60

Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for authentik-storybook canceled.

Name Link
🔨 Latest commit bb7599f
🔍 Latest deploy log https://app.netlify.com/sites/authentik-storybook/deploys/661da24c44afd700089a3962

Copy link

codecov bot commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.37%. Comparing base (33fa159) to head (bb7599f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9275      +/-   ##
==========================================
- Coverage   92.38%   92.37%   -0.01%     
==========================================
  Files         665      665              
  Lines       32609    32609              
==========================================
- Hits        30125    30124       -1     
- Misses       2484     2485       +1     
Flag Coverage Δ
e2e 50.67% <ø> (-0.02%) ⬇️
integration 26.01% <ø> (ø)
unit 89.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

authentik PR Installation instructions

Instructions for docker-compose

Add the following block to your .env file:

AUTHENTIK_IMAGE=ghcr.io/goauthentik/dev-server
AUTHENTIK_TAG=gh-ghcr.io/goauthentik/dev-server:gh-bb7599f3ee5106aa3787ea204efd995a2dac997f
AUTHENTIK_OUTPOSTS__CONTAINER_IMAGE_BASE=ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s

For arm64, use these values:

AUTHENTIK_IMAGE=ghcr.io/goauthentik/dev-server
AUTHENTIK_TAG=gh-ghcr.io/goauthentik/dev-server:gh-bb7599f3ee5106aa3787ea204efd995a2dac997f-arm64
AUTHENTIK_OUTPOSTS__CONTAINER_IMAGE_BASE=ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s

Afterwards, run the upgrade commands from the latest release notes.

Instructions for Kubernetes

Add the following block to your values.yml file:

authentik:
    outposts:
        container_image_base: ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s
image:
    repository: ghcr.io/goauthentik/dev-server
    tag: gh-ghcr.io/goauthentik/dev-server:gh-bb7599f3ee5106aa3787ea204efd995a2dac997f

For arm64, use these values:

authentik:
    outposts:
        container_image_base: ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s
image:
    repository: ghcr.io/goauthentik/dev-server
    tag: gh-ghcr.io/goauthentik/dev-server:gh-bb7599f3ee5106aa3787ea204efd995a2dac997f-arm64

Afterwards, run the upgrade commands from the latest release notes.

@lsjostro
Copy link

Can we add proper integration tests for this?

@BeryJu
Copy link
Member Author

BeryJu commented Apr 16, 2024

@lsjostro do you have any suggestion on a client application that could be used for this? The OpenID compliance test didn't complain about this

I'll still merge this as-is for the time being so it can be included in 2024.4

@BeryJu BeryJu merged commit cad5ff3 into main Apr 16, 2024
67 checks passed
@BeryJu BeryJu deleted the providers/oauth2/fix-refresh_token-grant-id_token branch April 16, 2024 11:14
@lsjostro
Copy link

I'm more searching for why this hasn't been noticed since the last fix 2 years ago? How can we prevent it will not happen in the future? issuing tokens valid for 30 days without letting users know is a big regression if you ask me. Writing e2e/integration tests and validating configuration vs token expire times etc would help? I dont think the compliance test would have caught this right? But yeah merge it and get it released asap is of course most important right now 👍🏻 and thanks for the quick fix!

@BeryJu
Copy link
Member Author

BeryJu commented Apr 16, 2024

@lsjostro I'm assuming that most applications don't use the id_token part of a refresh_token grant as even the OIDC spec says this is optional, and hence it hasn't been noticed

@lsjostro
Copy link

that's not true, first app that pops up is the k8s api server?

@BeryJu
Copy link
Member Author

BeryJu commented Apr 16, 2024

sorry I meant from the apps I've used with the OIDC provider, I've not set up my K8s clusters to use the authentik OIDC provider yet

kensternberg-authentik added a commit that referenced this pull request Apr 16, 2024
* main: (34 commits)
  web: bump API Client version (#9299)
  core: fix api schema for users and groups (#9298)
  providers/oauth2: fix refresh_token grant returning incorrect id_token (#9275)
  web: bump @sentry/browser from 7.110.0 to 7.110.1 in /web in the sentry group (#9278)
  core, web: update translations (#9277)
  web: bump the rollup group in /web with 3 updates (#9280)
  web: bump lit from 3.1.2 to 3.1.3 in /web (#9282)
  web: bump @lit/context from 1.1.0 to 1.1.1 in /web (#9281)
  website: bump @types/react from 18.2.78 to 18.2.79 in /website (#9286)
  core: bump goauthentik.io/api/v3 from 3.2024022.10 to 3.2024022.11 (#9285)
  core: bump sqlparse from 0.4.4 to 0.5.0 (#9276)
  lifecycle: gunicorn: fix app preload (#9274)
  events: add indexes (#9272)
  web/flows: fix passwordless hidden without input (#9273)
  root: fix geoipupdate arguments (#9271)
  website/docs: cleanup more (#9249)
  web: bump API Client version (#9270)
  sources: add SCIM source (#3051)
  core: delegated group member management (#9254)
  web: bump API Client version (#9269)
  ...
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.

2 participants