From 559288243f4ced31768ece9a6d4f87a9a92a78dc Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Thu, 18 Jul 2024 20:13:54 -0700 Subject: [PATCH 1/6] Expand owner list, show mfa status as an asterisk --- app/helpers/rubygems_helper.rb | 13 ++++++++++ app/javascript/src/pages.js | 10 -------- app/policies/rubygem_policy.rb | 4 +++ app/views/rubygems/_gem_members.html.erb | 32 ++++++++++-------------- 4 files changed, 30 insertions(+), 29 deletions(-) diff --git a/app/helpers/rubygems_helper.rb b/app/helpers/rubygems_helper.rb index 7898a9e9475..cb4e653d2b1 100644 --- a/app/helpers/rubygems_helper.rb +++ b/app/helpers/rubygems_helper.rb @@ -139,6 +139,19 @@ def links_to_owners_without_mfa(rubygem) rubygem.owners.without_mfa.sort_by(&:id).inject("") { |link, owner| link << link_to_user(owner) }.html_safe end + def link_to_owner(owner, show_mfa_status: false) + link_text = avatar(32, "gravatar-#{owner.id}", owner, class: "tw-rounded-full tw-mr-1") + link_text << " #{owner.display_handle}" + link_text << " *" if show_mfa_status && owner.mfa_disabled? + link_to( + link_text.html_safe, + profile_path(owner.display_id), + alt: owner.display_handle, + title: owner.display_handle, + class: "t-link tw-flex tw-items-center" + ) + end + def link_to_user(user) link_to avatar(48, "gravatar-#{user.id}", user), profile_path(user.display_id), alt: user.display_handle, title: user.display_handle diff --git a/app/javascript/src/pages.js b/app/javascript/src/pages.js index 6e0e0a753e1..1687267e637 100644 --- a/app/javascript/src/pages.js +++ b/app/javascript/src/pages.js @@ -6,13 +6,3 @@ $(function() { $(this).animate({ width: $(this).data("bar-width") + '%' }, 700).removeClass('t-item--hidden').css("display", "block"); }); }); - -//gem page -$(function() { - $('.gem__users__mfa-text.mfa-warn').on('click', function() { - $('.gem__users__mfa-text.mfa-warn').toggleClass('t-item--hidden'); - - $owners = $('.gem__users__mfa-disabled'); - $owners.toggleClass('t-item--hidden'); - }); -}); diff --git a/app/policies/rubygem_policy.rb b/app/policies/rubygem_policy.rb index 56a5c02ec58..2323c41b255 100644 --- a/app/policies/rubygem_policy.rb +++ b/app/policies/rubygem_policy.rb @@ -61,4 +61,8 @@ def show_events? def show_unconfirmed_ownerships? rubygem_owned_by?(user) end + + def show_owner_mfa_status? + rubygem_owned_by?(user) + end end diff --git a/app/views/rubygems/_gem_members.html.erb b/app/views/rubygems/_gem_members.html.erb index fc15a017930..1f498cefc49 100644 --- a/app/views/rubygems/_gem_members.html.erb +++ b/app/views/rubygems/_gem_members.html.erb @@ -1,28 +1,22 @@
<% if rubygem.owners.present? %>

<%= t '.owners_header' %>:

