Allow users to add Gerrit HTTP Credentials by pjlast · Pull Request #46763 · sourcegraph/sourcegraph-public-snapshot

pjlast · 2023-01-23T06:57:46Z

Adds a Gerrit auth provider. It does not provide a sign in option. It appears in the Account Security page where users can add Gerrit HTTP credentials to sync Gerrit permissions.

See loom: https://www.loom.com/share/1fa6d6e5f157481a8a62f7a3703e33ca

Test plan

Lots of unit tests added and updated and I did a lot of manual testing as well.

sg-e2e-regression-test-bob · 2023-01-23T07:06:21Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (0.00 kb)	0.02% (+2.87 kb)	0.02% (+2.87 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits ea486c8 and 5bea6c7 or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

sourcegraph-bot · 2023-01-23T11:10:16Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 5bea6c7...ea486c8.

Notify	File(s)
@eseliger	internal/extsvc/gerrit/account.go internal/extsvc/gerrit/client.go internal/extsvc/gerrit/client_test.go internal/extsvc/gerrit/externalaccount/externalaccount.go internal/extsvc/gerrit/testdata/golden/ListProjects.json internal/extsvc/gerrit/testdata/vcr/ListProjects.yaml
@indradhanush	internal/repos/gerrit.go internal/repos/gerrit_test.go
@ryanslade	internal/repos/gerrit.go internal/repos/gerrit_test.go
@sashaostrikov	internal/repos/gerrit.go internal/repos/gerrit_test.go
@sourcegraph/delivery	doc/admin/auth/index.md doc/admin/external_service/gerrit.md doc/admin/external_service/gerrit.schema.json doc/admin/external_service/index.md doc/admin/external_service/other.md doc/admin/repo/permissions.md
@unknwon	enterprise/cmd/frontend/internal/auth/gerrit/config.go enterprise/cmd/frontend/internal/auth/gerrit/config_test.go enterprise/cmd/frontend/internal/auth/init.go enterprise/internal/authz/authz.go enterprise/internal/authz/authz_test.go enterprise/internal/authz/bitbucketcloud/authz.go enterprise/internal/authz/bitbucketcloud/authz_test.go enterprise/internal/authz/bitbucketserver/authz.go enterprise/internal/authz/gerrit/authz.go enterprise/internal/authz/gerrit/client.go enterprise/internal/authz/gerrit/gerrit.go enterprise/internal/authz/gerrit/gerrit_test.go enterprise/internal/authz/github/authz.go enterprise/internal/authz/github/authz_test.go enterprise/internal/authz/gitlab/authz.go enterprise/internal/authz/perforce/authz.go enterprise/internal/authz/types/types.go

mrnugget

Nice!

mrnugget · 2023-01-23T14:59:55Z

+	permssync.SchedulePermsSync(ctx, r.logger, r.db, protocol.PermsSyncRequest{
+		UserIDs: []int32{account.UserID},
+	})


We didn't have this before? 🤨

Good catch 👍

mrnugget · 2023-01-23T15:02:54Z

        if (provider.serviceType === 'sourcegraph-operator') {
            return new URLSearchParams(location.search).has('sourcegraph-operator')
        }
+        if (provider.serviceType === 'gerrit') {
+            return false
+        }


Instead of building up a list of hardcoded auth provider names - can we add a property to the authProviders like supportsSignIn?

Plus 1 on this. I was wondering how the PR managed to get rid of gerrit on the sign in list.

We can theoretically use this to hide the builtin auth provider on customer instances after they finish setting up other auth provider. As there were multiple complaints about it being confusing for the engineers on customer side. Or to hide the sourcegraph-operator as well :)

kopancek

Great work! Sorry for many comments, but I really dived deep into this one as it's important to get it right 🙇

kopancek · 2023-01-23T16:24:35Z

        if (provider.serviceType === 'sourcegraph-operator') {
            return new URLSearchParams(location.search).has('sourcegraph-operator')
        }
+        if (provider.serviceType === 'gerrit') {
+            return false
+        }


Plus 1 on this. I was wondering how the PR managed to get rid of gerrit on the sign in list.

We can theoretically use this to hide the builtin auth provider on customer instances after they finish setting up other auth provider. As there were multiple complaints about it being confusing for the engineers on customer side. Or to hide the sourcegraph-operator as well :)

kopancek · 2023-01-23T16:36:10Z

+                username: { value: string }
+                password: { value: string }
+            }
+            event.preventDefault()


Do we need the preventDefault here? What will happen if it's removed? (I would expect nothing, since the action is defined as onSubmit on the Form element itself, but I may be wrong...

Based on the React docs: https://reactjs.org/docs/forms.html it seems like you still need to do preventDefault(), otherwise it will do a form submission and do a post request.

kopancek · 2023-01-23T16:38:39Z

+            setIsLoading(true)
+
+            addExternalAccount({
+                variables: {
+                    serviceType: 'gerrit',
+                    serviceID,
+                    accountDetails: JSON.stringify({
+                        username: target.username.value,
+                        password: target.password.value,
+                    }),
+                },
+            })
+                .then(() => {
+                    setIsLoading(false)
+                    onDidAdd()
+                })
+                .catch(() => {
+                    setIsLoading(false)
+                })
+        },
+        [addExternalAccount, onDidAdd, serviceID]


