diff --git a/app/assets/stylesheets/global.scss b/app/assets/stylesheets/global.scss index 1069c17c..7b4d9668 100644 --- a/app/assets/stylesheets/global.scss +++ b/app/assets/stylesheets/global.scss @@ -75,10 +75,14 @@ body > .container { margin-top: 0; margin-bottom: 0; } + &.is-invalid { + border-color: var(--bs-form-invalid-color); + } } input:checked[type="checkbox"] { --bs-form-check-bg-image: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 20 20'%3e%3cpath fill='none' stroke='%231C1C1C' stroke-linecap='round' stroke-linejoin='round' stroke-width='3' d='m6 10 3 3 6-6'/%3e%3c/svg%3e") !important; + background-color: $white !important; } .form-label { @@ -92,3 +96,7 @@ input:checked[type="checkbox"] { .width-fit-content { width: fit-content !important; } + +.invalid-feedback { + margin-left: 1rem; +} diff --git a/app/controllers/ui/sessions_controller.rb b/app/controllers/ui/sessions_controller.rb index 34729b5a..b8df8b95 100644 --- a/app/controllers/ui/sessions_controller.rb +++ b/app/controllers/ui/sessions_controller.rb @@ -66,7 +66,7 @@ def change_password @user = User.find_by(password_reset_token: @token) if @user authorize @user, :update_password? - @user.update({ password: params.require(:password), password_reset_token: nil }) + @user.update(params.permit(:password, :password_confirmation).merge(password_reset_token: nil)) flash[:success] = I18n.t(:password_reset_success, username: @user.username) redirect_to new_ui_session_path else diff --git a/app/controllers/ui/users_controller.rb b/app/controllers/ui/users_controller.rb index 7c04fbdd..365618dc 100644 --- a/app/controllers/ui/users_controller.rb +++ b/app/controllers/ui/users_controller.rb @@ -11,16 +11,22 @@ def new end def create - @user = User.create(params.permit( + @user = User.new(params.require(:user).permit( :username, :email, :password, :password_confirmation, :ts_and_cs, )) - session[:user_id] = @user.id - flash[:success] = I18n.t(:new_user_success) - redirect_to ui_users_path + if @user.valid? + @user.save + session[:user_id] = @user.id + flash[:success] = I18n.t(:new_user_success) + redirect_to ui_users_path + else + flash[:alert] = I18n.t(:new_user_failure) + render :new, status: :unprocessable_entity + end end end end diff --git a/app/controllers/v0/password_resets_controller.rb b/app/controllers/v0/password_resets_controller.rb index 7bdd09dd..2355f21a 100644 --- a/app/controllers/v0/password_resets_controller.rb +++ b/app/controllers/v0/password_resets_controller.rb @@ -36,7 +36,11 @@ def show def update @user = User.find_by!(password_reset_token: params[:id]) authorize @user, :update_password? - if @user.update({ password: params.require(:password), password_reset_token: nil }) + ActiveSupport::Deprecation.warn( + """Creating and updating user passwords in the API without providing a password confirmation + is deprecated, and will be removed in an upcomming API release""" + ) + if @user.update({ password: params.require(:password), password_confirmation: params.require(:password), password_reset_token: nil }) render 'users/show', status: :ok else raise Smartcitizen::UnprocessableEntity.new @user.errors diff --git a/app/controllers/v0/users_controller.rb b/app/controllers/v0/users_controller.rb index 5f050192..877bc6b9 100644 --- a/app/controllers/v0/users_controller.rb +++ b/app/controllers/v0/users_controller.rb @@ -55,7 +55,7 @@ def destroy private def user_params - params.permit(*[ + user_params = params.permit(*[ :email, :username, :password, @@ -64,6 +64,11 @@ def user_params :url, (:role_mask if current_user&.is_admin?) ].compact) + ActiveSupport::Deprecation.warn( + """Creating and updating user passwords in the API without providing a password confirmation + is deprecated, and will be removed in an upcomming API release""" + ) + user_params.merge(password_confirmation: user_params[:password]) end end diff --git a/app/models/user.rb b/app/models/user.rb index acd17292..3e0e3b29 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -24,11 +24,10 @@ class User < ActiveRecord::Base has_secure_password validations: false - validates :password, presence: { on: :create }, length: { minimum: 5, allow_blank: true } - validates :username, :email, presence: true - validates :username, uniqueness: true, if: :username? - validates :username, length: { in: 3..30 }, allow_nil: true - validates :email, format: { with: /@/ }, uniqueness: true + validates_acceptance_of :ts_and_cs, on: :create + validates :password, presence: { on: :create }, length: { minimum: 5 }, confirmation: true, if: :password + validates :username, format: { with: /\A@?[^@]+\z/ , if: :username }, length: { in: 3..30 }, presence: true, uniqueness: true + validates :email, format: { with: /@/ }, uniqueness: true, presence: true validates :url, format: URI::regexp(%w(http https)), allow_nil: true, allow_blank: true, on: :create has_many :devices, foreign_key: 'owner_id', after_add: :update_cached_device_ids!, after_remove: :update_cached_device_ids! diff --git a/config/locales/views/users/en.yml b/config/locales/views/users/en.yml index 7b66c11e..98a5e7ee 100644 --- a/config/locales/views/users/en.yml +++ b/config/locales/views/users/en.yml @@ -1,6 +1,7 @@ en: new_user_submit: "Sign up" - new_user_ts_and_cs_label_html: "I accept the Terms and Conditions" + new_user_ts_and_cs_label_html: "I accept the terms and conditions" new_user_success: "Thanks for signing up! You are now logged in." + new_user_failure: "Some errors prevented us from creating your account. Please check below and try again!" new_user_login_heading: "Already have an account?" new_user_login_link: "Log in" diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 705fdf84..66a03407 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -4,6 +4,8 @@ sequence(:username) { |n| "user#{n}" } sequence(:email) { |n| "user#{n}@bitsushi.com" } password { "password1" } + password_confirmation { "password1"} + ts_and_cs { true } url { "http://www.yahoo.com" } role_mask { 0 } diff --git a/spec/features/user_management_spec.rb b/spec/features/user_management_spec.rb index 2a06826c..f9c24329 100644 --- a/spec/features/user_management_spec.rb +++ b/spec/features/user_management_spec.rb @@ -2,6 +2,7 @@ feature "User signs up for an account" do scenario "User signs up for an account with correct details" do + user_count_before = User.count visit "/login" click_on "Sign up" expect(page).to have_current_path(new_ui_user_path) @@ -9,8 +10,112 @@ fill_in "Username", with: "myusername" fill_in "Password", with: "password123" fill_in "Password confirmation", with: "password123" - check "I accept the Terms and Conditions" + check "I accept the terms and conditions" click_on "Sign up" expect(page).to have_content("Thanks for signing up! You are now logged in.") + expect(User.count).to eq(user_count_before + 1) + end + + scenario "User uses invalid email" do + user_count_before = User.count + visit "/login" + click_on "Sign up" + expect(page).to have_current_path(new_ui_user_path) + fill_in "Email", with: "user_example.com" + fill_in "Username", with: "myusername" + fill_in "Password", with: "password123" + fill_in "Password confirmation", with: "password123" + check "I accept the terms and conditions" + click_on "Sign up" + expect(page).to have_current_path(ui_users_path) + expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!") + expect(page).to have_content("Email\nis invalid") + expect(User.count).to eq(user_count_before) + end + + scenario "User uses email that has already been taken" do + FactoryBot.create(:user, email: "user@example.com") + user_count_before = User.count + visit "/login" + click_on "Sign up" + expect(page).to have_current_path(new_ui_user_path) + fill_in "Email", with: "user@example.com" + fill_in "Username", with: "myusername" + fill_in "Password", with: "password123" + fill_in "Password confirmation", with: "password123" + check "I accept the terms and conditions" + click_on "Sign up" + expect(page).to have_current_path(ui_users_path) + expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!") + expect(page).to have_content("Email\nhas already been taken") + expect(User.count).to eq(user_count_before) + end + + scenario "User uses invalid username" do + user_count_before = User.count + visit "/login" + click_on "Sign up" + expect(page).to have_current_path(new_ui_user_path) + fill_in "Email", with: "user@example.com" + fill_in "Username", with: "my@usernameisnotanemail.com" + fill_in "Password", with: "password123" + fill_in "Password confirmation", with: "password123" + check "I accept the terms and conditions" + click_on "Sign up" + expect(page).to have_current_path(ui_users_path) + expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!") + expect(page).to have_content("Username\nis invalid") + expect(User.count).to eq(user_count_before) + end + + scenario "User uses username that has already been taken" do + FactoryBot.create(:user, username: "myusername") + user_count_before = User.count + visit "/login" + click_on "Sign up" + expect(page).to have_current_path(new_ui_user_path) + fill_in "Email", with: "user@example.com" + fill_in "Username", with: "myusername" + fill_in "Password", with: "password123" + fill_in "Password confirmation", with: "password123" + check "I accept the terms and conditions" + click_on "Sign up" + expect(page).to have_current_path(ui_users_path) + expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!") + expect(page).to have_content("Username\nhas already been taken") + expect(User.count).to eq(user_count_before) + end + + scenario "User provides passwords that dont match" do + user_count_before = User.count + visit "/login" + click_on "Sign up" + expect(page).to have_current_path(new_ui_user_path) + fill_in "Email", with: "user@example.com" + fill_in "Username", with: "myusername" + fill_in "Password", with: "password123" + fill_in "Password confirmation", with: "password246" + check "I accept the terms and conditions" + click_on "Sign up" + expect(page).to have_current_path(ui_users_path) + expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!") + expect(page).to have_content("Password confirmation\ndoesn't match Password") + expect(User.count).to eq(user_count_before) + end + + scenario "User does not confirm terms and conditions" do + user_count_before = User.count + visit "/login" + click_on "Sign up" + expect(page).to have_current_path(new_ui_user_path) + fill_in "Email", with: "user@example.com" + fill_in "Username", with: "myusername" + fill_in "Password", with: "password123" + fill_in "Password confirmation", with: "password123" + click_on "Sign up" + expect(page).to have_current_path(ui_users_path) + expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!") + expect(page).to have_css("#user_ts_and_cs.is-invalid") + expect(User.count).to eq(user_count_before) end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f2a6b69c..3a3df56b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -11,6 +11,7 @@ it { is_expected.to validate_uniqueness_of(:username) } it { is_expected.to validate_length_of(:username).is_at_least(3).is_at_most(30) } + it { is_expected.to validate_confirmation_of(:password) } it { is_expected.to have_many(:oauth_applications) } it { is_expected.to have_many(:devices) } @@ -29,6 +30,10 @@ expect(build(:user, email: "not an email")).to be_invalid end + it "is invalid with a username which is like an email adddress" do + expect(build(:user, username: "anemail@email.com")).to be_invalid + end + it "is invalid without an email" do expect(build(:user, email: nil)).to be_invalid end diff --git a/spec/requests/v0/me_spec.rb b/spec/requests/v0/me_spec.rb index 7315eed0..1b0ae364 100644 --- a/spec/requests/v0/me_spec.rb +++ b/spec/requests/v0/me_spec.rb @@ -3,7 +3,7 @@ describe V0::MeController, type: :request do let(:application) { create :application } - let(:user) { create :user, username: 'barney', password: '1234567' } + let(:user) { create :user, username: 'barney', password: '1234567', password_confirmation: '1234567' } let(:token) { create :access_token, application: application, resource_owner_id: user.id } before(:each) do diff --git a/spec/requests/v0/rack_attack_spec.rb b/spec/requests/v0/rack_attack_spec.rb index 380095fb..c81ac4f1 100644 --- a/spec/requests/v0/rack_attack_spec.rb +++ b/spec/requests/v0/rack_attack_spec.rb @@ -55,7 +55,7 @@ context "a user with role 'citizen' is logged in" do let(:user) { - FactoryBot.create(:user, username: username, password: password) + FactoryBot.create(:user, username: username, password: password, password_confirmation: password) } context "number of requests is lower than the limit" do @@ -71,7 +71,7 @@ context "a user with role 'researcher' is logged in" do let(:user) { - FactoryBot.create(:researcher, username: username, password: password) + FactoryBot.create(:researcher, username: username, password: password, password_confirmation: password) } context "number of requests is lower than the limit" do @@ -87,7 +87,7 @@ context "a user with role 'admin' is logged in" do let(:user) { - FactoryBot.create(:admin, username: username, password: password) + FactoryBot.create(:admin, username: username, password: password, password_confirmation: password) } context "number of requests is lower than the limit" do diff --git a/spec/requests/v0/sessions_spec.rb b/spec/requests/v0/sessions_spec.rb index e977fdd1..9039e98d 100644 --- a/spec/requests/v0/sessions_spec.rb +++ b/spec/requests/v0/sessions_spec.rb @@ -2,7 +2,7 @@ describe V0::SessionsController do - let!(:user) { create(:user, username: 'milhouse', password: 'greatpass', email: 'mil@house.com') } + let!(:user) { create(:user, username: 'milhouse', password: 'greatpass', password_confirmation: 'greatpass', email: 'mil@house.com') } it "gets access_token with valid parameters" do j = api_post 'sessions', { username: 'milhouse', password: 'greatpass'}