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

feat: Add basic auth support #159

Closed
wants to merge 1 commit into from
Closed

Conversation

cdvv7788
Copy link

@cdvv7788 cdvv7788 commented Apr 2, 2024

Minor PR that adds support for basic auth and add a small example.

@cdvv7788 cdvv7788 requested a review from a team as a code owner April 2, 2024 23:30
@markphelps
Copy link
Contributor

Thanks for the PR @cdvv7788 !

Since Flipt Server doesnt natively support basic auth, is this for the case where you are running Flipt behind a reverse proxy such as nginx or caddy?

@cdvv7788
Copy link
Author

cdvv7788 commented Apr 3, 2024

Yep, running it behind nginx. May be a common enough case, and it is simple to support it :)

@markphelps
Copy link
Contributor

@cdvv7788 what if instead we add the ability to set arbitrary headers? Adding support for manipulating headers in all clients seems more flexible / reasonable for us to maintain.

Especially since Flipt itself doesn't support basic auth, it might be a bit confusing to users of the SDKs if they see a BasicAuthentication method in the SDKs

@cdvv7788
Copy link
Author

cdvv7788 commented Apr 3, 2024

I don't mind either way. This is a thin wrapper on httpx, so other stuff will have to be implemented on the user side anyway. I created the PR because I already had done this, but feel free to close if it is not a good fit for the project 👍

@markphelps
Copy link
Contributor

@cdvv7788 wdyt about changing this PR to allow setting the headers directly instead?

Then we can open up issues to do it in the other languages as well

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.

None yet

2 participants