Fever: generate API key based on username/password (#947)

The fever spec requires this format, so this restores logic previously
removed.

Author: mockdeep

app/assets/stylesheets/application.css

@@ -23,6 +23,10 @@ code {
   white-space: normal;
 }
 
+.warning {
+  background-color: #F2DEDE;
+}
+
 .container {
   width: 100%;
   max-width: 720px;

app/controllers/profiles_controller.rb

@@ -22,6 +22,6 @@ def update
   private
 
   def user_params
-    params.require(:user).permit(:username)
+    params.require(:user).permit(:username, :password_challenge)
   end
 end

app/models/user.rb

@@ -4,7 +4,6 @@
 
 class User < ApplicationRecord
   has_secure_password
-  has_secure_token :api_key
 
   encrypts :api_key, deterministic: true
 
@@ -14,6 +13,8 @@ class User < ApplicationRecord
   validates :username, presence: true, uniqueness: { case_sensitive: false }
   validate :password_challenge_matches
 
+  before_save :update_api_key
+
   attr_accessor :password_challenge
 
   # `password_challenge` logic should be able to be removed in Rails 7.1
@@ -26,4 +27,19 @@ def password_challenge_matches
 
     errors.add(:original_password, "does not match")
   end
+
+  def update_api_key
+    return unless password_digest_changed? || username_changed?
+
+    if password_challenge.blank? && password.blank?
+      message = "Cannot change username without providing a password"
+
+      raise(ActiveRecord::ActiveRecordError, message)
+    end
+
+    password = password_challenge.presence || self.password.presence
+
+    # API key based on Fever spec: https://feedafever.com/api
+    self.api_key = Digest::MD5.hexdigest("#{username}:#{password}")
+  end
 end

app/views/profiles/edit.html.erb

@@ -1,22 +1,29 @@
 <h1><%= t('.title') %></h1>
 
+<header class="warning">
+  <p><%= t('.warning_html') %></p>
+</header>
+
 <%= form_with(model: user, url: profile_path) do |form| %>
   <fieldset>
+    <legend><%= t('.change_username') %></legend>
     <%= form.label :username %>
-    <%= form.text_field :username %>
+    <%= form.text_field :username, required: true %>
+    <%= form.label :password_challenge, "Existing password" %>
+    <%= form.password_field :password_challenge, required: true %>
   </fieldset>
-  <%= form.submit("Save") %>
+  <%= form.submit("Update username") %>
 <% end %>
 
-<h2><%= t('.change_password') %></h2>
 <%= form_with(model: user, url: password_path) do |form| %>
   <fieldset>
+    <legend><%= t('.change_password') %></legend>
     <%= form.label :password_challenge, "Existing password" %>
-    <%= form.password_field :password_challenge %>
+    <%= form.password_field :password_challenge, required: true %>
     <%= form.label :password, "New password" %>
-    <%= form.password_field :password %>
+    <%= form.password_field :password, required: true %>
     <%= form.label :password_confirmation %>
-    <%= form.password_field :password_confirmation %>
+    <%= form.password_field :password_confirmation, required: true %>
   </fieldset>
   <%= form.submit("Update password") %>
 <% end %>

config/locales/en.yml

@@ -124,6 +124,10 @@ en:
   profiles:
     edit:
       title: Edit profile
+      warning_html:
+        <strong>Warning!!!</strong> Editing your username or password will
+        change your API key as well. You will need to update your credentials
+        in any Fever clients you have connected to Stringer.
     update:
       success: Profile updated
       failure: 'Unable to update profile: %{errors}'

db/migrate/20230301024452_encrypt_api_key.rb

@@ -4,10 +4,7 @@ class EncryptAPIKey < ActiveRecord::Migration[7.0]
   def change
     ActiveRecord::Encryption.config.support_unencrypted_data = true
 
-    User.find_each do |user|
-      user.regenerate_api_key if user.api_key.blank?
-      user.encrypt
-    end
+    User.find_each(&:encrypt)
 
     ActiveRecord::Encryption.config.support_unencrypted_data = false
 

spec/models/user_spec.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+RSpec.describe User do
+  describe "#update_api_key" do
+    it "updates the api key when the username changed" do
+      user = create(:user, username: "stringer", password: "super-secret")
+      user.update!(username: "booger")
+
+      expect(user.api_key).to eq(Digest::MD5.hexdigest("booger:super-secret"))
+    end
+
+    it "updates the api key when the password changed" do
+      user = create(:user, username: "stringer", password: "super-secret")
+      user.update!(password: "new-password")
+
+      expect(user.api_key).to eq(Digest::MD5.hexdigest("stringer:new-password"))
+    end
+
+    it "does nothing when the username and password have not changed" do
+      user = create(:user, username: "stringer", password: "super-secret")
+      user = described_class.find(user.id)
+
+      expect { user.save! }.to not_change(user, :api_key).and not_raise_error
+    end
+
+    it "raises an error when password and password challenge are blank" do
+      user = create(:user, username: "stringer", password: "super-secret")
+      user = described_class.find(user.id)
+      expected_message = "Cannot change username without providing a password"
+
+      expect { user.update!(username: "booger") }
+        .to raise_error(ActiveRecord::ActiveRecordError, expected_message)
+    end
+  end
+end

spec/requests/profiles_controller_spec.rb

@@ -16,17 +16,19 @@
     context "when the user is valid" do
       it "updates the user's username" do
         login_as(default_user)
-
-        params = { user: { username: "new_username" } }
+        password_challenge = default_user.password
+        params = { user: { username: "new_username", password_challenge: } }
 
         expect { patch(profile_path, params:) }
           .to change_record(default_user, :username).to("new_username")
       end
 
       it "redirects to the news path" do
         login_as(default_user)
+        password_challenge = default_user.password
+        params = { user: { username: "new_username", password_challenge: } }
 
-        patch(profile_path, params: { user: { username: "new_username" } })
+        patch(profile_path, params:)
 
         expect(response).to redirect_to(news_path)
       end

spec/support/matchers.rb

@@ -23,5 +23,6 @@ def invoke(expected_method)
 RSpec::Matchers.define_negated_matcher(:not_change, :change)
 RSpec::Matchers.define_negated_matcher(:not_change_record, :change_record)
 RSpec::Matchers.define_negated_matcher(:not_delete_record, :delete_record)
+RSpec::Matchers.define_negated_matcher(:not_raise_error, :raise_error)
 
 Dir[File.join(__dir__, "./matchers/*.rb")].each { |path| require path }

spec/system/profile_spec.rb

@@ -5,12 +5,19 @@
     login_as(default_user)
     visit(edit_profile_path)
 
-    fill_in("Username", with: "new_username")
-    click_on("Save")
+    fill_in_username_fields(default_user.password)
+    click_on("Update username")
 
     expect(page).to have_text("Logged in as new_username")
   end
 
+  def fill_in_username_fields(existing_password)
+    within_fieldset("Change Username") do
+      fill_in("Username", with: "new_username")
+      fill_in("Existing password", with: existing_password)
+    end
+  end
+
   it "allows the user to edit their password" do
     login_as(default_user)
     visit(edit_profile_path)
@@ -22,8 +29,10 @@
   end
 
   def fill_in_password_fields(existing_password, new_password)
-    fill_in("Existing password", with: existing_password)
-    fill_in("New password", with: new_password)
-    fill_in("Password confirmation", with: new_password)
+    within_fieldset("Change Password") do
+      fill_in("Existing password", with: existing_password)
+      fill_in("New password", with: new_password)
+      fill_in("Password confirmation", with: new_password)
+    end
   end
 end