From acd5b175ed8189ff8a777d6b157e5336cb7aa0e3 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Mon, 28 Feb 2022 08:50:33 -0500 Subject: [PATCH] Ensuring POST api/v0/articles authorizes by policy (#16685) As part of the [Authorization System Use Case 1:1](https://github.com/orgs/forem/projects/46) project, we are driving towards the feature of: "Only admin's may post articles in this Forem." This commit ensures that the API's "create an article" end-point delivers on that feature. Along the way, I've added reading notes and comments, to help us further flex in the future (namely move all authorization checks into a policy object). Closes forem/forem#16488 --- app/controllers/api/v0/api_controller.rb | 28 +++++++++ app/controllers/api/v0/articles_controller.rb | 8 ++- spec/requests/api/v0/articles_spec.rb | 60 ++++++++++++------- 3 files changed, 75 insertions(+), 21 deletions(-) diff --git a/app/controllers/api/v0/api_controller.rb b/app/controllers/api/v0/api_controller.rb index ba7086bfc..d771d0052 100644 --- a/app/controllers/api/v0/api_controller.rb +++ b/app/controllers/api/v0/api_controller.rb @@ -50,6 +50,16 @@ module Api # Checks if the user is authenticated, sets @user to nil otherwise # # @return [User, NilClass] + # + # @see {#pundit_user} for one way we use this method + # @see {#authenticate_with_api_key_or_current_user} for the logic of building the user. + # + # @note We could memoize the `@user ||=` but Rubocop wants to rename that to + # `authenticate_with_api_key_or_current_user` which would be bad as descendant classes + # have chosen to reference the `@user` instance variable. Intsead [@jeremyf] is + # favoring leaving this method as is to reduce impact, and having `#pundit_user` do the + # memoization. + # def authenticate_with_api_key_or_current_user @user = authenticate_with_api_key || current_user end @@ -67,6 +77,24 @@ module Api private + # @note By default pundit_user is an alias of "#current_user". However, as "#current_user" + # only tells part of the story, we need to roll our own. That means checking if we have + # `@user` (which is set in #authenticate_with_api_key_or_current_user) but if that's not + # present, call the method. + # + # @return [User, NilClass] + # + # @note [@jeremyf] is choosing to reference the instance variable (e.g. `@user`) and if that's + # nil to call the `authenticate_with_api_key_or_current_user`. This way I'm not + # altering the implementation details of the `authenticate_with_api_key_or_current_user` + # function by introducing memoization. + # + # @see {#authenticate_with_api_key_or_current_user} + def pundit_user + # What's going on here? + @pundit_user ||= @user || authenticate_with_api_key_or_current_user + end + def authenticate_with_api_key api_key = request.headers["api-key"] return unless api_key diff --git a/app/controllers/api/v0/articles_controller.rb b/app/controllers/api/v0/articles_controller.rb index f5fa73968..51d7c2f96 100644 --- a/app/controllers/api/v0/articles_controller.rb +++ b/app/controllers/api/v0/articles_controller.rb @@ -1,14 +1,18 @@ module Api module V0 + # @note This controller partially authorizes with the ArticlePolicy, in an ideal world, it would + # fully authorize. However, that refactor would require significantly more work. class ArticlesController < ApiController before_action :authenticate!, only: %i[create update me] - before_action :validate_article_param_is_hash, only: %w[create update] + before_action :validate_article_param_is_hash, only: %i[create update] before_action :set_cache_control_headers, only: %i[index show show_by_slug] skip_before_action :verify_authenticity_token, only: %i[create update] + after_action :verify_authorized, only: %i[create] + INDEX_ATTRIBUTES_FOR_SERIALIZATION = %i[ id user_id organization_id collection_id title description main_image published_at crossposted_at social_image @@ -58,6 +62,8 @@ module Api end def create + authorize(Article) + @article = Articles::Creator.call(@user, article_params).decorate if @article.persisted? diff --git a/spec/requests/api/v0/articles_spec.rb b/spec/requests/api/v0/articles_spec.rb index 5771b9c82..6eb76342e 100644 --- a/spec/requests/api/v0/articles_spec.rb +++ b/spec/requests/api/v0/articles_spec.rb @@ -558,32 +558,52 @@ RSpec.describe "Api::V0::Articles", type: :request do end describe "POST /api/articles" do + # As written, it's envisioned that the subject and these "let" statements create a valid + # authentication and authorization. + subject(:the_response) do + # This looks a bit funny, I want to issue the request but test the response. The "post" + # method does not return a "response" object. + post api_articles_path, params: { article: params }.to_json, headers: headers + response + end + let(:api_secret) { create(:api_secret) } let(:user) { api_secret.user } + let(:headers) { { "api-key" => api_secret.secret, "content-type" => "application/json" } } + let(:params) { {} } - context "when unauthorized" do - it "fails with no api key" do - post api_articles_path, headers: { "content-type" => "application/json" } - expect(response).to have_http_status(:unauthorized) - end + context "when user suspended" do + before { user.add_role(:suspended) } - it "fails with a suspended user" do - user.add_role(:suspended) - post api_articles_path, headers: { "api-key" => api_secret.secret, "content-type" => "application/json" } - expect(response).to have_http_status(:unauthorized) - end + it { is_expected.to have_http_status(:unauthorized) } + end - it "fails with the wrong api key" do - post api_articles_path, headers: { "api-key" => "foobar", "content-type" => "application/json" } - expect(response).to have_http_status(:unauthorized) - end + context "when no api key provided" do + let(:headers) { { "content-type" => "application/json" } } - it "fails with a failing secure compare" do - allow(ActiveSupport::SecurityUtils) - .to receive(:secure_compare).and_return(false) - post api_articles_path, headers: { "api-key" => api_secret.secret, "content-type" => "application/json" } - expect(response).to have_http_status(:unauthorized) - end + it { is_expected.to have_http_status(:unauthorized) } + end + + context "when given invalid api key" do + let(:headers) { { "api-key" => "no you're never gonna get it", "content-type" => "application/json" } } + + it { is_expected.to have_http_status(:unauthorized) } + end + + context "when security comparision fails" do + before { allow(ActiveSupport::SecurityUtils).to receive(:secure_compare).and_return(false) } + + it { is_expected.to have_http_status(:unauthorized) } + end + + context "when only admins can post to site" do + # [@jeremyf] Part of me loaths the idea of writing this specific policy implementation. + # Another option is to do some "allow_any_instance_of" antics. For now, this is + # the concession, but as we move through further policy changes, I'm uncertain if + # we want our requests to bombard the nuances of policy. + before { allow(ArticlePolicy).to receive(:limit_post_creation_to_admins?).and_return(true) } + + it { is_expected.to have_http_status(:unauthorized) } end describe "when authorized" do