-
Notifications
You must be signed in to change notification settings - Fork 205
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(github): OAuth for GitHub #2016
Conversation
} | ||
|
||
metadata := map[string]string{ | ||
storageMetadataGithubAccessToken: oauthAccessTokenResponse.AccessToken, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we're really going to want to store this.
Particularly in this way, which will leak it through the API.
Instead, we should just use it to get identity information and just put the pieces we get from Github in the metadata (profile information etc.). That is what we do in the OIDC method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeorgeMac Okay was thinking that too. I did it like this because I was wondering if we wanted to keep that access token around for making authorized requests to GH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, once we have used it to finish the auth flow and get identity information we can discard it.
Somewhere down the line we might want to re-explore storing this, refresh tokens and so on.
But if we do, we would likely store it somewhere not API accessible.
|
||
var ( | ||
hostToAuthorizeBaseURL = map[string]string{ | ||
"github": "https://github.com/login/oauth/authorize", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just thinking out loud, the only thing that makes this GitHub specific and not 'general OAuth' support are URLS right? Also we may want to think about users who run GH Enterprise in their domain, but that could probably be added later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind of. I think what URLs you can access with the recieved access token, how you access them (query params vs e.g. body) and the shape of the responses of these endpoints can vary from one implementation to the next.
This is where OIDC has a standard around the UserInfo endpoint and solves that problem.
Plus they close the loop here on finishing the authorization leg in a standard way.
ui/src/app/auth/Login.tsx
Outdated
@@ -1,7 +1,8 @@ | |||
import { | |||
faGitlab, | |||
faGoogle, | |||
faOpenid | |||
faOpenid, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npx prettier -w .
in the ui
folder should take care of all of these
Codecov Report
@@ Coverage Diff @@
## main #2016 +/- ##
==========================================
- Coverage 71.04% 70.54% -0.50%
==========================================
Files 71 71
Lines 6768 6791 +23
==========================================
- Hits 4808 4791 -17
- Misses 1693 1732 +39
- Partials 267 268 +1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking awesome. Small handful of suggestions 👍
internal/cmd/auth.go
Outdated
|
||
authOpts = append(authOpts, auth.WithServerSkipsAuthentication(githubServer)) | ||
|
||
logger.Debug("authentication method \"oauth\" registered") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.Debug("authentication method \"oauth\" registered") | |
logger.Debug("authentication method \"github\" registered") |
internal/cmd/auth.go
Outdated
if cfg.Methods.Github.Enabled { | ||
githubMiddleware := method.NewHTTPMiddleware(cfg.Session) | ||
muxOpts = append(muxOpts, | ||
runtime.WithMetadata(method.ForwardCookies), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would if we should add a method to AuthenticationConfig
for whether or not session support needs enabling.
So we can do something like:
if cfg.SessionEnabled() {
muxOpts = append(muxOpts, runtime.WithMetadata(method.ForwardCookies))
}
It could perform the same operation as what happens in the validate method right now:
flipt/internal/config/authentication.go
Lines 117 to 120 in 2569b55
func (c *AuthenticationConfig) validate() error { | |
var sessionEnabled bool | |
for _, info := range c.Methods.AllMethods() { | |
sessionEnabled = sessionEnabled || (info.Enabled && info.SessionCompatible) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea being that we dont append the middleware twice when both are enabled.
ClientSecret: config.Methods.Github.Method.ClientSecret, | ||
Endpoint: oauth2GitHub.Endpoint, | ||
RedirectURL: callbackURL(config.Methods.Github.Method.RedirectAddress), | ||
Scopes: []string{strings.Join(config.Methods.Github.Method.Scopes, ":")}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong to me. This feels like we're combining not the full scopes, but the noun and verb parts of a Github scope.
For example, how would the user configure both user:email
and read:user
? This sticks all the parts in the slice together with :
.
I think we should just pass in the slice of scopes as is. Then the configurer has to do e.g. scopes: ["user:email", "read:user"]
.
Scopes: []string{strings.Join(config.Methods.Github.Method.Scopes, ":")}, | |
Scopes: config.Methods.Github.Method.Scopes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. It would be up to the user to provide the scopes as is. I like that idea!
if r.State != "" { | ||
md, ok := metadata.FromIncomingContext(ctx) | ||
if !ok { | ||
return nil, errors.ErrUnauthenticatedf("missing state parameter") | ||
} | ||
|
||
state, ok := md["flipt_client_state"] | ||
if !ok || len(state) < 1 { | ||
return nil, errors.ErrUnauthenticatedf("missing state parameter") | ||
} | ||
|
||
if r.State != state[0] { | ||
return nil, errors.ErrUnauthenticatedf("unexpected state parameter") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice to DRY this up into a utility function.
It could operate over the callback request or this interface:
type Stater interface {
GetState() string
}
Something like methods.CallbackValidateState()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can share it between both callback functions.
metadata := map[string]string{ | ||
storageMetadataGithubEmail: githubUserResponse.Email, | ||
storageMetadataGithubName: githubUserResponse.Name, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure how Github responds, but it might be nice to check if these are empty? (might need to confirm if these can ever be returned empty).
Then we should only set the keys if they're not empty.
Just incase the frontend something odd with empty values here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My one guess is email could be empty without the user:email
scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeorgeMac Something I was definitely considering too. I can do a quick test to see if that is the case, but I believe it might be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeorgeMac It seems as though this always gets populated without providing the scope
. Kinda weird 🤔, i'll track down what the actual meaning of this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be to do with your email visibility settings @yquansah
If you go to Settings > Emails > Keep my email address private and toggle that in GH, it might change the output.
They might still stick your private github email address in the space, so that might not be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the user can also allow less scopes than requested: https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps#requested-scopes-and-granted-scopes
im not sure if email is one of those scopes that can be 'removed', but good to guard against I think
internal/server/auth/method/http.go
Outdated
func parts(path string) (provider, method, prefix string, ok bool) { | ||
var oidcPrefix = "/auth/v1/method/oidc/" | ||
if strings.HasPrefix(path, oidcPrefix) { | ||
b, a, f := strings.Cut(path[len(oidcPrefix):], "/") | ||
return b, a, oidcPrefix, f | ||
} | ||
|
||
var githubPrefix = "/auth/v1/method/github" | ||
if strings.HasPrefix(path, githubPrefix) { | ||
b, a, f := strings.Cut(path[len(githubPrefix):], "/") | ||
return b, a, githubPrefix, f | ||
} | ||
|
||
return strings.Cut(path[len(prefix):], "/") | ||
return "", "", "", false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pedant suggestion here, but could we make it return (prefix, provider, method string, ok bool)
.
Just to preserve the original order of the parts.
Also, maybe pick some more meaningful variables names than b
, a
and f
. I am unsure what those refer to.
You could even:
const (
oidcPrefix = "/auth/v1/method/oidc/"
githubPrefix = "/auth/v1/method/github/"
)
func parts(path string) (prefix, provider, method string, ok bool) {
if strings.HasPrefix(path, oidcPrefix) {
prefix = oidcPrefix
provider, method, ok = strings.Cut(path[len(oidcPrefix):], "/")
} else if strings.HasPrefix(path, githubPrefix) {
prefix = githubPrefix
provider, method, ok = strings.Cut(path[len(githubPrefix):], "/")
}
return
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeorgeMac Love it!
internal/cmd/auth.go
Outdated
if cfg.SessionEnabled() && (cfg.Methods.OIDC.Enabled || cfg.Methods.Github.Enabled) { | ||
muxOpts = append(muxOpts, runtime.WithMetadata(method.ForwardCookies)) | ||
} | ||
|
||
if cfg.Methods.OIDC.Enabled || cfg.Methods.Github.Enabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might be able to boil this all down to the following.
SessionEnabled ensures at least one of these two methods are true (same as the .Enabled conditions here).
Also, SessionEnabled is only true, and is always true for OIDC or GitHub.
if cfg.SessionEnabled() && (cfg.Methods.OIDC.Enabled || cfg.Methods.Github.Enabled) { | |
muxOpts = append(muxOpts, runtime.WithMetadata(method.ForwardCookies)) | |
} | |
if cfg.Methods.OIDC.Enabled || cfg.Methods.Github.Enabled { | |
if cfg.SessionEnabled() { | |
muxOpts = append(muxOpts, runtime.WithMetadata(method.ForwardCookies)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple quick observations 👍
|
||
var githubUserResponse struct { | ||
Name string `json:"name,omitempty"` | ||
Email string `json:"email,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we also get the user's avatar_url
(https://docs.github.com/en/rest/users/users?apiVersion=2022-11-28) to display as the user profile pic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markphelps Good call!
metadata := map[string]string{ | ||
storageMetadataGithubEmail: githubUserResponse.Email, | ||
storageMetadataGithubName: githubUserResponse.Name, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the user can also allow less scopes than requested: https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps#requested-scopes-and-granted-scopes
im not sure if email is one of those scopes that can be 'removed', but good to guard against I think
77b8424
to
575a7af
Compare
internal/server/auth/method/http.go
Outdated
@@ -90,7 +92,7 @@ func (m Middleware) ForwardResponseOption(ctx context.Context, w http.ResponseWr | |||
// The payload is then also encoded as a http cookie which is bound to the callback path. | |||
func (m Middleware) Handler(next http.Handler) http.Handler { | |||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
provider, method, match := parts(r.URL.Path) | |||
prefix, provider, method, match := parts(r.URL.Path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take it or leave it:
I reflected a bit on this, after you raised the very valid point that there there is the subtle difference of a provider for OIDC and not for Github, which makes this cutting and prefixing fiddly; I think there might be just a clearer way to express all this.
In hindsight, all this path matching is only really attempting to do two things:
- Match valid
authorize
request paths (one of OIDC and one for Google). - Replace the
authorize
path entry with the wordcallback
for the callback path.
I previously had overengineered this with pulling out the provider, but I cant see where that is getting used.
I wonder if the inconsistencies and fiddly reconstruction could all be simplified with just a bit of inline code that does just this:
e.g.
func (m Middleware) Handler(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
prefix, method := path.Split(r.URL.Path)
if !((strings.HasPrefix(prefix, oidcPrefix) || strings.HasPrefix(prefix, ghPrefix)) && method == "authorize") {
next.ServeHTTP(w, r)
return
}
// ...
// bind state cookie to callback handler
Path: path.Join(prefix, "callback"),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you can even take the trailing slash off both the OIDC and Github prefixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeorgeMac Awesome suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh one important things needs doing. Otherwise, I think this is good to go.
Perhaps, along with a test case to cover this.
userResp, err := c.Do(userReq) | ||
if err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check for a 200
status code on this.
I assume there is a chance their non-200 status code returns valid json and we might authenticate the user in that situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeorgeMac Nice catch! Yeah let me do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! one minor request around unit test, no need for re-review
} | ||
|
||
func callbackURL(host string) string { | ||
// strip trailing slash from host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to add some basic unit tests here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markphelps Let me add that really quick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet 💪
This PR is meant to provide functionality with logging into Flipt via GH OAuth.
Completes FLI-212