From 5a9eb5504c5d44dc7e32cc697538c7b0ab7c6d37 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 27 Jun 2024 10:10:31 +0100 Subject: [PATCH 01/10] Consistently read BYPASS_OAUTH env var --- app/controllers/auth_controller.rb | 2 +- config/initializers/editor_api_config | 0 config/initializers/omniauth.rb | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) create mode 100644 config/initializers/editor_api_config diff --git a/app/controllers/auth_controller.rb b/app/controllers/auth_controller.rb index c6efc85b5..6f1778786 100644 --- a/app/controllers/auth_controller.rb +++ b/app/controllers/auth_controller.rb @@ -22,7 +22,7 @@ def destroy reset_session # Prevent redirect loops etc. - if ENV['BYPASS_OAUTH'].present? + if ENV.fetch('BYPASS_OAUTH', nil) == 'true' redirect_to root_path return end diff --git a/config/initializers/editor_api_config b/config/initializers/editor_api_config new file mode 100644 index 000000000..e69de29bb diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb index 87d82bb2d..fb50d9a6a 100644 --- a/config/initializers/omniauth.rb +++ b/config/initializers/omniauth.rb @@ -2,7 +2,7 @@ OmniAuth.config.logger = Rails.logger -if ENV['BYPASS_OAUTH'].present? +if ENV.fetch('BYPASS_OAUTH', nil) == 'true' using RpiAuthBypass extra = RpiAuthBypass::DEFAULT_EXTRA.deep_merge(raw_info: { roles: 'editor-admin' }) From 20650ac4c50ff58e9c6dad34c5f584a982750625 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 27 Jun 2024 14:02:10 +0100 Subject: [PATCH 02/10] Avoid defaulting env vars in `AuthController#destroy` I _think_ the only time it's safe to set these to nil is if BYPASS_OAUTH is set to true (i.e. when we're not logged in via Profile). In that case this `#destroy` method returns early so we can assume that they'll be set if we're logging out for real. --- app/controllers/auth_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/auth_controller.rb b/app/controllers/auth_controller.rb index 6f1778786..08a5eadba 100644 --- a/app/controllers/auth_controller.rb +++ b/app/controllers/auth_controller.rb @@ -27,7 +27,7 @@ def destroy return end - redirect_to "#{ENV.fetch('IDENTITY_URL', nil)}/logout?returnTo=#{ENV.fetch('HOST_URL', nil)}", + redirect_to "#{ENV.fetch('IDENTITY_URL')}/logout?returnTo=#{ENV.fetch('HOST_URL')}", allow_other_host: true end From eb2d9dcd23507d877309089e6cb316b8e76f9f51 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 27 Jun 2024 14:06:57 +0100 Subject: [PATCH 03/10] Add GITHUB_WEBHOOK_REF to .env.example This is read in GithubWebhooksController and UploadJob. We use `ENV.fetch` to read it which implies that we're expecting it to exist so I think it's reasonable to add it to our .env files. --- .env.example | 1 + 1 file changed, 1 insertion(+) diff --git a/.env.example b/.env.example index 1b94b6601..b523c604c 100644 --- a/.env.example +++ b/.env.example @@ -6,6 +6,7 @@ AWS_S3_REGION=changeme AWS_SECRET_ACCESS_KEY=changeme GITHUB_WEBHOOK_SECRET=test_token +GITHUB_WEBHOOK_REF=ref POSTGRES_DB=development POSTGRES_HOST=db From f2697284f307792631afc1633d913a6d66fa91d0 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 27 Jun 2024 10:14:01 +0100 Subject: [PATCH 04/10] Move BYPASS_OAUTH to Rails config --- app/controllers/auth_controller.rb | 2 +- config/application.rb | 2 ++ config/initializers/omniauth.rb | 2 +- lib/hydra_public_api_client.rb | 2 +- lib/user_info_api_client.rb | 2 +- spec/models/user_spec.rb | 20 ++++++++------------ 6 files changed, 14 insertions(+), 16 deletions(-) diff --git a/app/controllers/auth_controller.rb b/app/controllers/auth_controller.rb index 08a5eadba..cd51b7004 100644 --- a/app/controllers/auth_controller.rb +++ b/app/controllers/auth_controller.rb @@ -22,7 +22,7 @@ def destroy reset_session # Prevent redirect loops etc. - if ENV.fetch('BYPASS_OAUTH', nil) == 'true' + if Rails.configuration.bypass_oauth redirect_to root_path return end diff --git a/config/application.rb b/config/application.rb index 43a8c9b7d..7ebf74a68 100644 --- a/config/application.rb +++ b/config/application.rb @@ -57,5 +57,7 @@ class Application < Rails::Application config.middleware.insert_before 0, CorpMiddleware config.generators.system_tests = nil + + config.bypass_oauth = ENV.fetch('BYPASS_OAUTH', nil) == 'true' end end diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb index fb50d9a6a..bf7322aae 100644 --- a/config/initializers/omniauth.rb +++ b/config/initializers/omniauth.rb @@ -2,7 +2,7 @@ OmniAuth.config.logger = Rails.logger -if ENV.fetch('BYPASS_OAUTH', nil) == 'true' +if Rails.configuration.bypass_oauth using RpiAuthBypass extra = RpiAuthBypass::DEFAULT_EXTRA.deep_merge(raw_info: { roles: 'editor-admin' }) diff --git a/lib/hydra_public_api_client.rb b/lib/hydra_public_api_client.rb index 3daea3f92..6ec86fe82 100644 --- a/lib/hydra_public_api_client.rb +++ b/lib/hydra_public_api_client.rb @@ -24,7 +24,7 @@ def fetch_oauth_user(token:) private def bypass_oauth? - ENV.fetch('BYPASS_OAUTH', nil) == 'true' + Rails.configuration.bypass_oauth end def stubbed_user diff --git a/lib/user_info_api_client.rb b/lib/user_info_api_client.rb index 25a84c826..6dec726ad 100644 --- a/lib/user_info_api_client.rb +++ b/lib/user_info_api_client.rb @@ -21,7 +21,7 @@ def fetch_by_ids(user_ids) private def bypass_oauth? - ENV.fetch('BYPASS_OAUTH', nil) == 'true' + Rails.configuration.bypass_oauth end def transform_result(result) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b3c8c78f2..f29603ac7 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -85,11 +85,9 @@ end end - context 'when BYPASS_OAUTH is true' do - around do |example| - ClimateControl.modify(BYPASS_OAUTH: 'true') do - example.run - end + context 'when the app is configured to bypass oauth' do + before do + allow(Rails.configuration).to receive(:bypass_oauth).and_return(true) end it 'does not call the API' do @@ -297,15 +295,13 @@ expect(user.email).to eq 'school-owner@example.com' end - context 'when BYPASS_OAUTH is true' do - around do |example| - ClimateControl.modify(BYPASS_OAUTH: 'true') do - example.run - end - end - + context 'when the app is configured to bypass oauth' do let(:owner) { create(:owner, school:, id: '00000000-0000-0000-0000-000000000000') } + before do + allow(Rails.configuration).to receive(:bypass_oauth).and_return(true) + end + it 'does not call the API' do user expect(WebMock).not_to have_requested(:get, /.*/) From cdc9610f03ea1cc6312a5d903aaf54e3dca3bb0e Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 27 Jun 2024 10:25:26 +0100 Subject: [PATCH 05/10] Move IDENTITY_URL to Rails config --- app/controllers/auth_controller.rb | 2 +- config/application.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/auth_controller.rb b/app/controllers/auth_controller.rb index cd51b7004..5e4cb3038 100644 --- a/app/controllers/auth_controller.rb +++ b/app/controllers/auth_controller.rb @@ -27,7 +27,7 @@ def destroy return end - redirect_to "#{ENV.fetch('IDENTITY_URL')}/logout?returnTo=#{ENV.fetch('HOST_URL')}", + redirect_to "#{Rails.configuration.identity_url}/logout?returnTo=#{ENV.fetch('HOST_URL', nil)}", allow_other_host: true end diff --git a/config/application.rb b/config/application.rb index 7ebf74a68..bb5cf30d3 100644 --- a/config/application.rb +++ b/config/application.rb @@ -59,5 +59,6 @@ class Application < Rails::Application config.generators.system_tests = nil config.bypass_oauth = ENV.fetch('BYPASS_OAUTH', nil) == 'true' + config.identity_url = ENV.fetch('IDENTITY_URL') end end From f0422af0b8c4b95987368dfe73e5c10cd7952f61 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 27 Jun 2024 14:24:13 +0100 Subject: [PATCH 06/10] Move GITHUB_WEBHOOK_SECRET to Rails config Nested under `x` in preparation for moving `GITHUB_WEBHOOK_REF` here too. As per the Rails docs[1]. [1]: https://guides.rubyonrails.org/configuring.html#custom-configuration --- app/controllers/github_webhooks_controller.rb | 2 +- config/application.rb | 2 ++ .../github_webhooks/github_webhooks_controller_push_spec.rb | 6 ++++-- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/controllers/github_webhooks_controller.rb b/app/controllers/github_webhooks_controller.rb index e072b1d2e..6d8cd80ee 100644 --- a/app/controllers/github_webhooks_controller.rb +++ b/app/controllers/github_webhooks_controller.rb @@ -10,7 +10,7 @@ def github_push(payload) private def webhook_secret(_payload) - ENV.fetch('GITHUB_WEBHOOK_SECRET') + Rails.configuration.x.github_webhook.secret end def edited_code?(payload) diff --git a/config/application.rb b/config/application.rb index bb5cf30d3..bf31fa11d 100644 --- a/config/application.rb +++ b/config/application.rb @@ -60,5 +60,7 @@ class Application < Rails::Application config.bypass_oauth = ENV.fetch('BYPASS_OAUTH', nil) == 'true' config.identity_url = ENV.fetch('IDENTITY_URL') + + config.x.github_webhook.secret = ENV.fetch('GITHUB_WEBHOOK_SECRET') end end diff --git a/spec/requests/github_webhooks/github_webhooks_controller_push_spec.rb b/spec/requests/github_webhooks/github_webhooks_controller_push_spec.rb index fc41fd7a0..355bf5c20 100644 --- a/spec/requests/github_webhooks/github_webhooks_controller_push_spec.rb +++ b/spec/requests/github_webhooks/github_webhooks_controller_push_spec.rb @@ -4,11 +4,12 @@ RSpec.describe GithubWebhooksController do around do |example| - ClimateControl.modify GITHUB_WEBHOOK_SECRET: 'secret', GITHUB_WEBHOOK_REF: 'branches/whatever' do + ClimateControl.modify GITHUB_WEBHOOK_REF: 'branches/whatever' do example.run end end + let(:github_webhook_secret) { 'secret' } let(:params) do { ref:, @@ -18,13 +19,14 @@ let(:headers) do { - 'X-Hub-Signature-256': "sha256=#{OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha256'), ENV.fetch('GITHUB_WEBHOOK_SECRET'), params.to_json)}", + 'X-Hub-Signature-256': "sha256=#{OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha256'), github_webhook_secret, params.to_json)}", 'X-GitHub-Event': 'push', 'Content-Type': 'application/json' } end before do + allow(Rails.configuration.x.github_webhook).to receive(:secret).and_return(github_webhook_secret) allow(UploadJob).to receive(:perform_later) post '/github_webhooks', env: { RAW_POST_DATA: params.to_json }, headers: end From a3acd864659f5d2e3491c1aaf7d4b5c14c4dbccf Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 27 Jun 2024 14:38:17 +0100 Subject: [PATCH 07/10] Extract GithubWebhooksController#webhook_ref In preparation for reading for Rails config instead of ENV. --- app/controllers/github_webhooks_controller.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/controllers/github_webhooks_controller.rb b/app/controllers/github_webhooks_controller.rb index 6d8cd80ee..697857ac0 100644 --- a/app/controllers/github_webhooks_controller.rb +++ b/app/controllers/github_webhooks_controller.rb @@ -4,7 +4,7 @@ class GithubWebhooksController < ActionController::API include GithubWebhook::Processor def github_push(payload) - UploadJob.perform_later(payload) if payload[:ref] == ENV.fetch('GITHUB_WEBHOOK_REF') && edited_code?(payload) + UploadJob.perform_later(payload) if payload[:ref] == webhook_ref && edited_code?(payload) end private @@ -13,6 +13,10 @@ def webhook_secret(_payload) Rails.configuration.x.github_webhook.secret end + def webhook_ref + ENV.fetch('GITHUB_WEBHOOK_REF') + end + def edited_code?(payload) commits = payload[:commits] modified_paths = commits.map { |commit| commit[:added] | commit[:modified] | commit[:removed] }.flatten From bd50de477ff7778fd3576cf062745cfa80fea080 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 27 Jun 2024 14:38:47 +0100 Subject: [PATCH 08/10] Extract github_webhook_ref in upload_job_spec In preparation for reading from Rails config instead of ENV. --- spec/jobs/upload_job_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/jobs/upload_job_spec.rb b/spec/jobs/upload_job_spec.rb index 468df4e24..384257128 100644 --- a/spec/jobs/upload_job_spec.rb +++ b/spec/jobs/upload_job_spec.rb @@ -4,12 +4,13 @@ RSpec.describe UploadJob do around do |example| - ClimateControl.modify GITHUB_AUTH_TOKEN: 'secret', GITHUB_WEBHOOK_REF: 'branches/whatever' do + ClimateControl.modify GITHUB_AUTH_TOKEN: 'secret', GITHUB_WEBHOOK_REF: github_webhook_ref do example.run end end ActiveJob::Base.queue_adapter = :test + let(:github_webhook_ref) { 'branches/whatever' } let(:graphql_response) do GraphQL::Client::Response.new(raw_response, data: UploadJob::ProjectContentQuery.new(raw_response['data'], GraphQL::Client::Errors.new)) end @@ -17,7 +18,7 @@ { repository: { name: 'my-amazing-repo', owner: { name: 'me' } }, commits: [{ added: ['ja-JP/code/dont-collide-starter/main.py'], modified: [], removed: [] }] } end let(:variables) do - { repository: 'my-amazing-repo', owner: 'me', expression: "#{ENV.fetch('GITHUB_WEBHOOK_REF')}:ja-JP/code" } + { repository: 'my-amazing-repo', owner: 'me', expression: "#{github_webhook_ref}:ja-JP/code" } end let(:raw_response) do From 0e01bbea3941ec77c1770e943ed7ccff354361f7 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 27 Jun 2024 14:40:56 +0100 Subject: [PATCH 09/10] Move GITHUB_WEBHOOK_REF to Rails config This will require developers to ensure they have this env var set in .env --- app/controllers/github_webhooks_controller.rb | 2 +- app/jobs/upload_job.rb | 4 ++-- config/application.rb | 1 + spec/jobs/upload_job_spec.rb | 3 ++- .../github_webhooks_controller_push_spec.rb | 7 +------ 5 files changed, 7 insertions(+), 10 deletions(-) diff --git a/app/controllers/github_webhooks_controller.rb b/app/controllers/github_webhooks_controller.rb index 697857ac0..796c3d60c 100644 --- a/app/controllers/github_webhooks_controller.rb +++ b/app/controllers/github_webhooks_controller.rb @@ -14,7 +14,7 @@ def webhook_secret(_payload) end def webhook_ref - ENV.fetch('GITHUB_WEBHOOK_REF') + Rails.configuration.x.github_webhook.ref end def edited_code?(payload) diff --git a/app/jobs/upload_job.rb b/app/jobs/upload_job.rb index 7e80bb690..0d598ab7b 100644 --- a/app/jobs/upload_job.rb +++ b/app/jobs/upload_job.rb @@ -55,7 +55,7 @@ def modified_locales(payload) def load_projects_data(locale, repository, owner) GithubApi::Client.query( ProjectContentQuery, - variables: { repository:, owner:, expression: "#{ENV.fetch('GITHUB_WEBHOOK_REF')}:#{locale}/code" } + variables: { repository:, owner:, expression: "#{Rails.configuration.x.github_webhook.ref}:#{locale}/code" } ) end @@ -85,7 +85,7 @@ def component(file) def image(file, project_dir, locale, repository, owner) filename = file.name directory = project_dir.name - url = "https://github.com/#{owner}/#{repository}/raw/#{ENV.fetch('GITHUB_WEBHOOK_REF')}/#{locale}/code/#{directory}/#{filename}" + url = "https://github.com/#{owner}/#{repository}/raw/#{Rails.configuration.x.github_webhook.ref}/#{locale}/code/#{directory}/#{filename}" { filename:, io: URI.parse(url).open } end diff --git a/config/application.rb b/config/application.rb index bf31fa11d..bcf76d6ad 100644 --- a/config/application.rb +++ b/config/application.rb @@ -62,5 +62,6 @@ class Application < Rails::Application config.identity_url = ENV.fetch('IDENTITY_URL') config.x.github_webhook.secret = ENV.fetch('GITHUB_WEBHOOK_SECRET') + config.x.github_webhook.ref = ENV.fetch('GITHUB_WEBHOOK_REF') end end diff --git a/spec/jobs/upload_job_spec.rb b/spec/jobs/upload_job_spec.rb index 384257128..43d000fbf 100644 --- a/spec/jobs/upload_job_spec.rb +++ b/spec/jobs/upload_job_spec.rb @@ -4,7 +4,7 @@ RSpec.describe UploadJob do around do |example| - ClimateControl.modify GITHUB_AUTH_TOKEN: 'secret', GITHUB_WEBHOOK_REF: github_webhook_ref do + ClimateControl.modify GITHUB_AUTH_TOKEN: 'secret' do example.run end end @@ -94,6 +94,7 @@ end before do + allow(Rails.configuration.x.github_webhook).to receive(:ref).and_return(github_webhook_ref) allow(GithubApi::Client).to receive(:query).and_return(graphql_response) stub_request(:get, 'https://github.com/me/my-amazing-repo/raw/branches/whatever/ja-JP/code/dont-collide-starter/astronaut1.png').to_return(status: 200, body: '', headers: {}) allow(ProjectImporter).to receive(:new).and_call_original diff --git a/spec/requests/github_webhooks/github_webhooks_controller_push_spec.rb b/spec/requests/github_webhooks/github_webhooks_controller_push_spec.rb index 355bf5c20..6167df570 100644 --- a/spec/requests/github_webhooks/github_webhooks_controller_push_spec.rb +++ b/spec/requests/github_webhooks/github_webhooks_controller_push_spec.rb @@ -3,12 +3,6 @@ require 'rails_helper' RSpec.describe GithubWebhooksController do - around do |example| - ClimateControl.modify GITHUB_WEBHOOK_REF: 'branches/whatever' do - example.run - end - end - let(:github_webhook_secret) { 'secret' } let(:params) do { @@ -27,6 +21,7 @@ before do allow(Rails.configuration.x.github_webhook).to receive(:secret).and_return(github_webhook_secret) + allow(Rails.configuration.x.github_webhook).to receive(:ref).and_return('branches/whatever') allow(UploadJob).to receive(:perform_later) post '/github_webhooks', env: { RAW_POST_DATA: params.to_json }, headers: end From cc1ee6cac2c03fb745d915a42b99b9b156a2f774 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 27 Jun 2024 14:44:38 +0100 Subject: [PATCH 10/10] Move EDITOR_PUBLIC_URL to Rails config This env var is set for review apps, staging and prod in Heroku. It will need to be set by devs in .env. --- app/views/invitation_mailer/invite_teacher.text.erb | 2 +- config/application.rb | 1 + spec/mailers/invitation_mailer_spec.rb | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/views/invitation_mailer/invite_teacher.text.erb b/app/views/invitation_mailer/invite_teacher.text.erb index 98b88bfc1..d9a8b92d0 100644 --- a/app/views/invitation_mailer/invite_teacher.text.erb +++ b/app/views/invitation_mailer/invite_teacher.text.erb @@ -6,7 +6,7 @@ Being part of this school account will allow you to access the school dashboard Join school: -<%= "#{ENV.fetch('EDITOR_PUBLIC_URL')}/en/invitations/#{@token}" %> +<%= "#{Rails.configuration.editor_public_url}/en/invitations/#{@token}" %> -- Raspberry Pi Foundation diff --git a/config/application.rb b/config/application.rb index bcf76d6ad..89a39d34d 100644 --- a/config/application.rb +++ b/config/application.rb @@ -60,6 +60,7 @@ class Application < Rails::Application config.bypass_oauth = ENV.fetch('BYPASS_OAUTH', nil) == 'true' config.identity_url = ENV.fetch('IDENTITY_URL') + config.editor_public_url = ENV.fetch('EDITOR_PUBLIC_URL') config.x.github_webhook.secret = ENV.fetch('GITHUB_WEBHOOK_SECRET') config.x.github_webhook.ref = ENV.fetch('GITHUB_WEBHOOK_REF') diff --git a/spec/mailers/invitation_mailer_spec.rb b/spec/mailers/invitation_mailer_spec.rb index 7a377732b..8dbfa1cda 100644 --- a/spec/mailers/invitation_mailer_spec.rb +++ b/spec/mailers/invitation_mailer_spec.rb @@ -9,7 +9,7 @@ let(:invitation) { create(:teacher_invitation) } before do - allow(ENV).to receive(:fetch).with('EDITOR_PUBLIC_URL').and_return('http://example.com') + allow(Rails.configuration).to receive(:editor_public_url).and_return('http://example.com') end it 'includes the school name in the body' do