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(auth): allow authorization for Github and OIDC behind the reverse proxy #3135

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

erka
Copy link
Collaborator

@erka erka commented May 29, 2024

No description provided.

Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 79.68750% with 13 lines in your changes missing coverage. Please review.

Project coverage is 71.08%. Comparing base (f997fb9) to head (138bbbd).
Report is 384 commits behind head on main.

Current head 138bbbd differs from pull request most recent head 5ccfeee

Please upload reports for the commit 5ccfeee to get more accurate results.

Files Patch % Lines
internal/server/authn/method/http.go 50.00% 6 Missing ⚠️
internal/config/authentication.go 86.84% 5 Missing ⚠️
internal/cmd/authn.go 0.00% 1 Missing ⚠️
internal/cmd/http.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3135      +/-   ##
==========================================
+ Coverage   70.78%   71.08%   +0.29%     
==========================================
  Files          91      109      +18     
  Lines        8729     7981     -748     
==========================================
- Hits         6179     5673     -506     
+ Misses       2165     1869     -296     
- Partials      385      439      +54     

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

@markphelps
Copy link
Collaborator

@erka is this ready for review?

@erka
Copy link
Collaborator Author

erka commented May 31, 2024

@erka is this ready for review?

@markphelps It works, but it's not ideal. The callback_url and authorize_url in the authentication method metadata lack the necessary prefix. While modifying them is possible, it involves dealing with low-level gRPC objects, making performance impact and maintenance difficult.

proxy

Signed-off-by: Roman Dmytrenko <[email protected]>
Co-authored-by: George MacRorie <[email protected]>
@erka erka marked this pull request as ready for review June 6, 2024 14:53
@erka erka requested a review from a team as a code owner June 6, 2024 14:53
@erka erka requested a review from GeorgeMac June 6, 2024 14:56
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

This looks great, thank you @erka !

@markphelps markphelps added the needs docs Requires documentation updates label Jun 6, 2024
Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

👏 Nice one @erka . Thanks for making this happen, it was a fiddly one.
And @tvcsantos for initial contrib and ideas around x-forwarded-prefix.

@tvcsantos
Copy link
Contributor

Thanks for the efforts guys. Really cool 👍

@GeorgeMac GeorgeMac merged commit 22a157c into flipt-io:main Jun 7, 2024
27 checks passed
@erka erka deleted the forward-prefix branch June 7, 2024 12:59
@markphelps markphelps added needs docs Requires documentation updates and removed needs docs Requires documentation updates labels Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs docs Requires documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants