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

Update claims instead of overwrite from webhook response #3889

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

3schwartz
Copy link

According to the webhook documentation:

To accept the token exchange without modification, return a 204 or 200 HTTP status code without a response body.

However, this behavior is currently not functioning as expected. When an empty response is returned, it overwrites the additional claims sent to the webhook, effectively erasing them.

Related issue(s)

#3879

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@CLAassistant
Copy link

CLAassistant commented Nov 15, 2024

CLA assistant check
All committers have signed the CLA.

@3schwartz
Copy link
Author

Hi @aeneasr, @hperl, @alnr,

Would any of you have some time to take a look at this? I’d really appreciate your input.

@aeneasr
Copy link
Member

aeneasr commented Nov 26, 2024

Thank you for the PR and sorry for the slow response! I think the problem here really is with incorrect documentation. It should read:

To not make any changes to the token payload, mirror it in full:

requestBody := res.Body.Read()
// do nothing
send({
access_token_session: requestBody.session.ext
id_token_session: requestBody.session.id_token.ext
})

The issue with this PR is that it doesn't allow us to remove elements from the array and it's a breaking change in the behavior, hence I would prefer that we fix the incorrect documentation

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.

3 participants