auth: implement login and callback handlers #8

unknwon · 2024-03-28T22:03:22Z

Closes https://github.com/sourcegraph/sourcegraph-accounts/issues/43

This PR adds wrapping-handlers that are tailored to work with SAMS:

Always use OIDC protocol, as it provide discovery endpoint that only needs very minimal of the SAMS server configuration, i.e. the URL of the SAMS instance.
The callback handler does one extra thing that generic OIDC handler should have done: Hit the /userinfo endpoint for you, and return a structured type (UserInfo).
Allows passthrough of query parameters, prompt and prompt_auth, and could be more in the future.

FAQs

Why not manage state cookie OOTB like gologin?

As also mentioned in the added README content, unencrypted cookies are strongly discouraged as they can be tampered with. Although encrypted cookie is the secure alternative, we do strongly recommend storing auth state value in a backend component like Redis. Therefore, function hookers are provided for the service to implement.

Resolved

Function hookers or interface store?

I started writing function hookers for managing auth state naturally, but maybe an interface store would look/work better? 🤔 Open to thoughts.

type StateStore interface {
	SetState(w http.ResponseWriter, r *http.Request, state string) error
	GetState(r *http.Request) (string, error)
	DeleteState(w http.ResponseWriter, r *http.Request)
}

Test plan

Tests added for using those handlers to simulate authentication flow with SAMS.
Wrote up https://github.com/sourcegraph/cody-analytics/pull/48 to trial-integrate and tested authentication flow end-to-end.

auth/auth.go

chrsmith

Lots of minor suggestions and nit picks. But this all looks good! (Though I'll defer to @evict on approval.)

Only two things stand out:

Can we drop adding the http.ResponseWriter parameter to the StateSetter and StateDeleter hooks? It seems like they can only cause problems. (Unless they are just required, in which case ::sigh:: /shrug.)
Re: "function hookers vs. an interface store"

IMHO, having an interface would be preferable. Since it requires the caller to define a type that contains the various methods. Rather than the slightly more fast and loose "here's some lambda functions".

It also makes sense since practically speaking, those HTTP endpoint would need to rely on some shared services. (e.g. a handle to database infrastructure.) And it's a little less awesome to just capture that in the closure of inline StateSetter: func (...) expressions. (But I definitely don't feel strongly about this.)

README.md

chrsmith · 2024-03-29T20:56:58Z

README.md