-
- <%= links_to_owners(rubygem) %> -
+ - <% if rubygem.owned_by?(current_user) %> + <% if rubygem.owned_by?(current_user) %> <% if rubygem.owners.without_mfa.present? %> - <% if current_user.mfa_disabled? %> - - <%= t '.self_no_mfa_warning_html' %> - - <% else %> - <%= t '.not_using_mfa_warning_show' %> - <% end %> - -
- <%= t '.not_using_mfa_warning_hide' %> -
- <%= links_to_owners_without_mfa(rubygem) %> -
-
+ + <%= current_user.mfa_disabled? ? t(".self_no_mfa_warning_html") : t(".not_using_mfa_warning_show") %> + <% else %> - <%= t '.using_mfa_info' %> + <%= t '.using_mfa_info' %> <% end %> <% end %> <% end %> From e6438dcdb0521ecee16b2cafc63ef3b867a3c51b Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Sat, 31 Aug 2024 13:33:33 -0700 Subject: [PATCH 2/6] Use policy for more of the view --- app/views/rubygems/_gem_members.html.erb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/views/rubygems/_gem_members.html.erb b/app/views/rubygems/_gem_members.html.erb index 1f498cefc49..7ab8851b520 100644 --- a/app/views/rubygems/_gem_members.html.erb +++ b/app/views/rubygems/_gem_members.html.erb @@ -1,16 +1,15 @@
<% if rubygem.owners.present? %> + <% show_mfa_status = policy(rubygem).show_owner_mfa_status? %> +

<%= t '.owners_header' %>:

    - <% show_mfa_status = policy(rubygem).show_owner_mfa_status? %> <% rubygem.owners.sort_by(&:id).each do |owner| %> -
  • - <%= link_to_owner(owner, show_mfa_status:) %> -
    • +
  • <%= link_to_owner(owner, show_mfa_status:) %>
    • <% end %>
