Address second-round review findings in the PKCE core

- Enqueue the expiry job and the "app connected" notification on
  after_commit instead of after_create, so they only fire once the (now
  transactional) code exchange commits
- Enforce the PKCE code_challenge format (43-128 unreserved chars, S256)
  with model validations, not only at the controller
- Drop the now-unused RsAuthorizationCode#expired?
- Cover the client_id-vs-stored-client_id check

Part of #254

app/models/remote_storage_authorization.rb

@@ -17,8 +17,8 @@ class RemoteStorageAuthorization < ApplicationRecord
   before_create  :generate_token
   before_create  :store_token_in_redis
   before_create  :find_or_create_web_app
-  after_create   :schedule_token_expiry
-  after_create   :notify_user
+  after_commit   :schedule_token_expiry, on: :create
+  after_commit   :notify_user, on: :create
   before_destroy :delete_token_from_redis
   after_destroy  :remove_token_expiry_job
 

app/models/rs_authorization_code.rb

@@ -9,6 +9,10 @@ class RsAuthorizationCode < ApplicationRecord
 
   before_validation :generate_code
 
+  validates :code_challenge, presence: true,
+            format: { with: /\A[A-Za-z0-9\-._~]{43,128}\z/ }
+  validates :code_challenge_method, inclusion: { in: %w[S256] }
+
   scope :active, -> { where(expires_at: Time.current..) }
 
   def self.find_by_code(raw_code)
@@ -17,10 +21,6 @@ class RsAuthorizationCode < ApplicationRecord
     active.find_by(code_digest: Digest::SHA256.hexdigest(raw_code))
   end
 
-  def expired?
-    expires_at <= Time.current
-  end
-
   private
 
     def generate_code

spec/models/rs_authorization_code_spec.rb

@@ -3,17 +3,44 @@ require 'rails_helper'
 RSpec.describe RsAuthorizationCode, type: :model do
   let(:user) { create :user }
 
-  def build_code(**attrs)
-    RsAuthorizationCode.create!(
+  # Example value from RFC 7636, Appendix B
+  let(:challenge) { "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" }
+
+  def new_code(**attrs)
+    RsAuthorizationCode.new(
       user: user,
       client_id: "example.com",
       redirect_uri: "https://example.com",
       permissions: %w(documents:rw),
-      code_challenge: "challenge",
+      code_challenge: challenge,
       **attrs
     )
   end
 
+  def build_code(**attrs)
+    new_code(**attrs).tap(&:save!)
+  end
+
+  describe "validations" do
+    it "is valid with a well-formed S256 challenge" do
+      expect(new_code).to be_valid
+    end
+
+    it "rejects a challenge shorter than 43 characters" do
+      expect(new_code(code_challenge: "short")).not_to be_valid
+    end
+
+    it "rejects a challenge with invalid characters" do
+      expect(new_code(code_challenge: "#{'a' * 42}!")).not_to be_valid
+    end
+
+    it "rejects an unsupported challenge method" do
+      code = new_code(code_challenge_method: "plain")
+      expect(code).not_to be_valid
+      expect(code.errors[:code_challenge_method]).to be_present
+    end
+  end
+
   describe "#create" do
     it "generates a raw code and stores only its digest" do
       code = build_code
@@ -49,14 +76,4 @@ RSpec.describe RsAuthorizationCode, type: :model do
       expect(RsAuthorizationCode.find_by_code(code.code)).to be_nil
     end
   end
-
-  describe "#expired?" do
-    it "is false before the expiry" do
-      expect(build_code).not_to be_expired
-    end
-
-    it "is true after the expiry" do
-      expect(build_code(expires_at: 1.minute.ago)).to be_expired
-    end
-  end
 end

spec/services/rs_oauth_manager/exchange_code_spec.rb

@@ -94,6 +94,19 @@ RSpec.describe RsOauthManager::ExchangeCode do
       .to raise_error(RsOauthManagerService::InvalidGrant)
   end
 
+  it "rejects a code whose stored client_id host differs from the presented one" do
+    code = RsAuthorizationCode.create!(
+      user: user,
+      client_id: "evil.example.org",
+      redirect_uri: "https://example.com",
+      permissions: %w(documents:rw),
+      code_challenge: code_challenge,
+      code_challenge_method: "S256"
+    )
+
+    expect { exchange(code) }.to raise_error(RsOauthManagerService::InvalidGrant)
+  end
+
   it "rejects a bad code_verifier" do
     code = issue_code
 

spec/services/rs_oauth_manager/issue_code_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe RsOauthManager::IssueCode do
       client_id: "example.com",
       redirect_uri: "https://example.com",
       permissions: %w(documents:rw),
-      code_challenge: "challenge",
+      code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM",
       code_challenge_method: "S256",
       expire_at: 1.month.from_now
     )
@@ -20,7 +20,7 @@ RSpec.describe RsOauthManager::IssueCode do
     expect(subject.code).to be_present
     expect(subject.client_id).to eq("example.com")
     expect(subject.permissions).to eq(%w(documents:rw))
-    expect(subject.code_challenge).to eq("challenge")
+    expect(subject.code_challenge).to eq("E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM")
   end
 
   it "does not create a remote storage authorization" do