Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable MFA on specific API keys #2846

Merged
merged 3 commits into from
Dec 28, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Allow mfa to be enabled on specific keys
Co-authored-by: Betty Li <betty.li@shopify.com>
2 people authored and sonalkr132 committed Dec 28, 2021
commit 32b35771268d032f08a64cd72817ec84b4b9e208
2 changes: 1 addition & 1 deletion app/assets/stylesheets/modules/form.css
Original file line number Diff line number Diff line change
@@ -41,7 +41,7 @@
.form__input__addon-left .form__input {
padding-left: 45px; }

.form__input, .form__textarea, .form__select {
.form__input, .form__textarea, .form__select, .form__group {
margin-bottom: 30px; }

.form__input, .form__textarea {
7 changes: 7 additions & 0 deletions app/assets/stylesheets/modules/owners.css
Original file line number Diff line number Diff line change
@@ -27,6 +27,13 @@ span.owners__icon img {
padding-right: 2px;
}

.owners__cell[data-title="MFA"] img {
margin: auto 0;
height: 23px;
width: 23px;
border-radius: 50%;
}

@media screen and (max-width: 640px) {
.owners__tbody {
display: block;
4 changes: 2 additions & 2 deletions app/controllers/api/v1/api_keys_controller.rb
Original file line number Diff line number Diff line change
@@ -79,10 +79,10 @@ def otp
end

def api_key_create_params
params.permit(:name, *ApiKey::API_SCOPES)
params.permit(:name, *ApiKey::API_SCOPES, :mfa)
sonalkr132 marked this conversation as resolved.
Show resolved Hide resolved
end

def api_key_update_params
params.permit(*ApiKey::API_SCOPES)
params.permit(*ApiKey::API_SCOPES, :mfa)
end
end
2 changes: 1 addition & 1 deletion app/controllers/api_keys_controller.rb
Original file line number Diff line number Diff line change
@@ -66,7 +66,7 @@ def reset
private

def api_key_params
params.require(:api_key).permit(:name, *ApiKey::API_SCOPES)
params.require(:api_key).permit(:name, *ApiKey::API_SCOPES, :mfa)
end

def redirect_to_verify
22 changes: 17 additions & 5 deletions app/views/api_keys/edit.html.erb
Original file line number Diff line number Diff line change
@@ -5,11 +5,23 @@
<%= label_tag :name, t("api_keys.index.name"), class: "form__label" %>
<%= f.text_field :name, class: "form__input", autocomplete: :off %>

<%= label_tag :scope, t("api_keys.index.scope"), class: "form__label" %>
<% ApiKey::API_SCOPES.each do |api_scope| %>
<div class = "form__checkbox__item">
<%= f.check_box "#{api_scope}", { class: "form__checkbox__input", id: "#{api_scope}" } , "true", "false" %>
<%= label_tag api_scope, t("api_keys.index.#{api_scope}"), class: "form__checkbox__label" %>
<div class="form__group">
<%= label_tag :scope, t("api_keys.index.scope"), class: "form__label" %>
<% ApiKey::API_SCOPES.each do |api_scope| %>
<div class = "form__checkbox__item">
<%= f.check_box "#{api_scope}", { class: "form__checkbox__input", id: "#{api_scope}" } , "true", "false" %>
<%= label_tag api_scope, t("api_keys.index.#{api_scope}"), class: "form__checkbox__label" %>
</div>
<% end %>
</div>

<% unless current_user.mfa_disabled? || current_user.mfa_ui_and_api? %>
<div class="form__group">
<%= label_tag :mfa, t(".multifactor_auth"), class: "form__label" %>
<div class="form__checkbox__item">
<%= f.check_box :mfa, { class: "form__checkbox__input", id: :mfa } , "true", "false" %>
<%= label_tag :mfa, t(".enable_mfa"), class: "form__checkbox__label" %>
</div>
</div>
<% end %>

14 changes: 14 additions & 0 deletions app/views/api_keys/index.html.erb
Original file line number Diff line number Diff line change
@@ -21,6 +21,11 @@
<th class="owners__cell">
<%= t(".age") %>
</th>
<% if current_user.mfa_enabled? %>
<th class="owners__cell">
<%= t(".mfa") %>
</th>
<% end %>
<th class="owners__cell">
<%= t(".last_access") %>
</th>
@@ -41,6 +46,11 @@
<td class="owners__cell" data-title="Age">
<%= time_ago_in_words(api_key.created_at) %>
</td>
<% if current_user.mfa_enabled? %>
<td class="owners__cell" data-title="MFA">
<%= image_tag("/images/check.svg") if api_key.mfa_enabled? %>
</td>
<% end %>
<td class="owners__cell" data-title="Last access">
<%= api_key.last_accessed_at&.strftime("%Y-%m-%d %H:%M %Z") %>
</td>
@@ -70,6 +80,10 @@
</td>
<td class="owners__cell">
</td>
<% if current_user.mfa_enabled? %>
<td class="owners__cell">
</td>
<% end %>
<td class="owners__cell">
<%= button_to t(".reset"),
reset_profile_api_keys_path,
26 changes: 21 additions & 5 deletions app/views/api_keys/new.html.erb
Original file line number Diff line number Diff line change
@@ -5,11 +5,27 @@
<%= label_tag :name, t("api_keys.index.name"), class: "form__label" %>
<%= f.text_field :name, class: "form__input", autocomplete: :off %>

<%= label_tag :scope, t("api_keys.index.scope"), class: "form__label" %>
<% ApiKey::API_SCOPES.each do |api_scope| %>
<div class = "form__checkbox__item">
<%= f.check_box "#{api_scope}", { class: "form__checkbox__input", id: "#{api_scope}" } , "true", "false" %>
<%= label_tag api_scope, t("api_keys.index.#{api_scope}"), class: "form__checkbox__label" %>
<div class="form__group">
<%= label_tag :scope, t("api_keys.index.scope"), class: "form__label" %>
<% ApiKey::API_SCOPES.each do |api_scope| %>
<div class = "form__checkbox__item">
<%= f.check_box "#{api_scope}", { class: "form__checkbox__input", id: "#{api_scope}" } , "true", "false" %>
<%= label_tag api_scope, t("api_keys.index.#{api_scope}"), class: "form__checkbox__label" %>
</div>
<% end %>
</div>

<% if current_user.mfa_enabled? %>
<div class="form__group">
<%= label_tag :mfa, t(".multifactor_auth"), class: "form__label" %>
<% if current_user.mfa_ui_and_api? %>
<p><%= t(".mfa_api_enabled") %></p>
<% else %>
<div class="form__checkbox__item">
<%= f.check_box :mfa, { class: "form__checkbox__input", id: :mfa, checked: true } , "true", "false" %>
<%= label_tag :mfa, t(".enable_mfa"), class: "form__checkbox__label" %>
</div>
<% end %>
</div>
<% end %>

6 changes: 6 additions & 0 deletions config/locales/de.yml
Original file line number Diff line number Diff line change
@@ -77,14 +77,20 @@ de:
show_dashboard:
reset:
save_key:
mfa:
new:
new_api_key:
mfa_api_enabled:
multifactor_auth:
enable_mfa:
reset:
success:
update:
success:
edit:
edit_api_key:
multifactor_auth:
enable_mfa:
clearance_mailer:
change_password:
title:
6 changes: 6 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
@@ -85,14 +85,20 @@ en:
show_dashboard: Show dashboard
reset: Reset
save_key: "Note that we won't be able to show the key to you again. New API key:"
mfa: MFA
new:
new_api_key: New API key
mfa_api_enabled: Your new key will have multifactor authentication turned on because you have enabled the UI and API MFA level.
multifactor_auth: Multifactor authentication
enable_mfa: Enable MFA
reset:
success: "Deleted all API keys"
update:
success: "Successfully updated API key"
edit:
edit_api_key: "Edit API key"
multifactor_auth: Multifactor authentication
enable_mfa: Enable MFA
clearance_mailer:
change_password:
title: CHANGE PASSWORD
6 changes: 6 additions & 0 deletions config/locales/es.yml
Original file line number Diff line number Diff line change
@@ -85,14 +85,20 @@ es:
show_dashboard:
reset:
save_key:
mfa:
new:
new_api_key:
mfa_api_enabled:
multifactor_auth:
enable_mfa:
reset:
success:
update:
success:
edit:
edit_api_key:
multifactor_auth:
enable_mfa:
clearance_mailer:
change_password:
title: CAMBIAR CONTRASEÑA
6 changes: 6 additions & 0 deletions config/locales/fr.yml
Original file line number Diff line number Diff line change
@@ -77,14 +77,20 @@ fr:
show_dashboard:
reset:
save_key:
mfa:
new:
new_api_key:
mfa_api_enabled:
multifactor_auth:
enable_mfa:
reset:
success:
update:
success:
edit:
edit_api_key:
multifactor_auth:
enable_mfa:
clearance_mailer:
change_password:
title:
6 changes: 6 additions & 0 deletions config/locales/ja.yml
Original file line number Diff line number Diff line change
@@ -85,14 +85,20 @@ ja:
show_dashboard:
reset:
save_key:
mfa:
new:
new_api_key:
mfa_api_enabled:
multifactor_auth:
enable_mfa:
reset:
success:
update:
success:
edit:
edit_api_key:
multifactor_auth:
enable_mfa:
clearance_mailer:
change_password:
title:
6 changes: 6 additions & 0 deletions config/locales/nl.yml
Original file line number Diff line number Diff line change
@@ -77,14 +77,20 @@ nl:
show_dashboard:
reset:
save_key:
mfa:
new:
new_api_key:
mfa_api_enabled:
multifactor_auth:
enable_mfa:
reset:
success:
update:
success:
edit:
edit_api_key:
multifactor_auth:
enable_mfa:
clearance_mailer:
change_password:
title:
6 changes: 6 additions & 0 deletions config/locales/pt-BR.yml
Original file line number Diff line number Diff line change
@@ -82,14 +82,20 @@ pt-BR:
show_dashboard:
reset:
save_key:
mfa:
new:
new_api_key:
mfa_api_enabled:
multifactor_auth:
enable_mfa:
reset:
success:
update:
success:
edit:
edit_api_key:
multifactor_auth:
enable_mfa:
clearance_mailer:
change_password:
title:
6 changes: 6 additions & 0 deletions config/locales/zh-CN.yml
Original file line number Diff line number Diff line change
@@ -77,14 +77,20 @@ zh-CN:
show_dashboard:
reset:
save_key:
mfa:
new:
new_api_key:
mfa_api_enabled:
multifactor_auth:
enable_mfa:
reset:
success:
update:
success:
edit:
edit_api_key:
multifactor_auth:
enable_mfa:
clearance_mailer:
change_password:
title:
6 changes: 6 additions & 0 deletions config/locales/zh-TW.yml
Original file line number Diff line number Diff line change
@@ -77,14 +77,20 @@ zh-TW:
show_dashboard:
reset:
save_key:
mfa:
new:
new_api_key:
mfa_api_enabled:
multifactor_auth:
enable_mfa:
reset:
success:
update:
success:
edit:
edit_api_key:
multifactor_auth:
enable_mfa:
clearance_mailer:
change_password:
title:
17 changes: 16 additions & 1 deletion test/functional/api/v1/api_keys_controller_test.rb
Original file line number Diff line number Diff line change
@@ -276,6 +276,17 @@ def self.should_expect_otp_for_update
assert_equal "New API key created for rubygems.org", email.subject
assert_match "test-key", email.body.to_s
end

context "with MFA param set" do
setup do
post :create, params: { name: "mfa", index_rubygems: "true", mfa: "true" }, format: "text"
end

should "have MFA" do
created_key = @user.api_keys.find_by(name: "mfa")
assert created_key.mfa
end
end
end

context "when user has enabled MFA for UI and API" do
@@ -317,7 +328,7 @@ def self.should_expect_otp_for_update
setup do
@api_key = create(:api_key, user: @user, key: "12345", push_rubygem: true)
authorize_with("#{@user.email}:#{@user.password}")
put :update, params: { api_key: "12345", index_rubygems: "true" }
put :update, params: { api_key: "12345", index_rubygems: "true", mfa: "true" }
@api_key.reload
end

@@ -326,6 +337,10 @@ def self.should_expect_otp_for_update
assert @api_key.can_index_rubygems?
assert @api_key.can_push_rubygem?
end

should "update MFA" do
assert @api_key.mfa
end
end

context "when user has enabled MFA for UI and API" do
64 changes: 64 additions & 0 deletions test/integration/api_keys_test.rb
Original file line number Diff line number Diff line change
@@ -15,10 +15,39 @@ class ApiKeysTest < SystemTest

fill_in "api_key[name]", with: "test"
check "api_key[index_rubygems]"
refute page.has_content? "Enable MFA"
click_button "Create"

assert page.has_content? "Note that we won't be able to show the key to you again. New API key:"
assert @user.api_keys.last.can_index_rubygems?
refute @user.api_keys.last.mfa_enabled?
end

test "creating new api key with MFA UI enabled" do
@user.enable_mfa!(ROTP::Base32.random_base32, :ui_only)

visit_profile_api_keys_path

fill_in "api_key[name]", with: "test"
check "api_key[index_rubygems]"
check "mfa"
click_button "Create"

assert page.has_content? "Note that we won't be able to show the key to you again. New API key:"
assert @user.api_keys.last.mfa_enabled?
end

test "creating new api key with MFA UI and API enabled" do
@user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_api)

visit_profile_api_keys_path

fill_in "api_key[name]", with: "test"
check "api_key[index_rubygems]"
click_button "Create"

assert page.has_content? "Note that we won't be able to show the key to you again. New API key:"
assert @user.api_keys.last.mfa_enabled?
end

test "update api key" do
@@ -29,9 +58,44 @@ class ApiKeysTest < SystemTest

assert page.has_content? "Edit API key"
check "api_key[add_owner]"
refute page.has_content? "Enable MFA"
click_button "Update"

assert api_key.reload.can_add_owner?
end

test "update api key with MFA UI enabled" do
@user.enable_mfa!(ROTP::Base32.random_base32, :ui_only)

api_key = create(:api_key, user: @user)

visit_profile_api_keys_path
click_button "Edit"

assert page.has_content? "Edit API key"
check "api_key[add_owner]"
check "mfa"
click_button "Update"

assert api_key.reload.can_add_owner?
assert @user.api_keys.last.mfa_enabled?
end

test "update api key with MFA UI and API enabled" do
@user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_api)

api_key = create(:api_key, user: @user)

visit_profile_api_keys_path
click_button "Edit"

assert page.has_content? "Edit API key"
check "api_key[add_owner]"
refute page.has_content? "Enable MFA"
click_button "Update"

assert api_key.reload.can_add_owner?
assert @user.api_keys.last.mfa_enabled?
end

test "deleting api key" do