cody: replace access-control feature flag with RBAC by emidoots · Pull Request #58831 · sourcegraph/sourcegraph-public-snapshot

emidoots · 2023-12-07T19:28:28Z

This change replaces the old feature-flag which controls users who have access to Cody, with an RBAC-based implementation instead.

After this change, cody.restrictUsersFeatureFlag is deprecated. For Sourcegraph instances that still have it set, it will still be respected it until the site admin removes it from their config. A warning is displayed to let admins know this needs to be done:

Once cody.restrictUsersFeatureFlag is not set in the site config, the new RBAC system is used instead: site admins can manage user roles, and assign/remove cody:access to users or groups of users via the access control web UI.

By default, all users have access (admins can change this default via the access control web UI.)

The new Cody RBAC system defaults to enabled unless cody.restrictUsersFeatureFlag is set in the site config. It can be disabled by setting the new cody.permissions site config option to false, in which case Sourcegraph behaves as it did before this change.

Fixes #58623
Helps (fixes?) #58624
Resolves #58627

Remaining TODOs before this PR can be merged

Some Go tests need updating / CI needs to pass
These docs need to be updated to tell site admins on new versions what to do, tell site admins on old versions how they can use the old docs, and tell site admins upgrading what to do (remove the old config, and then use the RBAC system going forward.) with the exact Sourcegraph versions mentioned.

Test plan

Created a non-admin user, signed into VS Code' Cody via that user, confirmed I have access by default.
Using the site admin account, change normal users'

sourcegraph-bot · 2023-12-07T19:36:32Z

📖 Storybook live preview

chwarwick · 2023-12-07T19:41:44Z

 }

-func (r *siteResolver) IsCodyEnabled(ctx context.Context) bool { return cody.IsCodyEnabled(ctx) }
+func (r *siteResolver) IsCodyEnabled(ctx context.Context) bool { return cody.IsCodyEnabled(ctx, r.db) }


Looks like it's always been this way but feels strange to me that the site resolver is returning if Cody is enabled for the current user, not is Cody enabled for the site.

Yeah, it is very (functional, but) strange. It's also weird that we have Cody access control enforced in the feature-flag portion of internal/cody.

I haven't set out to 'correct' this in this PR with the hopes of keeping it from increasing in scope/risk

chwarwick

If i'm following correctly the RBAC permissions are only used if the config value is set to enable permissions, which means the default case is that all users have access (same as today 👍 )

But what happens in the RBAC UI? Does it look like you are configuring Cody access for groups/all users but because it's not enabled in the config it isn't considered?

emidoots · 2024-01-03T23:39:39Z

If i'm following correctly the RBAC permissions are only used if the config value is set to enable permissions, which means the default case is that all users have access (same as today 👍 )

But what happens in the RBAC UI? Does it look like you are configuring Cody access for groups/all users but because it's not enabled in the config it isn't considered?

That is correct; however if the RBAC permissions are not being respected then the warning would display at the top of the page indicating you're using the old permission system:

The one edge-case here would be if a site admin explicitly disables the new RBAC permissions model (cody.permissions set to false). Eventually I would like to remove the ability for a site admin to do that at all (in +1 release, so many months have passed and everyone has had a chance to upgrade.)

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

Co-authored-by: Chris Warwick <christopher.warwick@sourcegraph.com>

…sabled Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

emidoots · 2024-01-10T05:54:57Z

@chwarwick or @eseliger would either of you be up for a quick review? I'd love to land this soon as it's blocking some other work :)

chwarwick

Nice, this looks correct to me. See note about new docs process.

chwarwick · 2024-01-10T15:16:45Z

 ## Enable Cody only for some users

-To enable Cody only for some users, for example, when rolling out a Cody POC, follow all the steps mentioned in [Enabling Cody on your Sourcegraph instance](#enable-cody-on-your-sourcegraph-instance). Then, use the feature flag `cody` to turn Cody on selectively for some users. To do so:
+To enable Cody only for some users, for example, when rolling out a Cody POC, follow all the steps mentioned in [Enabling Cody on your Sourcegraph instance](#enable-cody-on-your-sourcegraph-instance). Then, do the following:


Docs changes need to go into the new docs repo. I think having them here will fail the build?

chwarwick · 2024-01-10T15:20:25Z

+		}
+	})
+
+	t.Run("RBAC, Enabled cody, but not completions", func(t *testing.T) {


What does but not completions mean? This test looks the same as the one above.

Good catch, I mirror'd the existing good test suite but for the case of RBAC - and this ended up being redundant. Removed.

eseliger

LGTM, might warrant a changelog entry to make people aware prior to the upgrade that they need to migrate their access settings :)

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

This reverts commit cf59539.

…#59540) This reverts commit cf59539.

…58831)" (#59540)" This reverts commit 13923a7.

