From f1bc68c73e7183a0ad30c6aefc6cc4cbbf1bc1f0 Mon Sep 17 00:00:00 2001
From: jessib <jessib@riseup.net>
Date: Mon, 18 Nov 2013 16:18:33 -0800
Subject: Need to cleanup some, but start to show public key for /key/username

---
 users/app/controllers/users_controller.rb      | 6 ++++++
 users/app/views/users/get_public_key.html.haml | 2 ++
 users/config/routes.rb                         | 2 ++
 3 files changed, 10 insertions(+)
 create mode 100644 users/app/views/users/get_public_key.html.haml

(limited to 'users')

diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb
index 3cbb6dc..3f4daeb 100644
--- a/users/app/controllers/users_controller.rb
+++ b/users/app/controllers/users_controller.rb
@@ -59,4 +59,10 @@ class UsersController < UsersBaseController
     end
   end
 
+  def get_public_key
+    @show_navigation = false
+    user = User.find_by_login(params[:login])
+    @public_key = user.public_key if user
+  end
+
 end
diff --git a/users/app/views/users/get_public_key.html.haml b/users/app/views/users/get_public_key.html.haml
new file mode 100644
index 0000000..eccb367
--- /dev/null
+++ b/users/app/views/users/get_public_key.html.haml
@@ -0,0 +1,2 @@
+- if @public_key
+  = @public_key
\ No newline at end of file
diff --git a/users/config/routes.rb b/users/config/routes.rb
index ccecfd5..1077561 100644
--- a/users/config/routes.rb
+++ b/users/config/routes.rb
@@ -22,4 +22,6 @@ Rails.application.routes.draw do
 
   get "/.well-known/host-meta" => 'webfinger#host_meta'
   get "/webfinger" => 'webfinger#search'
+  get "/key/:login" => 'users#get_public_key'
+
 end
-- 
cgit v1.2.3


From d82ea5da2aa705bcfa74f2a8b42a197883b694e3 Mon Sep 17 00:00:00 2001
From: jessib <jessib@riseup.net>
Date: Thu, 21 Nov 2013 12:15:03 -0800
Subject: Refactoring of code, and tests.

---
 users/app/controllers/keys_controller.rb       | 10 +++++++++
 users/app/controllers/users_controller.rb      |  6 -----
 users/app/views/users/get_public_key.html.haml |  2 --
 users/config/routes.rb                         |  2 +-
 users/test/functional/keys_controller_test.rb  | 31 ++++++++++++++++++++++++++
 5 files changed, 42 insertions(+), 9 deletions(-)
 create mode 100644 users/app/controllers/keys_controller.rb
 delete mode 100644 users/app/views/users/get_public_key.html.haml
 create mode 100644 users/test/functional/keys_controller_test.rb

(limited to 'users')

diff --git a/users/app/controllers/keys_controller.rb b/users/app/controllers/keys_controller.rb
new file mode 100644
index 0000000..9a39fc4
--- /dev/null
+++ b/users/app/controllers/keys_controller.rb
@@ -0,0 +1,10 @@
+class KeysController < ApplicationController
+
+  def show
+    user = User.find_by_login(params[:login])
+    # layout won't be included if we render text
+    # we will show blank page if user doesn't have key or user doesn't exist
+    render text: user ? user.public_key : ''
+  end
+
+end
diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb
index 3f4daeb..3cbb6dc 100644
--- a/users/app/controllers/users_controller.rb
+++ b/users/app/controllers/users_controller.rb
@@ -59,10 +59,4 @@ class UsersController < UsersBaseController
     end
   end
 
-  def get_public_key
-    @show_navigation = false
-    user = User.find_by_login(params[:login])
-    @public_key = user.public_key if user
-  end
-
 end
