Skip to content

Commit

Permalink
Use ApiKey#owner instead of user
Browse files Browse the repository at this point in the history
To be merged once backfill has run

Part of the work to support trusted publishers
  • Loading branch information
segiddins committed Nov 27, 2023
1 parent eb6aa51 commit 00bc4cd
Show file tree
Hide file tree
Showing 56 changed files with 265 additions and 199 deletions.
5 changes: 4 additions & 1 deletion app/avo/resources/api_key_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ class ExpiredFilter < ScopeBooleanFilter; end

field :name, as: :text, link_to_resource: true
field :hashed_key, as: :text, visible: ->(_) { false }
field :user, as: :belongs_to
field :user, as: :belongs_to, visible: ->(_) { false }
field :owner, as: :belongs_to,
polymorphic_as: :owner,
types: [::User]
field :last_accessed_at, as: :date_time
field :soft_deleted_at, as: :date_time
field :soft_deleted_rubygem_name, as: :text
Expand Down
14 changes: 9 additions & 5 deletions app/controllers/api/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,23 @@ def verify_with_otp
def verify_mfa_requirement
if @rubygem && !@rubygem.mfa_requirement_satisfied_for?(@api_key.user)
render plain: "Gem requires MFA enabled; You do not have MFA enabled yet.", status: :forbidden
elsif @api_key.user.mfa_required_not_yet_enabled?
elsif @api_key.mfa_required_not_yet_enabled?
render_mfa_setup_required_error
elsif @api_key.user.mfa_required_weak_level_enabled?
elsif @api_key.mfa_required_weak_level_enabled?
render_mfa_strong_level_required_error
end
end

def response_with_mfa_warning(response)
message = response
if @api_key.user.mfa_recommended_not_yet_enabled?
if @api_key.mfa_recommended_not_yet_enabled?
message += <<~WARN.chomp
[WARNING] For protection of your account and gems, we encourage you to set up multi-factor authentication \
at https://rubygems.org/multifactor_auth/new. Your account will be required to have MFA enabled in the future.
WARN
elsif @api_key.user.mfa_recommended_weak_level_enabled?
elsif @api_key.mfa_recommended_weak_level_enabled?
message += <<~WARN.chomp
Expand Down Expand Up @@ -99,10 +99,14 @@ def authenticate_with_api_key
hashed_key = Digest::SHA256.hexdigest(params_key)
@api_key = ApiKey.unexpired.find_by_hashed_key(hashed_key)
return render_unauthorized unless @api_key
set_tags "gemcutter.user.id" => @api_key.user_id, "gemcutter.user.api_key_id" => @api_key.id
set_tags "gemcutter.api_key.owner" => @api_key.owner.to_gid, "gemcutter.user.api_key_id" => @api_key.id
render_soft_deleted_api_key if @api_key.soft_deleted?
end

def verify_user_api_key
render_api_key_forbidden if @api_key.user.blank?
end

def render_unauthorized
render plain: t(:please_sign_up), status: :unauthorized
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/v1/api_keys_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def create

check_mfa(user) do
key = generate_unique_rubygems_key
build_params = { user: user, hashed_key: hashed_key(key), **api_key_create_params }
build_params = { owner: user, hashed_key: hashed_key(key), **api_key_create_params }
api_key = ApiKey.new(build_params)

save_and_respond(api_key, key)
Expand Down
1 change: 1 addition & 0 deletions app/controllers/api/v1/deletions_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
class Api::V1::DeletionsController < Api::BaseController
before_action :authenticate_with_api_key
before_action :verify_user_api_key
before_action :find_rubygem_by_name
before_action :verify_api_key_gem_scope
before_action :validate_gem_and_version
Expand Down
5 changes: 3 additions & 2 deletions app/controllers/api/v1/github_secret_scanning_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def revoke
resp = []
tokens.each do |t|
api_key = ApiKey.find_by(hashed_key: hashed_key(t[:token]))
label = if api_key&.destroy
label = if api_key&.expire!
schedule_revoke_email(api_key, t[:url])
"true_positive"
else
Expand All @@ -50,7 +50,8 @@ def revoke
private

