From 045324bf7b181292e5dee72db88b548f108d09ae Mon Sep 17 00:00:00 2001 From: Michael Ziwisky Date: Tue, 10 Aug 2021 00:28:15 -0700 Subject: [PATCH] allow apollo federation introspection query w/o authentication plus miscelaneous improvements to the gql controller specs refs INTEROP-6900 flag = none test plan: - request the subgraph schema w/o providing any auth, e.g.: $ curl http://localhost:3000/api/graphql/subgraph \ -X POST \ -d query='query { _service { sdl } }' - this should return the schema Change-Id: I705f694ffb490fd4c2ba2241291769a6877cee14 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/271051 Tested-by: Service Cloud Jenkins Reviewed-by: Ethan Vizitei QA-Review: Michael Ziwisky Product-Review: Michael Ziwisky --- app/controllers/graphql_controller.rb | 16 ++++- gems/inst_access/lib/inst_access.rb | 2 +- spec/controllers/graphql_controller_spec.rb | 69 +++++++++++++++------ 3 files changed, 66 insertions(+), 21 deletions(-) diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb index 55e1a3d6866..f4c13945a45 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -25,8 +25,8 @@ CanvasSchema.graphql_definition class GraphQLController < ApplicationController include Api::V1 - before_action :require_inst_access_token_auth, only: :subgraph_execute before_action :require_user, if: :require_auth? + before_action :require_inst_access_token_auth, only: :subgraph_execute, unless: :sdl_query? # This action is for use only with the federated API Gateway. See # `app/graphql/README.md` for details. @@ -79,13 +79,25 @@ class GraphQLController < ApplicationController return !::Account.site_admin.feature_enabled?(:disable_graphql_authentication) end + if action_name == 'subgraph_execute' && sdl_query? + return false + end + true end + def sdl_query? + query = (params[:query] || '').strip + return false unless query.starts_with?('query') || query.starts_with?('{') + query = query[/{.*/] # slice off leading "query" keyword and/or query name, if any + query.gsub!(/\s+/, '') # strip all whitespace + query == '{_service{sdl}}' + end + def require_inst_access_token_auth unless @authenticated_with_inst_access_token render( - json: {error: "InstAccess token auth required"}, + json: {errors: [{message: "InstAccess token auth required"}]}, status: 401 ) end diff --git a/gems/inst_access/lib/inst_access.rb b/gems/inst_access/lib/inst_access.rb index 25d81668c4e..90b711766f0 100644 --- a/gems/inst_access/lib/inst_access.rb +++ b/gems/inst_access/lib/inst_access.rb @@ -46,7 +46,7 @@ module InstAccess end def config - @config || raise(ConfigError, "InstID is not configured!") + @config || raise(ConfigError, "InstAccess is not configured!") end end end diff --git a/spec/controllers/graphql_controller_spec.rb b/spec/controllers/graphql_controller_spec.rb index 391a4a05a58..6ac2c3402ef 100644 --- a/spec/controllers/graphql_controller_spec.rb +++ b/spec/controllers/graphql_controller_spec.rb @@ -22,11 +22,16 @@ require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') describe GraphQLController do before :once do - student_in_course + student_in_course(user: user_with_pseudonym) end - let(:federation_query) do - 'query FED { _entities(representations: [ {__typename: Course, id: "1"} ]) { ...on Course { name } } }' + let(:federation_query_params) do + { + query: 'query ($representations: [_Any!]!) { _entities(representations: $representations) { ...on Course { name } } }', + variables: { + representations: [ {__typename: "Course", id: "1"} ] + } + } end context "graphiql" do @@ -59,8 +64,8 @@ describe GraphQLController do context "graphql, without a session" do it "requires a user" do - post :execute, params: {query: "{}"} - expect(response.location).to match(/\/login$/) + post :execute, params: {query: "{}"}, format: :json + expect(response).to be_unauthorized end end @@ -68,13 +73,15 @@ describe GraphQLController do before { user_session(@student) } it "works" do - post :execute, params: {query: "{}"} - expect(JSON.parse(response.body)["errors"]).not_to be_blank + post :execute, params: {query: '{ course(id: "1") { id } }'}, format: :json + expect(JSON.parse(response.body)["errors"]).to be_blank + expect(JSON.parse(response.body)["data"]).not_to be_blank end it "does not handle Apollo Federation queries" do - post :execute, params: {query: federation_query} + post :execute, params: federation_query_params, format: :json expect(JSON.parse(response.body)["errors"]).not_to be_blank + expect(JSON.parse(response.body)["data"]).to be_blank end context "data dog metrics" do @@ -82,22 +89,47 @@ describe GraphQLController do allow(InstStatsd::Statsd).to receive(:increment).and_call_original expect(InstStatsd::Statsd).to receive(:increment).with("graphql.ASDF.count", tags: anything) request.headers["GraphQL-Metrics"] = "true" - post :execute, params: {query: 'query ASDF { course(id: "1") { id } }'} + post :execute, params: {query: 'query ASDF { course(id: "1") { id } }'}, format: :json end end end describe "subgraph_execute" do - before { user_session(@student) } + context "with authentication" do + around do |example| + InstAccess.with_config(signing_key: signing_priv_key) do + example.run + end + end + let(:token_signing_keypair) { OpenSSL::PKey::RSA.new(2048) } + let(:signing_priv_key) { token_signing_keypair.to_s } + let(:token) { InstAccess::Token.for_user(user_uuid: @student.uuid, account_uuid: @student.account.uuid) } - it "handles standard queries" do - post :subgraph_execute, params: {query: 'query ASDF { course(id: "1") { id } }'} - expect(JSON.parse(response.body)["errors"]).to be_blank + it "handles standard queries" do + request.headers["Authorization"] = "Bearer #{token.to_unencrypted_token_string}" + post :subgraph_execute, params: {query: '{ course(id: "1") { id } }'}, format: :json + expect(JSON.parse(response.body)["errors"]).to be_blank + expect(JSON.parse(response.body)["data"]).not_to be_blank + end + + it "handles Apollo Federation queries" do + request.headers["Authorization"] = "Bearer #{token.to_unencrypted_token_string}" + post :subgraph_execute, params: federation_query_params, format: :json + expect(JSON.parse(response.body)["errors"]).to be_blank + end end - it "handles Apollo Federation queries" do - post :subgraph_execute, params: {query: federation_query} - expect(JSON.parse(response.body)["errors"]).to be_blank + describe "without authentication" do + it "services subgraph introspection queries" do + post :subgraph_execute, params: {query: 'query FederationSubgraphIntrospection { _service { sdl } }'}, format: :json + expect(JSON.parse(response.body)["errors"]).to be_blank + expect(JSON.parse(response.body)["data"]).not_to be_blank + end + + it "rejects other queries" do + post :subgraph_execute, params: federation_query_params, format: :json + expect(response).to be_unauthorized + end end end @@ -108,8 +140,9 @@ describe GraphQLController do expect(Account.site_admin).to( receive(:feature_enabled?).with(:disable_graphql_authentication).and_return(true) ) - post :execute, params: {query: "{}"} - expect(JSON.parse(response.body)["errors"]).not_to be_blank + post :execute, params: {query: '{ course(id: "1") { id } }'}, format: :json + expect(JSON.parse(response.body)["errors"]).to be_blank + expect(JSON.parse(response.body)["data"]).not_to be_blank end end end