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

fix: introspection response #792

Closed

Conversation

tjandrayana
Copy link

@tjandrayana tjandrayana commented Jul 17, 2021

I try to fixing the issue related oauth2_introspection not parsing single string aud value

Based on this protocol https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation the audience field need to changed, to receive []string or string.

There are some solutions we can implement :

  1. Change the audience as an interface
  1. Create new struct and then set the audience as a single string, so if there is possibility failed to unmarshal / casting and then unmarshal / cast with this new struct
    Effort :
  • Create new function to modify the response
  • Not break the current unit testing

So, because of that, I decide to create new struct named Fork and add the function modify struct in this PR.
With this Fork struct and this function the unit testing also work fine after I execute in my local.
Hopefully with this PR can help everyone that face same issue.
Thank you

Related issue(s)

#491

Checklist

@CLAassistant
Copy link

CLAassistant commented Jul 17, 2021

CLA assistant check
All committers have signed the CLA.

@tjandrayana tjandrayana changed the title Modify introspection response Fix / Modify introspection response Jul 17, 2021
@tjandrayana tjandrayana changed the title Fix / Modify introspection response fix: introspection response Jul 17, 2021
@codecov
Copy link

codecov bot commented Jul 17, 2021

Codecov Report

Merging #792 (03d71d9) into master (a0a1329) will increase coverage by 0.13%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #792      +/-   ##
==========================================
+ Coverage   62.21%   62.34%   +0.13%     
==========================================
  Files         102      102              
  Lines        4782     4799      +17     
==========================================
+ Hits         2975     2992      +17     
  Misses       1532     1532              
  Partials      275      275              
Impacted Files Coverage Δ
...peline/authn/authenticator_oauth2_introspection.go 80.00% <94.44%> (+2.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0a1329...03d71d9. Read the comment docs.

@tjandrayana tjandrayana reopened this Jul 17, 2021
@tjandrayana tjandrayana marked this pull request as draft July 24, 2021 03:04
@tjandrayana tjandrayana marked this pull request as ready for review July 24, 2021 03:05
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! However, unfortunately, I do not believe that it will resolve what you are trying to do, because the type assertion will always be false. It might make sense to use something like gjson.Get to see if you have a string or an array in the audience. It would also make sense to add an e2e test to ensure that the feature works as expected :)

aeneasr pushed a commit that referenced this pull request Sep 29, 2021
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