Fix: user with space reset password (#492)

* extract find_user method the user controller with correct id escaping

* add user reset password test

app/controllers/users_controller.rb

@@ -24,16 +24,13 @@ def index
   # GET /users/1.xml
   def show
     @user = if session[:user].admin? && params.has_key?(:id)
-              LinkedData::Client::Models::User.find_by_username(params[:id]).first
+              find_user(params[:id])
             else
-              LinkedData::Client::Models::User.find(session[:user].id)
+              find_user(session[:user].id)
             end
 
     @all_ontologies = LinkedData::Client::Models::Ontology.all(ignore_custom_ontologies: true)
 
-    logger.info 'user show'
-    logger.info @user.bring_remaining
-    logger.info @user
     @user_ontologies = @user.customOntology
 
     ## Copied from home controller , account action
@@ -51,10 +48,9 @@ def new
 
   # GET /users/1;edit
   def edit
-    @user = LinkedData::Client::Models::User.find(helpers.escape(params[:id]), include: 'all')
-    @user ||= LinkedData::Client::Models::User.find_by_username(helpers.escape(params[:id]), include: 'all').first
+    @user = find_user
 
-    if (params[:password].eql?("true"))
+    if params[:password].eql?("true")
       @user.validate_password = true
     end
   end
@@ -89,8 +85,7 @@ def create
   # PUT /users/1
   # PUT /users/1.xml
   def update
-    @user = LinkedData::Client::Models::User.find(helpers.escape(params[:id]), include: 'all')
-    @user = LinkedData::Client::Models::User.find_by_username(helpers.escape(params[:id]), include: 'all').first if @user.nil?
+    @user = find_user
     @errors = validate_update(user_params)
     if @errors.size < 1
 
@@ -127,12 +122,11 @@ def update
   # DELETE /users/1
   def destroy
     response = {errors: nil, success: ''}
-    @user = LinkedData::Client::Models::User.find(params[:id])
-    @user = LinkedData::Client::Models::User.find_by_username(params[:id]).first if @user.nil?
-    if(session[:user].admin?)
+    @user = find_user
+
+    if session[:user].admin?
       @user.delete
       response[:success] << 'User deleted successfully '
-
     else
       response[:errors] << 'Not permitted '
     end
@@ -152,8 +146,7 @@ def destroy
   end
 
   def custom_ontologies
-    @user = LinkedData::Client::Models::User.find(params[:id], include: 'all')
-    @user = LinkedData::Client::Models::User.find_by_username(params[:id], include: 'all').first if @user.nil?
+    @user = find_user
 
     custom_ontologies = params[:ontology] ? params[:ontology][:ontologyId] : []
     custom_ontologies.reject!(&:blank?)
@@ -176,10 +169,11 @@ def custom_ontologies
 
 
   def subscribe
-    @user = LinkedData::Client::Models::User.find_by_username(params[:username]).first
+    @user = find_user
     deliver "subscribe", SubscribeMailer.register_for_announce_list(@user.email,@user.firstName,@user.lastName)
   end
 
+
   def un_subscribe
     @email = params[:email]
     deliver "unsubscribe", SubscribeMailer.unregister_for_announce_list(@email)
@@ -188,6 +182,16 @@ def un_subscribe
 
   private
 
+  def find_user(id = params[:id])
+    id = helpers.unescape(id)
+    @user = LinkedData::Client::Models::User.find(helpers.escape(id), include: 'all')
+    @user ||= LinkedData::Client::Models::User.find_by_username(helpers.escape(id), include: 'all').first
+
+    not_found("User with id #{id} not found") if @user.nil?
+
+    @user
+  end
+
   def deliver(action,job)
     begin
       job.deliver
@@ -251,7 +255,9 @@ def validate(params)
         errors << "Please fill in the proper text from the supplied image"
       end
     end
-    if ((!params[:orcidId].match(/^\d{4}+(-\d{4})+$/)) || (params[:orcidId].length != 19)) && !(params[:orcidId].nil? || params[:orcidId].length < 1)
+
+
+    if ((!params[:orcidId]&.match(/^\d{4}+(-\d{4})+$/)) || (params[:orcidId].length != 19)) && !(params[:orcidId].nil? || params[:orcidId].length < 1)
       errors << "Please enter a valid ORCID."
     end
 

app/views/users/index.html.haml

@@ -26,10 +26,10 @@
           - count = (user.ontologies&.size || 0) +  (user.projects&.size || 0)
           %div.d-flex.align-items-center{style: 'width: 250px'}
             %span.mx-1
-              = link_to 'Detail', user_path(CGI.escape(user.id.split('/').last)), {data: {turbo_frame: '_top'}}
+              = link_to 'Detail', user_path(user.id.split('/').last), {data: {turbo_frame: '_top'}}
             %span.mx-1
               - if count.zero?
-                = button_to "Delete", user_path(CGI.escape(user.id.split('/').last)), method: :delete, class: 'btn btn-link',  form: {data: { turbo: true, turbo_confirm: "Are you sure?", turbo_frame: '_top'}}
+                = button_to "Delete", user_path(user.id.split('/').last), method: :delete, class: 'btn btn-link',  form: {data: { turbo: true, turbo_confirm: "Are you sure?", turbo_frame: '_top'}}
               - else
                 %span{data: { controller: 'tooltip' }, title: "Can't delete this user because still used"}
                   = link_to "Delete", "", class: 'btn btn-link disabled'

test/fixtures/users.yml

@@ -1,26 +1,26 @@
 john:
-  first_name: John
-  last_name: Doe
+  firstName: John
+  lastName: Doe
   username: johndoe
-  orcid_id: '0000-0001-2345-6789'
-  github_id: 'johndoe_github'
+  orcidId: '0000-0001-2345-6789'
+  githubId: 'johndoe_github'
   email: [email protected]
   password: password
 
 jane:
-  first_name: Jane
-  last_name: Smith
+  firstName: Jane
+  lastName: Smith
   username: janesmith
-  orcid_id: '0000-0002-3456-7890'
-  github_id: 'janesmith_github'
+  orcidId: '0000-0002-3456-7890'
+  githubId: 'janesmith_github'
   email: [email protected]
   password: password
 
 bob:
-  first_name: Bob
-  last_name: Jones
+  firstName: Bob
+  lastName: Jones
   username: bobjones
-  orcid_id: '0000-0003-4567-8901'
-  github_id: 'bobjones_github'
+  orcidId: '0000-0003-4567-8901'
+  githubId: 'bobjones_github'
   email: [email protected]
   password: password

test/integration/login_flows_test.rb

@@ -33,11 +33,11 @@ class LoginFlowsTest < ActionDispatch::IntegrationTest
 
     post users_path, params: {
       user: {
-        firstName: new_user.first_name,
-        lastName: new_user.last_name,
+        firstName: new_user.firstName,
+        lastName: new_user.lastName,
         username: new_user.username,
-        orcidId: new_user.orcid_id,
-        githubId: new_user.github_id,
+        orcidId: new_user.orcidId,
+        githubId: new_user.githubId,
         email: new_user.email,
         password: new_user.password,
         password_confirmation: new_user.password

test/system/login_flows_test.rb

@@ -23,11 +23,11 @@ class LoginFlowsTest < ApplicationSystemTestCase
 
     LinkedData::Client::Models::User.find_by_username(new_user.username).first&.delete
 
-    fill_in 'user_firstName', with: new_user.first_name
-    fill_in 'user_lastName', with: new_user.last_name
+    fill_in 'user_firstName', with: new_user.firstName
+    fill_in 'user_lastName', with: new_user.lastName
     fill_in 'user_username', with: new_user.username
-    fill_in 'user_orcidId', with: new_user.orcid_id
-    fill_in 'user_githubId', with: new_user.github_id
+    fill_in 'user_orcidId', with: new_user.orcidId
+    fill_in 'user_githubId', with: new_user.githubId
     fill_in 'user_email', with: new_user.email
     fill_in 'user_password', with: new_user.password
     fill_in 'user_password_confirmation', with: new_user.password
@@ -72,4 +72,24 @@ class LoginFlowsTest < ApplicationSystemTestCase
 
     assert_selector '.notification', text: "Welcome #{@user_bob.username}!", wait: 10
   end
+
+  test "login and reset password" do
+    login_in_as(@user_bob)
+
+    visit root_url + '/account'
+
+    find("a[href=\"#{edit_user_path(@user_bob.username)}\"]").click
+
+    click_on 'Change password'
+
+    fill_in 'user_password', with: "new password"
+    fill_in 'user_password_confirmation', with: "new password"
+
+    click_on 'Save'
+
+    logged_in_user = LinkedData::Client::Models::User.authenticate(@user_bob.username, "new password")
+
+    assert logged_in_user && !logged_in_user.errors
+  end
+
 end