Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

productsubscription: service accounts with RBAC, instead of feature flags#60796

Merged
bobheadxi merged 5 commits into
mainfrom
02-29-productsubscription_service_accounts_with_RBAC_instead_of_feature_flags
Mar 12, 2024
Merged

productsubscription: service accounts with RBAC, instead of feature flags#60796
bobheadxi merged 5 commits into
mainfrom
02-29-productsubscription_service_accounts_with_RBAC_instead_of_feature_flags

Conversation

@bobheadxi

@bobheadxi bobheadxi commented Feb 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Removes the feature flag service account stuff, replacing it with "real RBAC" on permissions introduced in https://github.com/sourcegraph/sourcegraph/pull/60795. Cody Gateway SAs will need to use roles with these permissions.

This change does not migrate all product-subscriptions-related CRUD to check for the new roles - it only updates the helper used by Cody Gateway related functionality. This could be used with Security team's "Entitler" service to help CEs manage subscriptions and licenses if we expand the RBAC checks, at least until RFC 885, which is still some time away.

Rollout plan would be to deploy https://github.com/sourcegraph/sourcegraph/pull/60795, create the appropriate role and assign it to the service accounts we have today, and then roll this change out.

Test plan

Automated tests, and basic manual testing:

  1. sg start dotcom
  2. Logged in as user with site admin: can CRUD on subscriptions
  3. Logged in as non-site admin user: cannot CRUD on subscriptions
    image
  4. Create a new role: image
  5. Assign to non-admin user: mutation { setRoles(user:"VXNlcjo0",roles:["Um9sZTo0"]) { __typename } } (must be done via API, as UI is currently disabled in dotcom mode)
  6. can now do some CRUD on subscriptions
  7. Remove the write permission from the role
  8. Get error on attempting to write to subscription, but read is still ok

@cla-bot cla-bot Bot added the cla-signed label Feb 29, 2024
@bobheadxi Graphite App

bobheadxi commented Feb 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bobheadxi and the rest of your teammates on Graphite Graphite

@bobheadxi bobheadxi mentioned this pull request Feb 29, 2024
Merged
@bobheadxi bobheadxi requested review from a team and BolajiOlajide February 29, 2024 12:31
@bobheadxi bobheadxi force-pushed the rbac-add-productsubscription-accounts branch from f667be3 to 4bfe7f4 Compare February 29, 2024 12:38
@bobheadxi bobheadxi force-pushed the 02-29-productsubscription_service_accounts_with_RBAC_instead_of_feature_flags branch from 8cdf436 to 9125390 Compare February 29, 2024 12:38
@bobheadxi bobheadxi force-pushed the rbac-add-productsubscription-accounts branch from 4bfe7f4 to d398418 Compare February 29, 2024 12:39
@bobheadxi bobheadxi force-pushed the 02-29-productsubscription_service_accounts_with_RBAC_instead_of_feature_flags branch 6 times, most recently from 9adbf60 to 0075927 Compare February 29, 2024 17:09
@bobheadxi bobheadxi force-pushed the rbac-add-productsubscription-accounts branch from 19ba930 to 93561ac Compare March 4, 2024 22:35
@bobheadxi bobheadxi force-pushed the 02-29-productsubscription_service_accounts_with_RBAC_instead_of_feature_flags branch 4 times, most recently from 02c7875 to 16ffae3 Compare March 4, 2024 23:16
@bobheadxi bobheadxi marked this pull request as ready for review March 4, 2024 23:44
@bobheadxi bobheadxi requested a review from a team March 4, 2024 23:49
eseliger
eseliger approved these changes Mar 4, 2024
View reviewed changes

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine, https://github.com/sourcegraph/sourcegraph/pull/60014 is probably gonna replace this once I get to it again :)

@bobheadxi

bobheadxi commented Mar 5, 2024

Copy link
Copy Markdown
Member Author

seems fine, https://github.com/sourcegraph/sourcegraph/pull/60014 is probably gonna replace this once I get to it again :)

Or, you could augment this by expanding on the same permissions and roles? 😉

BolajiOlajide
BolajiOlajide approved these changes Mar 5, 2024
View reviewed changes
Comment thread cmd/frontend/internal/dotcom/productsubscription/service_account.go Outdated
Comment thread cmd/frontend/internal/dotcom/productsubscription/service_account.go Outdated

@BolajiOlajide BolajiOlajide Mar 5, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The site-admin check is redundant. Every permission in a Sourcegraph instance is usually automaticallly applied to the SiteAdmin role.