diff --git a/users/app/views/users/get_public_key.html.haml b/users/app/views/users/get_public_key.html.haml
deleted file mode 100644
index eccb367..0000000
--- a/users/app/views/users/get_public_key.html.haml
+++ /dev/null
@@ -1,2 +0,0 @@
-- if @public_key
-  = @public_key
\ No newline at end of file
diff --git a/users/config/routes.rb b/users/config/routes.rb
index 1077561..69f9cf7 100644
--- a/users/config/routes.rb
+++ b/users/config/routes.rb
@@ -22,6 +22,6 @@ Rails.application.routes.draw do
 
   get "/.well-known/host-meta" => 'webfinger#host_meta'
   get "/webfinger" => 'webfinger#search'
-  get "/key/:login" => 'users#get_public_key'
+  get "/key/:login" => 'keys#show'
 
 end
diff --git a/users/test/functional/keys_controller_test.rb b/users/test/functional/keys_controller_test.rb
new file mode 100644
index 0000000..9cc88d1
--- /dev/null
+++ b/users/test/functional/keys_controller_test.rb
@@ -0,0 +1,31 @@
+require 'test_helper'
+
+class KeysControllerTest < ActionController::TestCase
+
+  test "get existing public key" do
+    public_key = 'my public key'
+    @user = stub_record :user, :public_key => public_key
+    User.stubs(:find_by_login).with(@user.login).returns(@user)
+    get :show, :login => @user.login
+    assert_response :success
+    assert_equal "text/html", response.content_type
+    assert_equal public_key, response.body
+  end
+
+  test "get non-existing public key for user" do
+    @user = stub_record :user
+    User.stubs(:find_by_login).with(@user.login).returns(@user)
+    get :show, :login => @user.login
+    assert_response :success
+    assert_equal "text/html", response.content_type
+    assert_equal '', response.body.strip
+  end
+
+  test "get public key for non-existing user" do
+    get :show, :login => 'asdkljslksjfdlskfj'
+    assert_response :success
+    assert_equal "text/html", response.content_type
+    assert_equal '', response.body.strip
+  end
+
+end
-- 
cgit v1.2.3


From 299dfdf4164ee10de63aa2543935eeed65437b3f Mon Sep 17 00:00:00 2001
From: jessib <jessib@riseup.net>
Date: Mon, 25 Nov 2013 11:31:33 -0800
Subject: Give 404 error if one goes to /key/user for non-existing user.

---
 users/app/controllers/keys_controller.rb      | 6 ++++--
 users/test/functional/keys_controller_test.rb | 9 +++++----
 2 files changed, 9 insertions(+), 6 deletions(-)

(limited to 'users')

diff --git a/users/app/controllers/keys_controller.rb b/users/app/controllers/keys_controller.rb
index 9a39fc4..949f2c0 100644
--- a/users/app/controllers/keys_controller.rb
+++ b/users/app/controllers/keys_controller.rb
@@ -3,8 +3,10 @@ class KeysController < ApplicationController
   def show
     user = User.find_by_login(params[:login])
     # layout won't be included if we render text
-    # we will show blank page if user doesn't have key or user doesn't exist
-    render text: user ? user.public_key : ''
+    # we will show blank page if user doesn't have key (which shouldn't generally occur)
+    # and a 404 error if user doesn't exist
+    user ? (render text: user.public_key) : (raise ActionController::RoutingError.new('Not Found'))
+
   end
 
 end
diff --git a/users/test/functional/keys_controller_test.rb b/users/test/functional/keys_controller_test.rb
index 9cc88d1..b69cbc0 100644
--- a/users/test/functional/keys_controller_test.rb
+++ b/users/test/functional/keys_controller_test.rb
@@ -13,6 +13,7 @@ class KeysControllerTest < ActionController::TestCase
   end
 
   test "get non-existing public key for user" do
+    # this isn't a scenerio that should generally occur.
     @user = stub_record :user
     User.stubs(:find_by_login).with(@user.login).returns(@user)
     get :show, :login => @user.login