* Revert "Revert "cody: replace access-control feature flag with RBAC (#58831)" (#59540)" This reverts commit 13923a7. * context/resolvers: fix test by correctly mocking user role with Cody access Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com> --------- Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

cla-bot Bot added the cla-signed label Dec 7, 2023

chwarwick reviewed Dec 7, 2023

View reviewed changes

Comment thread cmd/frontend/internal/completions/resolvers/resolver.go Outdated

chwarwick reviewed Dec 7, 2023

View reviewed changes

emidoots force-pushed the sg/cody-rbac branch from 4769490 to c5fec62 Compare January 3, 2024 23:40

Stephen Gutekanst and others added 8 commits January 8, 2024 19:11

rbac: add Cody role based access control namespace

f023c24

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

add cody.permissions feature flag, cody access controlled by RBAC

fe42a62

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

add warning for instances with deprecated cody.restrictUsersFeatureFlag

7950701

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

improve Cody access restriction error message

5f08aca

Co-authored-by: Chris Warwick <christopher.warwick@sourcegraph.com>

Run bazel configure

36a4267

Update generated files

0ed14db

internal/cody: run IsCodyEnabled tests with new permissions system di…

f7d199f

…sabled Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

add RBAC tests

24e43f8

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

emidoots force-pushed the sg/cody-rbac branch from 32b3462 to 24e43f8 Compare January 9, 2024 08:25

Stephen Gutekanst added 2 commits January 9, 2024 22:24

Merge branch 'main' into sg/cody-rbac

53de2f7

doc: explain how to use new Cody RBAC permissions

e66054c

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

chwarwick approved these changes Jan 10, 2024

View reviewed changes

eseliger approved these changes Jan 10, 2024

View reviewed changes

Stephen Gutekanst added 2 commits January 10, 2024 17:05

remove redundant test case

0508cd0

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

CHANGELOG: mention Cody RBAC permissions change

b1cd18f

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

emidoots mentioned this pull request Jan 11, 2024

add Cody role based access control docs sourcegraph/docs#31

Merged

Stephen Gutekanst added 2 commits January 10, 2024 17:23

send docs to sourcegraph/docs#31

db50f55

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

Merge branch 'main' into sg/cody-rbac

4df0e93

emidoots enabled auto-merge (squash) January 11, 2024 01:13

Stephen Gutekanst added 3 commits January 10, 2024 18:42

cmd/frontend/internal: update some tests to mock RBAC

5248508

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

pnpm run format

b6705cd

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

bazel run @//dev:write_all_generated

33acbaf

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

jamesmcnamara added 2 commits January 11, 2024 18:20

generate well formated TS for RBAC constants

964fd6f

removed trailing space

6d52115

emidoots merged commit cf59539 into main Jan 12, 2024

emidoots deleted the sg/cody-rbac branch January 12, 2024 02:31

burmudar added a commit that referenced this pull request Jan 12, 2024

Revert "cody: replace access-control feature flag with RBAC (#58831)"

615c9fc

This reverts commit cf59539.

burmudar added a commit that referenced this pull request Jan 12, 2024

Revert "cody: replace access-control feature flag with RBAC (#58831)" (…

13923a7

…#59540) This reverts commit cf59539.

emidoots pushed a commit that referenced this pull request Jan 17, 2024

Revert "Revert "cody: replace access-control feature flag with RBAC (#…

d483afa

…58831)" (#59540)" This reverts commit 13923a7.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cody: replace access-control feature flag with RBAC#58831

cody: replace access-control feature flag with RBAC#58831
emidoots merged 19 commits into
mainfrom
sg/cody-rbac

emidoots commented Dec 7, 2023 •

edited

Loading

Uh oh!

sourcegraph-bot commented Dec 7, 2023 •

edited

Loading

Uh oh!

chwarwick Dec 7, 2023

Uh oh!

emidoots Dec 7, 2023 •

edited

Loading

Uh oh!

Uh oh!

chwarwick left a comment

Uh oh!

emidoots commented Jan 3, 2024

Uh oh!

emidoots commented Jan 10, 2024

Uh oh!

chwarwick left a comment

Uh oh!

chwarwick Jan 10, 2024

Uh oh!

chwarwick Jan 10, 2024

Uh oh!

emidoots Jan 11, 2024

Uh oh!

eseliger left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

emidoots commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Remaining TODOs before this PR can be merged

Test plan

Uh oh!

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

Uh oh!

Uh oh!

chwarwick Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

emidoots Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chwarwick left a comment

Choose a reason for hiding this comment

Uh oh!

emidoots commented Jan 3, 2024

Uh oh!

emidoots commented Jan 10, 2024

Uh oh!

chwarwick left a comment

Choose a reason for hiding this comment

Uh oh!

chwarwick Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

chwarwick Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

emidoots Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

eseliger left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

emidoots commented Dec 7, 2023 •

edited

Loading

sourcegraph-bot commented Dec 7, 2023 •

edited

Loading

emidoots Dec 7, 2023 •

edited

Loading