Push new gems to organizations - #6762
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6762 +/- ##
==========================================
+ Coverage 97.79% 97.82% +0.03%
==========================================
Files 533 537 +4
Lines 11584 11730 +146
==========================================
+ Hits 11328 11475 +147
+ Misses 256 255 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7520d3b to
7ae8903
Compare
Why this change is being made: To let a gem push be attributed to an organization, an API key needs to carry an organization scope. Scoping through Membership (rather than Organization directly) ties the grant to the user's continued membership, so it can be revoked automatically when the membership changes or ends. What were the changes made to support this: - Migration creating api_key_organization_scopes (unique FK to api_keys, FK to memberships) and adding api_keys.soft_deleted_organization_name - ApiKeyOrganizationScope model (uniqueness validation, delegate :organization) - has_one :api_key_organization_scope/:membership/:organization on ApiKey, has_many :api_key_organization_scopes on Membership - Factory and model tests
Why this change is being made: Organization-scoped keys must only be created by members who can manage the organization (admin or owner, matching OrganizationPolicy#add_gem?), must not be combined with a gem scope, and must only be usable for gems that belong to that organization or for claiming new gem names. What were the changes made to support this: - ApiKey#organization_id= / #organization_handle= setters resolving the membership via user.memberships.confirmed.with_minimum_role(:admin) - Validations: gem and organization scopes are mutually exclusive; organization scope requires an applicable scope (push/yank/owner); the organization must be manageable by the key's user - ApiKey#scoped_to?(rubygem) / #organization_allows_gem?, wired into #scope? - User#manageable_organizations - Api::BaseController#verify_api_key_gem_scope now uses scoped_to?, so yank and owner endpoints enforce the organization scope too - organization attribute recorded on the API_KEY_CREATED user event - Model tests plus deletions/owners controller tests for the enforcement
Why this change is being made: This is the customer request itself: pushing a brand-new gem with an organization-scoped key should place the gem under the organization instead of the pusher's personal account. What were the changes made to support this: - New verify_organization_scope step in Pusher#process rejecting pushes to gems outside the key's organization - Ownership assignment for unowned gems extracted into assign_ownership_for_unowned_gem; with an organization-scoped key it checks OrganizationPolicy#add_gem? and sets rubygem.organization_id instead of creating a personal Ownership - Pusher unit tests and api/v1 rubygems controller tests: a new gem lands under the org; pushing another user's or another org's gem returns 403
Why this change is being made: Organization admins need a way to mint organization-scoped keys; the new-API-key form currently only offers a gem scope. What were the changes made to support this: - Organization selector in the API key form listing User#manageable_organizations - gem_scope_controller.js extended so the gem and organization selectors are mutually exclusive - ApiKeysController permits organization_id - ApiKeysHelper#gem_scope displays the organization scope in the keys list - Locale strings in en.yml propagated with bin/fill-locales - System test for creating an org-scoped key
Why this change is being made: API keys are also created programmatically (gem signin and CI tooling use POST /api/v1/api_keys), so the organization scope must be available there as well. What were the changes made to support this: - Api::V1::ApiKeysController permits an organization (handle) param mapped through ApiKey#organization_handle= - Functional tests covering success, unknown handle, and insufficient membership role
Why this change is being made: When a member is demoted below admin or removed from the organization, their organization-scoped keys must stop working immediately; otherwise they would retain publishing rights they no longer have. What were the changes made to support this: - Membership after_update hook soft-deletes org-scoped keys when the role drops below admin - ApiKeyOrganizationScope before_destroy (when destroyed through the membership association) soft-deletes the key, recording soft_deleted_organization_name - ApiKey#soft_delete! accepts a membership; #soft_deleted_by_organization? plus a tooltip in the API keys list explain why the key was invalidated - Tests for demotion, member removal, and membership destruction
Why this change is being made: CI pipelines that push via OIDC assume_role receive short-lived user keys built from OIDC::ApiKeyPermissions; without an organization option those pushes can only create gems under the personal account. What were the changes made to support this: - organization (handle) attribute on OIDC::ApiKeyPermissions, mutually exclusive with gems - OIDC::ApiKeyRole validates the role owner can manage that organization (mirrors gems_belong_to_user) - OIDC::ApiKeyPermissions#create_params resolves the organization scope for the minted key - Organization field in the role form - Tests: permissions/role models plus an assume_role integration test proving the minted key is scoped to the organization
Why this change is being made: Trusted publishing of a brand-new gem reifies a pending publisher into a personal ownership; organization members need those gems to land under the organization so keyless CI publishing matches the other push paths. What were the changes made to support this: - Migration adding a nullable organization_id FK to oidc_pending_trusted_publishers - Optional belongs_to :organization with a validation that the creating user can manage the organization - Organization selector in the pending trusted publisher form - Pusher reification sets rubygem.organization_id (still creating the RubygemTrustedPublisher) instead of a personal ownership when the pending publisher targets an organization - Model, pusher, and controller tests for the new path
7ae8903 to
611b6f5
Compare
| update_attribute(:soft_deleted_at, Time.now.utc) | ||
| update_attribute(:soft_deleted_rubygem_name, ownership.rubygem.name) if ownership | ||
| def soft_delete!(ownership: nil, membership: nil) | ||
| update_columns( |
There was a problem hiding this comment.
This might be a subtle difference that I don't think matters in this case but update_columns skips the validations and callbacks and update_attribute only skips the validations not the callbacks. I think you can add touch: true to this to also change the updated_at
|
|
||
| def update | ||
| @api_key = current_user.api_keys.find(params.expect(:id)) | ||
| @api_key.assign_attributes(api_key_update_params(@api_key)) |
There was a problem hiding this comment.
I think we want to guard here against someone passing in both the rubygem_id and the organization_id at the same time.
When you do something like
# From a test case
should "not persist either scope when a request sends a gem and an organization together" do
@user = create(:user)
@organization = create(:organization, owners: [@user])
@ownership = create(:ownership, user: @user, rubygem: create(:rubygem))
@api_key = create(:api_key, owner: @user, scopes: %i[push_rubygem], rubygem: @ownership.rubygem)
verified_sign_in_as(@user)
patch :update, params: {
id: @api_key.id,
api_key: { push_rubygem: true, rubygem_id: @ownership.rubygem.id, organization_id: @organization.id }
}
@api_key.reload
refute @api_key.rubygem.present? && @api_key.organization.present?
endthe assign_attributes automatically creates the has_one ... through: for both the organization and the rubygem before the validations run but doesn't clean it up when the save fails later. After that the api_key gets stuck in an invalid state and can only be deleted.
| belongs_to :user | ||
| belongs_to :organization | ||
|
|
||
| has_many :api_key_organization_scopes, dependent: :destroy |
There was a problem hiding this comment.
I think there's a weird quirk here. The Organizations only get soft_deleted (the deleted_at gets set and the default scope ignores it). It's possible for an Organization to be soft_deleted without triggering the dependent: :destroy.
We will want to add a mechanism to whatever does the organization deletion to make sure we are also invalidating the api_keys related to it.
It looks like when you push a gem with an org scoped api key for an org that has been soft_deleted, it falls back to the User owning the gem
| static targets = ["checkbox", "selector"]; | ||
| static targets = ["checkbox", "gemSelector", "orgSelector"]; | ||
|
|
||
| connect() { |
There was a problem hiding this comment.
Should this be initialize() instead of connect? I think initialize is called first and only once and connect is called every time the controller is added to the DOM
| rubygem.create_ownership(pending_publisher.user) | ||
| owner.rubygem_trusted_publishers.create!(rubygem: rubygem) | ||
| end | ||
| assign_ownership_for_unowned_gem |
There was a problem hiding this comment.
If the rubygem is unowned we check that the owner can add it to the organization in #assign_organization_ownership below, but if that fails, we return notify_unauthorized. At that point we will have already created the version record and rubygem in the #update_attributes_from_gem_specification!. Does it make sense to add the ownership check earlier or rollback the transaction for the version/rubygem updates?
I'm not really sure how that would happen unless the key owner was an owner or admin and was downgraded to maintainer and we skipped the callbacks. But now that I think about it I'm wondering if you think maintainers should also be able to gem push? I don't think we have anything existing in orgs for setting permissions for what levels can do what
| div do | ||
| t(".api_key_gem_html", gem: link_to(additional.gem, rubygem_path(additional.gem))) | ||
| end | ||
| raw t(".api_key_gem_html", gem: view_context.link_to(additional.gem, rubygem_path(additional.gem))) if additional.gem.present? |
There was a problem hiding this comment.
Do these need to use the #raw method?
|
|
||
| form_with(model: pending_trusted_publisher, url: profile_oidc_pending_trusted_publishers_path) do |form| | ||
| rubygem_name_field(form) | ||
| organization_field(form) |
There was a problem hiding this comment.
Should we hid the org field if you don't belong to any orgs?
2 participants
Description
Closes #6328
Right now pushing a new gem always creates personal ownership for the pusher. Getting it into an organization means transferring/onboarding afterward.
This adds an optional organization scope on API keys (and the OIDC / trusted-publishing equivalents) so a first push can assign the gem to the org directly. Scope goes through the user's admin/owner membership, so it goes away when they lose that role.
Covers all three publish paths: classic
gem push, OIDCassume_role, and pending trusted publishers.Approach
api_key_organization_scopestable linking an API key to aMembership(admin/owner only). Mutually exclusive with gem scope.rubygem.organization_idinstead of creating personal ownership.api_key_permissions.organization) and pending trusted publishers (organization_id).ApiKey#scoped_to?.POST /api/v1/api_keyswithorganization=<handle>.How to test
1. Profile UI key +
gem pushPush rubygem, pick the org under Organization Scope. Create the key and copy it.Rubygem.find_by!(name: "your-gem").organizationshould be the org, andownershipsshould be empty. You should still be able to manage it as an org member.Also worth checking:
2. HTTP API key creation
Same push checks as above. Bad handle / org you don't admin should 422.
3. OIDC assume_role
push_rubygem."organization": "<handle>".4. Trusted publishing