From e7615c71fcf3d9ba11ba7602fddea1d27aac2584 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Fri, 6 Sep 2024 15:34:42 -0400 Subject: [PATCH 1/6] Add better auth error propogation from interactor to controller; added interactor unit test --- app/controllers/certificates_controller.rb | 4 +-- app/interactors/authorize_request.rb | 3 ++ test/interactors/authorize_request_test.rb | 33 ++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 test/interactors/authorize_request_test.rb diff --git a/app/controllers/certificates_controller.rb b/app/controllers/certificates_controller.rb index 5355f80..f4f204a 100644 --- a/app/controllers/certificates_controller.rb +++ b/app/controllers/certificates_controller.rb @@ -6,9 +6,9 @@ def create if !req.valid? raise BadRequestError.new req.errors.full_messages end - result = IssueCert.call(request: req, identity: @identity) + result = IssueCert.call(request: req, identity: identity) if result.failure? - raise StandardError.new result.message + raise (result.error || StandardError.new(result.message)) end @cert = result.cert end diff --git a/app/interactors/authorize_request.rb b/app/interactors/authorize_request.rb index 8f033c5..1f55c7e 100644 --- a/app/interactors/authorize_request.rb +++ b/app/interactors/authorize_request.rb @@ -3,5 +3,8 @@ class AuthorizeRequest def call Services::DomainOwnershipService.new.authorize!(context.identity, context.request) + rescue AuthError => e + context.error = e + context.fail! end end diff --git a/test/interactors/authorize_request_test.rb b/test/interactors/authorize_request_test.rb new file mode 100644 index 0000000..30cd3e6 --- /dev/null +++ b/test/interactors/authorize_request_test.rb @@ -0,0 +1,33 @@ +require "test_helper" + +class AuthorizeRequestTest < ActiveSupport::TestCase + def setup + @domain = domains(:group_match) + @identity = Identity.new(subject: @domain.owner) + @cr = CertIssueRequest.new(common_name: @domain.fqdn) + @interactor = AuthorizeRequest + end + + test "successful call" do + request = CertIssueRequest.new(common_name: @domain.fqdn) + srv = Minitest::Mock.new + srv.expect :authorize!, nil, [ @identity, @cr ] + Services::DomainOwnershipService.stub :new, srv do + context = @interactor.call(identity: @identity, request: @cr) + assert context.success? + end + end + + test "unsuccessful call" do + request = CertIssueRequest.new(common_name: @domain.fqdn) + srv = Services::DomainOwnershipService.new + Services::DomainOwnershipService.stub :new, srv do + err = ->(_, _) { raise AuthError.new "no can do" } + srv.stub :authorize!, err do + context = @interactor.call(identity: @identity, request: @cr) + assert_not context.success? + assert_kind_of AuthError, context.error + end + end + end +end From 13d8b8932772aca8b1195b89e89f54e3f6779650 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Wed, 23 Oct 2024 16:25:48 -0400 Subject: [PATCH 2/6] Add json log formatter to standard logger --- app/lib/audit_logger.rb | 2 +- app/lib/{audit_log_formatter.rb => json_log_formatter.rb} | 2 +- config/initializers/logging.rb | 4 ++++ 3 files changed, 6 insertions(+), 2 deletions(-) rename app/lib/{audit_log_formatter.rb => json_log_formatter.rb} (85%) create mode 100644 config/initializers/logging.rb diff --git a/app/lib/audit_logger.rb b/app/lib/audit_logger.rb index 84c3442..788a6cc 100644 --- a/app/lib/audit_logger.rb +++ b/app/lib/audit_logger.rb @@ -1,6 +1,6 @@ class AuditLogger < ActiveSupport::Logger def initialize super(Config[:audit_log_file]) - self.formatter = AuditLogFormatter.new + self.formatter = JsonLogFormatter.new end end diff --git a/app/lib/audit_log_formatter.rb b/app/lib/json_log_formatter.rb similarity index 85% rename from app/lib/audit_log_formatter.rb rename to app/lib/json_log_formatter.rb index 0304ea4..5e759cd 100644 --- a/app/lib/audit_log_formatter.rb +++ b/app/lib/json_log_formatter.rb @@ -1,4 +1,4 @@ -class AuditLogFormatter < ActiveSupport::Logger::SimpleFormatter +class JsonLogFormatter < ActiveSupport::Logger::SimpleFormatter def call(severity, timestamp, _progname, message) # request_id is unique to the life of the api request request_id = Thread.current[:request_id] diff --git a/config/initializers/logging.rb b/config/initializers/logging.rb new file mode 100644 index 0000000..e6e15dd --- /dev/null +++ b/config/initializers/logging.rb @@ -0,0 +1,4 @@ +Rails.application.config.to_prepare do + Rails.logger = ActiveSupport::Logger.new(STDOUT) + Rails.logger.formatter = JsonLogFormatter.new +end From 8dbca492b0ac4961236e165a4462f358e50e7290 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Thu, 24 Oct 2024 10:56:31 -0400 Subject: [PATCH 3/6] remove the production logger config override --- config/environments/production.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/config/environments/production.rb b/config/environments/production.rb index 11bb8fd..467adad 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -47,14 +47,6 @@ # Skip http-to-https redirect for the default health check endpoint. # config.ssl_options = { redirect: { exclude: ->(request) { request.path == "/up" } } } - # Log to STDOUT by default - config.logger = ActiveSupport::Logger.new(STDOUT) - .tap { |logger| logger.formatter = ::Logger::Formatter.new } - .then { |logger| ActiveSupport::TaggedLogging.new(logger) } - - # Prepend all log lines with the following tags. - config.log_tags = [ :request_id ] - # "info" includes generic and useful information about system operation, but avoids logging too much # information to avoid inadvertent exposure of personally identifiable information (PII). If you # want to log everything, set the level to "debug". From 95112f04376738219191265b1c4e730c416c8798 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Thu, 24 Oct 2024 12:23:31 -0400 Subject: [PATCH 4/6] use json_tagged_logging gem --- Gemfile | 3 +++ Gemfile.lock | 4 ++++ config/initializers/logging.rb | 9 +++++---- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index 126add0..983ef67 100644 --- a/Gemfile +++ b/Gemfile @@ -22,6 +22,9 @@ gem "tzinfo-data", platforms: %i[ mswin jruby ] # Reduces boot times through caching; required in config/boot.rb gem "bootsnap", require: false +# json logging +gem "json_tagged_logger" + # Use Active Storage variants [https://guides.rubyonrails.org/active_storage_overview.html#transforming-images] # gem "image_processing", "~> 1.2" diff --git a/Gemfile.lock b/Gemfile.lock index 0f925c8..3646b5d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -112,6 +112,9 @@ GEM actionview (>= 5.0.0) activesupport (>= 5.0.0) json (2.7.2) + json_tagged_logger (0.8.0) + actionpack (>= 5.2) + activesupport (>= 5.2) jwt (2.9.3) base64 language_server-protocol (3.17.0.3) @@ -290,6 +293,7 @@ DEPENDENCIES faraday-retry interactor (~> 3.0) jbuilder + json_tagged_logger jwt ostruct puma (>= 5.0) diff --git a/config/initializers/logging.rb b/config/initializers/logging.rb index e6e15dd..a3bde5e 100644 --- a/config/initializers/logging.rb +++ b/config/initializers/logging.rb @@ -1,4 +1,5 @@ -Rails.application.config.to_prepare do - Rails.logger = ActiveSupport::Logger.new(STDOUT) - Rails.logger.formatter = JsonLogFormatter.new -end +Rails.logger = JsonTaggedLogger::Logger.new(Rails.logger) +Rails.configuration.log_tags = JsonTaggedLogger::LogTagsConfig.generate( + :request_id +) + From a5fd7b35f0349fea6b202fca97ea9c89480c1d05 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Thu, 24 Oct 2024 12:33:29 -0400 Subject: [PATCH 5/6] Fix linter complaint --- config/initializers/logging.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/config/initializers/logging.rb b/config/initializers/logging.rb index a3bde5e..9fb07bb 100644 --- a/config/initializers/logging.rb +++ b/config/initializers/logging.rb @@ -2,4 +2,3 @@ Rails.configuration.log_tags = JsonTaggedLogger::LogTagsConfig.generate( :request_id ) - From a51d2e7c9a8034b5c56dcf0857f299ca77fc8c45 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Thu, 24 Oct 2024 12:57:22 -0400 Subject: [PATCH 6/6] Fix test --- ...t_log_formatter_test.rb => json_log_formatter_test.rb} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename test/lib/{audit_log_formatter_test.rb => json_log_formatter_test.rb} (70%) diff --git a/test/lib/audit_log_formatter_test.rb b/test/lib/json_log_formatter_test.rb similarity index 70% rename from test/lib/audit_log_formatter_test.rb rename to test/lib/json_log_formatter_test.rb index e23e185..ad73b6e 100644 --- a/test/lib/audit_log_formatter_test.rb +++ b/test/lib/json_log_formatter_test.rb @@ -1,19 +1,19 @@ require "test_helper" -class AuditLogFormatterTest < ActiveSupport::TestCase +class JsonLogFormatterTest < ActiveSupport::TestCase setup do Thread.current[:request_id] = nil end test "#call formats logformatter inputs as json" do t = Time.now - result = AuditLogFormatter.new.call("info", t, nil, "some message") + result = JsonLogFormatter.new.call("info", t, nil, "some message") assert_equal %Q({"type":"info","time":"#{t}","request_id":null,"message":"some message"}\n), result end test "#call accepts and merges a Hash type for the message" do t = Time.now - result = AuditLogFormatter.new.call("info", t, nil, { key: "some message", key2: "another" }) + result = JsonLogFormatter.new.call("info", t, nil, { key: "some message", key2: "another" }) assert_equal %Q({"type":"info","time":"#{t}","request_id":null,"key":"some message","key2":"another"}\n), result end @@ -21,7 +21,7 @@ class AuditLogFormatterTest < ActiveSupport::TestCase t = Time.now req_id = SecureRandom.hex Thread.stub :current, { request_id: req_id } do - result = AuditLogFormatter.new.call("info", t, nil, { key: "some message" }) + result = JsonLogFormatter.new.call("info", t, nil, { key: "some message" }) assert_equal %Q({"type":"info","time":"#{t}","request_id":"#{req_id}","key":"some message"}\n), result end end