def schedule_revoke_email(api_key, url)
Mailer.api_key_revoked(api_key.user_id, api_key.name, api_key.enabled_scopes.join(", "), url).deliver_later
return unless api_key.user?
Mailer.api_key_revoked(api_key.owner_id, api_key.name, api_key.enabled_scopes.join(", "), url).deliver_later
end

def secret_scanning_key(key_id)
Expand Down
1 change: 1 addition & 0 deletions app/controllers/api/v1/oidc/api_key_roles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ class Api::V1::OIDC::ApiKeyRolesController < Api::BaseController
include ApiKeyable

before_action :authenticate_with_api_key, except: :assume_role
before_action :verify_user_api_key, except: :assume_role

with_options only: :assume_role do
before_action :set_api_key_role
Expand Down
1 change: 1 addition & 0 deletions app/controllers/api/v1/oidc/id_tokens_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
class Api::V1::OIDC::IdTokensController < Api::BaseController
before_action :authenticate_with_api_key
before_action :verify_user_api_key

def index
render json: @api_key.user.oidc_id_tokens
Expand Down
1 change: 1 addition & 0 deletions app/controllers/api/v1/oidc/providers_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
class Api::V1::OIDC::ProvidersController < Api::BaseController
before_action :authenticate_with_api_key
before_action :verify_user_api_key

def index
render json: OIDC::Provider.all
Expand Down
1 change: 1 addition & 0 deletions app/controllers/api/v1/owners_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
class Api::V1::OwnersController < Api::BaseController
before_action :authenticate_with_api_key, except: %i[show gems]
before_action :verify_user_api_key, except: %i[show gems]
before_action :find_rubygem, except: :gems
before_action :verify_api_key_gem_scope, except: %i[show gems]
before_action :verify_gem_ownership, except: %i[show gems]
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/api/v1/rubygems_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
class Api::V1::RubygemsController < Api::BaseController
before_action :authenticate_with_api_key, except: %i[show reverse_dependencies]
before_action :find_rubygem, only: %i[show reverse_dependencies]
before_action :verify_user_api_key, except: %i[show reverse_dependencies create]
before_action :find_rubygem, only: %i[show reverse_dependencies]
before_action :cors_preflight_check, only: :show
before_action :verify_with_otp, only: %i[create]
before_action :verify_mfa_requirement, only: %i[create]
Expand Down
1 change: 1 addition & 0 deletions app/controllers/api/v1/web_hooks_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
class Api::V1::WebHooksController < Api::BaseController
before_action :authenticate_with_api_key
before_action :verify_user_api_key
before_action :render_api_key_forbidden, if: :api_key_unauthorized?
before_action :find_rubygem_by_name, :set_url, except: :index

Expand Down
4 changes: 2 additions & 2 deletions app/controllers/api/v1/webauthn_verifications_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ def status
def authenticate_with_credentials
params_key = request.headers["Authorization"] || ""
hashed_key = Digest::SHA256.hexdigest(params_key)
api_key = ApiKey.find_by_hashed_key(hashed_key)
api_key = ApiKey.unexpired.find_by_hashed_key(hashed_key)

@user = authenticated_user(api_key)
end

def authenticated_user(api_key)
return api_key.user if api_key
return api_key.user if api_key&.user?
authenticate_or_request_with_http_basic do |username, password|
User.authenticate(username.strip, password)
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api_keys_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def edit

def create
key = generate_unique_rubygems_key
build_params = { user: current_user, hashed_key: hashed_key(key), **api_key_params }
build_params = { owner: current_user, hashed_key: hashed_key(key), **api_key_params }
@api_key = ApiKey.new(build_params)

if @api_key.errors.present?
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/dashboards_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def authenticate_with_api_key
hashed_key = Digest::SHA256.hexdigest(params_key)
return unless (@api_key = ApiKey.unexpired.find_by_hashed_key(hashed_key))

set_tags "gemcutter.user.id" => @api_key.user_id, "gemcutter.user.api_key_id" => @api_key.id
set_tags "gemcutter.api_key.owner" => @api_key.owner.to_gid, "gemcutter.api_key" => @api_key.to_gid
render plain: "An invalid API key cannot be used. Please delete it and create a new one.", status: :forbidden if @api_key.soft_deleted?
end

Expand Down
4 changes: 2 additions & 2 deletions app/mailers/mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ def notifiers_changed(user_id)
default: "You changed your RubyGems.org email notification settings")
end

