GitHub authn: allowSignup option #1155

beyang · 2018-11-28T07:22:10Z

Change summary:

41feb91: Rename and refactor the auth.CreateOrUpdateUser function to be auth.GetAndSaveUser. This makes the code clearer and gives clear contracts on how the user identity is inferred when signing in via external auth mechanism. See the docstring for that function.
34ad4c6: Adds a allowSignup field to the GitHub auth provider config. This is analogous to the allowSignup field in the builtin auth provider. If set to true, users can sign up by authenticating via their GitHub account. If set to false, an existing account must exist that corresponds to the GitHub identity by verified email equivalency. Its default value is false (cc @sqs okay with you?)
81f6270: Simplifies the auth.UpdateProviders method. Previously, there was a complicated (IMO) mechanism to ensure that existing Provider instances were not deleted/re-created if their config had not actually changed (i.e., reuse the same Provider instance if possible). Now, it just re-creates all auth.Provider instances on any config change. The added complexity didn't seem worth it to me, since none of the auth.Provider impls maintain internal state that would cause issues if it were discarded. There was a bug in the previous code that caused a panic on config update and I wasn't able to pin down the issue after about an hour of debugging. cc @sqs I think you were the original author—was there any other reason I missed why we could recreate all auth.Providers on every config refresh?
755b97e: Docs, including guidance on when to use each of the current auth providers.
The remaining commits are minor.

This will allow us to enable GitHub auth on dogfood.

sqs

The change (allowSignup for the GitHub auth provider) looks good. (And in general, we can make site config changes much more cheaply/cavalierly now because https://github.com/sourcegraph/sourcegraph/issues/965 makes it possible for Sourcegraph to auto-migrate site config when we change the structure of it.)

I added some code review notes, though.

sqs · 2018-11-28T07:33:09Z

cmd/frontend/auth/user.go


-// CreateOrUpdateUser creates or updates a user using the provided information, looking up a user by
+// UpdateUser creates or updates a user using the provided information, looking up a user by


UpdateUser creates or updates a user

meant to remove "creates or"?

To me, "UpdateUser" is clearer, since the caller has to specifically pass createIfNotExist=true if they want creation behavior.

sqs · 2018-11-28T07:33:44Z

cmd/frontend/auth/user.go

-// MockCreateOrUpdateUser is used in tests to mock CreateOrUpdateUser.
-var MockCreateOrUpdateUser func(db.NewUser, extsvc.ExternalAccountSpec) (int32, error)
+// MockUpdateUser is used in tests to mock UpdateUser.
+var MockUpdateUser func(db.NewUser, extsvc.ExternalAccountSpec) (int32, error)


db.NewUser struct sounds confusing if it's passed to an "update" func (doesn't make sense to "update" a "new user")

sqs · 2018-11-28T07:39:32Z

cmd/frontend/auth/user.go

@@ -109,3 +77,53 @@ func CreateOrUpdateUser(ctx context.Context, newOrUpdatedUser db.NewUser, extern
 }
 return user.ID, "", nil
 }
