diff --git a/app/controllers/api/v1/search_controller.rb b/app/controllers/api/v1/search_controller.rb index d30fcf185..0f324099b 100644 --- a/app/controllers/api/v1/search_controller.rb +++ b/app/controllers/api/v1/search_controller.rb @@ -3,12 +3,20 @@ module Api module V1 class SearchController < Api::V1::BaseController + USER_FILTER_CONTACTS = "contacts" + USER_FILTER_RECEIVING_CONTACTS = "contacts:receiving" + USER_FILTER_SHARING_CONTACTS = "contacts:sharing" + USER_FILTER_ASPECTS_PREFIX = "aspect:" + USER_FILTERS_EXACT_MATCH = [USER_FILTER_CONTACTS, USER_FILTER_RECEIVING_CONTACTS, + USER_FILTER_SHARING_CONTACTS].freeze + USER_FILTERS_PREFIX_MATCH = [USER_FILTER_ASPECTS_PREFIX].freeze + before_action do require_access_token %w[public:read] end - rescue_from ActionController::ParameterMissing, RuntimeError do - render_error 422, "Search request could not be processed" + rescue_from ActionController::ParameterMissing, RuntimeError do |e| + render_error 422, e.message end def user_index @@ -36,20 +44,46 @@ module Api end def people_query - parameters = params.permit(:tag, :name_or_handle) - raise RuntimeError if parameters.keys.length != 1 + tag = params[:tag] + name_or_handle = params[:name_or_handle] + raise "Parameters tag and name_or_handle are exclusive" if tag.present? && name_or_handle.present? - if params.has_key?(:tag) - Person.profile_tagged_with(params[:tag]) - else - connected_only = !private_read? - Person.search( - params[:name_or_handle], - current_user, - only_contacts: connected_only, - mutual: connected_only - ) + query = if tag.present? + # scope filters to only searchable people already + Person.profile_tagged_with(tag) + elsif name_or_handle.present? + Person.searchable(contacts_read? && current_user) # rubocop:disable Rails/DynamicFindBy + .find_by_substring(name_or_handle) + else + raise "Missing parameter tag or name_or_handle" + end + + query = query.where(closed_account: false) + + user_filters.each do |filter| + query = query.contacts_of(current_user) if filter == USER_FILTER_CONTACTS + + if filter == USER_FILTER_RECEIVING_CONTACTS + query = query.contacts_of(current_user).where(contacts: {receiving: true}) + end + + if filter == USER_FILTER_SHARING_CONTACTS + query = query.contacts_of(current_user).where(contacts: {sharing: true}) + end + + if filter.start_with?(USER_FILTER_ASPECTS_PREFIX) # rubocop:disable Style/Next + _, ids = filter.split(":", 2) + ids = ids.split(",").map {|id| + Integer(id) rescue raise("Invalid aspect filter") # rubocop:disable Style/RescueModifier + } + + raise "Invalid aspect filter" unless current_user.aspects.where(id: ids).count == ids.size + + query = Person.where(id: query.all_from_aspects(ids, current_user).select(:id)) + end end + + query.distinct end def posts_query @@ -63,6 +97,22 @@ module Api def tags_query ActsAsTaggableOn::Tag.autocomplete(params.require(:query)) end + + def user_filters + @user_filters ||= Array(params[:filter]).uniq.tap do |filters| + raise "Invalid filter" unless filters.all? {|filter| + USER_FILTERS_EXACT_MATCH.include?(filter) || + USER_FILTERS_PREFIX_MATCH.any? {|prefix| filter.start_with?(prefix) } + } + + # For now all filters require contacts:read + require_access_token %w[contacts:read] unless filters.empty? + end + end + + def contacts_read? + access_token? %w[contacts:read] + end end end end diff --git a/app/models/person.rb b/app/models/person.rb index 83dfe2024..5042a45e5 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -66,8 +66,15 @@ class Person < ApplicationRecord validates :diaspora_handle, :uniqueness => true scope :searchable, -> (user) { - joins(:profile).where("profiles.searchable = true OR contacts.user_id = ?", user.id) + if user + joins("LEFT OUTER JOIN contacts ON contacts.user_id = #{user.id} AND contacts.person_id = people.id") + .joins(:profile) + .where("profiles.searchable = true OR contacts.user_id = ?", user.id) + else + joins(:profile).where(profiles: {searchable: true}) + end } + scope :remote, -> { where('people.owner_id IS NULL') } scope :local, -> { where('people.owner_id IS NOT NULL') } scope :for_json, -> { select("people.id, people.guid, people.diaspora_handle").includes(:profile) } @@ -89,6 +96,15 @@ class Person < ApplicationRecord .where(aspect_memberships: {aspect_id: aspect_ids}).distinct } + scope :in_all_aspects, ->(aspect_ids) { + joins(contacts: :aspect_memberships) + .where(aspect_memberships: {aspect_id: aspect_ids}) + } + + scope :contacts_of, ->(user) { + joins(:contacts).where(contacts: {user_id: user.id}) + } + scope :profile_tagged_with, ->(tag_name) { joins(:profile => :tags) .where(:tags => {:name => tag_name}) @@ -233,11 +249,9 @@ class Person < ApplicationRecord return query if query.is_a?(ActiveRecord::NullRelation) query = if only_contacts - query.joins(:contacts).where(contacts: {user_id: user.id}) + query.contacts_of(user) else - query.joins( - "LEFT OUTER JOIN contacts ON contacts.user_id = #{user.id} AND contacts.person_id = people.id" - ).searchable(user) + query.searchable(user) end query = query.where(contacts: {sharing: true, receiving: true}) if mutual diff --git a/spec/integration/api/search_controller_spec.rb b/spec/integration/api/search_controller_spec.rb index 46524a32e..8ea002ad7 100644 --- a/spec/integration/api/search_controller_spec.rb +++ b/spec/integration/api/search_controller_spec.rb @@ -6,7 +6,7 @@ describe Api::V1::SearchController do let(:auth) { FactoryGirl.create( :auth_with_default_scopes, - scopes: %w[openid public:read public:modify private:read private:modify] + scopes: %w[openid public:read public:modify private:read contacts:read private:modify] ) } @@ -55,13 +55,19 @@ describe Api::V1::SearchController do end it "succeeds by tag" do + tag = SecureRandom.hex(5) + 5.times do + FactoryGirl.create(:person, profile: FactoryGirl.build(:profile, tag_string: "##{tag}")) + end + FactoryGirl.create(:person, closed_account: true, profile: FactoryGirl.build(:profile, tag_string: "##{tag}")) + get( "/api/v1/search/users", - params: {tag: "one", access_token: access_token} + params: {tag: tag, access_token: access_token} ) expect(response.status).to eq(200) users = response_body_data(response) - expect(users.length).to eq(15) + expect(users.length).to eq(5) expect(users.to_json).to match_json_schema(:api_v1_schema, fragment: "#/definitions/users") end @@ -90,6 +96,242 @@ describe Api::V1::SearchController do expect(users.to_json).to match_json_schema(:api_v1_schema, fragment: "#/definitions/users") end + context "with a contacts filter" do + let(:name) { "A contact" } + + it "only returns contacts" do + add_contact(true, false) + add_contact(false, true) + add_contact(true, true) + + get( + "/api/v1/search/users", + params: { + name_or_handle: name, + filter: "contacts", + access_token: access_token + } + ) + + expect(response.status).to eq(200) + users = response_body_data(response) + expect(users.length).to eq(3) + end + + it "only returns receiving contacts" do + add_contact(true, false) + add_contact(true, false) + add_contact(false, true) + + get( + "/api/v1/search/users", + params: { + name_or_handle: name, + filter: "contacts:receiving", + access_token: access_token + } + ) + + expect(response.status).to eq(200) + users = response_body_data(response) + expect(users.length).to eq(2) + end + + it "only returns sharing contacts" do + add_contact(true, false) + add_contact(false, true) + add_contact(false, true) + + get( + "/api/v1/search/users", + params: { + name_or_handle: name, + filter: "contacts:sharing", + access_token: access_token + } + ) + + expect(response.status).to eq(200) + users = response_body_data(response) + expect(users.length).to eq(2) + end + + it "only returns mutually sharing contacts" do + add_contact(true, false) + add_contact(false, true) + add_contact(true, true) + + get( + "/api/v1/search/users", + params: { + name_or_handle: name, + filter: ["contacts:receiving", "contacts:sharing"], + access_token: access_token + } + ) + + expect(response.status).to eq(200) + users = response_body_data(response) + expect(users.length).to eq(1) + end + + it "fails with an invalid filter" do + get( + "/api/v1/search/users", + params: { + name_or_handle: name, + filter: "contacts:thingsiwant", + access_token: access_token + } + ) + + confirm_api_error(response, 422, "Invalid filter") + end + + it "fails without contacts:read scope" do + get( + "/api/v1/search/users", + params: { + name_or_handle: name, + filter: "contacts", + access_token: access_token_read_only + } + ) + + confirm_api_error(response, 403, "insufficient_scope") + end + + def add_contact(receiving, sharing) + other = FactoryGirl.create(:user) + other.profile.update(first_name: name) + + if receiving + aspect = auth.user.aspects.find_or_create_by(name: "Test") + auth.user.share_with(other.person, aspect) + end + + if sharing # rubocop:disable Style/GuardClause + aspect = other.aspects.create(name: "Test") + other.share_with(auth.user.person, aspect) + end + end + end + + context "with an aspects filter" do + let(:contact_name) { "My aspect contact" } + let(:aspect) { auth.user.aspects.create(name: "Test") } + let(:second_aspect) { auth.user.aspects.create(name: "Second test") } + + it "only returns members of given aspects" do + add_contact(aspect) + add_contact(second_aspect) + + get( + "/api/v1/search/users", + params: { + name_or_handle: contact_name, + filter: "aspect:#{aspect.id}", + access_token: access_token + } + ) + + expect(response.status).to eq(200) + users = response_body_data(response) + expect(users.length).to eq(1) + + get( + "/api/v1/search/users", + params: { + name_or_handle: contact_name, + filter: "aspect:#{aspect.id},#{second_aspect.id}", + access_token: access_token + } + ) + + expect(response.status).to eq(200) + users = response_body_data(response) + expect(users.length).to eq(2) + end + + it "only returns people matching all aspect filters" do + add_contact(aspect) + add_contact(second_aspect) + add_contact(aspect, second_aspect) + + get( + "/api/v1/search/users", + params: { + name_or_handle: contact_name, + filter: ["aspect:#{aspect.id}", "aspect:#{second_aspect.id}"], + access_token: access_token + } + ) + + expect(response.status).to eq(200) + users = response_body_data(response) + expect(users.length).to eq(1) + end + + it "fails with an invalid aspect" do + get( + "/api/v1/search/users", + params: { + name_or_handle: contact_name, + filter: "aspect:0", + access_token: access_token + } + ) + + confirm_api_error(response, 422, "Invalid aspect filter") + + get( + "/api/v1/search/users", + params: { + name_or_handle: contact_name, + filter: "aspect:#{aspect.id},0", + access_token: access_token + } + ) + + confirm_api_error(response, 422, "Invalid aspect filter") + end + + it "fails without contacts:read scope" do + aspect = auth_read_only.user.aspects.create(name: "Test") + + get( + "/api/v1/search/users", + params: { + name_or_handle: contact_name, + filter: "aspect:#{aspect.id}", + access_token: access_token_read_only + } + ) + + confirm_api_error(response, 403, "insufficient_scope") + end + + def add_contact(*aspects) + other = FactoryGirl.create(:person, profile: FactoryGirl.build(:profile, first_name: contact_name)) + aspects.each do |aspect| + auth.user.share_with(other, aspect) + end + end + end + + it "fails with an invalid filter" do + get( + "/api/v1/search/users", + params: { + name_or_handle: "findable", + filter: "thingsiwant", + access_token: access_token + } + ) + + confirm_api_error(response, 422, "Invalid filter") + end + it "doesn't return closed accounts" do get( "/api/v1/search/users", @@ -128,7 +370,7 @@ describe Api::V1::SearchController do "/api/v1/search/users", params: {tag: "tag1", name_or_handle: "name", access_token: access_token} ) - confirm_api_error(response, 422, "Search request could not be processed") + confirm_api_error(response, 422, "Parameters tag and name_or_handle are exclusive") end it "fails with no fields" do @@ -136,7 +378,7 @@ describe Api::V1::SearchController do "/api/v1/search/users", params: {access_token: access_token} ) - confirm_api_error(response, 422, "Search request could not be processed") + confirm_api_error(response, 422, "Missing parameter tag or name_or_handle") end it "fails with bad credentials" do @@ -208,7 +450,7 @@ describe Api::V1::SearchController do "/api/v1/search/posts", params: {access_token: access_token} ) - confirm_api_error(response, 422, "Search request could not be processed") + confirm_api_error(response, 422, "param is missing or the value is empty: tag") end it "fails with bad credentials" do @@ -256,7 +498,7 @@ describe Api::V1::SearchController do "/api/v1/search/tags", params: {access_token: access_token} ) - confirm_api_error(response, 422, "Search request could not be processed") + confirm_api_error(response, 422, "param is missing or the value is empty: query") end it "fails with bad credentials" do