def gem_pushed(pushed_by_user_id, version_id, notified_user_id)
def gem_pushed(pushed_by, version_id, notified_user_id)
@version = Version.find(version_id)
notified_user = User.find(notified_user_id)
@pushed_by_user = User.find(pushed_by_user_id)
@pushed_by_user = pushed_by

mail to: notified_user.email,
subject: I18n.t("mailer.gem_pushed.subject", gem: @version.to_title, host: Gemcutter::HOST_DISPLAY,
Expand Down
16 changes: 15 additions & 1 deletion app/models/api_key.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ class ApiKey < ApplicationRecord
API_SCOPES = %i[index_rubygems push_rubygem yank_rubygem add_owner remove_owner access_webhooks show_dashboard].freeze
APPLICABLE_GEM_API_SCOPES = %i[push_rubygem yank_rubygem add_owner remove_owner].freeze

belongs_to :user, inverse_of: :api_keys
self.ignored_columns += %w[user_id]

belongs_to :owner, polymorphic: true

has_one :api_key_rubygem_scope, dependent: :destroy
Expand Down Expand Up @@ -47,13 +48,26 @@ def enabled_scopes
end
end

def user
owner if user?
end

def user?
owner_type == "User"
end

delegate :mfa_required_not_yet_enabled?, :mfa_required_weak_level_enabled?,
:mfa_recommended_not_yet_enabled?, :mfa_recommended_weak_level_enabled?,
to: :user, allow_nil: true

def mfa_authorized?(otp)
return true unless mfa_enabled?
return true if oidc_id_token.present?
user.api_mfa_verified?(otp)
end

def mfa_enabled?
return false unless user?
return false unless user.mfa_enabled?
user.mfa_ui_and_api? || mfa
end
Expand Down
58 changes: 26 additions & 32 deletions app/models/pusher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,26 @@ class Pusher
include TraceTagger
include SemanticLogger::Loggable

attr_reader :api_key, :user, :spec, :spec_contents, :message, :code, :rubygem, :body, :version, :version_id, :size

def initialize(api_key, body, remote_ip = "", scoped_rubygem = nil)
# this is ugly, but easier than updating all the unit tests, for now
case api_key
when ApiKey
@api_key = api_key
@user = api_key.user
raise ArgumentError if scoped_rubygem
scoped_rubygem = api_key.rubygem
else
@user = api_key
end
attr_reader :api_key, :owner, :spec, :spec_contents, :message, :code, :rubygem, :body, :version, :version_id, :size

def initialize(api_key, body, remote_ip = "")
@api_key = api_key
@owner = api_key.owner
@scoped_rubygem = api_key.rubygem

@body = StringIO.new(body.read)
@size = @body.size
@remote_ip = remote_ip
@scoped_rubygem = scoped_rubygem
end

def process
trace("gemcutter.pusher.process", tags: { "gemcutter.user.id" => user.id }) do
trace("gemcutter.pusher.process", tags: { "gemcutter.api_key.owner" => owner.to_gid }) do
pull_spec && find && authorize && verify_gem_scope && verify_mfa_requirement && validate && save
end
end

def authorize
rubygem.pushable? || rubygem.owned_by?(user) || notify_unauthorized
(rubygem.pushable? && api_key.user?) || owner.owns_gem?(rubygem) || notify_unauthorized
end

def verify_gem_scope
Expand All @@ -41,7 +33,7 @@ def verify_gem_scope
end

def verify_mfa_requirement
user.mfa_enabled? || !(version_mfa_required? || rubygem.metadata_mfa_required?) ||
api_key.mfa_enabled? || !(version_mfa_required? || rubygem.metadata_mfa_required?) ||
notify("Rubygem requires owners to enable MFA. You must enable MFA before pushing new version.", 403)
end

Expand All @@ -67,11 +59,11 @@ def save
write_gem @body, @spec_contents
end
rescue ArgumentError => e
@version.destroy
@version&.destroy
Rails.error.report(e, handled: true)
notify("There was a problem saving your gem. #{e}", 400)
rescue StandardError => e
@version.destroy
@version&.destroy
Rails.error.report(e, handled: true)
notify("There was a problem saving your gem. Please try again.", 500)
else
Expand All @@ -94,7 +86,7 @@ def pull_spec
MSG
end

def find
def find # rubocop:disable Metrics/AbcSize
name = spec.name.to_s
set_tag "gemcutter.rubygem.name", name

Expand Down Expand Up @@ -124,7 +116,7 @@ def find
size: size,
sha256: sha256,
spec_sha256: spec_sha256,
pusher: user,
pusher: api_key.user,
pusher_api_key: api_key,
cert_chain: spec.cert_chain

Expand All @@ -136,7 +128,7 @@ def find

# Overridden so we don't get megabytes of the raw data printing out
def inspect
attrs = %i[@rubygem @user @message @code].map do |attr|
attrs = %i[@rubygem @owner @message @code].map do |attr|
"#{attr}=#{instance_variable_get(attr).inspect}"
end
"<Pusher #{attrs.join(' ')}>"
Expand All @@ -147,7 +139,7 @@ def inspect
def after_write
@version_id = version.id
version.rubygem.push_notifiable_owners.each do |notified_user|
Mailer.gem_pushed(user.id, @version_id, notified_user.id).deliver_later
Mailer.gem_pushed(owner, @version_id, notified_user.id).deliver_later
end
Indexer.perform_later
UploadVersionsFileJob.perform_later
Expand All @@ -156,18 +148,18 @@ def after_write
ReindexRubygemJob.perform_later(rubygem:)
GemCachePurger.call(rubygem.name)
StoreVersionContentsJob.perform_later(version:) if ld_variation(key: "gemcutter.pusher.store_version_contents", default: false)
RackAttackReset.gem_push_backoff(@remote_ip, @user.display_id) if @remote_ip.present?
RackAttackReset.gem_push_backoff(@remote_ip, owner.to_gid) if @remote_ip.present?
StatsD.increment "push.success"
end

def ld_variation(key:, default:)
Rails.configuration.launch_darkly_client.variation(
key, user.ld_context, default
key, owner.ld_context, default
)
end

def notify(message, code)
logger.info { { message:, code:, user: user.id, api_key: api_key&.id, rubygem: rubygem&.name, version: version&.full_name } }
logger.info { { message:, code:, owner: owner, api_key: api_key&.id, rubygem: rubygem&.name, version: version&.full_name } }

@message = message
@code = code
Expand All @@ -177,7 +169,7 @@ def notify(message, code)
def update
rubygem.disown if rubygem.versions.indexed.count.zero?
rubygem.update_attributes_from_gem_specification!(version, spec)
rubygem.create_ownership(user)
rubygem.create_ownership(owner)
set_info_checksum

true
Expand All @@ -197,18 +189,20 @@ def republish_notification(version)
"Please bump the version number and push a new different release.\n" \
"See also `gem yank` if you want to unpublish the bad release.", 409)
else
different_owner = "pushed by a previous owner of this gem " unless version.rubygem.owners.include?(@user)
different_owner = "pushed by a previous owner of this gem " unless owner.owns_gem?(version.rubygem)
notify("A yanked version #{different_owner}already exists (#{version.full_name}).\n" \
"Repushing of gem versions is not allowed. Please use a new version and retry", 409)
end
end

def notify_unauthorized
if rubygem.unconfirmed_ownership?(user)
if !api_key.user?
notify("You are not allowed to push this gem.", 403)
elsif rubygem.unconfirmed_ownership?(owner)
notify("You do not have permission to push to this gem. " \
"Please confirm the ownership by clicking on the confirmation link sent your email #{user.email}", 403)
"Please confirm the ownership by clicking on the confirmation link sent your email #{owner.email}", 403)
else
notify("You do not have permission to push to this gem. Ask an owner to add you with: gem owner #{rubygem.name} --add #{user.email}", 403)
notify("You do not have permission to push to this gem. Ask an owner to add you with: gem owner #{rubygem.name} --add #{owner.email}", 403)
end
end

Expand Down Expand Up @@ -264,7 +258,7 @@ def log_pushing
}
end

{ message: "Pushing gem", version:, rubygem: @version.rubygem.name, pusher: user.as_json }
{ message: "Pushing gem", version:, rubygem: @version.rubygem.name, pusher: owner.as_json }
end
end

Expand Down

0 comments on commit 00bc4cd

Please sign in to comment.