ADO: Implement user permissions sync by indradhanush · Pull Request #48209 · sourcegraph/sourcegraph-public-snapshot

indradhanush · 2023-02-24T18:47:54Z

Originally created #48041 for early feedback. I've addressed those and created a new PR because it diverged a lot from the original branch.

Major progress on #47165. There's a little bit of follow up work required which would add more code to this PR - the implementation of ValidationCheck on the authz provider. I will follow up with a new PR, but the absence of it does not prevent a good configuration of ADO from working.

Note to reviewers

Not necessarily a big PR feature wise, ~500 lines is test code. But this does touch multiple modules that would make it hard to piece together for anyone not too familiar with the permissions syncing code. I've broken it down into sequential commits that should make sense when viewed in isolation for the most part. Each commit has a description describing the context and what to expect next. So it might be easier to view each commit at a time and then the PR as a whole.

Also note the usage of authn and authz in the commit messages.

Test plan

Added tests
Tested locally
- Part 1: https://www.loom.com/share/1fad17d488c84a60832ff5931cae196d
- Part 2: https://www.loom.com/share/f23e1e5cff3f41c38b47856b71c0f5f7
Builds should pass

The Azure Dev Ops API expects some custom key-value pairs in the POST body for refreshing the token. This commit adds support for custom key-value pairs so that the azuredevops authz provider can pass them in the token refresher func.

This will be needed to create a new azuredevops authz provider from the site config. Just a new type has been added here. The other changes are alphabetical sorting of the other providers for future readers.

In this commit we add some helpers and modify the client to support refreshing expired / close to expiry on the fly as part of the request.Do call path. We also rename azureDevOpsServicesURL to AZURE_DEV_OPS_API_URL because we will need it outside this package. Also we export HTTPError and modify it's Error() method to drop the body from the response because the body is raw HTML which pollutes the logs and error strings. Finally, we also add a helper GetRedirectURL that will be used in both ADO auth and authz providers.

When creating an external account we want to use AZURE_DEV_OPS_API_URL as the ServiceID when storing an entry in the user_external_accounts table because this is the URL used in the external service. We're not able to use s.ServiceID in the assignment because it would incorrectly assign the value of VISUAL_STUDIO_APP_URL to the service ID. The result of this would then be: During the permission sync, the ADO authz provider created uses AZURE_DEV_OPS_API_URL as the service ID. But the entry in the user_external_accounts_table would have the value of VISUAL_STUDIO_APP_URL as the ServiceID. Leading to a mismatch and the permissions syncing code falsely thinking that no external account exists for this authz provider. So why even use the VISUAL_STUDIO_APP_URL as the service ID in the auth provider? This is because that's the URL that is used for all oauth2 requests. While the API for product features like repos, orgs, user profiles is pointed to by the value of AZURE_DEV_OPS_API_URL leading to a deviation in expectations.

Instead of continuing to use the `authorization` pattern similar to other code hosts, it is simpler to add a boolean which has an explicit name and describes what it does better. The end result: we start to break away from the `authorization` pattern (some other code hosts can do that as well, maybe this is the start of it all) with an additional check for this new field in the write path of external_services.Create, where we check to ensure the correct value of `unrestricted` is written to the database. For existing auth providers, this is forwards compatible. While at the same time if they also wanted to get on the `enforcePermissions` way of life in the future, the change to `recalculateFields` will let them be backwards compatible. Also a small tweak to the doc string to make things less confusing.

This commit implements the permissions syncing for azuredevops code host connections.

indradhanush · 2023-02-28T14:13:15Z

After chatting with @kopancek and @pjlast on Slack, I've added a fix for handling multiple code host connections. I will add a test for this tomorrow as I need to step out now. Ran out of time. ⌛

sashaostrikov

Great work! Left a bunch of comments, but nothing is critical, mostly related to how the code is written and not about the correctness of implementation :)

sashaostrikov · 2023-02-28T14:46:31Z

+
+					l.Debug("skipping org",
+						log.String("org", org),
+						log.Int("http status code", httpErr.StatusCode),
+					)
+
+					continue


I'd make this log a WARN (and change the message a bit), because this errors if not surfaced, should be properly logged at least.
Also, removing unnecessary empty lines

Suggested change