@@ -22,10 +23,10 @@ class KeysControllerTest < ActionController::TestCase
   end
 
   test "get public key for non-existing user" do
-    get :show, :login => 'asdkljslksjfdlskfj'
-    assert_response :success
-    assert_equal "text/html", response.content_type
-    assert_equal '', response.body.strip
+    # raise 404 error if user doesn't exist (doesn't need to be this routing error, but seems fine to assume for now):
+    assert_raise(ActionController::RoutingError) {
+      get :show, :login => 'asdkljslksjfdlskfj'
+    }
   end
 
 end
-- 
cgit v1.2.3


From 7de12c71ce7eb4eeb6e0795275434ed4a4120c25 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Tue, 26 Nov 2013 11:22:47 +0100
Subject: ignore attempts to empty public_key, refactor

refactor: prepare validations of the uploaded pgp keys
---
 users/app/models/account.rb                     | 11 ++++++++---
 users/app/models/identity.rb                    |  6 +++---
 users/app/models/pgp_key.rb                     | 25 +++++++++++++++++++++++++
 users/test/integration/api/account_flow_test.rb |  6 +++---
 4 files changed, 39 insertions(+), 9 deletions(-)
 create mode 100644 users/app/models/pgp_key.rb

(limited to 'users')

diff --git a/users/app/models/account.rb b/users/app/models/account.rb
index 5c943bb..cf998e4 100644
--- a/users/app/models/account.rb
+++ b/users/app/models/account.rb
@@ -27,7 +27,8 @@ class Account
       @user.update_attributes attrs.slice(:password_verifier, :password_salt)
     end
     # TODO: move into identity controller
-    update_pgp_key(attrs[:public_key]) if attrs.has_key? :public_key
+    key = update_pgp_key(attrs[:public_key])
+    @user.errors.set :public_key, key.errors.full_messages
     @user.save && save_identities
     @user.refresh_identity
   end
@@ -49,8 +50,12 @@ class Account
   end
 
   def update_pgp_key(key)
-    @new_identity ||= Identity.for(@user)
-    @new_identity.set_key(:pgp, key)
+    PgpKey.new(key).tap do |key|
+      if key.present? && key.valid?
+        @new_identity ||= Identity.for(@user)
+        @new_identity.set_key(:pgp, key)
+      end
+    end
   end
 
   def save_identities
diff --git a/users/app/models/identity.rb b/users/app/models/identity.rb
index 97966d0..cbb540e 100644
--- a/users/app/models/identity.rb
+++ b/users/app/models/identity.rb
@@ -94,9 +94,9 @@ class Identity < CouchRest::Model::Base
     read_attribute('keys') || HashWithIndifferentAccess.new
   end
 
-  def set_key(type, value)
-    return if keys[type] == value
-    write_attribute('keys', keys.merge(type => value))
+  def set_key(type, key)
+    return if keys[type] == key.to_s
+    write_attribute('keys', keys.merge(type => key.to_s))
   end
 
   # for LoginFormatValidation
diff --git a/users/app/models/pgp_key.rb b/users/app/models/pgp_key.rb
new file mode 100644
index 0000000..fddec1e
--- /dev/null
+++ b/users/app/models/pgp_key.rb
@@ -0,0 +1,25 @@
+class PgpKey
+  include ActiveModel::Validations
+
+  # mostly for testing.
+  attr_accessor :key_block
+
+  def initialize(key_block = nil)
+    @key_block = key_block
+  end
+
+  def to_s
+    @key_block
+  end
+
+  def present?
+    @key_block.present?
+  end
+
+  # let's allow comparison with plain key_block strings.
+  def ==(other)
+    self.equal?(other) or
+    self.to_s == other
+  end
+
+end
diff --git a/users/test/integration/api/account_flow_test.rb b/users/test/integration/api/account_flow_test.rb
index e41befa..90f2a97 100644
--- a/users/test/integration/api/account_flow_test.rb
+++ b/users/test/integration/api/account_flow_test.rb
@@ -114,9 +114,9 @@ class AccountFlowTest < RackTest
     # should not overwrite public key:
     put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:blee => :blah}, :format => :json
     assert_equal test_public_key, Identity.for(@user).keys[:pgp]
