From bf415d7a2ef61e7b482f66dfcf5d125d048dedc5 Mon Sep 17 00:00:00 2001 From: Les De Ridder Date: Sat, 7 Mar 2020 23:32:07 +0100 Subject: [PATCH] Fix sign up and log in --- config/database.cr | 2 - spec/flows/authentication_spec.cr | 31 -------------- spec/flows/reset_password_spec.cr | 18 -------- .../api/{sign_ins => sign_in}/create_spec.cr | 8 ++-- .../api/{sign_ups => sign_up}/create_spec.cr | 8 ++-- spec/setup/clean_database.cr | 4 ++ spec/support/boxes/user_box.cr | 2 + spec/support/flows/authentication_flow.cr | 40 ------------------ spec/support/flows/reset_password_flow.cr | 42 ------------------- .../api/{sign_ins => sign_in}/create.cr | 2 +- .../api/{sign_ups => sign_up}/create.cr | 2 +- src/actions/home/index.cr | 4 +- src/actions/mixins/auth/require_sign_in.cr | 2 +- src/actions/password_reset_requests/create.cr | 2 +- src/actions/{sign_ins => sign_in}/create.cr | 2 +- src/actions/{sign_ins => sign_in}/delete.cr | 4 +- src/actions/{sign_ins => sign_in}/new.cr | 2 +- src/actions/{sign_ups => sign_up}/create.cr | 2 +- src/actions/{sign_ups => sign_up}/new.cr | 2 +- src/models/user.cr | 2 +- src/operations/mixins/user_from_name.cr | 7 ++++ src/operations/sign_in_user.cr | 22 ++++------ src/operations/sign_up_user.cr | 8 +++- src/pages/main_layout.cr | 4 +- src/pages/{sign_ins => sign_in}/new_page.cr | 8 ++-- src/pages/{sign_ups => sign_up}/new_page.cr | 9 ++-- src/serializers/user_serializer.cr | 2 +- tasks/create_required_seeds.cr | 7 +++- 28 files changed, 66 insertions(+), 182 deletions(-) delete mode 100644 spec/flows/authentication_spec.cr delete mode 100644 spec/flows/reset_password_spec.cr rename spec/requests/api/{sign_ins => sign_in}/create_spec.cr (72%) rename spec/requests/api/{sign_ups => sign_up}/create_spec.cr (73%) delete mode 100644 spec/support/flows/authentication_flow.cr delete mode 100644 spec/support/flows/reset_password_flow.cr rename src/actions/api/{sign_ins => sign_in}/create.cr (87%) rename src/actions/api/{sign_ups => sign_up}/create.cr (78%) rename src/actions/{sign_ins => sign_in}/create.cr (92%) rename src/actions/{sign_ins => sign_in}/delete.cr (57%) rename src/actions/{sign_ins => sign_in}/new.cr (76%) rename src/actions/{sign_ups => sign_up}/create.cr (89%) rename src/actions/{sign_ups => sign_up}/new.cr (76%) create mode 100644 src/operations/mixins/user_from_name.cr rename src/pages/{sign_ins => sign_in}/new_page.cr (69%) rename src/pages/{sign_ups => sign_up}/new_page.cr (63%) diff --git a/config/database.cr b/config/database.cr index 2515185..d90e1d3 100644 --- a/config/database.cr +++ b/config/database.cr @@ -7,9 +7,7 @@ AppDatabase.configure do |settings| settings.url = ENV["DATABASE_URL"]? || Avram::PostgresURL.build( database: database_name, hostname: ENV["DB_HOST"]? || "localhost", - # Some common usernames are "postgres", "root", or your system username (run 'whoami') username: ENV["DB_USERNAME"]? || "postgres", - # Some Postgres installations require no password. Use "" if that is the case. password: ENV["DB_PASSWORD"]? || "postgres" ) end diff --git a/spec/flows/authentication_spec.cr b/spec/flows/authentication_spec.cr deleted file mode 100644 index a7da55b..0000000 --- a/spec/flows/authentication_spec.cr +++ /dev/null @@ -1,31 +0,0 @@ -require "../spec_helper" - -describe "Authentication flow" do - it "works" do - flow = AuthenticationFlow.new("test@example.com") - - flow.sign_up "password" - flow.should_be_signed_in - flow.sign_out - flow.sign_in "wrong-password" - flow.should_have_password_error - flow.sign_in "password" - flow.should_be_signed_in - end - - # This is to show you how to sign in as a user during tests. - # Use the `visit` method's `as` option in your tests to sign in as that user. - # - # Feel free to delete this once you have other tests using the 'as' option. - it "allows sign in through backdoor when testing" do - user = UserBox.create - flow = BaseFlow.new - - flow.visit Me::Show, as: user - should_be_signed_in(flow) - end -end - -private def should_be_signed_in(flow) - flow.el("@sign-out-button").should be_on_page -end diff --git a/spec/flows/reset_password_spec.cr b/spec/flows/reset_password_spec.cr deleted file mode 100644 index dd024ca..0000000 --- a/spec/flows/reset_password_spec.cr +++ /dev/null @@ -1,18 +0,0 @@ -require "../spec_helper" - -describe "Reset password flow" do - it "works" do - user = UserBox.create - flow = ResetPasswordFlow.new(user) - - flow.request_password_reset - flow.should_have_sent_reset_email - flow.reset_password "new-password" - flow.should_be_signed_in - flow.sign_out - flow.sign_in "wrong-password" - flow.should_have_password_error - flow.sign_in "new-password" - flow.should_be_signed_in - end -end diff --git a/spec/requests/api/sign_ins/create_spec.cr b/spec/requests/api/sign_in/create_spec.cr similarity index 72% rename from spec/requests/api/sign_ins/create_spec.cr rename to spec/requests/api/sign_in/create_spec.cr index e92bf5a..87d467b 100644 --- a/spec/requests/api/sign_ins/create_spec.cr +++ b/spec/requests/api/sign_in/create_spec.cr @@ -1,11 +1,11 @@ require "../../../spec_helper" -describe Api::SignIns::Create do +describe Api::SignIn::Create do it "returns a token" do UserToken.stub_token("fake-token") do user = UserBox.create - response = AppClient.exec(Api::SignIns::Create, user: valid_params(user)) + response = AppClient.exec(Api::SignIn::Create, user: valid_params(user)) response.should send_json(200, token: "fake-token") end @@ -15,7 +15,7 @@ describe Api::SignIns::Create do user = UserBox.create invalid_params = valid_params(user).merge(password: "incorrect") - response = AppClient.exec(Api::SignIns::Create, user: invalid_params) + response = AppClient.exec(Api::SignIn::Create, user: invalid_params) response.should send_json( 400, @@ -27,7 +27,7 @@ end private def valid_params(user : User) { - email: user.email, + name: user.name, password: "password", } end diff --git a/spec/requests/api/sign_ups/create_spec.cr b/spec/requests/api/sign_up/create_spec.cr similarity index 73% rename from spec/requests/api/sign_ups/create_spec.cr rename to spec/requests/api/sign_up/create_spec.cr index 26025b0..f7813f9 100644 --- a/spec/requests/api/sign_ups/create_spec.cr +++ b/spec/requests/api/sign_up/create_spec.cr @@ -1,12 +1,13 @@ require "../../../spec_helper" -describe Api::SignUps::Create do +describe Api::SignUp::Create do it "creates user on sign up" do UserToken.stub_token("fake-token") do - response = AppClient.exec(Api::SignUps::Create, user: valid_params) + response = AppClient.exec(Api::SignUp::Create, user: valid_params) response.should send_json(200, token: "fake-token") new_user = UserQuery.first + new_user.name.should eq(valid_params[:name]) new_user.email.should eq(valid_params[:email]) end end @@ -14,7 +15,7 @@ describe Api::SignUps::Create do it "returns error for invalid params" do invalid_params = valid_params.merge(password_confirmation: "wrong") - response = AppClient.exec(Api::SignUps::Create, user: invalid_params) + response = AppClient.exec(Api::SignUp::Create, user: invalid_params) UserQuery.new.select_count.should eq(0) response.should send_json( @@ -27,6 +28,7 @@ end private def valid_params { + name: "test", email: "test@email.com", password: "password", password_confirmation: "password", diff --git a/spec/setup/clean_database.cr b/spec/setup/clean_database.cr index a1bc631..1b72fd2 100644 --- a/spec/setup/clean_database.cr +++ b/spec/setup/clean_database.cr @@ -1,3 +1,7 @@ +require "../../tasks/create_required_seeds" + Spec.before_each do AppDatabase.truncate + + Db::CreateRequiredSeeds.new(quiet: true).call end diff --git a/spec/support/boxes/user_box.cr b/spec/support/boxes/user_box.cr index 479a28c..ceaff27 100644 --- a/spec/support/boxes/user_box.cr +++ b/spec/support/boxes/user_box.cr @@ -1,6 +1,8 @@ class UserBox < Avram::Box def initialize + name "#{sequence("test")}" email "#{sequence("test-email")}@example.com" encrypted_password Authentic.generate_encrypted_password("password") + primary_role_id RoleQuery.new.name("user").first.id end end diff --git a/spec/support/flows/authentication_flow.cr b/spec/support/flows/authentication_flow.cr deleted file mode 100644 index fd2e30d..0000000 --- a/spec/support/flows/authentication_flow.cr +++ /dev/null @@ -1,40 +0,0 @@ -class AuthenticationFlow < BaseFlow - private getter email - - def initialize(@email : String) - end - - def sign_up(password) - visit SignUps::New - fill_form SignUpUser, - email: email, - password: password, - password_confirmation: password - click "@sign-up-button" - end - - def sign_out - visit Me::Show - sign_out_button.click - end - - def sign_in(password) - visit SignIns::New - fill_form SignInUser, - email: email, - password: password - click "@sign-in-button" - end - - def should_be_signed_in - sign_out_button.should be_on_page - end - - def should_have_password_error - el("body", text: "Password is wrong").should be_on_page - end - - private def sign_out_button - el("@sign-out-button") - end -end diff --git a/spec/support/flows/reset_password_flow.cr b/spec/support/flows/reset_password_flow.cr deleted file mode 100644 index 6173a7d..0000000 --- a/spec/support/flows/reset_password_flow.cr +++ /dev/null @@ -1,42 +0,0 @@ -class ResetPasswordFlow < BaseFlow - private getter user, authentication_flow - delegate sign_in, sign_out, should_have_password_error, should_be_signed_in, - to: authentication_flow - delegate email, to: user - - def initialize(@user : User) - @authentication_flow = AuthenticationFlow.new(user.email) - end - - def request_password_reset - with_fake_token do - visit PasswordResetRequests::New - fill_form RequestPasswordReset, - email: email - click "@request-password-reset-button" - end - end - - def should_have_sent_reset_email - with_fake_token do - user = UserQuery.new.email(email).first - PasswordResetRequestEmail.new(user).should be_delivered - end - end - - def reset_password(password) - user = UserQuery.new.email(email).first - token = Authentic.generate_password_reset_token(user) - visit PasswordResets::New.with(user.id, token) - fill_form ResetPassword, - password: password, - password_confirmation: password - click "@update-password-button" - end - - private def with_fake_token - PasswordResetRequestEmail.temp_config(stubbed_token: "fake") do - yield - end - end -end diff --git a/src/actions/api/sign_ins/create.cr b/src/actions/api/sign_in/create.cr similarity index 87% rename from src/actions/api/sign_ins/create.cr rename to src/actions/api/sign_in/create.cr index abf2afc..f8a7e01 100644 --- a/src/actions/api/sign_ins/create.cr +++ b/src/actions/api/sign_in/create.cr @@ -1,4 +1,4 @@ -class Api::SignIns::Create < ApiAction +class Api::SignIn::Create < ApiAction include Api::Auth::SkipRequireAuthToken route do diff --git a/src/actions/api/sign_ups/create.cr b/src/actions/api/sign_up/create.cr similarity index 78% rename from src/actions/api/sign_ups/create.cr rename to src/actions/api/sign_up/create.cr index ab6e9c6..4800b6a 100644 --- a/src/actions/api/sign_ups/create.cr +++ b/src/actions/api/sign_up/create.cr @@ -1,4 +1,4 @@ -class Api::SignUps::Create < ApiAction +class Api::SignUp::Create < ApiAction include Api::Auth::SkipRequireAuthToken route do diff --git a/src/actions/home/index.cr b/src/actions/home/index.cr index 9ad99f0..08222a5 100644 --- a/src/actions/home/index.cr +++ b/src/actions/home/index.cr @@ -7,12 +7,12 @@ class Home::Index < BrowserAction else # When you're ready change this line to: # - # redirect SignIns::New + # redirect SignIn::New # # Or maybe show signed out users a marketing page: # # html Marketing::IndexPage - redirect SignIns::New + redirect SignIn::New end end end diff --git a/src/actions/mixins/auth/require_sign_in.cr b/src/actions/mixins/auth/require_sign_in.cr index b424df9..4cbd335 100644 --- a/src/actions/mixins/auth/require_sign_in.cr +++ b/src/actions/mixins/auth/require_sign_in.cr @@ -9,7 +9,7 @@ module Auth::RequireSignIn else Authentic.remember_requested_path(self) flash.info = "Please sign in first" - redirect to: SignIns::New + redirect to: SignIn::New end end diff --git a/src/actions/password_reset_requests/create.cr b/src/actions/password_reset_requests/create.cr index 4d1601d..5c1a155 100644 --- a/src/actions/password_reset_requests/create.cr +++ b/src/actions/password_reset_requests/create.cr @@ -6,7 +6,7 @@ class PasswordResetRequests::Create < BrowserAction if user PasswordResetRequestEmail.new(user).deliver flash.success = "You should receive an email on how to reset your password shortly" - redirect SignIns::New + redirect SignIn::New else html NewPage, operation: operation end diff --git a/src/actions/sign_ins/create.cr b/src/actions/sign_in/create.cr similarity index 92% rename from src/actions/sign_ins/create.cr rename to src/actions/sign_in/create.cr index 5cd67f2..b5ffadd 100644 --- a/src/actions/sign_ins/create.cr +++ b/src/actions/sign_in/create.cr @@ -1,4 +1,4 @@ -class SignIns::Create < BrowserAction +class SignIn::Create < BrowserAction include Auth::RedirectSignedInUsers route do diff --git a/src/actions/sign_ins/delete.cr b/src/actions/sign_in/delete.cr similarity index 57% rename from src/actions/sign_ins/delete.cr rename to src/actions/sign_in/delete.cr index 8d34612..4d9a209 100644 --- a/src/actions/sign_ins/delete.cr +++ b/src/actions/sign_in/delete.cr @@ -1,7 +1,7 @@ -class SignIns::Delete < BrowserAction +class SignIn::Delete < BrowserAction delete "/sign_out" do sign_out flash.info = "You have been signed out" - redirect to: SignIns::New + redirect to: SignIn::New end end diff --git a/src/actions/sign_ins/new.cr b/src/actions/sign_in/new.cr similarity index 76% rename from src/actions/sign_ins/new.cr rename to src/actions/sign_in/new.cr index 3275b40..6d61546 100644 --- a/src/actions/sign_ins/new.cr +++ b/src/actions/sign_in/new.cr @@ -1,4 +1,4 @@ -class SignIns::New < BrowserAction +class SignIn::New < BrowserAction include Auth::RedirectSignedInUsers get "/sign_in" do diff --git a/src/actions/sign_ups/create.cr b/src/actions/sign_up/create.cr similarity index 89% rename from src/actions/sign_ups/create.cr rename to src/actions/sign_up/create.cr index 6f95f14..fb0d506 100644 --- a/src/actions/sign_ups/create.cr +++ b/src/actions/sign_up/create.cr @@ -1,4 +1,4 @@ -class SignUps::Create < BrowserAction +class SignUp::Create < BrowserAction include Auth::RedirectSignedInUsers route do diff --git a/src/actions/sign_ups/new.cr b/src/actions/sign_up/new.cr similarity index 76% rename from src/actions/sign_ups/new.cr rename to src/actions/sign_up/new.cr index 2299df6..2ce18f2 100644 --- a/src/actions/sign_ups/new.cr +++ b/src/actions/sign_up/new.cr @@ -1,4 +1,4 @@ -class SignUps::New < BrowserAction +class SignUp::New < BrowserAction include Auth::RedirectSignedInUsers get "/sign_up" do diff --git a/src/models/user.cr b/src/models/user.cr index 3d0a4a8..0a991fc 100644 --- a/src/models/user.cr +++ b/src/models/user.cr @@ -6,7 +6,7 @@ class User < BaseModel column name : String column encrypted_password : String column email : String - column created_at : Time + column created_at : Time, autogenerated: true belongs_to primary_role : Role has_many posts : Post, foreign_key: :creator_id diff --git a/src/operations/mixins/user_from_name.cr b/src/operations/mixins/user_from_name.cr new file mode 100644 index 0000000..aac690b --- /dev/null +++ b/src/operations/mixins/user_from_name.cr @@ -0,0 +1,7 @@ +module UserFromName + private def user_from_name : User? + name.value.try do |value| + UserQuery.new.name(value).first? + end + end +end diff --git a/src/operations/sign_in_user.cr b/src/operations/sign_in_user.cr index 09e21b5..97f08ee 100644 --- a/src/operations/sign_in_user.cr +++ b/src/operations/sign_in_user.cr @@ -1,14 +1,14 @@ class SignInUser < Avram::Operation param_key :user - # You can modify this in src/operations/mixins/user_from_email.cr - include UserFromEmail - attribute email : String + include UserFromName + + attribute name : String attribute password : String # Run validations and yields the operation and the user if valid def submit - user = user_from_email + user = user_from_name validate_credentials(user) if valid? @@ -18,23 +18,15 @@ class SignInUser < Avram::Operation end end - # `validate_credentials` determines if a user can sign in. - # - # If desired, you can add additional checks in this method, e.g. - # - # if user.locked? - # email.add_error "is locked out" - # end private def validate_credentials(user) + # TODO: If banned, disallow login + if user unless Authentic.correct_password?(user, password.value.to_s) password.add_error "is wrong" end else - # Usually ok to say that an email is not in the system: - # https://kev.inburke.com/kevin/invalid-username-or-password-useless/ - # https://github.com/luckyframework/lucky_cli/issues/192 - email.add_error "is not in our system" + name.add_error "is not in our system" end end end diff --git a/src/operations/sign_up_user.cr b/src/operations/sign_up_user.cr index 8b5c0a8..417a388 100644 --- a/src/operations/sign_up_user.cr +++ b/src/operations/sign_up_user.cr @@ -1,14 +1,18 @@ class SignUpUser < User::SaveOperation param_key :user - # Change password validations in src/operations/mixins/password_validations.cr + include PasswordValidations - permit_columns email + permit_columns name, email attribute password : String attribute password_confirmation : String before_save do + primary_role_id.value = RoleQuery.new.name("user").first.id + + validate_uniqueness_of name validate_uniqueness_of email + Authentic.copy_and_encrypt password, to: encrypted_password end end diff --git a/src/pages/main_layout.cr b/src/pages/main_layout.cr index fedd21f..48c514c 100644 --- a/src/pages/main_layout.cr +++ b/src/pages/main_layout.cr @@ -31,8 +31,8 @@ abstract class MainLayout end private def render_signed_in_user - text @current_user.email + text @current_user.name text " - " - link "Sign out", to: SignIns::Delete, flow_id: "sign-out-button" + link "Sign out", to: SignIn::Delete, flow_id: "sign-out-button" end end diff --git a/src/pages/sign_ins/new_page.cr b/src/pages/sign_in/new_page.cr similarity index 69% rename from src/pages/sign_ins/new_page.cr rename to src/pages/sign_in/new_page.cr index 51bd327..b80c1b4 100644 --- a/src/pages/sign_ins/new_page.cr +++ b/src/pages/sign_in/new_page.cr @@ -1,4 +1,4 @@ -class SignIns::NewPage < AuthLayout +class SignIn::NewPage < AuthLayout needs operation : SignInUser def content @@ -7,17 +7,17 @@ class SignIns::NewPage < AuthLayout end private def render_sign_in_form(op) - form_for SignIns::Create do + form_for SignIn::Create do sign_in_fields(op) submit "Sign In", flow_id: "sign-in-button" end link "Reset password", to: PasswordResetRequests::New text " | " - link "Sign up", to: SignUps::New + link "Sign up", to: SignUp::New end private def sign_in_fields(op) - mount Shared::Field.new(op.email), &.email_input(autofocus: "true") + mount Shared::Field.new(op.name), &.text_input(autofocus: "true") mount Shared::Field.new(op.password), &.password_input end end diff --git a/src/pages/sign_ups/new_page.cr b/src/pages/sign_up/new_page.cr similarity index 63% rename from src/pages/sign_ups/new_page.cr rename to src/pages/sign_up/new_page.cr index 6ba4a1a..3a5087a 100644 --- a/src/pages/sign_ups/new_page.cr +++ b/src/pages/sign_up/new_page.cr @@ -1,4 +1,4 @@ -class SignUps::NewPage < AuthLayout +class SignUp::NewPage < AuthLayout needs operation : SignUpUser def content @@ -7,15 +7,16 @@ class SignUps::NewPage < AuthLayout end private def render_sign_up_form(op) - form_for SignUps::Create do + form_for SignUp::Create do sign_up_fields(op) submit "Sign Up", flow_id: "sign-up-button" end - link "Sign in instead", to: SignIns::New + link "Sign in instead", to: SignIn::New end private def sign_up_fields(op) - mount Shared::Field.new(op.email), &.email_input(autofocus: "true") + mount Shared::Field.new(op.name), &.text_input(autofocus: "true") + mount Shared::Field.new(op.email), &.email_input mount Shared::Field.new(op.password), &.password_input mount Shared::Field.new(op.password_confirmation), &.password_input end diff --git a/src/serializers/user_serializer.cr b/src/serializers/user_serializer.cr index 61d7578..d8ac1db 100644 --- a/src/serializers/user_serializer.cr +++ b/src/serializers/user_serializer.cr @@ -3,6 +3,6 @@ class UserSerializer < Lucky::Serializer end def render - {email: @user.email} + {name: @user.name, email: @user.email} end end diff --git a/tasks/create_required_seeds.cr b/tasks/create_required_seeds.cr index ddf18e2..e1304f6 100644 --- a/tasks/create_required_seeds.cr +++ b/tasks/create_required_seeds.cr @@ -7,6 +7,9 @@ require "../spec/support/boxes/**" # Use `Db::CreateSampleSeeds` if your only want to add sample data helpful for # development. class Db::CreateRequiredSeeds < LuckyCli::Task + def initialize(@quiet : Bool = false) + end + summary "Add database records required for the app to work" def call @@ -20,6 +23,8 @@ class Db::CreateRequiredSeeds < LuckyCli::Task Role::SaveOperation.create!(id: 1000, name: "user", description: "Regular user") end - puts "Done adding required data" + unless @quiet + puts "Done adding required data" + end end end