l.Debug("skipping org",

log.String("org", org),

log.Int("http status code", httpErr.StatusCode),

)

continue

l.Warn("failed to list organization repositories",

log.String("org", org),

log.Int("http status code", httpErr.StatusCode),

)

continue

same comment applies to below projects piece of logic

Hmm. I wouldn't log it as a warn since that will show up on more or less every permissions sync - I'm guessing not all users have access to all orgs / projects. I'm keeping it as a DEBUG for times if we have to debug a permissions issue and we want to verify that this org or project is explicitly being skipped because of a 4xx error - that is the user does not have access to it.

okay, in this case I would make the log message a bit more elaborate. seeing "skipping org" in logs would make me confused :)

Good idea. I changed it to: user does not have access to this org. What do you think?

Also reduce code duplication.

kopancek · 2023-03-01T12:03:31Z

+    "enforcePermissions": {
+      "description": "A flag to enforce Azure DevOps repository access permissions",
+      "type": "boolean",
+      "default": false
+    },


Please, add docs for this once you start making docs changes. This is a very big step forward, but also completely different to what we configure for other code hosts to enable permissions.

Yes absolutely. I am going to add a doc on configuring Azure OAuth provider and will cover this in there. Is that what you were also suggesting?

So this replaces the authorization: {} thing from the other code host connections, yes?

It kinda makes me nervous that we do this only for one new provider now. Somehow I'd rather have us introduce to every provider at the same time and keep backwards compatibility. But I guess we can still do that.

I agree why it could make anyone nervous. When I was looking at this - the change turned out to be reasonably simple and easy to implement: https://github.com/sourcegraph/sourcegraph/pull/48209/files#diff-e64c2422ddc2f4da136695fcb608eee988e39ac2fb9ed1975646bf3f9bba7766R1612-R1615

Which gave me a high degree of confidence that this will not break for existing code hosts on the authorization: {} pattern. And it felt weird to continue to add on to this pattern as a result since the end user site-admin experience of configuring Azure DevOps with permisisons sync wouldn't be very intuitive.

Release timing wise this might be a better time to introduce this pattern limited to Azure DevOps and then refactor the existing ones on the new pattern after the upcoming release so we have sufficient time with testing and knowing if anything has gone wrong until the next release.

kopancek · 2023-03-01T12:05:37Z

+				// DEBUGGING NOTE: The token refresher (internal/oauthutil/token.go:newTokenRequest)
+				// adds some default key-value pairs to the body, some of which are eventually
+				// overridden by the values in this map. But some extra arg remain in the body that
+				// is sent in the request. This works for now, but if refreshing a token ever stops
+				// working for Azure Dev Ops this is a good place to start looking by writing a
+				// custom implementation that only sends the key-value pairs that the API expects.


Since you are aware of this problem, wouldn't it be a better architecture to actually do what you suggest? E.g. add a function on the context that would provide URL query parameters and default to the original one if it's not present?

If by context you mean OAuthContext here, then from what I understand you're suggesting:

Add a method on OAuthContext like: func (o *OAuthContext) AddExtraArgs()

Call that instead of reading from CustomQueryParams?

If it's not a blocker, I would like to keep it as is - for now, since this PR is big enough and because we do a very similar workflow in the OAuth login by passing in extra args as part of the Exchange method call.

Yes, that's what I had in mind. Not a blocker, just a suggestion for future

Noted. I can follow up once this is merged. 👍

sourcegraph-bot · 2023-03-01T14:22:19Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff bb06945...c19633e.

Notify	File(s)
@eseliger	internal/database/external_services.go internal/extsvc/azuredevops/client.go internal/extsvc/azuredevops/types.go internal/extsvc/azuredevops/users.go
@unknwon	enterprise/cmd/frontend/internal/auth/azureoauth/login.go enterprise/cmd/frontend/internal/auth/azureoauth/provider.go enterprise/cmd/frontend/internal/auth/azureoauth/session.go enterprise/internal/authz/authz.go enterprise/internal/authz/authz_test.go enterprise/internal/authz/azuredevops/provider.go enterprise/internal/authz/azuredevops/provider_test.go

It's okay.

sashaostrikov

At least all of my comments are resolved -- LGTM

kopancek

Approving to unblock, but left some comments for future PR maybe :)

kopancek · 2023-03-02T09:45:09Z

