diff --git a/app/controllers/settings_controller.rb b/app/controllers/settings_controller.rb index 2200bcb..e469d66 100644 --- a/app/controllers/settings_controller.rb +++ b/app/controllers/settings_controller.rb @@ -25,7 +25,8 @@ class SettingsController < ApplicationController def update @user.preferences.merge!(user_params[:preferences] || {}) @user.display_name = user_params[:display_name] - @user.avatar_new = user_params[:avatar] + @user.avatar_new = user_params[:avatar] + @user.pgp_pubkey = user_params[:pgp_pubkey] if @user.save if @user.display_name && (@user.display_name != @user.ldap_entry[:display_name]) @@ -36,6 +37,10 @@ class SettingsController < ApplicationController LdapManager::UpdateAvatar.call(dn: @user.dn, file: @user.avatar_new) end + if @user.pgp_pubkey && (@user.pgp_pubkey != @user.ldap_entry[:pgp_key]) + UserManager::UpdatePgpKey.call(user: @user) + end + redirect_to setting_path(@settings_section), flash: { success: 'Settings saved.' } @@ -157,7 +162,8 @@ class SettingsController < ApplicationController def user_params params.require(:user).permit( - :display_name, :avatar, preferences: UserPreferences.pref_keys + :display_name, :avatar, :pgp_pubkey, + preferences: UserPreferences.pref_keys ) end diff --git a/app/models/user.rb b/app/models/user.rb index b0539ec..67c6fcd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -52,7 +52,7 @@ class User < ApplicationRecord validate :acceptable_avatar - validate :acceptable_pgp_key_format, if: -> { defined?(@pgp_pubkey) && @pgp_pubkey != "" } + validate :acceptable_pgp_key_format, if: -> { defined?(@pgp_pubkey) && @pgp_pubkey.present? } # # Scopes diff --git a/app/services/ldap_manager/update_pgp_key.rb b/app/services/ldap_manager/update_pgp_key.rb new file mode 100644 index 0000000..fa5b982 --- /dev/null +++ b/app/services/ldap_manager/update_pgp_key.rb @@ -0,0 +1,16 @@ +module LdapManager + class UpdatePgpKey < LdapManagerService + def initialize(dn:, pubkey:) + @dn = dn + @pubkey = pubkey + end + + def call + if @pubkey.present? + replace_attribute @dn, :pgpKey, @pubkey + else + delete_attribute @dn, :pgpKey + end + end + end +end diff --git a/app/services/user_manager/update_pgp_key.rb b/app/services/user_manager/update_pgp_key.rb new file mode 100644 index 0000000..16b78c7 --- /dev/null +++ b/app/services/user_manager/update_pgp_key.rb @@ -0,0 +1,24 @@ +module UserManager + class UpdatePgpKey < UserManagerService + def initialize(user:) + @user = user + end + + def call + if @user.pgp_pubkey.blank? + @user.update! pgp_fpr: nil + else + result = GPGME::Key.import(@user.pgp_pubkey) + + if result.imports.present? + @user.update! pgp_fpr: result.imports.first.fpr + else + # TODO notify Sentry, user + raise "Failed to import OpenPGP pubkey" + end + end + + LdapManager::UpdatePgpKey.call(dn: @user.dn, pubkey: @user.pgp_pubkey) + end + end +end diff --git a/app/views/settings/_account.html.erb b/app/views/settings/_account.html.erb index b1b3430..90144b2 100644 --- a/app/views/settings/_account.html.erb +++ b/app/views/settings/_account.html.erb @@ -1,6 +1,6 @@ <%= tag.section data: { controller: "settings--account--email", - "settings--account--email-validation-failed-value": @validation_errors.present? + "settings--account--email-validation-failed-value": @validation_errors&.[](:email)&.present? } do %>

E-Mail

<%= form_for(@user, url: update_email_settings_path, method: "post") do |f| %> @@ -23,7 +23,7 @@

- <% if @validation_errors.present? && @validation_errors[:email].present? %> + <% if @validation_errors&.[](:email)&.present? %>

<%= @validation_errors[:email].first %>

<% end %>
@@ -41,10 +41,33 @@ <% end %>

Password

-

Use the following button to request an email with a password reset link:

+

Use the following button to request an email with a password reset link:

<%= form_with(url: reset_password_settings_path, method: :post) do %>

<%= submit_tag("Send me a password reset link", class: 'btn-md btn-gray w-full sm:w-auto') %>

<% end %>
+<%= form_for(@user, url: setting_path(:account), html: { :method => :put }) do |f| %> +
+

OpenPGP

+ +
+
+

+ <%= f.submit 'Save', class: "btn-md btn-blue w-full md:w-auto" %> +

