From 0a16bf934a0304057e46336007664746009f15e0 Mon Sep 17 00:00:00 2001 From: Anna Buianova Date: Wed, 18 Sep 2019 23:25:40 +0300 Subject: [PATCH] Added association between webhook_endpoints and oauth apps (#4060) * Added association between webhook_endpoints and oauth apps * Limit webhooks access by oauth_app_id when authorized by doorkeeper * Use rails route helpers in webhooks api test --- app/controllers/api/v0/webhooks_controller.rb | 18 +++- app/models/webhook/endpoint.rb | 6 ++ ...uth_application_id_to_webhook_endpoints.rb | 7 ++ db/schema.rb | 5 +- spec/requests/api/v0/webhooks_spec.rb | 89 +++++++++++++++---- 5 files changed, 102 insertions(+), 23 deletions(-) create mode 100644 db/migrate/20190918104106_add_oauth_application_id_to_webhook_endpoints.rb diff --git a/app/controllers/api/v0/webhooks_controller.rb b/app/controllers/api/v0/webhooks_controller.rb index 48b16397f..337fbe6e9 100644 --- a/app/controllers/api/v0/webhooks_controller.rb +++ b/app/controllers/api/v0/webhooks_controller.rb @@ -7,26 +7,36 @@ module Api skip_before_action :verify_authenticity_token, only: %w[create destroy] def index - @webhooks = @user.webhook_endpoints.order(:id) + @webhooks = webhooks_scope.order(:id) end def create - @webhook = @user.webhook_endpoints.create!(webhook_params) + @webhook = @user.webhook_endpoints.new(webhook_params) + @webhook.oauth_application_id = doorkeeper_token.application_id if doorkeeper_token + @webhook.save! render "show", status: :created end def show - @webhook = @user.webhook_endpoints.find(params[:id]) + @webhook = webhooks_scope.find(params[:id]) end def destroy - webhook = @user.webhook_endpoints.find(params[:id]) + webhook = webhooks_scope.find(params[:id]) webhook.destroy! head :no_content end private + def webhooks_scope + if doorkeeper_token + @user.webhook_endpoints.for_app(doorkeeper_token.application_id) + else + @user.webhook_endpoints + end + end + def webhook_params params.require(:webhook_endpoint).permit(:target_url, :source, events: []) end diff --git a/app/models/webhook/endpoint.rb b/app/models/webhook/endpoint.rb index f43101559..b7a5623a6 100644 --- a/app/models/webhook/endpoint.rb +++ b/app/models/webhook/endpoint.rb @@ -1,6 +1,11 @@ module Webhook class Endpoint < ApplicationRecord belongs_to :user, inverse_of: :webhook_endpoints + # rubocop:disable Rails/InverseOf + belongs_to :oauth_application, optional: true, + class_name: "Doorkeeper::Application", + foreign_key: :oauth_application_id + # rubocop:enable Rails/InverseOf validates :target_url, presence: true, uniqueness: true, url: { schemes: %w[https] } validates :source, :events, presence: true @@ -8,6 +13,7 @@ module Webhook attribute :events, :string, array: true, default: [] scope :for_events, ->(events) { where("events @> ARRAY[?]::varchar[]", Array(events)) } + scope :for_app, ->(app_id) { where(oauth_application_id: app_id) } def self.table_name_prefix "webhook_" diff --git a/db/migrate/20190918104106_add_oauth_application_id_to_webhook_endpoints.rb b/db/migrate/20190918104106_add_oauth_application_id_to_webhook_endpoints.rb new file mode 100644 index 000000000..dc5d33ea5 --- /dev/null +++ b/db/migrate/20190918104106_add_oauth_application_id_to_webhook_endpoints.rb @@ -0,0 +1,7 @@ +class AddOauthApplicationIdToWebhookEndpoints < ActiveRecord::Migration[5.2] + def change + change_table :webhook_endpoints do |t| + t.references :oauth_application, foreign_key: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 88eae5a63..4ca5a1a8d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -12,7 +12,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_09_10_153845) do +ActiveRecord::Schema.define(version: 2019_09_18_104106) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1153,11 +1153,13 @@ ActiveRecord::Schema.define(version: 2019_09_10_153845) do create_table "webhook_endpoints", force: :cascade do |t| t.datetime "created_at", null: false t.string "events", null: false, array: true + t.bigint "oauth_application_id" t.string "source" t.string "target_url", null: false t.datetime "updated_at", null: false t.bigint "user_id", null: false t.index ["events"], name: "index_webhook_endpoints_on_events" + t.index ["oauth_application_id"], name: "index_webhook_endpoints_on_oauth_application_id" t.index ["user_id"], name: "index_webhook_endpoints_on_user_id" end @@ -1174,5 +1176,6 @@ ActiveRecord::Schema.define(version: 2019_09_10_153845) do add_foreign_key "push_notification_subscriptions", "users" add_foreign_key "sponsorships", "organizations" add_foreign_key "sponsorships", "users" + add_foreign_key "webhook_endpoints", "oauth_applications" add_foreign_key "webhook_endpoints", "users" end diff --git a/spec/requests/api/v0/webhooks_spec.rb b/spec/requests/api/v0/webhooks_spec.rb index a722d7720..f13573070 100644 --- a/spec/requests/api/v0/webhooks_spec.rb +++ b/spec/requests/api/v0/webhooks_spec.rb @@ -2,26 +2,23 @@ require "rails_helper" RSpec.describe "Api::V0::Webhooks", type: :request do let(:user) { create(:user) } - let!(:webhook) { create(:webhook_endpoint, user: user, target_url: "https://api.example.com/go") } - - before do - sign_in user - end describe "GET /api/v0/webhooks" do + let!(:webhook) { create(:webhook_endpoint, user: user, target_url: "https://api.example.com/go") } let!(:webhook2) { create(:webhook_endpoint, user: user, target_url: "https://api.example.com/webhook") } before do + sign_in user create(:webhook_endpoint) end it "returns 200 on success" do - get "/api/webhooks" + get api_webhooks_path expect(response).to have_http_status(:ok) end it "returns json on success" do - get "/api/webhooks" + get api_webhooks_path json = JSON.parse(response.body) ids = json.map { |item| item["id"] } urls = json.map { |item| item["target_url"] } @@ -31,24 +28,30 @@ RSpec.describe "Api::V0::Webhooks", type: :request do end describe "GET /api/v0/webhooks/:id" do + let!(:webhook) { create(:webhook_endpoint, user: user, target_url: "https://api.example.com/go") } + + before do + sign_in user + end + it "returns 200 on success" do - get "/api/webhooks/#{webhook.id}" + get api_webhook_path(webhook.id) expect(response).to have_http_status(:ok) end it "returns 404 if the webhook does not exist" do - get "/api/webhooks/9999" + get api_webhook_path(9999) expect(response).to have_http_status(:not_found) end it "returns 404 if another user webhook is accessed" do other_webhook = create(:webhook_endpoint, user: create(:user)) - get "/api/webhooks/#{other_webhook.id}" + get api_webhook_path(other_webhook.id) expect(response).to have_http_status(:not_found) end it "returns json on success" do - get "/api/webhooks/#{webhook.id}" + get api_webhook_path(webhook.id) json = JSON.parse(response.body) expect(json["target_url"]).to eq(webhook.target_url) expect(json["user"]["username"]).to eq(user.username) @@ -64,22 +67,27 @@ RSpec.describe "Api::V0::Webhooks", type: :request do } end + before do + sign_in user + end + it "creates a webhook" do expect do - post "/api/webhooks", params: { webhook_endpoint: webhook_params } + post api_webhooks_path, params: { webhook_endpoint: webhook_params } end.to change(Webhook::Endpoint, :count).by(1) end it "creates a webhook with events and data" do - post "/api/webhooks", params: { webhook_endpoint: webhook_params } + post api_webhooks_path, params: { webhook_endpoint: webhook_params } created_webhook = user.webhook_endpoints.last expect(created_webhook.events).to eq(%w[article_created article_updated article_destroyed]) expect(created_webhook.target_url).to eq(webhook_params[:target_url]) expect(created_webhook.source).to eq(webhook_params[:source]) + expect(created_webhook.oauth_application_id).to eq(nil) end it "returns :created and json response on success" do - post "/api/webhooks", params: { webhook_endpoint: webhook_params } + post api_webhooks_path, params: { webhook_endpoint: webhook_params } expect(response).to have_http_status(:created) expect(response.content_type).to eq("application/json") json = JSON.parse(response.body) @@ -88,27 +96,72 @@ RSpec.describe "Api::V0::Webhooks", type: :request do end describe "DELETE /api/v0/webhooks/:id" do + let!(:webhook) { create(:webhook_endpoint, user: user, target_url: "https://api.example.com/go") } + + before do + sign_in user + end + it "deletes the webhook" do expect do - delete "/api/webhooks/#{webhook.id}" + delete api_webhook_path(webhook.id) end.to change(Webhook::Endpoint, :count).by(-1) end it "returns 204 on success" do - delete "/api/webhooks/#{webhook.id}" + delete api_webhook_path(webhook.id) expect(response).to have_http_status(:no_content) end it "doesn't allow to destroy other user webhook" do other_webhook = create(:webhook_endpoint, user: create(:user)) expect do - delete "/api/webhooks/#{other_webhook.id}" + delete api_webhook_path(other_webhook.id) end.not_to change(Webhook::Endpoint, :count) end it "returns 404 if another user webhook is accessed" do other_webhook = create(:webhook_endpoint, user: create(:user)) - delete "/api/webhooks/#{other_webhook.id}" + delete api_webhook_path(other_webhook.id) + expect(response).to have_http_status(:not_found) + end + end + + describe "authorized with doorkeeper" do + let!(:oauth_app) { create(:application) } + let!(:oauth_app2) { create(:application) } + let(:access_token) { create :doorkeeper_access_token, resource_owner: user, application: oauth_app2 } + + it "renders index successfully" do + get api_webhooks_path, params: { access_token: access_token.token } + expect(response.content_type).to eq("application/json") + expect(response).to have_http_status(:ok) + end + + it "renders only corresponding webhooks" do + create(:webhook_endpoint, oauth_application_id: oauth_app.id, user: user) + webhook2 = create(:webhook_endpoint, oauth_application_id: oauth_app2.id, user: user) + get api_webhooks_path, params: { access_token: access_token.token } + + json = JSON.parse(response.body) + ids = json.map { |item| item["id"] } + expect(ids).to eq([webhook2.id]) + end + + it "sets correct oauth app id for the webhook if needed" do + webhook_params = { + source: "stackbit", + target_url: Faker::Internet.url(scheme: "https"), + events: %w[article_created article_updated article_destroyed] + } + post api_webhooks_path, params: { access_token: access_token.token, webhook_endpoint: webhook_params } + webhook = user.webhook_endpoints.find_by(target_url: webhook_params[:target_url]) + expect(webhook.oauth_application_id).to eq(oauth_app2.id) + end + + it "doesn't allow destroying another app webhook" do + other_webhook = create(:webhook_endpoint, user: user) + delete api_webhook_path(other_webhook.id), params: { access_token: access_token.token } expect(response).to have_http_status(:not_found) end end