+
+func createUser(ctx context.Context, newUser db.NewUser, externalAccount extsvc.ExternalAccountSpec, data extsvc.ExternalAccountData) (


It doesn't make sense to call this func createUser (it was extracted from a func called CreateOrUpdateUser) because it doesn't just create a user. In some cases (the associateUser == true code path) it updates the user.

beyang · 2018-11-29T02:03:07Z

WIP'ing/unassigning to do a more extensive refactoring of the auth code to make this update possible.

golangcibot · 2018-11-30T01:36:52Z

cmd/frontend/auth/user_test.go

+ createUserAndSaveErr error
+ associateUserAndSaveErr error
+ getByVerifiedEmailErr error
+ getByIDErr error


getByIDErr is unused (from structcheck)

golangcibot · 2018-11-30T01:36:52Z

cmd/frontend/auth/user_test.go

+ associateUserAndSaveErr error
+ getByVerifiedEmailErr error
+ getByIDErr error
+ updateErr error


updateErr is unused (from structcheck)

golangcibot · 2018-11-30T01:36:52Z

cmd/frontend/auth/user_test.go

+
+type mockParams struct {
+ userInfos []userInfo
+ lookupUserAndSaveErr error


lookupUserAndSaveErr is unused (from structcheck)

golangcibot · 2018-11-30T01:36:52Z

cmd/frontend/auth/user_test.go

+type mockParams struct {
+ userInfos []userInfo
+ lookupUserAndSaveErr error
+ createUserAndSaveErr error


createUserAndSaveErr is unused (from structcheck)

golangcibot · 2018-11-30T01:36:53Z

cmd/frontend/auth/user_test.go

+ userInfos []userInfo
+ lookupUserAndSaveErr error
+ createUserAndSaveErr error
+ associateUserAndSaveErr error


associateUserAndSaveErr is unused (from structcheck)

golangcibot · 2018-11-30T01:36:53Z

cmd/frontend/auth/user_test.go

+ lookupUserAndSaveErr error
+ createUserAndSaveErr error
+ associateUserAndSaveErr error
+ getByVerifiedEmailErr error


getByVerifiedEmailErr is unused (from structcheck)

golangcibot · 2018-12-02T22:01:12Z

cmd/frontend/auth/providers2.go

+func GetProviderByConfigID(id ProviderConfigID) Provider {
+ var ps []Provider
+ if MockProviders != nil {
+ ps = MockProviders


ineffectual assignment to ps (from ineffassign)

golangcibot · 2018-12-02T22:01:12Z

cmd/frontend/auth/providers2.go

+ <-allProvidersRegistered
+ allProvidersMu.RLock()
+ defer allProvidersMu.RUnlock()
+ ps = allProviders


ineffectual assignment to ps (from ineffassign)

golangcibot · 2018-12-03T04:06:03Z

cmd/frontend/auth/providers.go

- close(allProvidersRegistered)
- })
+func GetProviderByConfigID(id ProviderConfigID) Provider {
+ if MockProviders != nil {


unnecessary nil check around range (from megacheck)

codecov-io · 2018-12-04T00:27:01Z

Codecov Report

Merging #1155 into master will increase coverage by 0.15%.
The diff coverage is 52.58%.

Impacted Files	Coverage Δ
cmd/frontend/db/users_mock.go	`0% <ø> (ø)`	⬆️
cmd/frontend/db/users.go	`47.04% <0%> (-0.29%)`	⬇️
cmd/frontend/db/test_util.go	`0% <0%> (ø)`
pkg/extsvc/github/user_emails.go	`0% <0%> (ø)`
enterprise/cmd/frontend/auth/gitlaboauth/config.go	`36.58% <0%> (+9.79%)`	⬆️
enterprise/cmd/frontend/auth/githuboauth/config.go	`45.23% <0%> (+11.3%)`	⬆️
...nterprise/cmd/frontend/auth/gitlaboauth/session.go	`0% <0%> (ø)`	⬆️
...erprise/cmd/frontend/auth/httpheader/middleware.go	`50% <100%> (+5.88%)`	⬆️
cmd/frontend/internal/auth/override.go	`64.7% <100%> (+9.15%)`	⬆️
enterprise/cmd/frontend/auth/openidconnect/user.go	`74.46% <100%> (+3.03%)`	⬆️
... and 13 more

beyang · 2018-12-04T00:38:47Z

@sqs @nicksnyder PTAL

sqs · 2018-12-05T21:02:59Z

Re: recreating all auth.Providers:

since none of the auth.Provider impls maintain internal state that would cause issues if it were discarded

If that is true, then it is fine to recreate them all each time. I think the state they maintain is things like the openid-configuration document. It probably isn't bad to fetch that a few times.

Are there any other things blocked on me?

slimsag · 2018-12-06T04:12:34Z

cmd/frontend/auth/user.go


-// CreateOrUpdateUser creates or updates a user using the provided information, looking up a user by
-// the external account provided.
+type GetUserOp struct {


Should this be GetAndSaveUserOp? GetUserOp implies to me that the fields within this struct can be incorrect without repercussion, but that is not true.

slimsag · 2018-12-06T04:36:54Z

cmd/frontend/auth/providers.go

+ for _, p := range providers {
+ k, err := json.Marshal(p.Config())
+ if err != nil {
+ log15.Error("Omitting auth provider, because could not JSON-marshal its config", "error", err, "configID", p.ConfigID())


Suggested change

log15.Error("Omitting auth provider, because could not JSON-marshal its config", "error", err, "configID", p.ConfigID())

log15.Error("Omitting auth provider (failed to marshal its JSON config)", "error", err, "configID", p.ConfigID())

Should this include the label of the provider? for improved debugging

p.ConfigID() identifies it

slimsag · 2018-12-06T04:38:47Z

cmd/frontend/auth/providers.go

+ // curProviders is a map (label -> (config string -> Provider)). The first key is the label
+ // under which the provider was registered. The second key is the normalized JSON serialization
+ // of Provider.Config().
+ curProviders = map[string]map[string]Provider{}


I feel like this double nested map is too complex for what we are doing. I am slightly worried about the lack of types (using string) too. Is there a way we could make it cleaner by using a struct or anything?

We should rename label to pkgName where we use it

slimsag · 2018-12-06T04:39:54Z

cmd/frontend/auth/providers.go

+ for _, p := range providers {
+ k, err := json.Marshal(p.Config())
+ if err != nil {
+ log15.Error("Omitting auth provider, because could not JSON-marshal its config", "error", err, "configID", p.ConfigID())


Should this include the label of the provider? for improved debugging

slimsag · 2018-12-06T04:40:43Z

cmd/frontend/auth/providers.go

-// registered via UpdateProviders).
-func GetProviderByConfigID(id ProviderConfigID) Provider {
- var ps []Provider
+func Providers() []Provider {


Let's add docs to Providers and clarify what it returns when there are no providers?

slimsag · 2018-12-06T04:42:16Z

cmd/frontend/auth/providers.go

- }
- if !op {
- panic(fmt.Sprintf("UpdateProviders: provider to remove did not exist: %+v", p))
+ providers := make([]Provider, 0, ct)


note: The double nested map curProviders makes this hard to read. I thought lp was the label name (human readable name) and not the submap of providers

Do we need the complexity here of calculating the array preallocation length? Why not just use something like 64 or even 128? (these are all pointer allocations, 128*8 == 1024 bytes memory, so very cheap anyway)

slimsag · 2018-12-06T04:46:25Z

cmd/frontend/auth/providers.go

- aj := allProviders[j].ConfigID()
- return ai.Type < aj.Type || (ai.Type == aj.Type && ai.ID < aj.ID)
- })
+ sort.Sort(sortProviders(providers))


Document why sorting these is important or not. If it's not important, why do we do it?

Do we need stable sorting here?

For the UI, will doc.

slimsag · 2018-12-06T04:48:48Z

cmd/frontend/auth/providers.go

+func (p sortProviders) Len() int {
+ return len(p)
+}
+func (p sortProviders) Less(i, j int) bool {


This less func is hard to read (that is fine / expected) but the intent is not documented. If there is a bug here, it would be hard to catch without knowing the intent. If I read it correctly, the intent is: builtin first, then by ConfigID().Type, then by ConfigID().ID

cmd/frontend/internal/auth/override.go

cmd/frontend/internal/auth/override_test.go

slimsag · 2018-12-06T04:58:45Z

cmd/frontend/auth/user.go

- if MockCreateOrUpdateUser != nil {
- userID, err = MockCreateOrUpdateUser(newOrUpdatedUser, externalAccount)
- return userID, "", err
+func GetAndSaveUser(ctx context.Context, op GetUserOp) (userID int32, safeErrMsg string, err error) {


As it stands this function returns two error message parameters that are easy to mix up by accident. A better approach would be to return a struct with all of these values, which means to access them you MUST acknowledge their names (result.UserID, result.SafeErrMsg, result.Err)

I'll follow up with this refactor in a later PR (want to get this deployed to dogfood for testing)

slimsag · 2018-12-06T05:00:58Z

cmd/frontend/internal/auth/userpasswd/config_watch.go

+ auth.UpdateProviders("builtin", nil)
+ } else {
+ auth.UpdateProviders("builtin", []auth.Provider{&provider{c: newPC}})
+ }


Return inside newPC == nil conditional instead and remove else?

cmd/frontend/internal/authz/gitlab/gitlab_test.go

enterprise/cmd/frontend/auth/githuboauth/session_test.go

enterprise/cmd/frontend/auth/httpheader/middleware_test.go

slimsag · 2018-12-06T19:30:35Z

enterprise/cmd/frontend/auth/httpheader/config_watch.go

+ if newPC == nil {
+ auth.UpdateProviders("httpheader", nil)
+ } else {
+ auth.UpdateProviders("httpheader", []auth.Provider{&provider{c: newPC}})


drop else / use early return here

slimsag · 2018-12-06T19:33:50Z

cmd/frontend/auth/user.go

 }
+ userID, userSaved, extAcctSaved, safeErrMsg, err := func() (int32, bool, bool, string, error) {


Probably makes sense to use a struct type here (e.g. declared inline). Otherwise it is hard to read + easy to mix up e.g. userSaved and extAcctSaved by accident

slimsag

LGTM aside from my unresolved inline comments

beyang · 2018-12-07T00:53:12Z

@sqs nothing in particular, but you've "requested changes"—if everything looks fine, can you approve?

…der instances); fixes panic when config cahnges

beyang requested a review from sqs as a code owner November 28, 2018 07:22

beyang requested review from keegancsmith and nicksnyder November 28, 2018 07:22

sqs suggested changes Nov 28, 2018

View reviewed changes

beyang changed the title ~~GitHub authn: allowSignup option~~ WIP GitHub authn: allowSignup option Nov 29, 2018

beyang requested review from sqs and removed request for keegancsmith, nicksnyder and sqs November 29, 2018 02:02

beyang force-pushed the bl/gh-no-signup branch from 6f533ba to 402c252 Compare November 29, 2018 19:49

golangcibot reviewed Nov 30, 2018

View reviewed changes

beyang force-pushed the bl/gh-no-signup branch 4 times, most recently from af467a3 to 34ad4c6 Compare December 2, 2018 21:10

golangcibot reviewed Dec 2, 2018

View reviewed changes

golangcibot reviewed Dec 3, 2018

View reviewed changes

beyang force-pushed the bl/gh-no-signup branch 2 times, most recently from 7a5cf69 to 81f6270 Compare December 3, 2018 19:31

beyang changed the title ~~WIP GitHub authn: allowSignup option~~ GitHub authn: allowSignup option Dec 4, 2018

beyang requested review from nicksnyder and keegancsmith December 4, 2018 00:37

beyang force-pushed the bl/gh-no-signup branch from b3842d0 to a0958b9 Compare December 4, 2018 00:38

beyang requested review from slimsag and removed request for nicksnyder and keegancsmith December 5, 2018 18:40

slimsag reviewed Dec 6, 2018

View reviewed changes

enterprise/cmd/frontend/auth/githuboauth/session_test.go Show resolved Hide resolved

slimsag reviewed Dec 6, 2018

View reviewed changes

enterprise/cmd/frontend/auth/httpheader/middleware_test.go Show resolved Hide resolved

slimsag reviewed Dec 6, 2018

View reviewed changes

slimsag approved these changes Dec 6, 2018

View reviewed changes

sqs approved these changes Dec 7, 2018

View reviewed changes

beyang added 8 commits December 7, 2018 11:48

auth.CreateOrUpdateUser -> GetAndSaveUser, required verified email

8ee4ada

githuboauth: allowSignup option, check verified emails

065ee77

simplify auth.UpdateProviders (don't try to reuse existing auth.Provi…

c3d6a73

…der instances); fixes panic when config cahnges

fix panic in callback handler

e6accd3

formatting

f81efe5

auth docs

f8cf458

GetUserOp -> GetAndSaveUserOp

6458e10

.

7de3b20

beyang force-pushed the bl/gh-no-signup branch from 755b97e to 7de3b20 Compare December 7, 2018 19:58

beyang added 4 commits December 7, 2018 12:02

.

d2dbcda

.

3e73fda

.

5dc46b4

update CHANGELOG

46cf67d

beyang merged commit 6392504 into master Dec 7, 2018

beyang deleted the bl/gh-no-signup branch December 7, 2018 21:31


		// CreateOrUpdateUser creates or updates a user using the provided information, looking up a user by
		// UpdateUser creates or updates a user using the provided information, looking up a user by

	log15.Error("Omitting auth provider, because could not JSON-marshal its config", "error", err, "configID", p.ConfigID())
	log15.Error("Omitting auth provider (failed to marshal its JSON config)", "error", err, "configID", p.ConfigID())

		}
		userID, userSaved, extAcctSaved, safeErrMsg, err := func() (int32, bool, bool, string, error) {

GitHub authn: allowSignup option #1155

GitHub authn: allowSignup option #1155

Conversation

beyang commented Nov 28, 2018 • edited Loading

sqs 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

beyang commented Nov 29, 2018

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

codecov-io commented Dec 4, 2018 • edited Loading

Codecov Report

beyang commented Dec 4, 2018

sqs commented Dec 5, 2018

Choose a reason for hiding this comment

slimsag Dec 6, 2018 • edited Loading

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

slimsag left a comment

Choose a reason for hiding this comment

beyang commented Dec 7, 2018

beyang commented Nov 28, 2018 •

edited

Loading

codecov-io commented Dec 4, 2018 •

edited

Loading

slimsag Dec 6, 2018 •

edited

Loading