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(auth/github): add organization membership check to GitHub #2508

Merged
merged 12 commits into from
Dec 10, 2023

Conversation

erka
Copy link
Collaborator

@erka erka commented Dec 9, 2023

add organization membership check to GitHub authentication method

fixes #2065

Copy link
Contributor

github-actions bot commented Dec 9, 2023

Uffizzi Ephemeral Environment deployment-42486

☁️ https://app.uffizzi.com/github.com/flipt-io/flipt/pull/2508

📄 View Application Logs etc.

⏰ This Preview will be destroyed in 1 hours at: Sun Dec 10 19:02:54 UTC 2023

What is Uffizzi? Learn more!

Copy link

codecov bot commented Dec 9, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (37f13e8) 70.71% compared to head (8edeac5) 70.87%.

Files Patch % Lines
internal/server/auth/method/github/server.go 83.33% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2508      +/-   ##
==========================================
+ Coverage   70.71%   70.87%   +0.15%     
==========================================
  Files          81       81              
  Lines        8134     8168      +34     
==========================================
+ Hits         5752     5789      +37     
+ Misses       2035     2033       -2     
+ Partials      347      346       -1     

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

@erka erka marked this pull request as ready for review December 9, 2023 17:44
@erka erka requested a review from a team as a code owner December 9, 2023 17:44
@erka
Copy link
Collaborator Author

erka commented Dec 9, 2023

@markphelps I believe there could be something missing. It requires read:org scope to get user's organizations. Maybe there should be some configuration check but it's an open question for me. Please take a look and provide the feedback.

@markphelps
Copy link
Collaborator

@markphelps I believe there could be something missing. It requires read:org scope to get user's organizations. Maybe there should be some configuration check but it's an open question for me. Please take a lot and provide the feedback.

Cheers @erka ! Thanks for taking this on! I have a proposed solution to ^, erka#4

I still need to add tests however, but can do that in the fork PR or this one

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.

looking great, couple suggestions

internal/config/authentication.go Outdated Show resolved Hide resolved
internal/server/auth/method/github/server.go Outdated Show resolved Hide resolved
@markphelps markphelps added the needs docs Requires documentation updates label Dec 10, 2023
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.

Looks and works great!

CleanShot 2023-12-10 at 14 08 05

I tried it out by setting up our test OAuth app on Github

When I configured it to check:
allowed_organizations: flipt-io

It let me in. But when I changed it to allowed_organizations: flip-io purposefully misspelled, it gave me a 401 response!

CleanShot 2023-12-10 at 14 10 56

@markphelps
Copy link
Collaborator

also checked that it errors if you don't request the read:org scope when setting allowed_organizations !

Error: loading configuration scopes must contain read:org when allowed_organizations is not empty

thanks @erka ! I will work on docs updates and get this out probably tomorrow in a release!

@markphelps markphelps merged commit 72a06bb into flipt-io:main Dec 10, 2023
28 checks passed
@erka erka deleted the github-auth-member-of-org branch December 10, 2023 20:33
@markphelps markphelps removed the needs docs Requires documentation updates label Dec 20, 2023
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.

[FLI-710] Add organization membership check to GitHub authentication method
2 participants