- <% if rubygem.owned_by?(current_user) %> + <% if show_mfa_status %> <% if rubygem.owners.without_mfa.present? %> <%= current_user.mfa_disabled? ? t(".self_no_mfa_warning_html") : t(".not_using_mfa_warning_show") %> From 800f38f22abe6ca81ebd9c516a9856b1240df65f Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Sat, 31 Aug 2024 14:18:22 -0700 Subject: [PATCH 3/6] Clean up not using mfa * text --- app/helpers/rubygems_helper.rb | 8 +------- app/views/rubygems/_gem_members.html.erb | 2 +- config/locales/de.yml | 3 +-- config/locales/en.yml | 5 ++--- config/locales/es.yml | 5 +---- config/locales/fr.yml | 3 +-- config/locales/ja.yml | 3 +-- config/locales/nl.yml | 3 +-- config/locales/pt-BR.yml | 5 +---- config/locales/zh-CN.yml | 3 +-- config/locales/zh-TW.yml | 3 +-- 11 files changed, 12 insertions(+), 31 deletions(-) diff --git a/app/helpers/rubygems_helper.rb b/app/helpers/rubygems_helper.rb index cb4e653d2b1..85cbc38d26f 100644 --- a/app/helpers/rubygems_helper.rb +++ b/app/helpers/rubygems_helper.rb @@ -143,13 +143,7 @@ def link_to_owner(owner, show_mfa_status: false) link_text = avatar(32, "gravatar-#{owner.id}", owner, class: "tw-rounded-full tw-mr-1") link_text << " #{owner.display_handle}" link_text << " *" if show_mfa_status && owner.mfa_disabled? - link_to( - link_text.html_safe, - profile_path(owner.display_id), - alt: owner.display_handle, - title: owner.display_handle, - class: "t-link tw-flex tw-items-center" - ) + link_to(link_text.html_safe, profile_path(owner.display_id), class: "t-link tw-flex tw-items-center") end def link_to_user(user) diff --git a/app/views/rubygems/_gem_members.html.erb b/app/views/rubygems/_gem_members.html.erb index 7ab8851b520..340e545e54a 100644 --- a/app/views/rubygems/_gem_members.html.erb +++ b/app/views/rubygems/_gem_members.html.erb @@ -12,7 +12,7 @@ <% if show_mfa_status %> <% if rubygem.owners.without_mfa.present? %> - <%= current_user.mfa_disabled? ? t(".self_no_mfa_warning_html") : t(".not_using_mfa_warning_show") %> + <%= current_user.mfa_disabled? ? t(".self_no_mfa_warning_html") : t(".not_using_mfa_warning") %> <% else %> <%= t '.using_mfa_info' %> diff --git a/config/locales/de.yml b/config/locales/de.yml index fdbace32003..a3a946dad62 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -760,8 +760,7 @@ de: gem_members: authors_header: Autoren self_no_mfa_warning_html: - not_using_mfa_warning_show: - not_using_mfa_warning_hide: + not_using_mfa_warning: owners_header: Besitzer pushed_by: using_mfa_info: diff --git a/config/locales/en.yml b/config/locales/en.yml index b3c9b6c60fd..7dd76cf81a2 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -673,11 +673,10 @@ en: gem_members: authors_header: Authors self_no_mfa_warning_html: Please consider enabling multi-factor authentication (MFA) to keep your account secure. - not_using_mfa_warning_show: "* Some owners are not using multi-factor authentication (MFA). Click for the complete list." - not_using_mfa_warning_hide: "* The following owners are not using multi-factor authentication (MFA). Click to hide." + not_using_mfa_warning: "* Some owners are not using multi-factor authentication (MFA)." owners_header: Owners pushed_by: Pushed by - using_mfa_info: "* All owners are using multi-factor authentication (MFA)." + using_mfa_info: "All owners are using multi-factor authentication (MFA)." yanked_by: Yanked by sha_256_checksum: SHA 256 checksum signature_period: Signature validity period diff --git a/config/locales/es.yml b/config/locales/es.yml index 4c4f5d65958..4f54b0eb2ec 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -772,10 +772,7 @@ es: authors_header: Autores self_no_mfa_warning_html: Considera por favor activar la autenticación de múltiples factores (AMF) para mantener tu cuenta segura. - not_using_mfa_warning_show: "* Algunos propietarios no están usando AMF. Haga - click para ver la lista completa." - not_using_mfa_warning_hide: "* Los siguientes propietarios no están usando AMF. - Haga click para ocultar." + not_using_mfa_warning: "* Algunos propietarios no están usando AMF." owners_header: Propietarios pushed_by: Subida por using_mfa_info: "* Todos los propietarios están usando AMF." diff --git a/config/locales/fr.yml b/config/locales/fr.yml index 84a029f80e7..bd684272566 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -697,8 +697,7 @@ fr: gem_members: authors_header: Auteurs self_no_mfa_warning_html: - not_using_mfa_warning_show: - not_using_mfa_warning_hide: + not_using_mfa_warning: owners_header: Propriétaires pushed_by: using_mfa_info: diff --git a/config/locales/ja.yml b/config/locales/ja.yml index a2c008f9079..3887444c4b7 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -667,8 +667,7 @@ ja: authors_header: 作者 self_no_mfa_warning_html: アカウントをセキュアに保つため、多要素認証 (MFA) の有効化をご検討ください。 - not_using_mfa_warning_show: 多要素認証 (MFA) を使っていない所有者がいます。完全な一覧を見るにはクリックしてください。 - not_using_mfa_warning_hide: "* 以下の所有者は多要素認証 (MFA) を使っていません。クリックして隠します。" + not_using_mfa_warning: 多要素認証 (MFA) を使っていない所有者がいます。 owners_header: 所有者 pushed_by: プッシュ者 using_mfa_info: "* 全ての所有者が多要素認証 (MFA) を使っています。" diff --git a/config/locales/nl.yml b/config/locales/nl.yml index 2c41e12c4f2..09f4dc0aec8 100644 --- a/config/locales/nl.yml +++ b/config/locales/nl.yml @@ -665,8 +665,7 @@ nl: gem_members: authors_header: self_no_mfa_warning_html: - not_using_mfa_warning_show: - not_using_mfa_warning_hide: + not_using_mfa_warning: owners_header: Eigenaren pushed_by: using_mfa_info: diff --git a/config/locales/pt-BR.yml b/config/locales/pt-BR.yml index 34a5748e8d5..91942155426 100644 --- a/config/locales/pt-BR.yml +++ b/config/locales/pt-BR.yml @@ -676,10 +676,7 @@ pt-BR: gem_members: authors_header: Autores self_no_mfa_warning_html: - not_using_mfa_warning_show: "* Alguns donos não estão usando MFA. Clique para - a lista completa." - not_using_mfa_warning_hide: "* Os seguintes donos não estão usando MFA. Clique - para esconder." + not_using_mfa_warning: "* Alguns donos não estão usando MFA." owners_header: Donos pushed_by: using_mfa_info: "* Todos os donos estão usando MFA." diff --git a/config/locales/zh-CN.yml b/config/locales/zh-CN.yml index 7bac93b6af7..f3e5415206c 100644 --- a/config/locales/zh-CN.yml +++ b/config/locales/zh-CN.yml @@ -673,8 +673,7 @@ zh-CN: gem_members: authors_header: 作者 self_no_mfa_warning_html: 请考虑 启用多因素身份验证（MFA） 来保障您的帐户安全。 - not_using_mfa_warning_show: "* 一些业主当前还没有使用多因素验证（MFA）。请点击查看完整列表。" - not_using_mfa_warning_hide: "* 以下业主还未使用多因素验证（MFA）。点击隐藏。" + not_using_mfa_warning: "* 一些业主当前还没有使用多因素验证（MFA）。" owners_header: 业主 pushed_by: 推送 using_mfa_info: "* 所有业主都已使用多因素验证（MFA）。" diff --git a/config/locales/zh-TW.yml b/config/locales/zh-TW.yml index 3dca89fcae8..2858c579db6 100644 --- a/config/locales/zh-TW.yml +++ b/config/locales/zh-TW.yml @@ -667,8 +667,7 @@ zh-TW: gem_members: authors_header: 作者 self_no_mfa_warning_html: - not_using_mfa_warning_show: "* 某些擁有者未使用多重要素驗證 (MFA)。點擊此處以顯示完整名單。" - not_using_mfa_warning_hide: "* 下列擁有者未使用多重要素驗證 (MFA)。點擊此處以隱藏。" + not_using_mfa_warning: "* 某些擁有者未使用多重要素驗證 (MFA)。" owners_header: 擁有者 pushed_by: 推送者 using_mfa_info: "* 擁有者皆使用多重要素驗證 (MFA)。" From 8f476a49930fb0548a80512609ed38413c800250 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Sat, 31 Aug 2024 14:22:13 -0700 Subject: [PATCH 4/6] Add a policy test --- test/policies/rubygem_policy_test.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/policies/rubygem_policy_test.rb b/test/policies/rubygem_policy_test.rb index c5cefe7a214..0b6f08bf9ad 100644 --- a/test/policies/rubygem_policy_test.rb +++ b/test/policies/rubygem_policy_test.rb @@ -95,4 +95,12 @@ def policy!(user) refute_authorized nil, :show_unconfirmed_ownerships? end end + + context "#show_owner_mfa_status?" do + should "only allow the owner" do + assert_authorized @owner, :show_owner_mfa_status? + refute_authorized @user, :show_owner_mfa_status? + refute_authorized nil, :show_owner_mfa_status? + end + end end From b9d4595b3995000293275a2b3f3ac87f82239513 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Sat, 31 Aug 2024 14:55:40 -0700 Subject: [PATCH 5/6] fix tests --- test/integration/api/v1/owner_test.rb | 12 ++++++------ test/integration/gems_test.rb | 2 -- test/integration/yank_test.rb | 4 +--- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/test/integration/api/v1/owner_test.rb b/test/integration/api/v1/owner_test.rb index 936b49c6124..4bcdba04b41 100644 --- a/test/integration/api/v1/owner_test.rb +++ b/test/integration/api/v1/owner_test.rb @@ -29,8 +29,8 @@ class Api::V1::OwnerTest < ActionDispatch::IntegrationTest get rubygem_path(@rubygem.slug) - assert page.has_selector?("a[alt='#{@user.handle}']") - assert page.has_selector?("a[alt='#{@other_user.handle}']") + page.assert_selector("div.gem__members a", text: @user.handle) + page.assert_selector("div.gem__members a", text: @other_user.handle) end test "removing an owner" do @@ -41,8 +41,8 @@ class Api::V1::OwnerTest < ActionDispatch::IntegrationTest get rubygem_path(@rubygem.slug) - assert page.has_selector?("a[alt='#{@user.handle}']") - refute page.has_selector?("a[alt='#{@other_user.handle}']") + page.assert_selector("div.gem__members a", text: @user.handle) + page.assert_no_selector("div.gem__members a", text: @other_user.handle) end test "transferring ownership" do @@ -54,8 +54,8 @@ class Api::V1::OwnerTest < ActionDispatch::IntegrationTest get rubygem_path(@rubygem.slug) - refute page.has_selector?("a[alt='#{@user.handle}']") - assert page.has_selector?("a[alt='#{@other_user.handle}']") + page.assert_no_selector("div.gem__members a", text: @user.handle) + page.assert_selector("div.gem__members a", text: @other_user.handle) end test "adding ownership without permission" do diff --git a/test/integration/gems_test.rb b/test/integration/gems_test.rb index 4a16312365f..e4710fa6508 100644 --- a/test/integration/gems_test.rb +++ b/test/integration/gems_test.rb @@ -114,7 +114,6 @@ class GemsSystemTest < SystemTest visit rubygem_path(@rubygem.slug, as: @user.id) - assert page.has_selector?(".gem__users__mfa-disabled .gem__users a") assert page.has_content? "Please consider enabling multi-factor" end @@ -127,7 +126,6 @@ class GemsSystemTest < SystemTest visit rubygem_path(@rubygem.slug, as: @user.id) - assert page.has_selector?(".gem__users__mfa-disabled .gem__users a") assert page.has_selector?(".gem__users__mfa-text.mfa-warn") end diff --git a/test/integration/yank_test.rb b/test/integration/yank_test.rb index 3d433b0da4d..e9c8e342b18 100644 --- a/test/integration/yank_test.rb +++ b/test/integration/yank_test.rb @@ -42,9 +42,7 @@ class YankTest < SystemTest assert page.has_content?("Yanked by") - css = %(div.gem__users a[alt=#{@user.handle}]) - - assert page.has_css?(css, count: 3) + assert_selector("div.gem__members a", text: @user.handle) assert_event Events::RubygemEvent::VERSION_YANKED, { number: "2.2.2", From d85468f04d4dfb53774aba972d4c7306da3ac8b1 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Sat, 31 Aug 2024 15:05:24 -0700 Subject: [PATCH 6/6] one more test --- app/assets/stylesheets/tailwind.deprecated.css | 2 +- test/integration/push_test.rb | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/assets/stylesheets/tailwind.deprecated.css b/app/assets/stylesheets/tailwind.deprecated.css index cbb0ee2e997..6e97526704b 100644 --- a/app/assets/stylesheets/tailwind.deprecated.css +++ b/app/assets/stylesheets/tailwind.deprecated.css @@ -3,4 +3,4 @@ * We no longer compute this build and the tailwind config for this is gone. * It's only here until the prefixed `tw-` classes are replaced by the new design. */ -.tw-mx-auto{margin-left:auto;margin-right:auto}.tw-my-4{margin-bottom:1rem;margin-top:1rem}.\!tw-mb-0{margin-bottom:0!important}.tw-mt-2{margin-top:.5rem}.tw-mt-4{margin-top:1rem}.tw-flex{display:flex}.tw-flex-col{flex-direction:column}.tw-items-baseline{align-items:baseline}.tw-gap-2{gap:.5rem}.tw-gap-4{gap:1rem}.tw-space-y-2>:not([hidden])~:not([hidden]){--tw-space-y-reverse:0;margin-bottom:calc(.5rem*var(--tw-space-y-reverse));margin-top:calc(.5rem*(1 - var(--tw-space-y-reverse)))}.tw-space-y-4>:not([hidden])~:not([hidden]){--tw-space-y-reverse:0;margin-bottom:calc(1rem*var(--tw-space-y-reverse));margin-top:calc(1rem*(1 - var(--tw-space-y-reverse)))}.tw-divide-y>:not([hidden])~:not([hidden]){--tw-divide-y-reverse:0;border-bottom-width:calc(1px*var(--tw-divide-y-reverse));border-top-width:calc(1px*(1 - var(--tw-divide-y-reverse)))}.tw-break-all{word-break:break-all}.tw-border{border-width:1px}.tw-border-solid{border-style:solid}.tw-border-red-500{--tw-border-opacity:1;border-color:rgb(239 68 68/var(--tw-border-opacity))}.\!tw-bg-white{--tw-bg-opacity:1!important;background-color:rgb(255 255 255/var(--tw-bg-opacity))!important}.tw-p-5{padding:1.25rem}.\!tw-py-0{padding-bottom:0!important;padding-top:0!important}.\!tw-text-start{text-align:start!important}@media (min-width:640px){.sm\:tw-flex{display:flex}.sm\:tw-grid{display:grid}.sm\:tw-grid-cols-2{grid-template-columns:repeat(2,minmax(0,1fr))}.sm\:tw-flex-row{flex-direction:row}.sm\:tw-items-baseline{align-items:baseline}} +.tw-mx-auto{margin-left:auto;margin-right:auto}.tw-my-4{margin-bottom:1rem;margin-top:1rem}.\!tw-mb-0{margin-bottom:0!important}.tw-mt-2{margin-top:.5rem}.tw-mt-4{margin-top:1rem}.tw-mr-1{margin-right:.25rem}.tw-flex{display:flex}.tw-flex-col{flex-direction:column}.tw-items-baseline{align-items:baseline}.tw-items-center{align-items:center}.tw-gap-2{gap:.5rem}.tw-gap-4{gap:1rem}.tw-space-y-2>:not([hidden])~:not([hidden]){--tw-space-y-reverse:0;margin-bottom:calc(.5rem*var(--tw-space-y-reverse));margin-top:calc(.5rem*(1 - var(--tw-space-y-reverse)))}.tw-space-y-4>:not([hidden])~:not([hidden]){--tw-space-y-reverse:0;margin-bottom:calc(1rem*var(--tw-space-y-reverse));margin-top:calc(1rem*(1 - var(--tw-space-y-reverse)))}.tw-divide-y>:not([hidden])~:not([hidden]){--tw-divide-y-reverse:0;border-bottom-width:calc(1px*var(--tw-divide-y-reverse));border-top-width:calc(1px*(1 - var(--tw-divide-y-reverse)))}.tw-break-all{word-break:break-all}.tw-border{border-width:1px}.tw-border-solid{border-style:solid}.tw-border-red-500{--tw-border-opacity:1;border-color:rgb(239 68 68/var(--tw-border-opacity))}.\!tw-bg-white{--tw-bg-opacity:1!important;background-color:rgb(255 255 255/var(--tw-bg-opacity))!important}.tw-p-5{padding:1.25rem}.\!tw-py-0{padding-bottom:0!important;padding-top:0!important}.\!tw-text-start{text-align:start!important}@media (min-width:640px){.sm\:tw-flex{display:flex}.sm\:tw-grid{display:grid}.sm\:tw-grid-cols-2{grid-template-columns:repeat(2,minmax(0,1fr))}.sm\:tw-flex-row{flex-direction:row}.sm\:tw-items-baseline{align-items:baseline}} diff --git a/test/integration/push_test.rb b/test/integration/push_test.rb index b3ec204aa0f..419e818237a 100644 --- a/test/integration/push_test.rb +++ b/test/integration/push_test.rb @@ -24,9 +24,7 @@ class PushTest < ActionDispatch::IntegrationTest assert page.has_content?("1.0.0") assert page.has_content?("Pushed by") - css = %(div.gem__users a[alt=#{@user.handle}]) - - assert page.has_css?(css, count: 2) + assert_selector("div.gem__members a", text: @user.handle) assert_equal Digest::MD5.hexdigest(<<~INFO), Rubygem.find_by!(name: "sandworm").versions.sole.info_checksum ---