-    # should overwrite public key:
-    put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:public_key => nil}, :format => :json
-    assert_nil Identity.for(@user).keys[:pgp]
+    # should not empty public key:
+    put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:public_key => ""}, :format => :json
+    assert_equal test_public_key, Identity.for(@user).keys[:pgp]
   end
 
 end
-- 
cgit v1.2.3


From e34141c3265c6daeda92bcb83fa508de00551bc3 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Tue, 26 Nov 2013 14:39:42 +0100
Subject: simple validation for pgp key format

---
 users/app/models/pgp_key.rb                     | 37 ++++++++++++++++++++-----
 users/test/factories.rb                         |  8 ++++++
 users/test/integration/api/account_flow_test.rb | 34 ++++++++++++++++-------
 users/test/integration/browser/account_test.rb  |  4 +--
 4 files changed, 64 insertions(+), 19 deletions(-)

(limited to 'users')

diff --git a/users/app/models/pgp_key.rb b/users/app/models/pgp_key.rb
index fddec1e..66f8660 100644
--- a/users/app/models/pgp_key.rb
+++ b/users/app/models/pgp_key.rb
@@ -1,25 +1,48 @@
 class PgpKey
   include ActiveModel::Validations
 
+  KEYBLOCK_IDENTIFIERS = [
+    '-----BEGIN PGP PUBLIC KEY BLOCK-----',
+    '-----END PGP PUBLIC KEY BLOCK-----',
+  ]
+
   # mostly for testing.
-  attr_accessor :key_block
+  attr_accessor :keyblock
+
+  validate :validate_keyblock_format
 
-  def initialize(key_block = nil)
-    @key_block = key_block
+  def initialize(keyblock = nil)
+    @keyblock = keyblock
   end
 
   def to_s
-    @key_block
+    @keyblock
   end
 
   def present?
-    @key_block.present?
+    @keyblock.present?
   end
 
-  # let's allow comparison with plain key_block strings.
+  # allow comparison with plain keyblock strings.
   def ==(other)
     self.equal?(other) or
-    self.to_s == other
+    # relax the comparison on line ends.
+    self.to_s.tr_s("\n\r", '') == other.tr_s("\r\n", '')
+  end
+
+  protected
+
+  def validate_keyblock_format
+    if keyblock_identifier_missing?
+      errors.add :public_key_block,
+        "does not look like an armored pgp public key block"
+    end
+  end
+
+  def keyblock_identifier_missing?
+    KEYBLOCK_IDENTIFIERS.find do |identify|
+      !@keyblock.include?(identify)
+    end
   end
 
 end
diff --git a/users/test/factories.rb b/users/test/factories.rb
index f5fb77d..ae00d43 100644
--- a/users/test/factories.rb
+++ b/users/test/factories.rb
@@ -23,4 +23,12 @@ FactoryGirl.define do
     user
   end
 
+  factory :pgp_key do
+    keyblock <<-EOPGP
+-----BEGIN PGP PUBLIC KEY BLOCK-----
++Dummy+PGP+KEY+++Dummy+PGP+KEY+++Dummy+PGP+KEY+++Dummy+PGP+KEY+
+#{SecureRandom.base64(4032)}
+-----END PGP PUBLIC KEY BLOCK-----
+    EOPGP
+  end
 end
diff --git a/users/test/integration/api/account_flow_test.rb b/users/test/integration/api/account_flow_test.rb
index 90f2a97..9aee38b 100644
--- a/users/test/integration/api/account_flow_test.rb
+++ b/users/test/integration/api/account_flow_test.rb
@@ -96,27 +96,41 @@ class AccountFlowTest < RackTest
     assert server_auth["M2"]
   end
 