So rbac.CheckCurrentUserHasPermission(ctx, db, rbac.ProductSubscriptionsWritePermission) will always return nil for site admins.

@bobheadxi bobheadxi Mar 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm unable to get this behaviour in tests - test cases that call db.Users().SetIsSiteAdmin(ctx, test.user.ID, true) leads to an unauthorized response

@bobheadxi bobheadxi Mar 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also kind of weird to make this work well in non-integration tests, where mocking permsStore.GetPermissionForUserFunc.SetDefaultHook means reimplementing this site admin behaviour

@bobheadxi bobheadxi Mar 11, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up keeping the explicit site-admin check in https://github.com/sourcegraph/sourcegraph/pull/60796/commits/a7e5dd1234bd871ae5c1f07fdace840f112a96d9 to cover those integration tests

@BolajiOlajide BolajiOlajide Mar 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's because in test environment we don't assign the permissions to the SiteAdmin role implicitly, you'll have to manually add it to the database in the setup for your tests.

As discussed in Merge, a test helper will be helpful to avoid folks running into this gotcha.

@bobheadxi bobheadxi Mar 11, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be neat if SetIsSiteAdmin assigned the role, but this is more a shortcoming of how we use DB directly so often leading to discrepancies like this, or maybe we should just get rid of site admin usage entirely

I think it's probably safer for this change to not change the nature of the checks too much, let's revisit this in a more concerted follow-up

@BolajiOlajide BolajiOlajide Mar 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, unfortunately SetIsSiteAdmin is tied to role assignment while the problem here is with permissions assignment.

But you're right, let's revisit this later when #58114 is merged. That would be a good time for this improvement I guess.

Base automatically changed from rbac-add-productsubscription-accounts to main March 11, 2024 07:01
bobheadxi referenced this pull request Mar 11, 2024
@bobheadxi @BolajiOlajide
rbac: add dotcom-only roles and productsubscription roles (#60795)
55a5ece 
Adds productsubscription RBAC roles (read and write), with the intent of replacing "cody gateway feature flag service account": https://github.com/sourcegraph/sourcegraph/pull/60796

This _could_ be used with Security team's "Entitler" service to help CEs manage subscriptions and licenses, at least until [RFC 885](https://docs.google.com/document/d/1tiaW1IVKm_YSSYhH-z7Q8sv4HSO_YJ_Uu6eYDjX7uU4/edit#heading=h.tdaxc5h34u7q)

---------

Co-authored-by: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com>
@bobheadxi bobheadxi force-pushed the 02-29-productsubscription_service_accounts_with_RBAC_instead_of_feature_flags branch from f6737ec to a7e5dd1 Compare March 11, 2024 07:36
@bobheadxi

bobheadxi commented Mar 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Rolled out the roles (roles now available via GraphQL after https://github.com/sourcegraph/sourcegraph/pull/60992):

query {
  writer: user(username:"llm-proxy") {
    username
    roles {
      nodes {
        name
      }
    }
  }
  reader: user(username:"llm-proxy-readonly") {
    username
    roles {
      nodes {
        name
      }
    }
  }
}

{
  "data": {
    "writer": {
      "username": "llm-proxy",
      "roles": {
        "nodes": [
          {
            "name": "Product Subscriptions Writer"
          }
        ]
      }
    },
    "reader": {
      "username": "llm-proxy-readonly",
      "roles": {
        "nodes": [
          {
            "name": "Product Subscriptions Reader"
          }
        ]
      }
    }
  }
}

I will roll this out tomorrow

@bobheadxi bobheadxi enabled auto-merge (squash) March 12, 2024 09:07
@bobheadxi bobheadxi merged commit 876aa22 into main Mar 12, 2024
@bobheadxi bobheadxi deleted the 02-29-productsubscription_service_accounts_with_RBAC_instead_of_feature_flags branch March 12, 2024 09:17
@bobheadxi bobheadxi mentioned this pull request Mar 13, 2024
Merged
bobheadxi referenced this pull request in sourcegraph/handbook Mar 13, 2024
@bobheadxi
cody-gateway: update SA docs to mention the new RBAC roles (#8708)
5c2b05e 
Documents https://github.com/sourcegraph/sourcegraph/pull/60796 and
https://github.com/sourcegraph/sourcegraph/pull/60795. I rolled this out
yesterday without issue.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@BolajiOlajide BolajiOlajide BolajiOlajide approved these changes

@evict evict evict approved these changes

@eseliger eseliger eseliger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bobheadxi @evict @eseliger @BolajiOlajide