+
+<% end %> diff --git a/spec/features/settings/account_spec.rb b/spec/features/settings/account_spec.rb index 51f1c83..47bb9ad 100644 --- a/spec/features/settings/account_spec.rb +++ b/spec/features/settings/account_spec.rb @@ -14,6 +14,7 @@ RSpec.describe 'Account settings', type: :feature do .with("invalid password").and_return(false) allow_any_instance_of(User).to receive(:valid_ldap_authentication?) .with("valid password").and_return(true) + allow_any_instance_of(User).to receive(:pgp_pubkey).and_return(nil) end scenario 'fails with invalid password' do @@ -55,4 +56,44 @@ RSpec.describe 'Account settings', type: :feature do end end end + + feature "Update OpenPGP key" do + let(:invalid_key) { File.read("#{Rails.root}/spec/fixtures/files/pgp_key_invalid.asc") } + let(:valid_key_alice) { File.read("#{Rails.root}/spec/fixtures/files/pgp_key_valid_alice.asc") } + let(:fingerprint_alice) { "EB85BB5FA33A75E15E944E63F231550C4F47E38E" } + + before do + login_as user, :scope => :user + allow_any_instance_of(User).to receive(:ldap_entry).and_return({ + uid: user.cn, ou: user.ou, display_name: nil, pgp_key: nil + }) + end + + scenario 'rejects an invalid key' do + expect(UserManager::UpdatePgpKey).not_to receive(:call) + + visit setting_path(:account) + fill_in 'Public key', with: invalid_key + click_button "Save" + + expect(current_url).to eq(setting_url(:account)) + within ".error-msg" do + expect(page).to have_content("This is not a valid armored PGP public key block") + end + end + + scenario 'stores a valid key' do + expect(UserManager::UpdatePgpKey).to receive(:call) + .with(user: user).and_return(true) + + visit setting_path(:account) + fill_in 'Public key', with: valid_key_alice + click_button "Save" + + expect(current_url).to eq(setting_url(:account)) + within ".flash-msg" do + expect(page).to have_content("Settings saved") + end + end + end end diff --git a/spec/features/settings/profile_spec.rb b/spec/features/settings/profile_spec.rb index 1d5fa8c..55b861e 100644 --- a/spec/features/settings/profile_spec.rb +++ b/spec/features/settings/profile_spec.rb @@ -9,7 +9,7 @@ RSpec.describe 'Profile settings', type: :feature do allow(user).to receive(:display_name).and_return("Mark") allow_any_instance_of(User).to receive(:dn).and_return("cn=mwahlberg,ou=kosmos.org,cn=users,dc=kosmos,dc=org") allow_any_instance_of(User).to receive(:ldap_entry).and_return({ - uid: user.cn, ou: user.ou, display_name: "Mark" + uid: user.cn, ou: user.ou, display_name: "Mark", pgp_key: nil }) allow_any_instance_of(User).to receive(:avatar).and_return(avatar_base64) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 60ce8ed..a4e0f57 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -262,7 +262,6 @@ RSpec.describe User, type: :model do let(:valid_key_jimmy) { File.read("#{Rails.root}/spec/fixtures/files/pgp_key_valid_jimmy.asc") } let(:fingerprint_alice) { "EB85BB5FA33A75E15E944E63F231550C4F47E38E" } let(:fingerprint_jimmy) { "316BF516236DAF77236B15F6057D93972FB862C3" } - let(:gnupg_key_alice) { } let(:invalid_key) { File.read("#{Rails.root}/spec/fixtures/files/pgp_key_invalid.asc") } before do @@ -275,8 +274,8 @@ RSpec.describe User, type: :model do end after do - GPGME::Key.get(fingerprint_alice).delete! - GPGME::Key.get(fingerprint_jimmy).delete! + alice.gnupg_key.delete! + jimmy.gnupg_key.delete! end describe "#acceptable_pgp_key_format" do diff --git a/spec/services/user_manager/update_pgp_key_spec.rb b/spec/services/user_manager/update_pgp_key_spec.rb new file mode 100644 index 0000000..844eaf4 --- /dev/null +++ b/spec/services/user_manager/update_pgp_key_spec.rb @@ -0,0 +1,74 @@ +require 'rails_helper' + +RSpec.describe UserManager::UpdatePgpKey, type: :model do + include ActiveJob::TestHelper + + let(:alice) { create :user, cn: "alice" } + let(:dn) { "cn=alice,ou=kosmos.org,cn=users,dc=kosmos,dc=org" } + let(:pubkey_asc) { File.read("#{Rails.root}/spec/fixtures/files/pgp_key_valid_alice.asc") } + let(:fingerprint) { "EB85BB5FA33A75E15E944E63F231550C4F47E38E" } + + before do + allow(alice).to receive(:dn).and_return(dn) + allow(alice).to receive(:ldap_entry).and_return({ + uid: alice.cn, ou: alice.ou, pgp_key: nil + }) + end + + describe "#call" do + context "with valid key" do + before do + alice.pgp_pubkey = pubkey_asc + + allow(LdapManager::UpdatePgpKey).to receive(:call) + .with(dn: alice.dn, pubkey: pubkey_asc) + end + + after do + alice.gnupg_key.delete! + end + + it "imports the key into the GnuPG keychain" do + described_class.call(user: alice) + expect(alice.gnupg_key).to be_present + end + + it "stores the key's fingerprint on the user record" do + described_class.call(user: alice) + expect(alice.pgp_fpr).to eq(fingerprint) + end + + it "updates the user's LDAP entry with the new key" do + expect(LdapManager::UpdatePgpKey).to receive(:call) + .with(dn: alice.dn, pubkey: pubkey_asc) + described_class.call(user: alice) + end + end + + context "with empty key" do + before do + alice.update pgp_fpr: fingerprint + alice.pgp_pubkey = "" + + allow(LdapManager::UpdatePgpKey).to receive(:call) + .with(dn: alice.dn, pubkey: "") + end + + it "does not attempt to import the key" do + expect(GPGME::Key).not_to receive(:import) + described_class.call(user: alice) + end + + it "removes the key's fingerprint from the user record" do + described_class.call(user: alice) + expect(alice.pgp_fpr).to be_nil + end + + it "removes the key from the user's LDAP entry" do + expect(LdapManager::UpdatePgpKey).to receive(:call) + .with(dn: alice.dn, pubkey: "") + described_class.call(user: alice) + end + end + end +end