diff options
author | jessib <jessib@riseup.net> | 2013-08-27 12:18:35 -0700 |
---|---|---|
committer | jessib <jessib@riseup.net> | 2013-08-27 12:18:35 -0700 |
commit | dc41ae0a3fb0a137e716d8ec63084b0ec3a7299b (patch) | |
tree | c09fef161f105e7c03c35d1edcb2d257144cb97d | |
parent | a87c750d1f12f15272beb117f8ee12ab711cc6d1 (diff) | |
parent | e481b8cbc05a858674a59ef36d695973622f6b3a (diff) |
Merge branch 'master' into billing_with_tests
43 files changed, 658 insertions, 385 deletions
@@ -31,3 +31,5 @@ bin .*.swp public/1/* vendor/bundle/* +public/img +config/couchdb.yml.* diff --git a/app/views/layouts/_navigation.html.haml b/app/views/layouts/_navigation.html.haml index 3eb2289..fb018a9 100644 --- a/app/views/layouts/_navigation.html.haml +++ b/app/views/layouts/_navigation.html.haml @@ -1,7 +1,6 @@ %ul.nav.sidenav = link_to_navigation t(:overview), user_overview_path(@user), :active => controller?(:overviews) = link_to_navigation t(:account_settings), edit_user_path(@user), :active => controller?(:users) - = link_to_navigation t(:email_settings), edit_user_email_settings_path(@user), :active => controller?(:email_settings) = link_to_navigation t(:support_tickets), auto_tickets_path, :active => controller?(:tickets) = link_to_navigation t(:billing_settings), show_or_new_customer_link(@user), :active => controller?(:customer, :payments, :subscriptions, :credit_card_info) if engine_enabled('LeapWebBilling') = link_to_navigation t(:logout), logout_path, :method => :delete diff --git a/help/app/controllers/tickets_controller.rb b/help/app/controllers/tickets_controller.rb index 094612c..a03ef22 100644 --- a/help/app/controllers/tickets_controller.rb +++ b/help/app/controllers/tickets_controller.rb @@ -18,6 +18,7 @@ class TicketsController < ApplicationController @ticket = Ticket.new(params[:ticket]) @ticket.comments.last.posted_by = (logged_in? ? current_user.id : nil) #protecting posted_by isn't working, so this should protect it. + @ticket.comments.last.private = false unless admin? @ticket.created_by = current_user.id if logged_in? @ticket.email = current_user.email_address if logged_in? and current_user.email_address @@ -58,6 +59,7 @@ class TicketsController < ApplicationController if @ticket.comments_changed? @ticket.comments.last.posted_by = (current_user ? current_user.id : nil) + @ticket.comments.last.private = false unless admin? end if @ticket.changed? diff --git a/help/app/models/ticket_comment.rb b/help/app/models/ticket_comment.rb index 1df7eec..13bea2b 100644 --- a/help/app/models/ticket_comment.rb +++ b/help/app/models/ticket_comment.rb @@ -7,7 +7,7 @@ class TicketComment property :posted_at, Time#, :protected => true #property :posted_verified, TrueClass, :protected => true #should be true if current_user is set when the comment is created property :body, String - property :private, TrueClass # private comments are only viewable by admins + property :private, TrueClass # private comments are only viewable by admins #this is checked when set, to make sure it was set by an admin # ? timestamps! validates :body, :presence => true diff --git a/test/unit/email_test.rb b/test/unit/email_test.rb new file mode 100644 index 0000000..e858bd5 --- /dev/null +++ b/test/unit/email_test.rb @@ -0,0 +1,18 @@ +require 'test_helper' + +class EmailTest < ActiveSupport::TestCase + + test "valid format" do + email = Email.new(email_string) + assert email.valid? + end + + test "validates format" do + email = Email.new("email") + assert !email.valid? + end + + def email_string + @email_string ||= Faker::Internet.email + end +end diff --git a/users/app/controllers/controller_extension/authentication.rb b/users/app/controllers/controller_extension/authentication.rb index 5fac884..dca3664 100644 --- a/users/app/controllers/controller_extension/authentication.rb +++ b/users/app/controllers/controller_extension/authentication.rb @@ -7,28 +7,8 @@ module ControllerExtension::Authentication helper_method :current_user, :logged_in?, :admin? end - def authentication_errors - return unless attempted_login? - errors = get_warden_errors - errors.inject({}) do |translated,err| - translated[err.first] = I18n.t(err.last) - translated - end - end - - def get_warden_errors - if strategy = warden.winning_strategy - message = strategy.message - # in case we get back the default message to fail! - message.respond_to?(:inject) ? message : { base: message } - else - { login: :all_strategies_failed } - end - end - - def attempted_login? - request.env['warden.options'] && - request.env['warden.options'][:attempted_path] + def current_user + @current_user ||= token_authenticate || warden.user end def logged_in? @@ -62,4 +42,27 @@ module ControllerExtension::Authentication access_denied unless admin? end + def authentication_errors + return unless attempted_login? + errors = get_warden_errors + errors.inject({}) do |translated,err| + translated[err.first] = I18n.t(err.last) + translated + end + end + + def get_warden_errors + if strategy = warden.winning_strategy + message = strategy.message + # in case we get back the default message to fail! + message.respond_to?(:inject) ? message : { base: message } + else + { login: :all_strategies_failed } + end + end + + def attempted_login? + request.env['warden.options'] && + request.env['warden.options'][:attempted_path] + end end diff --git a/users/app/controllers/controller_extension/token_authentication.rb b/users/app/controllers/controller_extension/token_authentication.rb new file mode 100644 index 0000000..3e2816d --- /dev/null +++ b/users/app/controllers/controller_extension/token_authentication.rb @@ -0,0 +1,23 @@ +module ControllerExtension::TokenAuthentication + extend ActiveSupport::Concern + + def token_authenticate + authenticate_with_http_token do |token_id, options| + @token = Token.find(token_id) + end + @token.user if @token + end + + def logout + super + clear_token + end + + def clear_token + authenticate_with_http_token do |token_id, options| + @token = Token.find(token_id) + @token.destroy if @token + end + end +end + diff --git a/users/app/controllers/email_settings_controller.rb b/users/app/controllers/email_settings_controller.rb deleted file mode 100644 index f7d85be..0000000 --- a/users/app/controllers/email_settings_controller.rb +++ /dev/null @@ -1,41 +0,0 @@ -class EmailSettingsController < UsersBaseController - - before_filter :authorize - before_filter :fetch_user - - def edit - @email_alias = LocalEmail.new - end - - def update - @user.attributes = cleanup_params(params[:user]) - if @user.changed? - if @user.save - flash[:notice] = t(:changes_saved) - redirect - else - if @user.email_aliases.last && !@user.email_aliases.last.valid? - # display bad alias in text field: - @email_alias = @user.email_aliases.pop - end - render 'email_settings/edit' - end - else - redirect - end - end - - private - - def redirect - redirect_to edit_user_email_settings_url(@user) - end - - def cleanup_params(user) - if !user['email_forward'].nil? && user['email_forward'].empty? - user.delete('email_forward') # don't allow "" as an email forward - end - user - end - -end diff --git a/users/app/controllers/v1/sessions_controller.rb b/users/app/controllers/v1/sessions_controller.rb index 295c327..1b20a82 100644 --- a/users/app/controllers/v1/sessions_controller.rb +++ b/users/app/controllers/v1/sessions_controller.rb @@ -29,7 +29,7 @@ module V1 def destroy logout - redirect_to root_path + head :no_content end protected diff --git a/users/app/controllers/v1/users_controller.rb b/users/app/controllers/v1/users_controller.rb index fda56f2..f380c19 100644 --- a/users/app/controllers/v1/users_controller.rb +++ b/users/app/controllers/v1/users_controller.rb @@ -18,17 +18,23 @@ module V1 end def create - @user = User.create(params[:user]) + @user = signup_service.register(params[:user]) respond_with @user # return ID instead? end def update - @user.update_attributes params[:user] - if @user.valid? - flash[:notice] = t(:user_updated_successfully) - end + account_settings.update params[:user] respond_with @user end + protected + + def account_settings + AccountSettings.new(@user) + end + + def signup_service + SignupService.new + end end end diff --git a/users/app/models/account_settings.rb b/users/app/models/account_settings.rb new file mode 100644 index 0000000..27fa227 --- /dev/null +++ b/users/app/models/account_settings.rb @@ -0,0 +1,36 @@ +class AccountSettings + + def initialize(user) + @user = user + end + + def update(attrs) + if attrs[:password_verifier].present? + update_login(attrs[:login]) + @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 + @user.save && save_identities + end + + protected + + def update_login(login) + return unless login.present? + @old_identity = Identity.for(@user) + @user.login = login + @new_identity = Identity.for(@user) # based on the new login + @old_identity.destination = @user.email_address # alias old -> new + end + + def update_pgp_key(key) + @new_identity ||= Identity.for(@user) + @new_identity.set_key(:pgp, key) + end + + def save_identities + @new_identity.try(:save) && @old_identity.try(:save) + end + +end diff --git a/users/app/models/email.rb b/users/app/models/email.rb index 6d82f2a..1bcff1c 100644 --- a/users/app/models/email.rb +++ b/users/app/models/email.rb @@ -1,33 +1,22 @@ -module Email - extend ActiveSupport::Concern +class Email < String + include ActiveModel::Validations - included do - validates :email, - :format => { - :with => /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/, - :message => "needs to be a valid email address" - } - end - - def initialize(attributes = nil, &block) - attributes = {:email => attributes} if attributes.is_a? String - super(attributes, &block) - end - - def to_s - email - end - - def ==(other) - other.is_a?(Email) ? self.email == other.email : self.email == other - end + validates :email, + :format => { + :with => /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/, + :message => "needs to be a valid email address" + } def to_partial_path "emails/email" end def to_param - email + to_s + end + + def email + self end end diff --git a/users/app/models/identity.rb b/users/app/models/identity.rb new file mode 100644 index 0000000..355f67a --- /dev/null +++ b/users/app/models/identity.rb @@ -0,0 +1,82 @@ +class Identity < CouchRest::Model::Base + + use_database :identities + + belongs_to :user + + property :address, LocalEmail + property :destination, Email + property :keys, HashWithIndifferentAccess + + validate :unique_forward + validate :alias_available + + design do + view :by_user_id + view :by_address_and_destination + view :by_address + view :pgp_key_by_email, + map: <<-EOJS + function(doc) { + if (doc.type != 'Identity') { + return; + } + emit(doc.address, doc.keys["pgp"]); + } + EOJS + + end + + def self.for(user, attributes = {}) + find_for(user, attributes) || build_for(user, attributes) + end + + def self.find_for(user, attributes = {}) + attributes.reverse_merge! attributes_from_user(user) + find_by_address_and_destination [attributes[:address], attributes[:destination]] + end + + def self.build_for(user, attributes = {}) + attributes.reverse_merge! attributes_from_user(user) + Identity.new(attributes) + end + + def self.create_for(user, attributes = {}) + identity = build_for(user, attributes) + identity.save + identity + end + + def self.attributes_from_user(user) + { user_id: user.id, + address: user.email_address, + destination: user.email_address + } + end + + def keys + read_attribute('keys') || HashWithIndifferentAccess.new + end + + def set_key(type, value) + return if keys[type] == value + write_attribute('keys', keys.merge(type => value)) + end + + protected + + def unique_forward + same = Identity.find_by_address_and_destination([address, destination]) + if same && same != self + errors.add :base, "This alias already exists" + end + end + + def alias_available + same = Identity.find_by_address(address) + if same && same.user != self.user + errors.add :base, "This email has already been taken" + end + end + +end diff --git a/users/app/models/local_email.rb b/users/app/models/local_email.rb index 69cba01..c1f7c11 100644 --- a/users/app/models/local_email.rb +++ b/users/app/models/local_email.rb @@ -1,63 +1,39 @@ -class LocalEmail - include CouchRest::Model::Embeddable - include Email +class LocalEmail < Email - property :username, String - before_validation :strip_domain_if_needed - - validates :username, - :presence => true, - :format => { :with => /\A([^@\s]+)(@#{APP_CONFIG[:domain]})?\Z/i, :message => "needs to be a valid login or email address @#{APP_CONFIG[:domain]}"} - - validate :unique_on_server - validate :unique_alias_for_user - validate :differs_from_login - - validates :casted_by, :presence => true - - def email - return '' if username.nil? - username + '@' + APP_CONFIG[:domain] + def self.domain + APP_CONFIG[:domain] end - def email=(value) - return if value.blank? - self.username = value - strip_domain_if_needed + validates :email, + :format => { + :with => /@#{domain}\Z/i, + :message => "needs to end in @#{domain}" + } + + def initialize(s) + super + append_domain_if_needed end def to_key - [username] + [handle] end - protected - - def unique_on_server - has_email = User.find_by_login_or_alias(username) - if has_email && has_email != self.casted_by - errors.add :username, "has already been taken" - end + def handle + gsub(/@#{domain}/i, '') end - def unique_alias_for_user - aliases = self.casted_by.email_aliases - if aliases.select{|a|a.username == self.username}.count > 1 - errors.add :username, "is already your alias" - end + def domain + LocalEmail.domain end - def differs_from_login - # If this has not changed but the email let's mark the email invalid instead. - return if self.persisted? - user = self.casted_by - if user.login == self.username - errors.add :username, "may not be the same as your email address" - end - end + protected - def strip_domain_if_needed - self.username.gsub! /@#{APP_CONFIG[:domain]}/i, '' + def append_domain_if_needed + unless self.index('@') + self << '@' + domain + end end end diff --git a/users/app/models/login_format_validation.rb b/users/app/models/login_format_validation.rb new file mode 100644 index 0000000..1d02bd1 --- /dev/null +++ b/users/app/models/login_format_validation.rb @@ -0,0 +1,19 @@ +module LoginFormatValidation + extend ActiveSupport::Concern + + included do + # Have multiple regular expression validations so we can get specific error messages: + validates :login, + :format => { :with => /\A.{2,}\z/, + :message => "Login must have at least two characters"} + validates :login, + :format => { :with => /\A[a-z\d_\.-]+\z/, + :message => "Only lowercase letters, digits, . - and _ allowed."} + validates :login, + :format => { :with => /\A[a-z].*\z/, + :message => "Login must begin with a lowercase letter"} + validates :login, + :format => { :with => /\A.*[a-z\d]\z/, + :message => "Login must end with a letter or digit"} + end +end diff --git a/users/app/models/remote_email.rb b/users/app/models/remote_email.rb deleted file mode 100644 index 4fe7425..0000000 --- a/users/app/models/remote_email.rb +++ /dev/null @@ -1,14 +0,0 @@ -class RemoteEmail - include CouchRest::Model::Embeddable - include Email - - property :email, String - - def username - email.spilt('@').first - end - - def domain - email.split('@').last - end -end diff --git a/users/app/models/session.rb b/users/app/models/session.rb index a9fdb1b..0d7e10e 100644 --- a/users/app/models/session.rb +++ b/users/app/models/session.rb @@ -1,12 +1,10 @@ class Session < SRP::Session include ActiveModel::Validations + include LoginFormatValidation attr_accessor :login - validates :login, - :presence => true, - :format => { :with => /\A[A-Za-z\d_]+\z/, - :message => "Only letters, digits and _ allowed" } + validates :login, :presence => true def initialize(user = nil, aa = nil) super(user, aa) if user diff --git a/users/app/models/signup_service.rb b/users/app/models/signup_service.rb new file mode 100644 index 0000000..f316ca9 --- /dev/null +++ b/users/app/models/signup_service.rb @@ -0,0 +1,9 @@ +class SignupService + + def register(attrs) + User.create(attrs).tap do |user| + Identity.create_for user + end + end + +end diff --git a/users/app/models/token.rb b/users/app/models/token.rb index cc62778..514b97f 100644 --- a/users/app/models/token.rb +++ b/users/app/models/token.rb @@ -6,6 +6,10 @@ class Token < CouchRest::Model::Base validates :user_id, presence: true + def user + User.find(self.user_id) + end + def initialize(*args) super self.id = SecureRandom.urlsafe_base64(32).gsub(/^_*/, '') diff --git a/users/app/models/user.rb b/users/app/models/user.rb index 413b4ac..c1988f3 100644 --- a/users/app/models/user.rb +++ b/users/app/models/user.rb @@ -1,4 +1,5 @@ class User < CouchRest::Model::Base + include LoginFormatValidation use_database :users @@ -6,11 +7,6 @@ class User < CouchRest::Model::Base property :password_verifier, String, :accessible => true property :password_salt, String, :accessible => true - property :email_forward, String, :accessible => true - property :email_aliases, [LocalEmail] - - property :public_key, :accessible => true - property :enabled, TrueClass, :default => true validates :login, :password_salt, :password_verifier, @@ -20,20 +16,6 @@ class User < CouchRest::Model::Base :uniqueness => true, :if => :serverside? - # Have multiple regular expression validations so we can get specific error messages: - validates :login, - :format => { :with => /\A.{2,}\z/, - :message => "Login must have at least two characters"} - validates :login, - :format => { :with => /\A[a-z\d_\.-]+\z/, - :message => "Only lowercase letters, digits, . - and _ allowed."} - validates :login, - :format => { :with => /\A[a-z].*\z/, - :message => "Login must begin with a lowercase letter"} - validates :login, - :format => { :with => /\A.*[a-z\d]\z/, - :message => "Login must end with a letter or digit"} - validate :login_is_unique_alias validates :password_salt, :password_verifier, @@ -43,10 +25,6 @@ class User < CouchRest::Model::Base :confirmation => true, :format => { :with => /.{8}.*/, :message => "needs to be at least 8 characters long" } - validates :email_forward, - :allow_blank => true, - :format => { :with => /\A(([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,}))?\Z/, :message => "needs to be a valid email address"} - timestamps! design do @@ -54,19 +32,6 @@ class User < CouchRest::Model::Base load_views(own_path.join('..', 'designs', 'user')) view :by_login view :by_created_at - view :pgp_key_by_handle, - map: <<-EOJS - function(doc) { - if (doc.type != 'User') { - return; - } - emit(doc.login, doc.public_key); - doc.email_aliases.forEach(function(alias){ - emit(alias.username, doc.public_key); - }); - } - EOJS - end # end of design class << self @@ -105,16 +70,30 @@ class User < CouchRest::Model::Base APP_CONFIG['admins'].include? self.login end - # this currently only adds the first email address submitted. - # All the ui needs for now. - def email_aliases_attributes=(attrs) - email_aliases.build(attrs.values.first) if attrs - end - def most_recent_tickets(count=3) Ticket.for_user(self).limit(count).all #defaults to having most recent updated first end + # DEPRECATED + # + # Please set the key on the identity directly + # WARNING: This will not be serialized with the user record! + # It is only a workaround for the key form. + def public_key=(value) + identity.set_key(:pgp, value) + end + + # DEPRECATED + # + # Please access identity.keys[:pgp] directly + def public_key + identity.keys[:pgp] + end + + def identity + @identity ||= Identity.for(self) + end + protected ## @@ -122,12 +101,10 @@ class User < CouchRest::Model::Base ## def login_is_unique_alias - has_alias = User.find_by_login_or_alias(username) - return if has_alias.nil? - if has_alias != self + alias_identity = Identity.find_by_address(self.email_address) + return if alias_identity.blank? + if alias_identity.user != self errors.add(:login, "has already been taken") - elsif has_alias.login != self.login - errors.add(:login, "may not be the same as one of your aliases") end end diff --git a/users/app/views/email_settings/edit.html.haml b/users/app/views/email_settings/edit.html.haml deleted file mode 100644 index 7757a31..0000000 --- a/users/app/views/email_settings/edit.html.haml +++ /dev/null @@ -1,38 +0,0 @@ -- form_options = {:url => user_email_settings_path(@user), :html => {:class => 'form-horizontal'}, :validate => true} -- alias_error_class = @email_alias.username && !@email_alias.valid? ? 'error' : '' - -- content_for :head do - :css - table.aliases tr:first-child td { - border-top: none; - } - -= simple_form_for @user, form_options.dup do |f| - %legend= t(:email_aliases) - .control-group - %label.control-label= t(:current_aliases) - .controls - %table.table.table-condensed.no-header.slim.aliases - - if @user.email_aliases.any? - - @user.email_aliases.each do |email| - %tr - %td= email - %td= link_to(icon(:remove) + t(:remove), user_email_alias_path(@user, email), :method => :delete) - - else - %tr - %td{:colspan=>2}= t(:none) - .control-group{:class => alias_error_class} - %label.control-label= t(:add_email_alias) - .controls - = f.simple_fields_for :email_aliases, @email_alias do |e| - .input-append - = e.input_field :username - = e.submit t(:add), :class => 'btn' - = e.error :username - -= simple_form_for @user, form_options do |f| - %legend= t(:advanced_options) - = f.input :email_forward - = f.input :public_key, :as => :text, :hint => t(:use_ascii_key), :input_html => {:class => "full-width", :rows => 4} - .form-actions - = f.submit t(:save), :class => 'btn btn-primary' diff --git a/users/app/views/users/_edit.html.haml b/users/app/views/users/_edit.html.haml index 0402f37..5f74d32 100644 --- a/users/app/views/users/_edit.html.haml +++ b/users/app/views/users/_edit.html.haml @@ -23,6 +23,20 @@ = f.submit t(:save), :class => 'btn btn-primary' -# +-# CHANGE PGP KEY +-# +-# this will be replaced by a identities controller/view at some point +-# + +- form_options = {:html => {:class => user_form_class('form-horizontal'), :id => 'update_pgp_key'}, :validate => true} += simple_form_for [:api, @user], form_options do |f| + %legend= t(:advanced_options) + = f.input :public_key, :as => :text, :hint => t(:use_ascii_key), :input_html => {:class => "full-width", :rows => 4} + .control-group + .controls + = f.submit t(:save), :class => 'btn' + +-# -# DESTROY ACCOUNT -# @@ -48,4 +62,4 @@ %p= t(:enable_description) = link_to enable_user_path(@user), :method => :post, :class => "btn btn-warning" do %i.icon-ok.icon-white - = t(:enable)
\ No newline at end of file + = t(:enable) diff --git a/users/app/views/users/_warnings.html.haml b/users/app/views/users/_warnings.html.haml index 7e0b2ce..79ab103 100644 --- a/users/app/views/users/_warnings.html.haml +++ b/users/app/views/users/_warnings.html.haml @@ -1,5 +1,5 @@ %noscript - %div.alert.alert-error=t :js_required + %div.alert.alert-error=t :js_required_html #cookie_warning.alert.alert-error{:style => "display:none"} =t :cookie_disabled_warning :javascript diff --git a/users/config/initializers/add_controller_methods.rb b/users/config/initializers/add_controller_methods.rb index 2579176..f572ecb 100644 --- a/users/config/initializers/add_controller_methods.rb +++ b/users/config/initializers/add_controller_methods.rb @@ -1,3 +1,4 @@ ActiveSupport.on_load(:application_controller) do include ControllerExtension::Authentication + include ControllerExtension::TokenAuthentication end diff --git a/users/config/locales/en.yml b/users/config/locales/en.yml index 1aa7005..55ba3a1 100644 --- a/users/config/locales/en.yml +++ b/users/config/locales/en.yml @@ -12,6 +12,7 @@ en: change_password: "Change Password" login_message: "Please log in with your account." invalid_user_pass: "Not a valid username/password combination" + invalid_ephemeral: "Invalid random key used. This looked like an attempt to hack the site to us. If it wasn't please contact support so we can look into the issue." all_strategies_failed: "Could not understand your login attempt. Please first send your login and a SRP ephemeral value A and then send the client_auth in the same session (using cookies)." update_login_and_password: "Update Login and Password" destroy_my_account: "Destroy my account" @@ -31,7 +32,7 @@ en: not_authorized_login: "Please log in to perform that action." search: "Search" cookie_disabled_warning: "You have cookies disabled. You will not be able to login until you enable cookies." - js_required: "We are sorry, but this doesn't work without javascript enabled. This is for security reasons." + js_required_html: "We are sorry, but this doesn't work without javascript enabled. This is because the authentication system used, <a href='http://srp.stanford.edu/'>SRP</a>, requires javascript." enable_account: "Enable the account %{username}" enable_description: "This will restore the account to full functionality" deactivate_account: "Deactivate the account %{username}" diff --git a/users/leap_web_users.gemspec b/users/leap_web_users.gemspec index d33328a..7d1f220 100644 --- a/users/leap_web_users.gemspec +++ b/users/leap_web_users.gemspec @@ -17,6 +17,6 @@ Gem::Specification.new do |s| s.add_dependency "leap_web_core", LeapWeb::VERSION - s.add_dependency "ruby-srp", "~> 0.2.0" + s.add_dependency "ruby-srp", "~> 0.2.1" s.add_dependency "rails_warden" end diff --git a/users/lib/warden/strategies/secure_remote_password.rb b/users/lib/warden/strategies/secure_remote_password.rb index 2c681be..4688fcd 100644 --- a/users/lib/warden/strategies/secure_remote_password.rb +++ b/users/lib/warden/strategies/secure_remote_password.rb @@ -49,6 +49,8 @@ module Warden else fail! :base => 'invalid_user_pass' end + rescue SRP::InvalidEphemeral + fail!(:base => "invalid_ephemeral") end def json_response(object) diff --git a/users/test/factories.rb b/users/test/factories.rb index 777704b..c87e290 100644 --- a/users/test/factories.rb +++ b/users/test/factories.rb @@ -18,4 +18,7 @@ FactoryGirl.define do end end end + + factory :token + end diff --git a/users/test/functional/helper_methods_test.rb b/users/test/functional/helper_methods_test.rb index 2b2375c..44226ae 100644 --- a/users/test/functional/helper_methods_test.rb +++ b/users/test/functional/helper_methods_test.rb @@ -11,7 +11,7 @@ class HelperMethodsTest < ActionController::TestCase # we test them right in here... include ApplicationController._helpers - # they all reference the controller. + # the helpers all reference the controller. def controller @controller end diff --git a/users/test/functional/test_helpers_test.rb b/users/test/functional/test_helpers_test.rb new file mode 100644 index 0000000..9bd01ad --- /dev/null +++ b/users/test/functional/test_helpers_test.rb @@ -0,0 +1,38 @@ +# +# There are a few test helpers for dealing with login etc. +# We test them here and also document their behaviour. +# + +require 'test_helper' + +class TestHelpersTest < ActionController::TestCase + tests ApplicationController # testing no controller in particular + + def test_login_stubs_warden + login + assert_equal @current_user, request.env['warden'].user + end + + def test_login_token_authenticates + login + assert_equal @current_user, @controller.send(:token_authenticate) + end + + def test_login_stubs_token + login + assert @token + assert_equal @current_user, @token.user + end + + def test_login_adds_token_header + login + token_present = @controller.authenticate_with_http_token do |token, options| + assert_equal @token.id, token + end + # authenticate_with_http_token just returns nil and does not + # execute the block if there is no token. So we have to also + # ensure it was run: + assert token_present + end +end + diff --git a/users/test/functional/users_controller_test.rb b/users/test/functional/users_controller_test.rb index 0ce5cc2..96ae48c 100644 --- a/users/test/functional/users_controller_test.rb +++ b/users/test/functional/users_controller_test.rb @@ -59,19 +59,23 @@ class UsersControllerTest < ActionController::TestCase assert_access_denied end - test "show for non-existing user" do + test "may not show non-existing user without auth" do nonid = 'thisisnotanexistinguserid' - # when unauthenticated: get :show, :id => nonid assert_access_denied(true, false) + end - # when authenticated but not admin: + test "may not show non-existing user without admin" do + nonid = 'thisisnotanexistinguserid' login + get :show, :id => nonid assert_access_denied + end - # when authenticated as admin: + test "redirect admin to user list for non-existing user" do + nonid = 'thisisnotanexistinguserid' login :is_admin? => true get :show, :id => nonid assert_response :redirect diff --git a/users/test/functional/v1/sessions_controller_test.rb b/users/test/functional/v1/sessions_controller_test.rb index 0c4e325..ff9fca1 100644 --- a/users/test/functional/v1/sessions_controller_test.rb +++ b/users/test/functional/v1/sessions_controller_test.rb @@ -7,7 +7,7 @@ class V1::SessionsControllerTest < ActionController::TestCase setup do @request.env['HTTP_HOST'] = 'api.lvh.me' - @user = stub_record :user + @user = stub_record :user, {}, true @client_hex = 'a123' end @@ -48,13 +48,22 @@ class V1::SessionsControllerTest < ActionController::TestCase assert_response :success assert json_response.keys.include?("id") assert json_response.keys.include?("token") + assert token = Token.find(json_response['token']) + assert_equal @user.id, token.user_id end - test "logout should reset warden user" do + test "logout should reset session" do expect_warden_logout delete :destroy - assert_response :redirect - assert_redirected_to root_url + assert_response 204 + end + + test "logout should destroy token" do + login + expect_warden_logout + @token.expects(:destroy) + delete :destroy + assert_response 204 end def expect_warden_logout @@ -65,5 +74,4 @@ class V1::SessionsControllerTest < ActionController::TestCase request.env['warden'].expects(:logout) end - end diff --git a/users/test/functional/v1/users_controller_test.rb b/users/test/functional/v1/users_controller_test.rb index 0d44e50..a330bf3 100644 --- a/users/test/functional/v1/users_controller_test.rb +++ b/users/test/functional/v1/users_controller_test.rb @@ -5,7 +5,9 @@ class V1::UsersControllerTest < ActionController::TestCase test "user can change settings" do user = find_record :user changed_attribs = record_attributes_for :user_with_settings - user.expects(:update_attributes).with(changed_attribs) + account_settings = stub + account_settings.expects(:update).with(changed_attribs) + AccountSettings.expects(:new).with(user).returns(account_settings) login user put :update, :user => changed_attribs, :id => user.id, :format => :json @@ -18,7 +20,9 @@ class V1::UsersControllerTest < ActionController::TestCase test "admin can update user" do user = find_record :user changed_attribs = record_attributes_for :user_with_settings - user.expects(:update_attributes).with(changed_attribs) + account_settings = stub + account_settings.expects(:update).with(changed_attribs) + AccountSettings.expects(:new).with(user).returns(account_settings) login :is_admin? => true put :update, :user => changed_attribs, :id => user.id, :format => :json diff --git a/users/test/integration/api/account_flow_test.rb b/users/test/integration/api/account_flow_test.rb index 4c94389..e41befa 100644 --- a/users/test/integration/api/account_flow_test.rb +++ b/users/test/integration/api/account_flow_test.rb @@ -5,6 +5,7 @@ class AccountFlowTest < RackTest setup do @login = "integration_test_user" + Identity.find_by_address(@login + '@' + APP_CONFIG[:domain]).tap{|i| i.destroy if i} User.find_by_login(@login).tap{|u| u.destroy if u} @password = "srp, verify me!" @srp = SRP::Client.new @login, :password => @password @@ -18,7 +19,10 @@ class AccountFlowTest < RackTest end teardown do - @user.destroy if @user + if @user.reload + @user.identity.destroy + @user.destroy + end Warden.test_reset! end @@ -74,25 +78,45 @@ class AccountFlowTest < RackTest assert_nil server_auth end + test "update password via api" do + @srp.authenticate(self) + @password = "No! Verify me instead." + @srp = SRP::Client.new @login, :password => @password + @user_params = { + # :login => @login, + :password_verifier => @srp.verifier.to_s(16), + :password_salt => @srp.salt.to_s(16) + } + put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', + :user => @user_params, + :format => :json + server_auth = @srp.authenticate(self) + assert last_response.successful? + assert_nil server_auth["errors"] + assert server_auth["M2"] + end + test "update user" 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 - @user.reload - assert_equal test_public_key, @user.public_key - assert_equal new_login, @user.login + 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 - @user.reload - assert_equal test_public_key, @user.public_key + 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 - # TODO: not sure why i need this, but when public key is removed, the DB is updated but @user.reload doesn't seem to actually reload. - @user = User.find(@user.id) # @user.reload - assert_nil @user.public_key + assert_nil Identity.for(@user).keys[:pgp] end end diff --git a/users/test/integration/api/python/flow_with_srp.py b/users/test/integration/api/python/flow_with_srp.py index 7b741d6..9fc168b 100755 --- a/users/test/integration/api/python/flow_with_srp.py +++ b/users/test/integration/api/python/flow_with_srp.py @@ -11,68 +11,86 @@ import binascii safe_unhexlify = lambda x: binascii.unhexlify(x) if (len(x) % 2 == 0) else binascii.unhexlify('0'+x) +# using globals for now +# server = 'https://dev.bitmask.net/1' +server = 'http://api.lvh.me:3000/1' + +def run_tests(): + login = 'test_' + id_generator() + password = id_generator() + id_generator() + usr = srp.User( login, password, srp.SHA256, srp.NG_1024 ) + print_and_parse(signup(login, password)) + + auth = print_and_parse(authenticate(usr)) + verify_or_debug(auth, usr) + assert usr.authenticated() + + usr = change_password(auth['id'], login, auth['token']) + + auth = print_and_parse(authenticate(usr)) + verify_or_debug(auth, usr) + # At this point the authentication process is complete. + assert usr.authenticated() + # let's have some random name def id_generator(size=6, chars=string.ascii_lowercase + string.digits): return ''.join(random.choice(chars) for x in range(size)) -# using globals for a start -server = 'https://api.bitmask.net:4430/1' -login = id_generator() -password = id_generator() + id_generator() - -# print ' username = "' + login + '"' -# print ' password = "' + password + '"' - # log the server communication def print_and_parse(response): - print response.request.method + ': ' + response.url - print " " + json.dumps(response.request.data) + request = response.request + print request.method + ': ' + response.url + if hasattr(request, 'data'): + print " " + json.dumps(response.request.data) print " -> " + response.text - return json.loads(response.text) + try: + return json.loads(response.text) + except ValueError: + return None -def signup(session): +def signup(login, password): salt, vkey = srp.create_salted_verification_key( login, password, srp.SHA256, srp.NG_1024 ) - # print ' salt = "' + binascii.hexlify(salt) + '"' - # print ' v = "' + binascii.hexlify(vkey) + '"' user_params = { 'user[login]': login, 'user[password_verifier]': binascii.hexlify(vkey), 'user[password_salt]': binascii.hexlify(salt) } - return session.post(server + '/users.json', data = user_params, verify = False) + return requests.post(server + '/users.json', data = user_params, verify = False) -usr = srp.User( login, password, srp.SHA256, srp.NG_1024 ) +def change_password(user_id, login, token): + password = id_generator() + id_generator() + salt, vkey = srp.create_salted_verification_key( login, password, srp.SHA256, srp.NG_1024 ) + user_params = { + 'user[password_verifier]': binascii.hexlify(vkey), + 'user[password_salt]': binascii.hexlify(salt) + } + auth_headers = { 'Authorization': 'Token token="' + token + '"'} + print user_params + print_and_parse(requests.put(server + '/users/' + user_id + '.json', data = user_params, verify = False, headers = auth_headers)) + return srp.User( login, password, srp.SHA256, srp.NG_1024 ) -def authenticate(session, login): + +def authenticate(usr): + session = requests.session() uname, A = usr.start_authentication() - # print ' aa = "' + binascii.hexlify(A) + '"' params = { 'login': uname, 'A': binascii.hexlify(A) } init = print_and_parse(session.post(server + '/sessions', data = params, verify=False)) - # print ' b = "' + init['b'] + '"' - # print ' bb = "' + init['B'] + '"' M = usr.process_challenge( safe_unhexlify(init['salt']), safe_unhexlify(init['B']) ) - # print ' m = "' + binascii.hexlify(M) + '"' - return session.put(server + '/sessions/' + login, verify = False, + return session.put(server + '/sessions/' + uname, verify = False, data = {'client_auth': binascii.hexlify(M)}) -session = requests.session() -user = print_and_parse(signup(session)) - -# SRP signup would happen here and calculate M hex -auth = print_and_parse(authenticate(session, user['login'])) -if ( 'errors' in auth ): - print ' u = "%x"' % usr.u - print ' x = "%x"' % usr.x - print ' v = "%x"' % usr.v - print ' S = "%x"' % usr.S - print ' K = "' + binascii.hexlify(usr.K) + '"' - print ' M = "%x"' % usr.M -else: - usr.verify_session( safe_unhexlify(auth["M2"]) ) - -# At this point the authentication process is complete. -assert usr.authenticated() +def verify_or_debug(auth, usr): + if ( 'errors' in auth ): + print ' u = "%x"' % usr.u + print ' x = "%x"' % usr.x + print ' v = "%x"' % usr.v + print ' S = "%x"' % usr.S + print ' K = "' + binascii.hexlify(usr.K) + '"' + print ' M = "' + binascii.hexlify(usr.M) + '"' + else: + usr.verify_session( safe_unhexlify(auth["M2"]) ) +run_tests() diff --git a/users/test/integration/browser/account_test.rb b/users/test/integration/browser/account_test.rb index ce63baf..b412980 100644 --- a/users/test/integration/browser/account_test.rb +++ b/users/test/integration/browser/account_test.rb @@ -20,4 +20,24 @@ class AccountTest < BrowserIntegrationTest assert_equal '/', current_path end + # trying to seed an invalid A for srp login + test "detects attempt to circumvent SRP" do + user = FactoryGirl.create :user + visit '/sessions/new' + fill_in 'Username', with: user.login + fill_in 'Password', with: "password" + inject_malicious_js + click_on 'Log In' + assert page.has_content?("Invalid random key") + assert page.has_no_content?("Welcome") + end + + def inject_malicious_js + page.execute_script <<-EOJS + var calc = new srp.Calculate(); + calc.A = function(_a) {return "00";}; + calc.S = calc.A; + srp.session = new srp.Session(null, calc); + EOJS + end end diff --git a/users/test/support/auth_test_helper.rb b/users/test/support/auth_test_helper.rb index 555b5db..47147fc 100644 --- a/users/test/support/auth_test_helper.rb +++ b/users/test/support/auth_test_helper.rb @@ -13,8 +13,9 @@ module AuthTestHelper if user_or_method_hash.respond_to?(:reverse_merge) user_or_method_hash.reverse_merge! :is_admin? => false end - @current_user = stub_record(:user, user_or_method_hash, true) + @current_user = stub_record(:user, user_or_method_hash) request.env['warden'] = stub :user => @current_user + request.env['HTTP_AUTHORIZATION'] = header_for_token_auth return @current_user end @@ -37,6 +38,12 @@ module AuthTestHelper end end + protected + + def header_for_token_auth + @token = find_record(:token, :user => @current_user) + ActionController::HttpAuthentication::Token.encode_credentials @token.id + end end class ActionController::TestCase diff --git a/users/test/support/stub_record_helper.rb b/users/test/support/stub_record_helper.rb index 8aa1973..5bccb66 100644 --- a/users/test/support/stub_record_helper.rb +++ b/users/test/support/stub_record_helper.rb @@ -7,9 +7,8 @@ module StubRecordHelper # If no record is given but a hash or nil will create a stub based on # that instead and returns the stub. # - def find_record(factory, attribs_hash = {}) - attribs_hash = attribs_hash.reverse_merge(:id => Random.rand(10000).to_s) - record = stub_record factory, attribs_hash + def find_record(factory, record_or_attribs_hash = {}) + record = stub_record factory, record_or_attribs_hash, true klass = record.class finder = klass.respond_to?(:find_by_param) ? :find_by_param : :find klass.stubs(finder).with(record.to_param.to_s).returns(record) diff --git a/users/test/unit/email_aliases_test.rb b/users/test/unit/email_aliases_test.rb deleted file mode 100644 index 86d14aa..0000000 --- a/users/test/unit/email_aliases_test.rb +++ /dev/null @@ -1,66 +0,0 @@ -require 'test_helper' - -class EmailAliasTest < ActiveSupport::TestCase - - setup do - @user = FactoryGirl.build :user - @alias = "valid_alias" - # make sure no existing records are in the way... - User.find_by_login_or_alias(@alias).try(:destroy) - end - - test "no email aliases set in the beginning" do - assert_equal [], @user.email_aliases - end - - test "adding email alias through params" do - @user.attributes = {:email_aliases_attributes => {"0" => {:email => @alias}}} - assert @user.changed? - assert @user.save - assert_equal @alias, @user.email_aliases.first.username - end - - test "adding email alias directly" do - @user.email_aliases.build :email => @alias - assert @user.save - assert_equal @alias, @user.email_aliases.first.username - end - - test "duplicated email aliases are invalid" do - @user.email_aliases.build :email => @alias - @user.save - assert_invalid_alias @alias - end - - test "email alias needs to be different from other peoples login" do - other_user = FactoryGirl.create :user, :login => @alias - assert_invalid_alias @alias - other_user.destroy - end - - test "email needs to be different from other peoples email aliases" do - other_user = FactoryGirl.create :user, :email_aliases_attributes => {'1' => @alias} - assert_invalid_alias @alias - other_user.destroy - end - - test "login is invalid as email alias" do - @user.login = @alias - assert_invalid_alias @alias - end - - test "find user by email alias" do - @user.email_aliases.build :email => @alias - assert @user.save - assert_equal @user, User.find_by_login_or_alias(@alias) - assert_equal @user, User.find_by_alias(@alias) - assert_nil User.find_by_login(@alias) - end - - def assert_invalid_alias(string) - email_alias = @user.email_aliases.build :email => string - assert !email_alias.valid? - assert !@user.valid? - end - -end diff --git a/users/test/unit/email_test.rb b/users/test/unit/email_test.rb new file mode 100644 index 0000000..7cfbc84 --- /dev/null +++ b/users/test/unit/email_test.rb @@ -0,0 +1,19 @@ +require 'test_helper' + +class EmailTest < ActiveSupport::TestCase + + test "valid format" do + email = Email.new(email_string) + assert email.valid? + end + + test "validates format" do + email = Email.new("email") + assert !email.valid? + assert_equal ["needs to be a valid email address"], email.errors[:email] + end + + def email_string + @email_string ||= Faker::Internet.email + end +end diff --git a/users/test/unit/identity_test.rb b/users/test/unit/identity_test.rb new file mode 100644 index 0000000..bf24f02 --- /dev/null +++ b/users/test/unit/identity_test.rb @@ -0,0 +1,86 @@ +require 'test_helper' + +class IdentityTest < ActiveSupport::TestCase + + setup do + @user = FactoryGirl.create(:user) + end + + teardown do + @user.destroy + end + + test "initial identity for a user" do + id = Identity.for(@user) + assert_equal @user.email_address, id.address + assert_equal @user.email_address, id.destination + assert_equal @user, id.user + end + + test "add alias" do + id = Identity.for @user, address: alias_name + assert_equal LocalEmail.new(alias_name), id.address + assert_equal @user.email_address, id.destination + assert_equal @user, id.user + end + + test "add forward" do + id = Identity.for @user, destination: forward_address + assert_equal @user.email_address, id.address + assert_equal Email.new(forward_address), id.destination + assert_equal @user, id.user + end + + test "forward alias" do + id = Identity.for @user, address: alias_name, destination: forward_address + assert_equal LocalEmail.new(alias_name), id.address + assert_equal Email.new(forward_address), id.destination + assert_equal @user, id.user + id.save + end + + test "prevents duplicates" do + id = Identity.create_for @user, address: alias_name, destination: forward_address + dup = Identity.build_for @user, address: alias_name, destination: forward_address + assert !dup.valid? + assert_equal ["This alias already exists"], dup.errors[:base] + end + + test "validates availability" do + other_user = FactoryGirl.create(:user) + id = Identity.create_for @user, address: alias_name, destination: forward_address + taken = Identity.build_for other_user, address: alias_name + assert !taken.valid? + assert_equal ["This email has already been taken"], taken.errors[:base] + other_user.destroy + end + + test "setting and getting pgp key" do + id = Identity.for(@user) + id.set_key(:pgp, pgp_key_string) + assert_equal pgp_key_string, id.keys[:pgp] + end + + test "querying pgp key via couch" do + id = Identity.for(@user) + id.set_key(:pgp, pgp_key_string) + id.save + view = Identity.pgp_key_by_email.key(id.address) + assert_equal 1, view.rows.count + assert result = view.rows.first + assert_equal id.address, result["key"] + assert_equal id.keys[:pgp], result["value"] + end + + def alias_name + @alias_name ||= Faker::Internet.user_name + end + + def forward_address + @forward_address ||= Faker::Internet.email + end + + def pgp_key_string + @pgp_key ||= "DUMMY PGP KEY ... "+SecureRandom.base64(4096) + end +end diff --git a/users/test/unit/local_email_test.rb b/users/test/unit/local_email_test.rb new file mode 100644 index 0000000..b25f46f --- /dev/null +++ b/users/test/unit/local_email_test.rb @@ -0,0 +1,34 @@ +require 'test_helper' + +class LocalEmailTest < ActiveSupport::TestCase + + test "appends domain" do + local = LocalEmail.new(handle) + assert_equal LocalEmail.new(email), local + assert local.valid? + end + + test "returns handle" do + local = LocalEmail.new(email) + assert_equal handle, local.handle + end + + test "prints full email" do + local = LocalEmail.new(handle) + assert_equal email, "#{local}" + end + + test "validates domain" do + local = LocalEmail.new(Faker::Internet.email) + assert !local.valid? + assert_equal ["needs to end in @#{LocalEmail.domain}"], local.errors[:email] + end + + def handle + @handle ||= Faker::Internet.user_name + end + + def email + handle + "@" + APP_CONFIG[:domain] + end +end diff --git a/users/test/unit/user_test.rb b/users/test/unit/user_test.rb index c8c837b..89ee749 100644 --- a/users/test/unit/user_test.rb +++ b/users/test/unit/user_test.rb @@ -56,23 +56,30 @@ class UserTest < ActiveSupport::TestCase other_user.destroy end - test "login needs to be different from other peoples email aliases" do + test "login needs to be unique amongst aliases" do other_user = FactoryGirl.create :user - other_user.email_aliases.build :email => @user.login - other_user.save + Identity.create_for other_user, address: @user.login assert !@user.valid? other_user.destroy end + test "deprecated public key api still works" do + key = SecureRandom.base64(4096) + @user.public_key = key + assert_equal key, @user.public_key + end + test "pgp key view" do - @user.public_key = SecureRandom.base64(4096) - @user.save + key = SecureRandom.base64(4096) + identity = Identity.create_for @user + identity.set_key('pgp', key) + identity.save - view = User.pgp_key_by_handle.key(@user.login) + view = Identity.pgp_key_by_email.key(@user.email_address) assert_equal 1, view.rows.count assert result = view.rows.first - assert_equal @user.login, result["key"] - assert_equal @user.public_key, result["value"] + assert_equal @user.email_address, result["key"] + assert_equal key, result["value"] end end |