Why is this so complicated? Can we use {loading, data, error} from the useMutation hook instead of calling setIsLoading and handling the promise? For calling onDidAdd you could leverage the onCompleted option on the useMutation hook. Or does it not want to work well with useCallback above?

I just completely blanked on the loading state and the onCompleted option 🤦

Updated, looks a gazillion times better now, thanks!

kopancek · 2023-01-23T16:43:53Z

+        if (authProvider.serviceType === 'gerrit') {
+            setIsGerritAccountModalOpen(true)
+            return
+        }


We could theoretically get around special casing here by creating another route, say /login/gerrit/openmodal and routing to that from the auth provider button.

But it might be too much hassle for not such a big gain, so feel free to disregard.

kopancek · 2023-01-23T17:03:19Z

+	ServiceType string
+}
+
+func parseConfig(cfg conftypes.SiteConfigQuerier) (ps []Provider, problems conf.Problems) {


Consider adding a boolean to return parameters and return true if a gerrit provider config changed (which would mean that the list of providers changed).

kopancek · 2023-01-23T17:07:02Z

+func (p *Provider) ConfigID() providers.ConfigID {
+	return providers.ConfigID{
+		Type: extsvc.TypeGerrit,
+		ID:   p.ServiceID,


Other auth providers add an identifier to the providers.ConfigID.ID, should we do a similar thing here?

So that makes sense in the case of OAuth because you can have multiple OAuth clients for the same host. But here we're using basic HTTP auth, so there's only client for the entire host.

kopancek · 2023-01-23T17:07:58Z

 	githuboauth.Init(logger, db)
 	gitlaboauth.Init(logger, db)
 	bitbucketcloudoauth.Init(logger, db)
+	gerrit.Init()


Don't we need logger in the gerrit instance? Or db?

The Gerrit auth provider does nothing fancy because it's basic HTTP auth. The other ones have a logger and db because they do complex stuff with all the back and forth while doing OAuth, and all those actions get initialised from there.

But for Gerrit it's only "oh here's the URL on you go then".

kopancek · 2023-01-23T17:09:04Z

+		Login:       &blank,
+		URL:         &blank,


Instead of returning empty string, why not omit the lines? These are pointers to a string in the end...

Suggested change

Login: &blank,

URL: &blank,

kopancek · 2023-01-23T17:11:02Z

+	if err != nil {
+		return err
+	}
+	serializedCreds, err := json.Marshal(creds)


Since we store the credentials in the DB as is, what should we do about our dev instances, where DB encryption is off by default if I remember correctly? This might theoretically leak Gerrit credentials...

I mean in those cases we're also storing private keys and oauth tokens unencrypted as well. I'm not sure if this is any more scary than that.

cla-bot Bot added the cla-signed label Jan 23, 2023

github-actions Bot added the team/iam label Jan 23, 2023

pjlast marked this pull request as ready for review January 23, 2023 11:08

pjlast requested a review from a team January 23, 2023 11:08

mrnugget approved these changes Jan 23, 2023

View reviewed changes

kopancek reviewed Jan 23, 2023

View reviewed changes

pjlast force-pushed the pjlast/46622-gerrit-account branch from 7616b27 to aa3a8b4 Compare January 26, 2023 08:08

kopancek mentioned this pull request Jan 26, 2023

Add Gerrit user permissions syncing #46774

Merged

pjlast added 20 commits January 27, 2023 16:40

Add gerrit account adding options and interface

41f5604

Add resolver implementation for adding gerrit account

d6e2e19

Addition and removal of Gerrit accounts fully functional

e1be063

Use correct errors lib

bbd573d

Add sign in page test

aa09db5

Fix tests

a118d0a

Generalise AddGerritExternalAccount to AddExternalAccount

5cc5080

Use correct GraphQL imports

a0a3032

Expand on test cases

55f9dfd

Enforce single provider for gerrit

26ffd5a

Add last test case

4de2a57

Add separate method for adding a gerrit external account

1ca823a

Move db actions to transaction

397e510

Remove dependence on Gerrit connection for verifying gerrit account

e465ecf

Remove print and unused variables

9991545

Remove loop variable capture

95f6e10

Improve usage of useMutation

87b539a

Last bit of PR comments

d45aa45

Fix NewClient params

eca03e0

Update snapshots

d23b532

pjlast added 4 commits January 27, 2023 16:40

Switch from switch statement

f0af2bc

Add Gerrit user permissions syncing (#46774)

038f507

Add repo filtering to Gerrit code host connections (#47021)

cf3d764

Add Gerrit section and Gerrit as supported code host to docs (#47019)

48b27d0

pjlast force-pushed the pjlast/46622-gerrit-account branch from f65e34e to 48b27d0 Compare January 27, 2023 14:40

Update CHANGELOG

ea486c8

pjlast enabled auto-merge (squash) January 27, 2023 15:19

pjlast merged commit 6394b92 into main Jan 27, 2023

pjlast deleted the pjlast/46622-gerrit-account branch January 27, 2023 15:33

Conversation

pjlast commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

sg-e2e-regression-test-bob commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

sourcegraph-bot commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrnugget left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kopancek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pjlast commented Jan 23, 2023 •

edited

Loading

sg-e2e-regression-test-bob commented Jan 23, 2023 •

edited

Loading

sourcegraph-bot commented Jan 23, 2023 •

edited

Loading