+}
+
+func (p *Provider) FetchRepoPerms(ctx context.Context, repo *extsvc.Repository, opts authz.FetchPermsOptions) ([]extsvc.AccountID, error) {
+	return nil, authz.ErrUnimplemented{Feature: "azuredevops.FetchRepoPerms"}


Can you add a comment about it to the code?

kopancek · 2023-03-02T09:45:47Z

+}
+
+func GetOAuthContext(refreshToken string) (*oauthutil.OAuthContext, error) {
+	for _, authProvider := range conf.SiteConfig().AuthProviders {


I still think the above ☝️ would be a nice improvement to the code.

kopancek · 2023-03-02T09:46:50Z

+				// DEBUGGING NOTE: The token refresher (internal/oauthutil/token.go:newTokenRequest)
+				// adds some default key-value pairs to the body, some of which are eventually
+				// overridden by the values in this map. But some extra arg remain in the body that
+				// is sent in the request. This works for now, but if refreshing a token ever stops
+				// working for Azure Dev Ops this is a good place to start looking by writing a
+				// custom implementation that only sends the key-value pairs that the API expects.


Yes, that's what I had in mind. Not a blocker, just a suggestion for future

pjlast · 2023-03-02T11:05:29Z

+      "description": "A flag to enforce Azure DevOps repository access permissions",
+      "type": "boolean",
+      "default": false
+    },


GitHub won't allow me to comment further down, so I'm going to leave the comment here.
We can consider making the "what repos do I sync" part of the config a bit more expressive. Here's the scenario as I see it:

There are organizations, projects, and repositories

If you want to sync all repositories from an organization, you only specify that organization. It is redundant to be able to specify org/project if you've already specified org

Ditto for org/project/repository

So, construct the object in such a way that we get an object as follows:

"repositories": [ { "org": "orgName", // required "projects": [ // optional { "project": "projectName", // required "repositoryIds": [ "1", "2" ] // optional } ] } ]

So now you cannot specify redundant fields. You either specify an entire org, or you specify an org and some projects, or you specify an org and some projects and repo IDs for some projects. Or any combination of these.

Not a blocker, but I think "ergonomically" it makes a lot of sense.

This is a good callout but I'm also not sure if the propsed structure would be less confusing since we nest org and projects under repositories - which feels wrong hierarchically since repos are part of an org and not the other way round.

We could however make our backend smarter to ignore projects if orgs have already been synced.

Let's make this a separate discussion and get others opinions.

I mean I just called it repositories to give it a name. It's just a JSON key you can name it anything

But yes, splitting the projects by org and dropping redundant ones is the other option

Continuing the discussion here: https://github.com/sourcegraph/sourcegraph/discussions/48536

Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>

pjlast · 2023-03-02T12:01:47Z

+	for _, c := range conns {
+		// The list of orgs and projects may have duplicates if there are multiple Azure DevOps code
+		// host connections that have the same project in their config.
+		//
+		// Add them to a map so that we may filter out duplicates before passing them over to the
+		// provider.
+		for _, name := range c.Orgs {
+			orgs[name] = struct{}{}
+		}
+
+		for _, name := range c.Projects {
+			projects[name] = struct{}{}
+		}
+	}


One thing I forgot to mention here: you need to make sure that you are only doing this for connections with authorization enabled, otherwise perms syncing will possibly be fetching a whole lot of redundant orgs and projects.

Just saw this. Will fix in a follow up.

Follow up here: #48549

cla-bot Bot added the cla-signed label Feb 24, 2023

indradhanush force-pushed the ig/ado-user-perms-backup branch 2 times, most recently from 95bf1c9 to d7061d1 Compare February 28, 2023 06:02

indradhanush added 2 commits February 28, 2023 11:37

internal/type: Add new type AzureDevOpsConnection

3f0f9e6

This will be needed to create a new azuredevops authz provider from the site config. Just a new type has been added here. The other changes are alphabetical sorting of the other providers for future readers.

indradhanush force-pushed the ig/ado-user-perms-backup branch 2 times, most recently from dc20e83 to b892b26 Compare February 28, 2023 10:35

indradhanush added 4 commits February 28, 2023 16:29

internal/extsvc/azuredevops: Add trailing slash to VISUAL_STUDIO_APP_URL

d0d95c3

indradhanush force-pushed the ig/ado-user-perms-backup branch 2 times, most recently from 90610ff to 7ff128d Compare February 28, 2023 12:06

indradhanush changed the title ~~Ig/ado user perms backup~~ ADO: Implement user permissions sync Feb 28, 2023

indradhanush added 3 commits February 28, 2023 17:40

enterprise/internal/authz: Implement new provider for ADO

83ed325

This commit implements the permissions syncing for azuredevops code host connections.

authz/azuredevops: Add provider tests

d27fbf1

enterprise/internal/authz: Fix failing test

b0773d0

indradhanush marked this pull request as ready for review February 28, 2023 12:10

indradhanush force-pushed the ig/ado-user-perms-backup branch from 7ff128d to b0773d0 Compare February 28, 2023 12:10

indradhanush mentioned this pull request Feb 28, 2023

WIP: ADO permissions syncing #48041

Closed

empty commit to trigger CI

cd87fe6

indradhanush requested review from a team February 28, 2023 12:17

indradhanush self-assigned this Feb 28, 2023

indradhanush added feature/permissions ADO Tier 1 support for Azure DevOps labels Feb 28, 2023

authz/azuredevops: Handle multiple code host connections

fbe34ea

sashaostrikov reviewed Feb 28, 2023

View reviewed changes

authz/azuredevops: Add tests for multiple code host connections

5a2ecf5

Also reduce code duplication.

kopancek reviewed Mar 1, 2023

View reviewed changes

indradhanush added 3 commits March 1, 2023 18:11

Improve log message, handle ignored auth error and use t.Cleanup

a187c0e

Address code review comments

92ef989

s/CustomURLArgs/CustomQueryParams

9f9b506

pjlast reviewed Mar 1, 2023

View reviewed changes

indradhanush added 2 commits March 1, 2023 18:27

Make client assertion type a const, also not continue on error

582a9ba

internal/authz: Filter duplicate orgs/projects during provider creation

5616b9c

indradhanush force-pushed the ig/ado-user-perms-backup branch from 4d7abe7 to 5616b9c Compare March 1, 2023 13:28

intenral/authz: Fix linting and test flake

2851e09

indradhanush added 3 commits March 1, 2023 19:54

internal/extsvc: Ignore error on resp.Body.Close()

71d200f

It's okay.

internal/authz: Use ServiceID() as the client URN

bbef21a

internal/authz: Simpler checks and remove redundant checks

e527d81

indradhanush requested review from a team March 1, 2023 14:41

go lint

5c25d9a

indradhanush commented Mar 1, 2023

View reviewed changes

Comment thread enterprise/internal/authz/azuredevops/provider.go

This was referenced Mar 1, 2023

internal/authz/azuredevops: Implement ValidateConnection #48465

Merged

Implement user permissions sync for Azure code host #47165

Closed

sashaostrikov approved these changes Mar 2, 2023

View reviewed changes

kopancek approved these changes Mar 2, 2023

View reviewed changes

indradhanush mentioned this pull request Mar 2, 2023

Add support for Azure as an auth provider #47163

Closed

10 tasks

pjlast approved these changes Mar 2, 2023

View reviewed changes

indradhanush and others added 3 commits March 2, 2023 16:44

Update enterprise/internal/authz/azuredevops/provider.go

c07cc0a

Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>

Fix import

0f3ad48

internal/authz/azuredevops: Add comment on FetchRepoPerms

c19633e

pjlast reviewed Mar 2, 2023

View reviewed changes

indradhanush merged commit 4b62fa2 into main Mar 2, 2023

indradhanush deleted the ig/ado-user-perms-backup branch March 2, 2023 12:15

indradhanush mentioned this pull request Mar 2, 2023

authz/azuredevops: Check enforcePermissions in NewAuthzProviders #48549

Merged

Conversation

indradhanush commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note to reviewers

Test plan

Uh oh!

indradhanush commented Feb 28, 2023

Uh oh!

sashaostrikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

sashaostrikov left a comment

Choose a reason for hiding this comment

Uh oh!

kopancek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

indradhanush commented Feb 24, 2023 •

edited

Loading

sourcegraph-bot commented Mar 1, 2023 •

edited

Loading

indradhanush Mar 2, 2023 •

edited

Loading