From 0fe1678cd37c8e917cb28eed9eb28777d3a92283 Mon Sep 17 00:00:00 2001 From: jessib Date: Tue, 1 Oct 2013 13:56:59 -0700 Subject: Allow admins to view past-due subscriptions. --- users/app/views/overviews/show.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'users') diff --git a/users/app/views/overviews/show.html.haml b/users/app/views/overviews/show.html.haml index d3409df..7bea370 100644 --- a/users/app/views/overviews/show.html.haml +++ b/users/app/views/overviews/show.html.haml @@ -19,4 +19,4 @@ %li= icon('user') + link_to(t(:overview_account), edit_user_path(@user)) - # %li= icon('envelope') + link_to(t(:overview_email), {insert path for user identities, presuambly} %li= icon('question-sign') + link_to(t(:overview_tickets), user_tickets_path(@user)) - %li= icon('shopping-cart') + link_to(t(:overview_billing), show_or_new_customer_link(@user)) if APP_CONFIG[:payment].present? + %li= icon('shopping-cart') + link_to(t(:overview_billing), billing_top_link(@user)) if APP_CONFIG[:payment].present? -- cgit v1.2.3 From 155c2d25395a02916543131348a703cd13573d4c Mon Sep 17 00:00:00 2001 From: Azul Date: Thu, 17 Oct 2013 10:13:44 +0200 Subject: use latest version of srp_js to fix #4002 We were not encoding the srp password properly before. So umlauts in the password would cause the login procedure to fail. --- users/app/assets/javascripts/srp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'users') diff --git a/users/app/assets/javascripts/srp b/users/app/assets/javascripts/srp index d22bf3b..8f33d32 160000 --- a/users/app/assets/javascripts/srp +++ b/users/app/assets/javascripts/srp @@ -1 +1 @@ -Subproject commit d22bf3b9fe2fd31192e1e1b358e97e5a0f3f90b3 +Subproject commit 8f33d32d40b1e21ae7fb9a92c78a275422af4217 -- cgit v1.2.3 From 9f4b1bcf315f09fd6d302ad187281ec4ed443f04 Mon Sep 17 00:00:00 2001 From: Azul Date: Thu, 17 Oct 2013 12:05:26 +0200 Subject: blacklist system logins for aliases and logins We blacklist based on three things: * blacklist in APP_CONFIG[:handle_blacklist] * emails in RFC 2142 * usernames in /etc/passwd The latter two can be allowed by explicitly whitelisting them in APP_CONFIG[:handle_whitelist]. We stick to blocking names that have been configured as both blacklisted and whitelisted - better be save than sorry. --- users/app/models/local_email.rb | 33 +++++++++++++++++++++++++++++++++ users/test/unit/local_email_test.rb | 31 +++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) (limited to 'users') diff --git a/users/app/models/local_email.rb b/users/app/models/local_email.rb index 6303bb6..2b4c65e 100644 --- a/users/app/models/local_email.rb +++ b/users/app/models/local_email.rb @@ -1,5 +1,10 @@ class LocalEmail < Email + BLACKLIST_FROM_RFC2142 = [ + 'postmaster', 'hostmaster', 'domainadmin', 'webmaster', 'www', + 'abuse', 'noc', 'security', 'usenet', 'news', 'uucp', + 'ftp', 'sales', 'marketing', 'support', 'info' + ] def self.domain APP_CONFIG[:domain] @@ -11,6 +16,8 @@ class LocalEmail < Email :message => "needs to end in @#{domain}" } + validate :handle_allowed + def initialize(s) super append_domain_if_needed @@ -32,4 +39,30 @@ class LocalEmail < Email end end + def handle_allowed + errors.add(:handle, "is reserved.") if handle_reserved? + end + + def handle_reserved? + # *ARRAY in a case statement tests if ARRAY includes the handle. + case handle + when *APP_CONFIG[:handle_blacklist] + true + when *APP_CONFIG[:handle_whitelist] + false + when *BLACKLIST_FROM_RFC2142 + true + else + handle_in_passwd? + end + end + + def handle_in_passwd? + begin + !!Etc.getpwnam(handle) + rescue ArgumentError + # handle was not found + return false + end + end end diff --git a/users/test/unit/local_email_test.rb b/users/test/unit/local_email_test.rb index b25f46f..20ee7f1 100644 --- a/users/test/unit/local_email_test.rb +++ b/users/test/unit/local_email_test.rb @@ -24,6 +24,37 @@ class LocalEmailTest < ActiveSupport::TestCase assert_equal ["needs to end in @#{LocalEmail.domain}"], local.errors[:email] end + test "blacklists rfc2142" do + black_listed = LocalEmail.new('hostmaster') + assert !black_listed.valid? + end + + test "blacklists etc passwd" do + black_listed = LocalEmail.new('nobody') + assert !black_listed.valid? + end + + test "whitelist overwrites automatic blacklists" do + with_config handle_whitelist: ['nobody', 'hostmaster'] do + white_listed = LocalEmail.new('nobody') + assert white_listed.valid? + white_listed = LocalEmail.new('hostmaster') + assert white_listed.valid? + end + end + + test "blacklists from config" do + black_listed = LocalEmail.new('www-data') + assert !black_listed.valid? + end + + test "blacklist from config overwrites whitelist" do + with_config handle_whitelist: ['www-data'] do + black_listed = LocalEmail.new('www-data') + assert !black_listed.valid? + end + end + def handle @handle ||= Faker::Internet.user_name end -- cgit v1.2.3 From 18c6f005d0eb0099a1d41235818564fbab94fb1b Mon Sep 17 00:00:00 2001 From: Azul Date: Fri, 18 Oct 2013 12:20:23 +0200 Subject: test logging in through the API using python with umlauts --- users/test/integration/api/python/umlauts.py | 79 ++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100755 users/test/integration/api/python/umlauts.py (limited to 'users') diff --git a/users/test/integration/api/python/umlauts.py b/users/test/integration/api/python/umlauts.py new file mode 100755 index 0000000..96fecbf --- /dev/null +++ b/users/test/integration/api/python/umlauts.py @@ -0,0 +1,79 @@ +#!/usr/bin/env python +# coding: utf-8 + +# under development + +import requests +import json +import string +import random +import srp._pysrp as srp +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() + + +# 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)) + +# log the server communication +def print_and_parse(response): + request = response.request + print request.method + ': ' + response.url + if hasattr(request, 'data'): + print " " + json.dumps(response.request.data) + print " -> " + response.text + try: + return json.loads(response.text) + except ValueError: + return None + +def signup(login, password): + salt, vkey = srp.create_salted_verification_key( login, password, srp.SHA256, srp.NG_1024 ) + user_params = { + 'user[login]': login, + 'user[password_verifier]': binascii.hexlify(vkey), + 'user[password_salt]': binascii.hexlify(salt) + } + print json.dumps(user_params) + return requests.post(server + '/users.json', data = user_params, verify = False) + +def authenticate(usr): + session = requests.session() + uname, A = usr.start_authentication() + params = { + 'login': uname, + 'A': binascii.hexlify(A) + } + init = print_and_parse(session.post(server + '/sessions', data = params, verify=False)) + M = usr.process_challenge( safe_unhexlify(init['salt']), safe_unhexlify(init['B']) ) + return session.put(server + '/sessions/' + uname, verify = False, + data = {'client_auth': binascii.hexlify(M)}) + +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() -- cgit v1.2.3 From 936015afe051c82d5677601f7f58944ed42b4623 Mon Sep 17 00:00:00 2001 From: Azul Date: Fri, 18 Oct 2013 13:12:50 +0200 Subject: use https sources in Gemfiles and also in the documentation (#4109) --- users/Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'users') diff --git a/users/Gemfile b/users/Gemfile index e30033a..4101ead 100644 --- a/users/Gemfile +++ b/users/Gemfile @@ -1,4 +1,4 @@ -source "http://rubygems.org" +source "https://rubygems.org" eval(File.read(File.dirname(__FILE__) + '/../common_dependencies.rb')) eval(File.read(File.dirname(__FILE__) + '/../ui_dependencies.rb')) -- cgit v1.2.3 From 315a7c9aa3d6cc6cde51a67b6dcc91aea085f518 Mon Sep 17 00:00:00 2001 From: Azul Date: Mon, 28 Oct 2013 10:48:53 +0100 Subject: reset button loading... state on error (#4231) including test refactored error display some --- users/app/assets/javascripts/users.js | 51 ++++++++++++++++++-------- users/test/integration/browser/account_test.rb | 14 +++++++ 2 files changed, 49 insertions(+), 16 deletions(-) (limited to 'users') diff --git a/users/app/assets/javascripts/users.js b/users/app/assets/javascripts/users.js index aaeba6e..8486756 100644 --- a/users/app/assets/javascripts/users.js +++ b/users/app/assets/javascripts/users.js @@ -46,6 +46,13 @@ $(form).find('input[type="submit"]').button('loading'); }; + resetButtons = function(submitEvent) { + var form = $('form.submitted') + // bootstrap loading state: + $(form).find('input[type="submit"]').button('reset'); + $(form).removeClass('submitted') + }; + // // PUBLIC FUNCTIONS // @@ -70,24 +77,36 @@ // srp.error = function(message) { clear_errors(); - var element, error, field; + var errors = extractErrors(message); + displayErrors(errors); + resetButtons(); + } + + function extractErrors(message) { if ($.isPlainObject(message) && message.errors) { - for (field in message.errors) { - if (field == 'base') { - alert_message(message.errors[field]); - continue; - } - error = message.errors[field]; - element = $('form input[name$="[' + field + ']"]'); - if (!element) { - continue; - } - element.trigger('element:validate:fail.ClientSideValidations', error).data('valid', false); - } - } else if (message.error) { - alert_message(message.error); + return message.errors; } else { - alert_message(JSON.stringify(message)); + return { + base: (message.error || JSON.stringify(message)) + }; + } + } + + function displayErrors(errors) { + for (var field in errors) { + var error = errors[field]; + if (field === 'base') { + alert_message(error); + } else { + displayFieldError(field, error); + } + } + } + + function displayFieldError(field, error) { + var element = $('form input[name$="[' + field + ']"]'); + if (element) { + element.trigger('element:validate:fail.ClientSideValidations', error).data('valid', false); } }; diff --git a/users/test/integration/browser/account_test.rb b/users/test/integration/browser/account_test.rb index 1deda45..243ccfb 100644 --- a/users/test/integration/browser/account_test.rb +++ b/users/test/integration/browser/account_test.rb @@ -27,6 +27,20 @@ class AccountTest < BrowserIntegrationTest User.find_by_login(username).account.destroy end + test "failed login" do + username, password = submit_signup + click_on 'Logout' + click_on 'Log In' + fill_in 'Username', with: username + fill_in 'Password', with: "wrong password" + click_on 'Log In' + assert page.has_selector? 'input.btn-primary.disabled' + assert page.has_content? I18n.t(:invalid_user_pass) + assert page.has_no_content?("Welcome #{username}") + assert page.has_no_selector? 'input.btn-primary.disabled' + User.find_by_login(username).account.destroy + end + test "change password" do username, password = submit_signup click_on "Account Settings" -- cgit v1.2.3 From 1ef3e9df271934b983ff5afe60c2dcf34c090a98 Mon Sep 17 00:00:00 2001 From: Azul Date: Mon, 28 Oct 2013 11:08:31 +0100 Subject: no need to create a user for testing failed login attempt --- users/test/integration/browser/account_test.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'users') diff --git a/users/test/integration/browser/account_test.rb b/users/test/integration/browser/account_test.rb index 243ccfb..8e03856 100644 --- a/users/test/integration/browser/account_test.rb +++ b/users/test/integration/browser/account_test.rb @@ -28,17 +28,14 @@ class AccountTest < BrowserIntegrationTest end test "failed login" do - username, password = submit_signup - click_on 'Logout' + visit '/' click_on 'Log In' - fill_in 'Username', with: username + fill_in 'Username', with: "username" fill_in 'Password', with: "wrong password" click_on 'Log In' assert page.has_selector? 'input.btn-primary.disabled' assert page.has_content? I18n.t(:invalid_user_pass) - assert page.has_no_content?("Welcome #{username}") assert page.has_no_selector? 'input.btn-primary.disabled' - User.find_by_login(username).account.destroy end test "change password" do -- cgit v1.2.3 From 7aaedeaf6fdd2d84ebab7bde2f6a6bdcf8d930b8 Mon Sep 17 00:00:00 2001 From: jessib Date: Mon, 28 Oct 2013 13:10:12 -0700 Subject: Fix button to enable account: https://leap.se/code/issues/4246 --- users/app/views/users/_edit.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'users') diff --git a/users/app/views/users/_edit.html.haml b/users/app/views/users/_edit.html.haml index 9d2473b..b86172e 100644 --- a/users/app/views/users/_edit.html.haml +++ b/users/app/views/users/_edit.html.haml @@ -63,4 +63,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) + = t(:enable) -- cgit v1.2.3 From dd88c7f84cb3c497c6327c364b3c08993c51a08f Mon Sep 17 00:00:00 2001 From: Azul Date: Mon, 28 Oct 2013 12:47:46 +0100 Subject: notify user their account was successfully deleted (refs #4216) Also fixes a cornercase when admins deleted their own account. So far they would be redirected to the users list - which then refused access. Now they'll be redirected to the home landing page as well. --- users/app/controllers/users_controller.rb | 10 +++++++++- users/config/locales/en.yml | 1 + users/test/integration/browser/account_test.rb | 8 ++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) (limited to 'users') diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb index f66277d..de21983 100644 --- a/users/app/controllers/users_controller.rb +++ b/users/app/controllers/users_controller.rb @@ -48,7 +48,15 @@ class UsersController < UsersBaseController def destroy @user.destroy - redirect_to admin? ? users_url : root_url + flash[:notice] = I18n.t(:account_destroyed) + # admins can destroy other users + if @user != current_user + redirect_to users_url + else + # let's remove the invalid session + logout + redirect_to root_url + end end end diff --git a/users/config/locales/en.yml b/users/config/locales/en.yml index b69f7f4..1b5dd5e 100644 --- a/users/config/locales/en.yml +++ b/users/config/locales/en.yml @@ -17,6 +17,7 @@ en: destroy_my_account: "Destroy my account" destroy_account_info: "This will permanently destroy your account and all the data associated with it. Proceed with caution!" admin_destroy_account: "Destroy the account %{username}" + account_destroyed: "The account has been destroyed successfully." set_email_address: "Set email address" forward_email: "Forward Email" email_aliases: "Email Aliases" diff --git a/users/test/integration/browser/account_test.rb b/users/test/integration/browser/account_test.rb index 8e03856..b712c95 100644 --- a/users/test/integration/browser/account_test.rb +++ b/users/test/integration/browser/account_test.rb @@ -38,6 +38,14 @@ class AccountTest < BrowserIntegrationTest assert page.has_no_selector? 'input.btn-primary.disabled' end + test "account destruction" do + username, password = submit_signup + click_on I18n.t('account_settings') + click_on I18n.t('destroy_my_account') + page.save_screenshot('/tmp/destroy.png') + assert page.has_content?(I18n.t('account_destroyed')) + end + test "change password" do username, password = submit_signup click_on "Account Settings" -- cgit v1.2.3 From f6924bfe3b540c384fa53e55db9db3a64a34ced3 Mon Sep 17 00:00:00 2001 From: Azul Date: Wed, 30 Oct 2013 20:13:16 +0100 Subject: test helper to expect_logout. Currently it expects both the session and the token to be cleared. This might change. But we'll always have a definition of what it means to logout we can test this way. --- users/test/functional/sessions_controller_test.rb | 14 +++----------- users/test/functional/users_controller_test.rb | 1 + users/test/functional/v1/sessions_controller_test.rb | 19 ++----------------- users/test/support/auth_test_helper.rb | 14 ++++++++++++++ 4 files changed, 20 insertions(+), 28 deletions(-) (limited to 'users') diff --git a/users/test/functional/sessions_controller_test.rb b/users/test/functional/sessions_controller_test.rb index a630e6e..28143da 100644 --- a/users/test/functional/sessions_controller_test.rb +++ b/users/test/functional/sessions_controller_test.rb @@ -41,20 +41,12 @@ class SessionsControllerTest < ActionController::TestCase assert_json_error :login => I18n.t(:all_strategies_failed) end - test "logout should reset warden user" do - expect_warden_logout + test "destory should logout" do + login + expect_logout delete :destroy assert_response :redirect assert_redirected_to root_url end - def expect_warden_logout - raw = mock('raw session') do - expects(:inspect) - end - request.env['warden'].expects(:raw_session).returns(raw) - request.env['warden'].expects(:logout) - end - - end diff --git a/users/test/functional/users_controller_test.rb b/users/test/functional/users_controller_test.rb index 052de04..75d900f 100644 --- a/users/test/functional/users_controller_test.rb +++ b/users/test/functional/users_controller_test.rb @@ -91,6 +91,7 @@ class UsersControllerTest < ActionController::TestCase user.expects(:destroy) login user + expect_logout delete :destroy, :id => @current_user.id assert_response :redirect diff --git a/users/test/functional/v1/sessions_controller_test.rb b/users/test/functional/v1/sessions_controller_test.rb index ff9fca1..4200e8f 100644 --- a/users/test/functional/v1/sessions_controller_test.rb +++ b/users/test/functional/v1/sessions_controller_test.rb @@ -52,26 +52,11 @@ class V1::SessionsControllerTest < ActionController::TestCase assert_equal @user.id, token.user_id end - test "logout should reset session" do - expect_warden_logout - delete :destroy - assert_response 204 - end - - test "logout should destroy token" do + test "destroy should logout" do login - expect_warden_logout - @token.expects(:destroy) + expect_logout delete :destroy assert_response 204 end - def expect_warden_logout - raw = mock('raw session') do - expects(:inspect) - end - request.env['warden'].expects(:raw_session).returns(raw) - request.env['warden'].expects(:logout) - end - end diff --git a/users/test/support/auth_test_helper.rb b/users/test/support/auth_test_helper.rb index 609f115..50e9453 100644 --- a/users/test/support/auth_test_helper.rb +++ b/users/test/support/auth_test_helper.rb @@ -38,12 +38,26 @@ module AuthTestHelper end end + def expect_logout + expect_warden_logout + @token.expects(:destroy) if @token + end + protected def header_for_token_auth @token = find_record(:token, :authenticate => @current_user) ActionController::HttpAuthentication::Token.encode_credentials @token.id end + + def expect_warden_logout + raw = mock('raw session') do + expects(:inspect) + end + request.env['warden'].expects(:raw_session).returns(raw) + request.env['warden'].expects(:logout) + end + end class ActionController::TestCase -- cgit v1.2.3 From b4ca13257341792f5e6496264c421af1888bcdb8 Mon Sep 17 00:00:00 2001 From: Azul Date: Tue, 5 Nov 2013 11:40:25 +0100 Subject: refactor: Identity.disable_all_for(user) on user destruction This way the identity model defines how identities should be disabled. We currently still destroy them. But it will be easy and nicely isolated to change this next. --- users/app/models/account.rb | 4 +--- users/app/models/identity.rb | 12 ++++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) (limited to 'users') diff --git a/users/app/models/account.rb b/users/app/models/account.rb index 5368a1b..726f642 100644 --- a/users/app/models/account.rb +++ b/users/app/models/account.rb @@ -29,9 +29,7 @@ class Account def destroy return unless @user - Identity.by_user_id.key(@user.id).each do |identity| - identity.destroy - end + Identity.disable_all_for(@user) @user.destroy end diff --git a/users/app/models/identity.rb b/users/app/models/identity.rb index e0a24e9..c24af73 100644 --- a/users/app/models/identity.rb +++ b/users/app/models/identity.rb @@ -50,6 +50,12 @@ class Identity < CouchRest::Model::Base identity end + def self.disable_all_for(user) + Identity.by_user_id.key(user.id).each do |identity| + identity.disable + end + end + def self.attributes_from_user(user) { user_id: user.id, address: user.email_address, @@ -57,6 +63,12 @@ class Identity < CouchRest::Model::Base } end + # + # about to change towards actually disabling the identity instead of + # destroying it. + # + alias_method :disable, :destroy + def keys read_attribute('keys') || HashWithIndifferentAccess.new end -- cgit v1.2.3 From 99ecdbf71632970d4c83f99beea325e5d213e4c6 Mon Sep 17 00:00:00 2001 From: Azul Date: Tue, 5 Nov 2013 12:12:13 +0100 Subject: disabled identities to block handles after a user was deleted --- users/app/models/identity.rb | 17 +++++++++++------ users/test/unit/account_test.rb | 3 ++- users/test/unit/identity_test.rb | 20 ++++++++++++++++++++ 3 files changed, 33 insertions(+), 7 deletions(-) (limited to 'users') diff --git a/users/app/models/identity.rb b/users/app/models/identity.rb index c24af73..40ce4ae 100644 --- a/users/app/models/identity.rb +++ b/users/app/models/identity.rb @@ -53,6 +53,7 @@ class Identity < CouchRest::Model::Base def self.disable_all_for(user) Identity.by_user_id.key(user.id).each do |identity| identity.disable + identity.save end end @@ -63,11 +64,14 @@ class Identity < CouchRest::Model::Base } end - # - # about to change towards actually disabling the identity instead of - # destroying it. - # - alias_method :disable, :destroy + def enabled? + self.destination && self.user_id + end + + def disable + self.destination = nil + self.user_id = nil + end def keys read_attribute('keys') || HashWithIndifferentAccess.new @@ -105,7 +109,8 @@ class Identity < CouchRest::Model::Base end def destination_email - return if destination.valid? #this ensures it is Email + return if destination.nil? # this identity is disabled + return if destination.valid? # this ensures it is Email self.errors.add(:destination, destination.errors.messages[:email].first) #assumes only one error #TODO end diff --git a/users/test/unit/account_test.rb b/users/test/unit/account_test.rb index 94a9980..a8c6efd 100644 --- a/users/test/unit/account_test.rb +++ b/users/test/unit/account_test.rb @@ -13,7 +13,8 @@ class AccountTest < ActiveSupport::TestCase end test "create and remove a user account" do - assert_no_difference "Identity.count" do + # We keep an identity that will block the handle from being reused. + assert_difference "Identity.count" do assert_no_difference "User.count" do user = Account.create(FactoryGirl.attributes_for(:user)) user.account.destroy diff --git a/users/test/unit/identity_test.rb b/users/test/unit/identity_test.rb index 0842a77..8270689 100644 --- a/users/test/unit/identity_test.rb +++ b/users/test/unit/identity_test.rb @@ -90,6 +90,26 @@ class IdentityTest < ActiveSupport::TestCase assert id.errors.messages[:destination].include? "needs to be a valid email address" end + test "disabled identity" do + id = Identity.for(@user) + id.disable + assert_equal @user.email_address, id.address + assert_equal nil, id.destination + assert_equal nil, id.user + assert !id.enabled? + assert id.valid? + end + + test "disabled identity blocks handle" do + id = Identity.for(@user) + id.disable + id.save + other_user = find_record :user + taken = Identity.build_for other_user, address: id.address + assert !taken.valid? + id.destroy + end + def alias_name @alias_name ||= Faker::Internet.user_name end -- cgit v1.2.3 From 4a2490cc5eac1803be80fade65bbe9d32fa0bd9b Mon Sep 17 00:00:00 2001 From: Azul Date: Tue, 5 Nov 2013 17:03:55 +0100 Subject: Identity.destroy_all_disabled will clean up disabled identities This is mostly for cleaning up after tests so far. But we might expand this to destroy all identities disabled before a certain date. --- users/app/models/identity.rb | 17 +++++++++++++++++ users/test/unit/account_test.rb | 4 ++++ users/test/unit/identity_test.rb | 11 ++++++++++- 3 files changed, 31 insertions(+), 1 deletion(-) (limited to 'users') diff --git a/users/app/models/identity.rb b/users/app/models/identity.rb index 40ce4ae..97966d0 100644 --- a/users/app/models/identity.rb +++ b/users/app/models/identity.rb @@ -27,6 +27,17 @@ class Identity < CouchRest::Model::Base emit(doc.address, doc.keys["pgp"]); } EOJS + view :disabled, + map: <<-EOJS + function(doc) { + if (doc.type != 'Identity') { + return; + } + if (typeof doc.user_id === "undefined") { + emit(doc._id, 1); + } + } + EOJS end @@ -57,6 +68,12 @@ class Identity < CouchRest::Model::Base end end + def self.destroy_all_disabled + Identity.disabled.each do |identity| + identity.destroy + end + end + def self.attributes_from_user(user) { user_id: user.id, address: user.email_address, diff --git a/users/test/unit/account_test.rb b/users/test/unit/account_test.rb index a8c6efd..4fb3c3d 100644 --- a/users/test/unit/account_test.rb +++ b/users/test/unit/account_test.rb @@ -2,6 +2,10 @@ require 'test_helper' class AccountTest < ActiveSupport::TestCase + teardown do + Identity.destroy_all_disabled + end + test "create a new account" do user = Account.create(FactoryGirl.attributes_for(:user)) assert user.valid? diff --git a/users/test/unit/identity_test.rb b/users/test/unit/identity_test.rb index 8270689..78ef52c 100644 --- a/users/test/unit/identity_test.rb +++ b/users/test/unit/identity_test.rb @@ -107,7 +107,16 @@ class IdentityTest < ActiveSupport::TestCase other_user = find_record :user taken = Identity.build_for other_user, address: id.address assert !taken.valid? - id.destroy + Identity.destroy_all_disabled + end + + test "destroy all disabled identities" do + id = Identity.for(@user) + id.disable + id.save + assert Identity.count > 0 + Identity.destroy_all_disabled + assert_equal 0, Identity.count end def alias_name -- cgit v1.2.3 From 40f24e2887672957acf7ecedce58e692cc9505ca Mon Sep 17 00:00:00 2001 From: Azul Date: Wed, 6 Nov 2013 10:10:44 +0100 Subject: refactor: extract method on account test also test one can't login anymore after destroying the account. --- users/test/integration/browser/account_test.rb | 35 ++++++++++++++------------ 1 file changed, 19 insertions(+), 16 deletions(-) (limited to 'users') diff --git a/users/test/integration/browser/account_test.rb b/users/test/integration/browser/account_test.rb index b712c95..6e9aab5 100644 --- a/users/test/integration/browser/account_test.rb +++ b/users/test/integration/browser/account_test.rb @@ -19,31 +19,24 @@ class AccountTest < BrowserIntegrationTest test "successful login" do username, password = submit_signup click_on 'Logout' - click_on 'Log In' - fill_in 'Username', with: username - fill_in 'Password', with: password - click_on 'Log In' + attempt_login(username, password) assert page.has_content?("Welcome #{username}") User.find_by_login(username).account.destroy end test "failed login" do visit '/' - click_on 'Log In' - fill_in 'Username', with: "username" - fill_in 'Password', with: "wrong password" - click_on 'Log In' - assert page.has_selector? 'input.btn-primary.disabled' - assert page.has_content? I18n.t(:invalid_user_pass) - assert page.has_no_selector? 'input.btn-primary.disabled' + attempt_login("username", "wrong password") + assert_invalid_login(page) end test "account destruction" do username, password = submit_signup click_on I18n.t('account_settings') click_on I18n.t('destroy_my_account') - page.save_screenshot('/tmp/destroy.png') assert page.has_content?(I18n.t('account_destroyed')) + attempt_login(username, password) + assert_invalid_login(page) end test "change password" do @@ -55,10 +48,7 @@ class AccountTest < BrowserIntegrationTest click_on 'Save' end click_on 'Logout' - click_on 'Log In' - fill_in 'Username', with: username - fill_in 'Password', with: "other password" - click_on 'Log In' + attempt_login(username, "other password") assert page.has_content?("Welcome #{username}") User.find_by_login(username).account.destroy end @@ -100,6 +90,19 @@ class AccountTest < BrowserIntegrationTest assert page.has_content?("server failed") end + def attempt_login(username, password) + click_on 'Log In' + fill_in 'Username', with: username + fill_in 'Password', with: password + click_on 'Log In' + end + + def assert_invalid_login(page) + assert page.has_selector? 'input.btn-primary.disabled' + assert page.has_content? I18n.t(:invalid_user_pass) + assert page.has_no_selector? 'input.btn-primary.disabled' + end + def inject_malicious_js page.execute_script <<-EOJS var calc = new srp.Calculate(); -- cgit v1.2.3 From 44d273fd03645af5e546133adf4e9906800d3d5f Mon Sep 17 00:00:00 2001 From: Azul Date: Wed, 6 Nov 2013 10:20:33 +0100 Subject: integration test for blocking handles after account destroyed has not been run yet. --- users/test/integration/browser/account_test.rb | 12 ++++++++++++ users/test/support/integration_test_helper.rb | 6 +++--- 2 files changed, 15 insertions(+), 3 deletions(-) (limited to 'users') diff --git a/users/test/integration/browser/account_test.rb b/users/test/integration/browser/account_test.rb index 6e9aab5..b349489 100644 --- a/users/test/integration/browser/account_test.rb +++ b/users/test/integration/browser/account_test.rb @@ -6,6 +6,10 @@ class AccountTest < BrowserIntegrationTest Capybara.current_driver = Capybara.javascript_driver end + teardown do + Identity.destroy_all_disabled + end + test "normal account workflow" do username, password = submit_signup assert page.has_content?("Welcome #{username}") @@ -39,6 +43,14 @@ class AccountTest < BrowserIntegrationTest assert_invalid_login(page) end + test "handle blocked after account destruction" do + username, password = submit_signup + click_on I18n.t('account_settings') + click_on I18n.t('destroy_my_account') + submit_signup(username) + assert page.has_content?('has already been taken') + end + test "change password" do username, password = submit_signup click_on "Account Settings" diff --git a/users/test/support/integration_test_helper.rb b/users/test/support/integration_test_helper.rb index cfe72cf..51e47c6 100644 --- a/users/test/support/integration_test_helper.rb +++ b/users/test/support/integration_test_helper.rb @@ -1,7 +1,7 @@ module IntegrationTestHelper - def submit_signup - username = "test_#{SecureRandom.urlsafe_base64}".downcase - password = SecureRandom.base64 + def submit_signup(username = nil, password = nil) + username ||= "test_#{SecureRandom.urlsafe_base64}".downcase + password ||= SecureRandom.base64 visit '/users/new' fill_in 'Username', with: username fill_in 'Password', with: password -- cgit v1.2.3 From 24598b5c5e4df20c423ec74ea8e9df1592483c6b Mon Sep 17 00:00:00 2001 From: Azul Date: Wed, 6 Nov 2013 11:26:46 +0100 Subject: destroy all tickets created by a user when account is destroyed In order to keep the users engine independent of the tickets engine i added a generic load hook to the account model. The tickets engine then monkeypatches the account destruction and destroys all tickets before the user is destroyed. The tickets are destroyed first so that even if things break there should never be tickets with an outdated user id. I would have prefered to use super over using an alias_method_chain but I have not been able to figure out a way to make account a superclass of the account extension and still refer to Account from the users engine. --- users/app/models/account.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'users') diff --git a/users/app/models/account.rb b/users/app/models/account.rb index 726f642..5c943bb 100644 --- a/users/app/models/account.rb +++ b/users/app/models/account.rb @@ -1,5 +1,10 @@ # -# A Composition of a User record and it's identity records. +# The Account model takes care of the livecycle of a user. +# It composes a User record and it's identity records. +# It also allows for other engines to hook into the livecycle by +# monkeypatching the create, update and destroy methods. +# There's an ActiveSupport load_hook at the end of this file to +# make this more easy. # class Account @@ -52,4 +57,7 @@ class Account @new_identity.try(:save) && @old_identity.try(:save) end + # You can hook into the account lifecycle from different engines using + # ActiveSupport.on_load(:account) do ... + ActiveSupport.run_load_hooks(:account, self) end -- cgit v1.2.3 From ded302ebc6a9e145775f7847c5e89f91d683c777 Mon Sep 17 00:00:00 2001 From: Azul Date: Wed, 6 Nov 2013 11:55:43 +0100 Subject: use the account lifecycle from UsersController#destroy --- users/app/controllers/users_controller.rb | 2 +- users/app/controllers/v1/users_controller.rb | 8 +------- users/test/functional/users_controller_test.rb | 8 ++++++++ 3 files changed, 10 insertions(+), 8 deletions(-) (limited to 'users') diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb index de21983..3cbb6dc 100644 --- a/users/app/controllers/users_controller.rb +++ b/users/app/controllers/users_controller.rb @@ -47,7 +47,7 @@ class UsersController < UsersBaseController end def destroy - @user.destroy + @user.account.destroy flash[:notice] = I18n.t(:account_destroyed) # admins can destroy other users if @user != current_user diff --git a/users/app/controllers/v1/users_controller.rb b/users/app/controllers/v1/users_controller.rb index 03a5a62..0903888 100644 --- a/users/app/controllers/v1/users_controller.rb +++ b/users/app/controllers/v1/users_controller.rb @@ -24,15 +24,9 @@ module V1 end def update - account.update params[:user] + @user.account.update params[:user] respond_with @user end - protected - - def account - @user.account - end - end end diff --git a/users/test/functional/users_controller_test.rb b/users/test/functional/users_controller_test.rb index 75d900f..9c5f8d9 100644 --- a/users/test/functional/users_controller_test.rb +++ b/users/test/functional/users_controller_test.rb @@ -77,7 +77,11 @@ class UsersControllerTest < ActionController::TestCase test "admin can destroy user" do user = find_record :user + + # we destroy the user record and the associated data... user.expects(:destroy) + Identity.expects(:disable_all_for).with(user) + Ticket.expects(:destroy_all_from).with(user) login :is_admin? => true delete :destroy, :id => user.id @@ -88,7 +92,11 @@ class UsersControllerTest < ActionController::TestCase test "user can cancel account" do user = find_record :user + + # we destroy the user record and the associated data... user.expects(:destroy) + Identity.expects(:disable_all_for).with(user) + Ticket.expects(:destroy_all_from).with(user) login user expect_logout -- cgit v1.2.3 From a0e72e6ee7786e3b1fd7276f1c64912c606f4559 Mon Sep 17 00:00:00 2001 From: Azul Date: Thu, 7 Nov 2013 12:46:28 +0100 Subject: only check number of disabled identities to make test more robust --- users/test/unit/identity_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'users') diff --git a/users/test/unit/identity_test.rb b/users/test/unit/identity_test.rb index 78ef52c..eca104f 100644 --- a/users/test/unit/identity_test.rb +++ b/users/test/unit/identity_test.rb @@ -116,7 +116,7 @@ class IdentityTest < ActiveSupport::TestCase id.save assert Identity.count > 0 Identity.destroy_all_disabled - assert_equal 0, Identity.count + assert_equal 0, Identity.disabled.count end def alias_name -- cgit v1.2.3 From e2c0962077cf759b23639276cca42606ea2135ec Mon Sep 17 00:00:00 2001 From: Azul Date: Thu, 7 Nov 2013 23:27:27 +0100 Subject: Token.destroy_all_expired to cleanup expired tokens (#4411) --- users/app/models/token.rb | 35 ++++++++++++++++++++++++----------- users/test/unit/token_test.rb | 15 +++++++++++++++ 2 files changed, 39 insertions(+), 11 deletions(-) (limited to 'users') diff --git a/users/app/models/token.rb b/users/app/models/token.rb index dd87344..bf9b0d0 100644 --- a/users/app/models/token.rb +++ b/users/app/models/token.rb @@ -11,6 +11,24 @@ class Token < CouchRest::Model::Base validates :user_id, presence: true + design do + view :by_last_seen_at + end + + def self.expires_after + APP_CONFIG[:auth] && APP_CONFIG[:auth][:token_expires_after] + end + + def self.expired + self.by_last_seen_at.endkey(expires_after.minutes.ago) + end + + def self.destroy_all_expired + self.expired.each do |token| + token.destroy + end + end + def authenticate if expired? destroy @@ -27,21 +45,16 @@ class Token < CouchRest::Model::Base end def expired? - expires_after and - last_seen_at + expires_after.minutes < Time.now - end - - def expires_after - APP_CONFIG[:auth] && APP_CONFIG[:auth][:token_expires_after] + Token.expires_after and + last_seen_at < Token.expires_after.minutes.ago end def initialize(*args) super - self.id = SecureRandom.urlsafe_base64(32).gsub(/^_*/, '') - self.last_seen_at = Time.now - end - - design do + if new_record? + self.id = SecureRandom.urlsafe_base64(32).gsub(/^_*/, '') + self.last_seen_at = Time.now + end end end diff --git a/users/test/unit/token_test.rb b/users/test/unit/token_test.rb index f56c576..445a20c 100644 --- a/users/test/unit/token_test.rb +++ b/users/test/unit/token_test.rb @@ -61,6 +61,21 @@ class ClientCertificateTest < ActiveSupport::TestCase end end + test "Token.destroy_all_expired cleans up expired tokens only" do + expired = Token.new(user_id: @user.id) + expired.last_seen_at = 2.hours.ago + expired.save + fresh = Token.new(user_id: @user.id) + fresh.save + with_config auth: {token_expires_after: 60} do + Token.destroy_all_expired + end + assert_nil Token.find(expired.id) + assert_equal fresh, Token.find(fresh.id) + fresh.destroy + end + + end -- cgit v1.2.3 From a7cd2ef0877e79302f27fb175384a0cf4ded52d9 Mon Sep 17 00:00:00 2001 From: Azul Date: Thu, 7 Nov 2013 23:36:37 +0100 Subject: fix cornercase of non expiring tokens --- users/app/models/token.rb | 3 ++- users/test/factories.rb | 4 +++- users/test/unit/token_test.rb | 18 ++++++++++-------- 3 files changed, 15 insertions(+), 10 deletions(-) (limited to 'users') diff --git a/users/app/models/token.rb b/users/app/models/token.rb index bf9b0d0..001eb40 100644 --- a/users/app/models/token.rb +++ b/users/app/models/token.rb @@ -20,7 +20,8 @@ class Token < CouchRest::Model::Base end def self.expired - self.by_last_seen_at.endkey(expires_after.minutes.ago) + return [] unless expires_after + by_last_seen_at.endkey(expires_after.minutes.ago) end def self.destroy_all_expired diff --git a/users/test/factories.rb b/users/test/factories.rb index c87e290..f5fb77d 100644 --- a/users/test/factories.rb +++ b/users/test/factories.rb @@ -19,6 +19,8 @@ FactoryGirl.define do end end - factory :token + factory :token do + user + end end diff --git a/users/test/unit/token_test.rb b/users/test/unit/token_test.rb index 445a20c..6c9f209 100644 --- a/users/test/unit/token_test.rb +++ b/users/test/unit/token_test.rb @@ -7,9 +7,6 @@ class ClientCertificateTest < ActiveSupport::TestCase @user = find_record :user end - teardown do - end - test "new token for user" do sample = Token.new(:user_id => @user.id) assert sample.valid? @@ -61,12 +58,17 @@ class ClientCertificateTest < ActiveSupport::TestCase end end + test "Token.destroy_all_expired is noop if no expiry is set" do + expired = FactoryGirl.create :token, last_seen_at: 2.hours.ago + with_config auth: {} do + Token.destroy_all_expired + end + assert_equal expired, Token.find(expired.id) + end + test "Token.destroy_all_expired cleans up expired tokens only" do - expired = Token.new(user_id: @user.id) - expired.last_seen_at = 2.hours.ago - expired.save - fresh = Token.new(user_id: @user.id) - fresh.save + expired = FactoryGirl.create :token, last_seen_at: 2.hours.ago + fresh = FactoryGirl.create :token with_config auth: {token_expires_after: 60} do Token.destroy_all_expired end -- cgit v1.2.3 From 8e9b65b01bbd9d44d4077d94f2dc4ac375cf8e85 Mon Sep 17 00:00:00 2001 From: jessib Date: Mon, 18 Nov 2013 15:44:54 -0800 Subject: Start of service level code, which will be tweaked * stores desired & effective service level * whenever desired level is changed, effective level will be updated * allows user to set their desired service level * allow admin to update desired & effective service level --- users/app/controllers/users_controller.rb | 6 ++++++ users/app/models/unauthenticated_user.rb | 2 ++ users/app/models/user.rb | 12 ++++++++++++ users/app/views/users/_edit.html.haml | 18 ++++++++++++++++++ 4 files changed, 38 insertions(+) (limited to 'users') diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb index 3cbb6dc..8b4715c 100644 --- a/users/app/controllers/users_controller.rb +++ b/users/app/controllers/users_controller.rb @@ -34,6 +34,12 @@ class UsersController < UsersBaseController def edit end + ## added so updating service level works, but not sure we will actually want this. also not sure that this is place to prevent user from updating own effective service level, but here as placeholder: + def update + @user.update_attributes(params[:user]) unless (!admin? and params[:user][:effective_service_level]) + respond_with @user + end + def deactivate @user.enabled = false @user.save diff --git a/users/app/models/unauthenticated_user.rb b/users/app/models/unauthenticated_user.rb index 99a6874..0fc17d2 100644 --- a/users/app/models/unauthenticated_user.rb +++ b/users/app/models/unauthenticated_user.rb @@ -1,4 +1,6 @@ # The nil object for the user class class UnauthenticatedUser < Object + # will probably want something here to return service level as APP_CONFIG[:service_levels][0] but not sure how will be accessing. + end diff --git a/users/app/models/user.rb b/users/app/models/user.rb index a14fcb5..35212a1 100644 --- a/users/app/models/user.rb +++ b/users/app/models/user.rb @@ -9,6 +9,12 @@ class User < CouchRest::Model::Base property :enabled, TrueClass, :default => true + # these will be null by default. should we set to APP_CONFIG[:default_service_level] by default, or have code assume that until these get set?: + property :desired_service_level, Integer, :accessible => true + property :effective_service_level, Integer, :accessible => true + + before_save :update_effective_service_level + validates :login, :password_salt, :password_verifier, :presence => true @@ -116,4 +122,10 @@ class User < CouchRest::Model::Base def serverside? true end + + def update_effective_service_level + if self.desired_service_level_changed? + self.effective_service_level = self.desired_service_level + end + end end diff --git a/users/app/views/users/_edit.html.haml b/users/app/views/users/_edit.html.haml index b86172e..d5a0ff1 100644 --- a/users/app/views/users/_edit.html.haml +++ b/users/app/views/users/_edit.html.haml @@ -37,6 +37,24 @@ .controls = f.submit t(:save), :class => 'btn', :data => {"loading-text" => "Saving..."} + +-# TODO: probably won't want here, but here for now. Also, we will need way to ensure payment if they pick a non-free plan. +-# +-# SERVICE LEVEL +-# +- form_options = {:html => {:class => user_form_class('form-horizontal'), :id => 'update_service_level', :data => {token: session[:token]}}, :validate => true} += simple_form_for @user, form_options do |f| + %legend= t(:service_level) + - if @user != current_user + = t(:desired_service_level) + = f.select :desired_service_level, [[APP_CONFIG[:service_levels][1][:description], 1],[APP_CONFIG[:service_levels][2][:description], 2]], :selected => @user.desired_service_level || APP_CONFIG[:default_service_level] + - if @user != current_user + %p + = t(:effective_service_level) + = f.select :effective_service_level, [[APP_CONFIG[:service_levels][1][:description], 1],[APP_CONFIG[:service_levels][2][:description], 2]], :selected => @user.effective_service_level || APP_CONFIG[:default_service_level] + .control-group + .controls + = f.submit t(:save), :class => 'btn', :data => {"loading-text" => "Saving..."} -# -# DESTROY ACCOUNT -# -- cgit v1.2.3 From f1bc68c73e7183a0ad30c6aefc6cc4cbbf1bc1f0 Mon Sep 17 00:00:00 2001 From: jessib 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 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 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 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 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 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 From 35761333404e3cc2c93bca23036d0fd8e47fd10b Mon Sep 17 00:00:00 2001 From: jessib Date: Tue, 3 Dec 2013 12:17:51 -0800 Subject: Add ServiceLevel class to wrap config and give accessors. Has some hacky parts, but seems like okay generic start for now. --- users/app/models/service_level.rb | 31 +++++++++++++++++++++++++++++++ users/app/models/user.rb | 22 +++++++++++++++++----- users/app/views/users/_edit.html.haml | 6 ++++-- 3 files changed, 52 insertions(+), 7 deletions(-) create mode 100644 users/app/models/service_level.rb (limited to 'users') diff --git a/users/app/models/service_level.rb b/users/app/models/service_level.rb new file mode 100644 index 0000000..ac5244f --- /dev/null +++ b/users/app/models/service_level.rb @@ -0,0 +1,31 @@ +class ServiceLevel + + def initialize(attributes = {}) + @level = attributes[:level] || APP_CONFIG[:default_service_level] + end + + def level + @level + end + + def name + APP_CONFIG[:service_levels][@level][:name] + end + + def cert_prefix + APP_CONFIG[:service_levels][@level][:cert_prefix] + end + + def description + APP_CONFIG[:service_levels][@level][:description] + end + + def cost + APP_CONFIG[:service_levels][@level][:cost] + end + + def quota + APP_CONFIG[:service_levels][@level][:quota] + end + +end diff --git a/users/app/models/user.rb b/users/app/models/user.rb index 35212a1..621ff4e 100644 --- a/users/app/models/user.rb +++ b/users/app/models/user.rb @@ -9,9 +9,9 @@ class User < CouchRest::Model::Base property :enabled, TrueClass, :default => true - # these will be null by default. should we set to APP_CONFIG[:default_service_level] by default, or have code assume that until these get set?: - property :desired_service_level, Integer, :accessible => true - property :effective_service_level, Integer, :accessible => true + # these will be null by default but we shouldn't ever pull them directly, but only via the methods that will return the full ServiceLevel + property :desired_service_level_code, Integer, :accessible => true + property :effective_service_level_code, Integer, :accessible => true before_save :update_effective_service_level @@ -100,6 +100,16 @@ class User < CouchRest::Model::Base @identity = Identity.for(self) end + def desired_service_level + code = self.desired_service_level_code || APP_CONFIG[:default_service_level] + ServiceLevel.new({level: code}) + end + + def effective_service_level + code = self.effective_service_level_code || self.desired_service_level.level + ServiceLevel.new({level: code}) + end + protected ## @@ -124,8 +134,10 @@ class User < CouchRest::Model::Base end def update_effective_service_level - if self.desired_service_level_changed? - self.effective_service_level = self.desired_service_level + # TODO: Is this always the case? Might there be a situation where the admin has set the effective service level and we don't want it changed to match the desired one? + if self.desired_service_level_code_changed? + self.effective_service_level_code = self.desired_service_level_code end end + end diff --git a/users/app/views/users/_edit.html.haml b/users/app/views/users/_edit.html.haml index d5a0ff1..d2c2d95 100644 --- a/users/app/views/users/_edit.html.haml +++ b/users/app/views/users/_edit.html.haml @@ -47,11 +47,13 @@ %legend= t(:service_level) - if @user != current_user = t(:desired_service_level) - = f.select :desired_service_level, [[APP_CONFIG[:service_levels][1][:description], 1],[APP_CONFIG[:service_levels][2][:description], 2]], :selected => @user.desired_service_level || APP_CONFIG[:default_service_level] + - sl1 = ServiceLevel.new({level: 1}) #ugly but okay for now + - sl2 = ServiceLevel.new({level: 2}) + = f.select :desired_service_level_code, [[sl1.description, sl1.level],[sl2.description, sl2.level]], :selected => @user.desired_service_level.level - if @user != current_user %p = t(:effective_service_level) - = f.select :effective_service_level, [[APP_CONFIG[:service_levels][1][:description], 1],[APP_CONFIG[:service_levels][2][:description], 2]], :selected => @user.effective_service_level || APP_CONFIG[:default_service_level] + = f.select :effective_service_level_code, [[sl1.description, sl1.level],[sl2.description, sl2.level]], :selected => @user.effective_service_level.level .control-group .controls = f.submit t(:save), :class => 'btn', :data => {"loading-text" => "Saving..."} -- cgit v1.2.3 From 7d4a9658c29cad526cfe5c952f71109e8eb304e7 Mon Sep 17 00:00:00 2001 From: jessib Date: Tue, 3 Dec 2013 15:07:11 -0800 Subject: Some simplification of code. --- users/app/models/service_level.rb | 26 +++++++------------------- users/app/models/user.rb | 6 +++--- users/app/views/users/_edit.html.haml | 6 ++---- 3 files changed, 12 insertions(+), 26 deletions(-) (limited to 'users') diff --git a/users/app/models/service_level.rb b/users/app/models/service_level.rb index ac5244f..299aaf1 100644 --- a/users/app/models/service_level.rb +++ b/users/app/models/service_level.rb @@ -1,31 +1,19 @@ class ServiceLevel def initialize(attributes = {}) - @level = attributes[:level] || APP_CONFIG[:default_service_level] + @id = attributes[:id] || APP_CONFIG[:default_service_level] end - def level - @level + def self.authenticated_select_options + APP_CONFIG[:service_levels].map { |id,config_hash| [config_hash[:description], id] if config_hash[:name] != 'anonymous'}.compact end - def name - APP_CONFIG[:service_levels][@level][:name] + def id + @id end - def cert_prefix - APP_CONFIG[:service_levels][@level][:cert_prefix] - end - - def description - APP_CONFIG[:service_levels][@level][:description] - end - - def cost - APP_CONFIG[:service_levels][@level][:cost] - end - - def quota - APP_CONFIG[:service_levels][@level][:quota] + def config_hash + APP_CONFIG[:service_levels][@id] end end diff --git a/users/app/models/user.rb b/users/app/models/user.rb index 621ff4e..720f5a9 100644 --- a/users/app/models/user.rb +++ b/users/app/models/user.rb @@ -102,12 +102,12 @@ class User < CouchRest::Model::Base def desired_service_level code = self.desired_service_level_code || APP_CONFIG[:default_service_level] - ServiceLevel.new({level: code}) + ServiceLevel.new({id: code}) end def effective_service_level - code = self.effective_service_level_code || self.desired_service_level.level - ServiceLevel.new({level: code}) + code = self.effective_service_level_code || self.desired_service_level.id + ServiceLevel.new({id: code}) end protected diff --git a/users/app/views/users/_edit.html.haml b/users/app/views/users/_edit.html.haml index d2c2d95..897c54b 100644 --- a/users/app/views/users/_edit.html.haml +++ b/users/app/views/users/_edit.html.haml @@ -47,13 +47,11 @@ %legend= t(:service_level) - if @user != current_user = t(:desired_service_level) - - sl1 = ServiceLevel.new({level: 1}) #ugly but okay for now - - sl2 = ServiceLevel.new({level: 2}) - = f.select :desired_service_level_code, [[sl1.description, sl1.level],[sl2.description, sl2.level]], :selected => @user.desired_service_level.level + = f.select :desired_service_level_code, ServiceLevel.authenticated_select_options, :selected => @user.desired_service_level.id - if @user != current_user %p = t(:effective_service_level) - = f.select :effective_service_level_code, [[sl1.description, sl1.level],[sl2.description, sl2.level]], :selected => @user.effective_service_level.level + = f.select :effective_service_level_code, ServiceLevel.authenticated_select_options, :selected => @user.effective_service_level.id .control-group .controls = f.submit t(:save), :class => 'btn', :data => {"loading-text" => "Saving..."} -- cgit v1.2.3 From 6ebf095bc553345a3e0b8c48cadc3e1440c59ca5 Mon Sep 17 00:00:00 2001 From: jessib Date: Thu, 5 Dec 2013 11:11:39 -0800 Subject: We won't want service levels in production mode, and have it so this initial service level code won't break anything if it isn't set in the config. --- users/app/views/users/_edit.html.haml | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) (limited to 'users') diff --git a/users/app/views/users/_edit.html.haml b/users/app/views/users/_edit.html.haml index 897c54b..0b36d6e 100644 --- a/users/app/views/users/_edit.html.haml +++ b/users/app/views/users/_edit.html.haml @@ -42,19 +42,20 @@ -# -# SERVICE LEVEL -# -- form_options = {:html => {:class => user_form_class('form-horizontal'), :id => 'update_service_level', :data => {token: session[:token]}}, :validate => true} -= simple_form_for @user, form_options do |f| - %legend= t(:service_level) - - if @user != current_user - = t(:desired_service_level) - = f.select :desired_service_level_code, ServiceLevel.authenticated_select_options, :selected => @user.desired_service_level.id - - if @user != current_user - %p - = t(:effective_service_level) - = f.select :effective_service_level_code, ServiceLevel.authenticated_select_options, :selected => @user.effective_service_level.id - .control-group - .controls - = f.submit t(:save), :class => 'btn', :data => {"loading-text" => "Saving..."} +- if APP_CONFIG[:service_levels] + - form_options = {:html => {:class => user_form_class('form-horizontal'), :id => 'update_service_level', :data => {token: session[:token]}}, :validate => true} + = simple_form_for @user, form_options do |f| + %legend= t(:service_level) + - if @user != current_user + = t(:desired_service_level) + = f.select :desired_service_level_code, ServiceLevel.authenticated_select_options, :selected => @user.desired_service_level.id + - if @user != current_user + %p + = t(:effective_service_level) + = f.select :effective_service_level_code, ServiceLevel.authenticated_select_options, :selected => @user.effective_service_level.id + .control-group + .controls + = f.submit t(:save), :class => 'btn', :data => {"loading-text" => "Saving..."} -# -# DESTROY ACCOUNT -# -- cgit v1.2.3 From e64e746759bb241536612c949361442f269ef2f0 Mon Sep 17 00:00:00 2001 From: Azul 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 242f55a55cc51ebc21dd027966cbdf598fcd071d Mon Sep 17 00:00:00 2001 From: Azul 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 7fa78df26fccea56ef2911c93a26f41f8eae3fdf Mon Sep 17 00:00:00 2001 From: Azul 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 From a013b03b0b715ec1209d2812da52ff5f0831c833 Mon Sep 17 00:00:00 2001 From: elijah Date: Fri, 6 Dec 2013 15:45:02 -0800 Subject: make sure key responses are plain text --- users/app/controllers/keys_controller.rb | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) (limited to 'users') diff --git a/users/app/controllers/keys_controller.rb b/users/app/controllers/keys_controller.rb index 949f2c0..fb28901 100644 --- a/users/app/controllers/keys_controller.rb +++ b/users/app/controllers/keys_controller.rb @@ -1,12 +1,18 @@ class KeysController < ApplicationController + # + # Render the user's key as plain text, without a layout. + # + # 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 + # 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 (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')) - + if user + render text: user.public_key, content_type: 'text/text' + else + raise ActionController::RoutingError.new('Not Found') + end end end -- cgit v1.2.3 From ce1d6ddacca09062ed90a40de559454c7ce912b5 Mon Sep 17 00:00:00 2001 From: jessib Date: Mon, 9 Dec 2013 11:50:47 -0800 Subject: Update tests to reflect using plaintext key. --- users/test/functional/keys_controller_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'users') diff --git a/users/test/functional/keys_controller_test.rb b/users/test/functional/keys_controller_test.rb index b69cbc0..863be93 100644 --- a/users/test/functional/keys_controller_test.rb +++ b/users/test/functional/keys_controller_test.rb @@ -8,7 +8,7 @@ class KeysControllerTest < ActionController::TestCase 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 "text/text", response.content_type assert_equal public_key, response.body end @@ -18,7 +18,7 @@ class KeysControllerTest < ActionController::TestCase 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 "text/text", response.content_type assert_equal '', response.body.strip end -- cgit v1.2.3 From b7db3e8f5d9a65ff3b83316bdd802dd40111c64d Mon Sep 17 00:00:00 2001 From: Azul Date: Fri, 13 Dec 2013 13:32:20 +0100 Subject: refactor: move edit sections into partials --- users/app/views/users/_change_password.html.haml | 21 ++++++ users/app/views/users/_change_pgp_key.html.haml | 13 ++++ .../views/users/_change_service_level.html.haml | 18 +++++ users/app/views/users/_destroy_account.html.haml | 27 +++++++ users/app/views/users/_edit.html.haml | 84 ++-------------------- 5 files changed, 83 insertions(+), 80 deletions(-) create mode 100644 users/app/views/users/_change_password.html.haml create mode 100644 users/app/views/users/_change_pgp_key.html.haml create mode 100644 users/app/views/users/_change_service_level.html.haml create mode 100644 users/app/views/users/_destroy_account.html.haml (limited to 'users') diff --git a/users/app/views/users/_change_password.html.haml b/users/app/views/users/_change_password.html.haml new file mode 100644 index 0000000..425e3ee --- /dev/null +++ b/users/app/views/users/_change_password.html.haml @@ -0,0 +1,21 @@ +-# +-# CHANGE PASSWORD +-# +-# * everything about this form is handled with javascript. So take care when changing any ids. +-# * the login is required when changing the password because it is used as part of the salt when calculating the password verifier. +-# however, we don't want the user to change their login without generating a new key, so we hide the ui for this +-# (although it works perfectly fine to change username if the field was visible). +-# + +- form_options = {:url => '/not-used', :html => {:class => user_form_class('form-horizontal'), :id => 'update_login_and_password', :data => {token: session[:token]}}, :validate => true} += simple_form_for @user, form_options do |f| + %legend= t(:change_password) + = hidden_field_tag 'user_param', @user.to_param + .hidden + = f.input :login, :label => t(:username), :required => false, :input_html => {:id => :srp_username} + = f.input :password, :required => false, :validate => true, :input_html => { :id => :srp_password } + = f.input :password_confirmation, :required => false, :input_html => { :id => :srp_password_confirmation } + .control-group + .controls + = f.submit t(:save), :class => 'btn btn-primary' + diff --git a/users/app/views/users/_change_pgp_key.html.haml b/users/app/views/users/_change_pgp_key.html.haml new file mode 100644 index 0000000..e465125 --- /dev/null +++ b/users/app/views/users/_change_pgp_key.html.haml @@ -0,0 +1,13 @@ +-# +-# 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', :data => {token: session[:token]}}, :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', :data => {"loading-text" => "Saving..."} diff --git a/users/app/views/users/_change_service_level.html.haml b/users/app/views/users/_change_service_level.html.haml new file mode 100644 index 0000000..61e67d9 --- /dev/null +++ b/users/app/views/users/_change_service_level.html.haml @@ -0,0 +1,18 @@ +-# TODO: probably won't want here, but here for now. Also, we will need way to ensure payment if they pick a non-free plan. +-# +-# SERVICE LEVEL +-# +- if APP_CONFIG[:service_levels] + - form_options = {:html => {:class => user_form_class('form-horizontal'), :id => 'update_service_level', :data => {token: session[:token]}}, :validate => true} + = simple_form_for @user, form_options do |f| + %legend= t(:service_level) + - if @user != current_user + = t(:desired_service_level) + = f.select :desired_service_level_code, ServiceLevel.authenticated_select_options, :selected => @user.desired_service_level.id + - if @user != current_user + %p + = t(:effective_service_level) + = f.select :effective_service_level_code, ServiceLevel.authenticated_select_options, :selected => @user.effective_service_level.id + .control-group + .controls + = f.submit t(:save), :class => 'btn', :data => {"loading-text" => "Saving..."} diff --git a/users/app/views/users/_destroy_account.html.haml b/users/app/views/users/_destroy_account.html.haml new file mode 100644 index 0000000..445f3c4 --- /dev/null +++ b/users/app/views/users/_destroy_account.html.haml @@ -0,0 +1,27 @@ +-# +-# DESTROY ACCOUNT +-# + +%legend + - if @user == current_user + = t(:destroy_my_account) + - else + = t(:admin_destroy_account, :username => @user.login) +%p= t(:destroy_account_info) += link_to user_path(@user), :method => :delete, :confirm => t(:are_you_sure), :class => "btn btn-danger" do + %i.icon-remove.icon-white + = t(:destroy_my_account) +- if @user != current_user and @user.enabled? + %legend + = t(:deactivate_account, :username => @user.login) + %p= t(:deactivate_description) + = link_to deactivate_user_path(@user), :method => :post, :class => "btn btn-warning" do + %i.icon-pause.icon-white + = t(:deactivate) +- elsif @user != current_user and !@user.enabled? + %legend + = t(:enable_account, :username => @user.login) + %p= t(:enable_description) + = link_to enable_user_path(@user), :method => :post, :class => "btn btn-warning" do + %i.icon-ok.icon-white + = t(:enable) diff --git a/users/app/views/users/_edit.html.haml b/users/app/views/users/_edit.html.haml index 0b36d6e..b52da3d 100644 --- a/users/app/views/users/_edit.html.haml +++ b/users/app/views/users/_edit.html.haml @@ -2,84 +2,8 @@ -# edit user form, used by both show and edit actions. -# --# --# CHANGE PASSWORD --# --# * everything about this form is handled with javascript. So take care when changing any ids. --# * the login is required when changing the password because it is used as part of the salt when calculating the password verifier. --# however, we don't want the user to change their login without generating a new key, so we hide the ui for this --# (although it works perfectly fine to change username if the field was visible). --# - -- form_options = {:url => '/not-used', :html => {:class => user_form_class('form-horizontal'), :id => 'update_login_and_password', :data => {token: session[:token]}}, :validate => true} -= simple_form_for @user, form_options do |f| - %legend= t(:change_password) - = hidden_field_tag 'user_param', @user.to_param - .hidden - = f.input :login, :label => t(:username), :required => false, :input_html => {:id => :srp_username} - = f.input :password, :required => false, :validate => true, :input_html => { :id => :srp_password } - = f.input :password_confirmation, :required => false, :input_html => { :id => :srp_password_confirmation } - .control-group - .controls - = 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', :data => {token: session[:token]}}, :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', :data => {"loading-text" => "Saving..."} - - --# TODO: probably won't want here, but here for now. Also, we will need way to ensure payment if they pick a non-free plan. --# --# SERVICE LEVEL --# -- if APP_CONFIG[:service_levels] - - form_options = {:html => {:class => user_form_class('form-horizontal'), :id => 'update_service_level', :data => {token: session[:token]}}, :validate => true} - = simple_form_for @user, form_options do |f| - %legend= t(:service_level) - - if @user != current_user - = t(:desired_service_level) - = f.select :desired_service_level_code, ServiceLevel.authenticated_select_options, :selected => @user.desired_service_level.id - - if @user != current_user - %p - = t(:effective_service_level) - = f.select :effective_service_level_code, ServiceLevel.authenticated_select_options, :selected => @user.effective_service_level.id - .control-group - .controls - = f.submit t(:save), :class => 'btn', :data => {"loading-text" => "Saving..."} --# --# DESTROY ACCOUNT --# += render 'change_password' += render 'change_pgp_key' += render 'change_service_level' += render 'destroy_account' -%legend - - if @user == current_user - = t(:destroy_my_account) - - else - = t(:admin_destroy_account, :username => @user.login) -%p= t(:destroy_account_info) -= link_to user_path(@user), :method => :delete, :confirm => t(:are_you_sure), :class => "btn btn-danger" do - %i.icon-remove.icon-white - = t(:destroy_my_account) -- if @user != current_user and @user.enabled? - %legend - = t(:deactivate_account, :username => @user.login) - %p= t(:deactivate_description) - = link_to deactivate_user_path(@user), :method => :post, :class => "btn btn-warning" do - %i.icon-pause.icon-white - = t(:deactivate) -- elsif @user != current_user and !@user.enabled? - %legend - = t(:enable_account, :username => @user.login) - %p= t(:enable_description) - = link_to enable_user_path(@user), :method => :post, :class => "btn btn-warning" do - %i.icon-ok.icon-white - = t(:enable) -- cgit v1.2.3 From 5ed00a63ea3da98e8dfaef88752f8afe851c81e9 Mon Sep 17 00:00:00 2001 From: Azul Date: Fri, 13 Dec 2013 14:10:41 +0100 Subject: make the possible actions for users and admins configurable with tests --- users/app/views/users/_edit.html.haml | 17 ++++--- users/test/integration/browser/account_test.rb | 66 +++++++++++++++++--------- 2 files changed, 55 insertions(+), 28 deletions(-) (limited to 'users') diff --git a/users/app/views/users/_edit.html.haml b/users/app/views/users/_edit.html.haml index b52da3d..1d2b68a 100644 --- a/users/app/views/users/_edit.html.haml +++ b/users/app/views/users/_edit.html.haml @@ -1,9 +1,14 @@ -# -# edit user form, used by both show and edit actions. -# - -= render 'change_password' -= render 'change_pgp_key' -= render 'change_service_level' -= render 'destroy_account' - +-# We render a bunch of forms here. Which we use depends upon config settings +-# user_actions and admin_actions. They both include an array of actions +-# allowed to users and admins. +-# Possible forms are: +-# 'change_password' +-# 'change_pgp_key' +-# 'change_service_level' +-# 'destroy_account' +- actions = APP_CONFIG[admin? ? :admin_actions : :user_actions] || [] +- actions.each do |action| + = render action diff --git a/users/test/integration/browser/account_test.rb b/users/test/integration/browser/account_test.rb index 3d281ae..4cefe35 100644 --- a/users/test/integration/browser/account_test.rb +++ b/users/test/integration/browser/account_test.rb @@ -51,35 +51,57 @@ class AccountTest < BrowserIntegrationTest assert page.has_content?('has already been taken') end - test "change password" do + test "default user actions" do username, password = submit_signup click_on "Account Settings" - within('#update_login_and_password') do - fill_in 'Password', with: "other password" - fill_in 'Password confirmation', with: "other password" - click_on 'Save' + assert page.has_content? I18n.t('destroy_my_account') + assert page.has_no_css? '#update_login_and_password' + assert page.has_no_css? '#update_pgp_key' + end + + test "default admin actions" do + username, password = submit_signup + with_config admins: [username] do + click_on "Account Settings" + assert page.has_content? I18n.t('destroy_my_account') + assert page.has_no_css? '#update_login_and_password' + assert page.has_css? '#update_pgp_key' + end + end + + test "change password" do + with_config user_actions: ['change_password'] do + username, password = submit_signup + click_on "Account Settings" + within('#update_login_and_password') do + fill_in 'Password', with: "other password" + fill_in 'Password confirmation', with: "other password" + click_on 'Save' + end + click_on 'Logout' + attempt_login(username, "other password") + assert page.has_content?("Welcome #{username}") + User.find_by_login(username).account.destroy end - click_on 'Logout' - attempt_login(username, "other password") - assert page.has_content?("Welcome #{username}") - User.find_by_login(username).account.destroy end test "change pgp key" do - pgp_key = FactoryGirl.build :pgp_key - username, password = submit_signup - click_on "Account Settings" - within('#update_pgp_key') do - fill_in 'Public key', with: pgp_key - click_on 'Save' + with_config user_actions: ['change_pgp_key'] do + pgp_key = FactoryGirl.build :pgp_key + username, password = submit_signup + click_on "Account Settings" + within('#update_pgp_key') do + fill_in 'Public key', with: pgp_key + click_on 'Save' + end + 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.to_s + user = User.find_by_login(username) + assert_equal pgp_key, user.public_key + user.account.destroy end - 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.to_s - user = User.find_by_login(username) - assert_equal pgp_key, user.public_key - user.account.destroy end -- cgit v1.2.3 From 496817bd512fe43c4cb80cc49a19dae3ed3eb165 Mon Sep 17 00:00:00 2001 From: Azul Date: Fri, 13 Dec 2013 14:27:15 +0100 Subject: refactor: remove Overview controller - we can use Users#show we were only using Users#show to redirect to the edit action. So I replaced that with the overview and we have no more use for the extra controller. This also simplifies linking to the users in question a lot. --- users/app/controllers/overviews_controller.rb | 9 --------- users/app/controllers/sessions_controller.rb | 4 ++-- users/app/controllers/users_controller.rb | 2 +- users/app/views/overviews/show.html.haml | 22 ---------------------- users/app/views/users/_user.html.haml | 2 +- users/app/views/users/show.html.haml | 23 ++++++++++++++++++++++- users/config/routes.rb | 3 +-- 7 files changed, 27 insertions(+), 38 deletions(-) delete mode 100644 users/app/controllers/overviews_controller.rb delete mode 100644 users/app/views/overviews/show.html.haml (limited to 'users') diff --git a/users/app/controllers/overviews_controller.rb b/users/app/controllers/overviews_controller.rb deleted file mode 100644 index 52ce267..0000000 --- a/users/app/controllers/overviews_controller.rb +++ /dev/null @@ -1,9 +0,0 @@ -class OverviewsController < UsersBaseController - - before_filter :authorize - before_filter :fetch_user - - def show - end - -end diff --git a/users/app/controllers/sessions_controller.rb b/users/app/controllers/sessions_controller.rb index 0494b51..85a022e 100644 --- a/users/app/controllers/sessions_controller.rb +++ b/users/app/controllers/sessions_controller.rb @@ -14,12 +14,12 @@ class SessionsController < ApplicationController end # - # this is a bad hack, but user_overview_url(user) is not available + # this is a bad hack, but user_url(user) is not available # also, this doesn't work because the redirect happens as a PUT. no idea why. # #Warden::Manager.after_authentication do |user, auth, opts| # response = Rack::Response.new - # response.redirect "/users/#{user.id}/overview" + # response.redirect "/users/#{user.id}" # throw :warden, response.finish #end diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb index 8b4715c..0b32ec7 100644 --- a/users/app/controllers/users_controller.rb +++ b/users/app/controllers/users_controller.rb @@ -13,7 +13,7 @@ class UsersController < UsersBaseController def index if params[:query] if @user = User.find_by_login(params[:query]) - redirect_to user_overview_url(@user) + redirect_to @user return else @users = User.by_login.startkey(params[:query]).endkey(params[:query].succ) diff --git a/users/app/views/overviews/show.html.haml b/users/app/views/overviews/show.html.haml deleted file mode 100644 index 7bea370..0000000 --- a/users/app/views/overviews/show.html.haml +++ /dev/null @@ -1,22 +0,0 @@ -.overview - - %h2.first= t(:overview_welcome, :username => @user.login) - - - if admin? - %p - = t(:created) - = @user.created_at - %br - = t(:updated) - = @user.updated_at - %br - = t(:enabled) - = @user.enabled? - - %p= t(:overview_intro) - - %ul.unstyled - %li= icon('user') + link_to(t(:overview_account), edit_user_path(@user)) - - # %li= icon('envelope') + link_to(t(:overview_email), {insert path for user identities, presuambly} - %li= icon('question-sign') + link_to(t(:overview_tickets), user_tickets_path(@user)) - %li= icon('shopping-cart') + link_to(t(:overview_billing), billing_top_link(@user)) if APP_CONFIG[:payment].present? diff --git a/users/app/views/users/_user.html.haml b/users/app/views/users/_user.html.haml index 990d9cf..583d22f 100644 --- a/users/app/views/users/_user.html.haml +++ b/users/app/views/users/_user.html.haml @@ -1,4 +1,4 @@ %tr - %td= link_to user.login, user_overview_path(user) + %td= link_to user.login, user %td= l(user.created_at, :format => :short) %td= l(user.updated_at, :format => :short) diff --git a/users/app/views/users/show.html.haml b/users/app/views/users/show.html.haml index 434c025..7bea370 100644 --- a/users/app/views/users/show.html.haml +++ b/users/app/views/users/show.html.haml @@ -1 +1,22 @@ -= render 'edit' +.overview + + %h2.first= t(:overview_welcome, :username => @user.login) + + - if admin? + %p + = t(:created) + = @user.created_at + %br + = t(:updated) + = @user.updated_at + %br + = t(:enabled) + = @user.enabled? + + %p= t(:overview_intro) + + %ul.unstyled + %li= icon('user') + link_to(t(:overview_account), edit_user_path(@user)) + - # %li= icon('envelope') + link_to(t(:overview_email), {insert path for user identities, presuambly} + %li= icon('question-sign') + link_to(t(:overview_tickets), user_tickets_path(@user)) + %li= icon('shopping-cart') + link_to(t(:overview_billing), billing_top_link(@user)) if APP_CONFIG[:payment].present? diff --git a/users/config/routes.rb b/users/config/routes.rb index 69f9cf7..de2ff37 100644 --- a/users/config/routes.rb +++ b/users/config/routes.rb @@ -13,9 +13,8 @@ Rails.application.routes.draw do get "signup" => "users#new", :as => "signup" resources :users, :except => [:create, :update] do - resource :overview, :only => [:show] # resource :email_settings, :only => [:edit, :update] - resources :email_aliases, :only => [:destroy], :id => /.*/ + # resources :email_aliases, :only => [:destroy], :id => /.*/ post 'deactivate', on: :member post 'enable', on: :member end -- cgit v1.2.3 From 6012b588911dac3d47bbdb53e11dcb70ba8be6df Mon Sep 17 00:00:00 2001 From: jessib Date: Mon, 16 Dec 2013 11:37:05 -0800 Subject: Fix issue 4756: /login should not crash if one goes there when logged in. --- users/app/controllers/sessions_controller.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'users') diff --git a/users/app/controllers/sessions_controller.rb b/users/app/controllers/sessions_controller.rb index 85a022e..ca228c2 100644 --- a/users/app/controllers/sessions_controller.rb +++ b/users/app/controllers/sessions_controller.rb @@ -1,6 +1,7 @@ class SessionsController < ApplicationController def new + redirect_to root_path if logged_in? @session = Session.new if authentication_errors @errors = authentication_errors -- cgit v1.2.3 From 91aea91d7091b740630551ec17d0274236545f4c Mon Sep 17 00:00:00 2001 From: jessib Date: Mon, 16 Dec 2013 11:49:16 -0800 Subject: Add test. --- users/test/functional/sessions_controller_test.rb | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'users') diff --git a/users/test/functional/sessions_controller_test.rb b/users/test/functional/sessions_controller_test.rb index 28143da..8b49005 100644 --- a/users/test/functional/sessions_controller_test.rb +++ b/users/test/functional/sessions_controller_test.rb @@ -17,6 +17,13 @@ class SessionsControllerTest < ActionController::TestCase assert_template "sessions/new" end + test "redirect to root_url if logged in" do + login + get :new + assert_response :redirect + assert_redirected_to root_url + end + test "renders json" do get :new, :format => :json assert_response :success -- cgit v1.2.3