-  test "update user" do
+  test "changing login" do
     server_auth = @srp.authenticate(self)
-    test_public_key = 'asdlfkjslfdkjasd'
     original_login = @user.login
     new_login = 'zaph'
     User.find_by_login(new_login).try(:destroy)
     Identity.by_address.key(new_login + '@' + APP_CONFIG[:domain]).each do |identity|
       identity.destroy
     end
-    put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:public_key => test_public_key, :login => new_login}, :format => :json
+    put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:login => new_login}, :format => :json
     assert last_response.successful?
-    assert_equal test_public_key, Identity.for(@user).keys[:pgp]
     # does not change login if no password_verifier is present
     assert_equal original_login, @user.login
-    # eventually probably want to remove most of this into a non-integration functional test
-    # should not overwrite public key:
-    put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:blee => :blah}, :format => :json
-    assert_equal test_public_key, Identity.for(@user).keys[:pgp]
-    # should not empty public key:
+  end
+
+  test "upload pgp key" do
+    server_auth = @srp.authenticate(self)
+    key = FactoryGirl.build :pgp_key
+    put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:public_key => key}, :format => :json
+    assert_equal key, Identity.for(@user).keys[:pgp]
+  end
+
+  # eventually probably want to remove most of this into a non-integration
+  # functional test
+  test "prevent uploading invalid key" do
+    server_auth = @srp.authenticate(self)
+    put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:public_key => :blah}, :format => :json
+    assert_nil Identity.for(@user).keys[:pgp]
+  end
+
+  test "prevent emptying public key" do
+    server_auth = @srp.authenticate(self)
+    key = FactoryGirl.build :pgp_key
+    put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:public_key => key}, :format => :json
     put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:public_key => ""}, :format => :json
-    assert_equal test_public_key, Identity.for(@user).keys[:pgp]
+    assert_equal key, Identity.for(@user).keys[:pgp]
   end
 
 end
diff --git a/users/test/integration/browser/account_test.rb b/users/test/integration/browser/account_test.rb
index b349489..3d281ae 100644
--- a/users/test/integration/browser/account_test.rb
+++ b/users/test/integration/browser/account_test.rb
@@ -66,7 +66,7 @@ class AccountTest < BrowserIntegrationTest
   end
 
   test "change pgp key" do
-    pgp_key = "My PGP Key Stub"
+    pgp_key = FactoryGirl.build :pgp_key
     username, password = submit_signup
     click_on "Account Settings"
     within('#update_pgp_key') do
@@ -76,7 +76,7 @@ class AccountTest < BrowserIntegrationTest
     page.assert_selector 'input[value="Saving..."]'
     # at some point we're done:
     page.assert_no_selector 'input[value="Saving..."]'
-    assert page.has_field? 'Public key', with: pgp_key
+    assert page.has_field? 'Public key', with: pgp_key.to_s
     user = User.find_by_login(username)
     assert_equal pgp_key, user.public_key
     user.account.destroy
-- 
cgit v1.2.3


From dade6497424a869db5f1dfb030f88f4711278b81 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Wed, 27 Nov 2013 09:43:28 +0100
Subject: minor: rename test to what it actually tests

[skip ci]
---
 users/test/integration/api/account_flow_test.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'users')

diff --git a/users/test/integration/api/account_flow_test.rb b/users/test/integration/api/account_flow_test.rb
index 9aee38b..edd0859 100644
--- a/users/test/integration/api/account_flow_test.rb
+++ b/users/test/integration/api/account_flow_test.rb
@@ -96,7 +96,7 @@ class AccountFlowTest < RackTest
     assert server_auth["M2"]
   end
 
-  test "changing login" do
+  test "prevent changing login without changing password_verifier" do
     server_auth = @srp.authenticate(self)
     original_login = @user.login
     new_login = 'zaph'
-- 
cgit v1.2.3