+		return "", nil
+	}
+	stateDeleter := func(w http.ResponseWriter, r *http.Request) {
+		// TODO: Delete from session data.


When is stateDeleter called? Presumably this should be called whenever a user logs out? When will the samsauth.NewHandler call it?

It is called within the callback handler: https://github.com/sourcegraph/sourcegraph-accounts-sdk-go/pull/8/files#diff-f5c9a39fac1719f2a73593994b931d8a874449548e8a3753e1be46e761e503eaR140

i.e. deletes upon auth flow completes (regardless of whether the auth flow was successfully or not)

chrsmith · 2024-03-29T20:59:21Z

README.md

+			Issuer:         "https://accounts.sourcegraph.com",
+			ClientID:       os.Getenv("SAMS_CLIENT_ID"),
+			ClientSecret:   os.Getenv("SAMS_CLIENT_SECRET"),
+			RequestScopes:  []scopes.Scope{scopes.OpenID, scopes.Email, scopes.Profile},


What is the significance of these scopes here? I am guessing that these are the scopes/permissions that the user would be authorizing when direct to SAMS, right? e.g. "the application 'cody backend' would like to have access to your user profile, and subscription:write"?

This is probably worth calling out in a comment. Since it's an important configuration knob. (Because anything the person using this SAMS login flow will need to know what scopes the user would have access to.) something something "when using the refresh token and generating an access token" the OAuth RFC says you can only request a subset of the scopes on the refresh token or something?

What is the significance of these scopes here? I am guessing that these are the scopes/permissions that the user would be authorizing when direct to SAMS, right? e.g. "the application 'cody backend' would like to have access to your user profile, and subscription:write"?

Yes, however since all SAMS clients are internal/official clients, we do not show the consent screen to users at all (which we may in the far future).

This is probably worth calling out in a comment. Since it's an important configuration knob. (Because anything the person using this SAMS login flow will need to know what scopes the user would have access to.) something something "when using the refresh token and generating an access token" the OAuth RFC says you can only request a subset of the scopes on the refresh token or something?

Sure we can add a quick comment mentioning this, although we are really only using/having default OIDC scopes for user authentication (to get user profile info), for the sake of OIDC-compliant. Due to the fact we provide first-class Clients API, as long as the client has granted scopes registered in the SAMS database, clients can always ask for any granted scopes for any access tokens because that's not doing things on behalf of the user (unlike GitHub APIs) but on behalf of the client itself.

Pushed a comment in 53afa47

chrsmith · 2024-03-29T21:00:54Z

README.md

+	mux.Handle("/auth/login", samsauthHandler.LoginHandler())
+	mux.Handle("/auth/callback", samsauthHandler.CallbackHandler(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			userInfo := samsauth.UserInfoFromContext(r.Context())


For clarity I'd suggest having a comment here to explain what's going on:

// The SAMS auth handler will complete the login process. And if successful, // the user will be accessible from the request context. // // You can assume the `userInfo` will be present. As if there were any errors // during the login process, the user-supplied handler will not be invoked. // (And the FailureHandler will be serve the HTTP response instead.)

Another thing that I noticed when patching this locally and wiring up SAMS auth:

Seems like we are missing a /logout handler too... while SAMS isn't really needed on this code path, the caller is going to add whatever "local session creation" logic in the /auth/callback, so they would want to have some sort of "local session deletion" logic in a /auth/logout method as well. Right?

Longer term, if we put the SAMS login cookie on the sourcegraph.com domain, would we want samsauth to handle creating/loading/deleting the session? (Or perhaps that would be some other package, e.g. samsauth_sessions.NewHandler?

For clarity I'd suggest having a comment here to explain what's going on:

Added in 2fa4f3f

Seems like we are missing a /logout handler too... while SAMS isn't really needed on this code path, the caller is going to add whatever "local session creation" logic in the /auth/callback, so they would want to have some sort of "local session deletion" logic in a /auth/logout method as well. Right?

Yes but this is a bit tricky, because the logout logic doesn't really follow a "manner", in the sense that it does not have to be a handler. Having a handler for /auth/logout makes sense for some services, but probably not for a service that provide GQL endpoint (in which case the web app would actually hit GQL to accomplish sign-out).

Longer term, if we put the SAMS login cookie on the sourcegraph.com domain, would we want samsauth to handle creating/loading/deleting the session? (Or perhaps that would be some other package, e.g. samsauth_sessions.NewHandler?

This PR is specially targeting decoupled-session use case, but I think the building blocks for doing shared-session is already possible using Clients API (SessionsService). Once we established a usage pattern (aka. best practices), we can add more example docs but for now, I think we can/should leave that out. WDYT?

I agree that we can add some example/docs in the future, but let's not worry about that now. 👍

chrsmith · 2024-03-29T21:11:12Z

auth/auth.go

+		ctx := r.Context()
+
+		nonce, err := h.config.StateGetter(r)
+		h.config.StateDeleter(w, r) // Delete the state after getting it to make sure it's one-time use.


The "... to make sure it's one-time use." would probably be helpful to add as a note in the doc comments elsewhere, since it wasn't immediately obvious (to me at least).

🤔 Where to add as a doc comment are you thinking of?

This is really internal detail about how to handle state in the most strict way, as the users of SAMS auth handler, should they care? i.e. All they need to make sure is to implement the StateStore interface, and delete state when it is told to delete.

auth/auth.go

chrsmith · 2024-03-29T21:16:30Z

auth/auth.go

+}
+
+// UserInfo contains the information about the authenticated user.
+type UserInfo struct {


Where does this come from? Is it from the OIDC or OAuth spec? Or is it specific to the SAMS system? It would be nice to call that out, since if it were the latter we'd need to generally keep it in-sync with any other fields we add to the SAMS gRPC API, right?

It is mapping to whatever the SAMS discovery endpoint (1) said the "userinfo" endpoint's (2) response body is.

All of fields included in this PR are from OIDC spec, the server-side counterpart https://github.com/sourcegraph/sourcegraph-accounts/blob/c40d8f0d8374bb2222124f87f4f3626c51c4bd2e/backend/internal/api/model.go#L11-L17

chrsmith · 2024-03-29T21:18:26Z

auth/auth_test.go

+				return mockState, nil
+			},
+			StateDeleter: func(http.ResponseWriter, *http.Request) {
+				mockState = ""


We should ensure that we don't call StateDeleter multiple times in a row, right? e.g.

assert.True(t, mockState != "", "mockState already deleted?")

We should ensure that we don't call StateDeleter multiple times in a row, right?

Does it matter (practically)? 🤔

Eh, probably not.

I mean, if we called StateDeleter somewhere between 1x and 100x times for a given state value that would be weird. Right? But would it be worth the trouble to enforce that we only ever call it 1-time instead of 1 or possibly in some rare cases 2x? No.

auth/auth_test.go

unknwon · 2024-04-01T19:12:04Z

Can we drop adding the http.ResponseWriter parameter to the StateSetter and StateDeleter hooks? It seems like they can only cause problems. (Unless they are just required, in which case ::sigh:: /shrug.)

I definitely do want to get rid of http.ResponseWriter as well, the only problem removing that would be Cody Analytics won't be able to "plug and play" this SDK without completely changing its session store implementation (which currently uses encrypted cookie values). It is 100% fair IMO to have a strong stance that any service that wants to use SDK just have to do what's considered best practices. 🤔

I am going to remove http.ResponseWriter from this PR but still want to hear what @evict thinks! 🙏

It also makes sense since practically speaking, those HTTP endpoint would need to rely on some shared services.

Yep yep, that's what I am thinking too, OK, converting to interface store then!

# Conflicts: # go.mod

unknwon · 2024-04-01T20:58:53Z

@chrsmith thanks for the review! I think I have addressed all the feedback, PTAL!

chrsmith

LGTM. Thanks for making the changes.

I cloned this into a private repo on my machine, and everything worked fine. The only quirk was that this part of the code failed for me. (The SAMS dev environment wasn't returning the id_token value in the access token's extra metadata.)

// Extract the ID Token from the access token, see http://openid.net/specs/openid-connect-core-1_0.html#TokenResponse.
	rawIDToken, ok := token.Extra("id_token").(string)
	if !ok {
		return nil, errors.New(`missing "id_token" from the issuer's authorization response`)
	}

But I assume I'm just doing something wrong, and/or this might require some other SAMS-changes that haven't been rolled out yet.

chrsmith · 2024-04-02T04:52:56Z

README.md

+}
+
+func (s *stateStore) SetState(r *http.Request, state string) error {
+	// TODO: Save to session data.


Thanks for adding type stateStore struct, with that big comment. IMHO, that helps clarify what the implementer needs to think about in order to use this.

If you want to go for the gold, you could remove all of these TODO's and just implement it with something like:

// { big descriptive comment from above } // // This example implementation of state store just keeps // everything in-memory. Perhaps not something you'd want // to take into production... type exampleStateStore map[string]struct{}

Then maybe in theory the entire example could be a complete Go program that actually works?

Frankly I am a bit hesitant to provide a dummy implementation here that is after all "incorrect" thus meant not to be used. Unless this SDK also provide a full-blown session solution, it is just too free form at the moment how user sessions are managed by each service. 🤔 I am more willing to point to the code of a real service and people can study/copy from under the current state of things.

evict · 2024-04-02T09:25:13Z

README.md

+	mux.Handle("/auth/login", samsauthHandler.LoginHandler())
+	mux.Handle("/auth/callback", samsauthHandler.CallbackHandler(


Do we need to enforce the allowed HTTP methods of these routes?

Ahh, the curse of example code haha, the stdlib mux doesn't provide method selector, but ultimately, I think there is no reason why we have to be strict about it.

evict

Overall LGTM from security POV. Only 2 comments but neither of them are blockers. 👍

unknwon · 2024-04-03T14:39:11Z

Thanks for all the reviews! Let's land this and iterative on it while integrating a real service that uses it.

unknwon force-pushed the jc/auth branch 6 times, most recently from de657f4 to ff87e3b Compare March 29, 2024 20:22

unknwon marked this pull request as ready for review March 29, 2024 20:41

unknwon requested a review from a team as a code owner March 29, 2024 20:41

unknwon requested review from chrsmith and evict March 29, 2024 20:41

unknwon commented Mar 29, 2024

View reviewed changes

auth/auth.go Outdated Show resolved Hide resolved

auth: implement login and callback handlers

1cec914

unknwon force-pushed the jc/auth branch from ff87e3b to 1cec914 Compare March 29, 2024 20:43

chrsmith reviewed Mar 29, 2024

View reviewed changes

unknwon added 8 commits April 1, 2024 15:17

Remove http.ResponseWriter and use interface store

2ea72ba

Merge branch 'main' into jc/auth

b586fad

# Conflicts: # go.mod

Fixup README example and add some intro

659830e

Improve comment for auth state

b5675aa

Improve RequestScopes comment

53afa47

Add comment for user-provided handler

2fa4f3f

Improve RedirectURI docstring

9e9ced7

Add auth flow failure case

633f386

unknwon requested a review from chrsmith April 1, 2024 20:58

chrsmith approved these changes Apr 2, 2024

View reviewed changes

evict reviewed Apr 2, 2024

View reviewed changes

evict approved these changes Apr 2, 2024

View reviewed changes

Use different values for state and nonce

436cb3a

unknwon merged commit 28e08dd into main Apr 3, 2024
2 checks passed

unknwon deleted the jc/auth branch April 3, 2024 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth: implement login and callback handlers #8

auth: implement login and callback handlers #8

unknwon commented Mar 28, 2024 •

edited

Loading

chrsmith left a comment

chrsmith Mar 29, 2024

unknwon Apr 1, 2024

chrsmith Mar 29, 2024

unknwon Apr 1, 2024

unknwon Apr 1, 2024

chrsmith Mar 29, 2024

chrsmith Mar 31, 2024

unknwon Apr 1, 2024

chrsmith Apr 2, 2024

chrsmith Mar 29, 2024

unknwon Apr 1, 2024

chrsmith Mar 29, 2024

unknwon Apr 1, 2024

chrsmith Mar 29, 2024

unknwon Apr 1, 2024

chrsmith Apr 2, 2024

unknwon commented Apr 1, 2024

unknwon commented Apr 1, 2024

chrsmith left a comment

chrsmith Apr 2, 2024

unknwon Apr 2, 2024

evict Apr 2, 2024

unknwon Apr 2, 2024

evict left a comment

unknwon commented Apr 3, 2024

		mux.Handle("/auth/login", samsauthHandler.LoginHandler())
		mux.Handle("/auth/callback", samsauthHandler.CallbackHandler(

auth: implement login and callback handlers #8

auth: implement login and callback handlers #8

Conversation

unknwon commented Mar 28, 2024 • edited Loading

FAQs

Test plan

chrsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unknwon commented Apr 1, 2024

unknwon commented Apr 1, 2024

chrsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evict left a comment

Choose a reason for hiding this comment

unknwon commented Apr 3, 2024

unknwon commented Mar 28, 2024 •

edited

Loading