Skip to content

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

Merged
BeryJu merged 1 commit intomainfrom
providers/oauth2/fix-refresh_token-grant-id_token
Apr 16, 2024
Merged

providers/oauth2: fix refresh_token grant returning incorrect id_token#9275
BeryJu merged 1 commit intomainfrom
providers/oauth2/fix-refresh_token-grant-id_token

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
providers/oauth2: fix refresh_token grant returning incorrect id_token
bb7599f 
Signed-off-by: Jens Langhammer <jens@goauthentik.io>
@BeryJu BeryJu requested a review from a team as a code owner April 15, 2024 21:55
@netlify
Copy link

netlify bot commented Apr 15, 2024 •
edited
Loading

✅ Deploy Preview for authentik-docs canceled.

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

@netlify
Copy link

netlify bot commented Apr 15, 2024 •
edited
Loading

✅ Deploy Preview for authentik-storybook canceled.

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

@BeryJu BeryJu mentioned this pull request Apr 15, 2024
Closed
@codecov
Copy link

codecov bot commented Apr 15, 2024 •
edited
Loading

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.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 15, 2024

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

lsjostro commented Apr 16, 2024

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
@BeryJu BeryJu deleted the providers/oauth2/fix-refresh_token-grant-id_token branch April 16, 2024 11:14
@lsjostro
Copy link

lsjostro commented Apr 16, 2024

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

lsjostro commented Apr 16, 2024

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
@kensternberg-authentik
Merge branch 'main' into dev
f19ed14 
* 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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@BeryJu @lsjostro