From 77c4e9ccdec1f5293623522dbbffb20e6d9b2db3 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Tue, 1 Mar 2022 16:11:20 -0800 Subject: [PATCH 01/22] Proof of Concept: Document analytics events via YARD (LG-5590) - Adds AnalyticsEvents module where we can put explicitly typed events - Example script that uses YARD to parse these and output JSON that we could use to power more accessible documentation --- Gemfile | 1 + Gemfile.lock | 1 + .../account_reset/cancel_controller.rb | 2 +- .../delete_account_controller.rb | 4 +- .../account_reset/request_controller.rb | 2 +- .../grant_requests_and_send_emails.rb | 3 +- app/services/analytics.rb | 3 +- app/services/analytics_events.rb | 55 +++++++++++++++++++ scripts/document-analytics-events | 39 +++++++++++++ .../account_reset/cancel_controller_spec.rb | 12 ++-- .../delete_account_controller_spec.rb | 17 ++---- .../account_reset/request_controller_spec.rb | 9 +-- spec/support/fake_analytics.rb | 2 + 13 files changed, 118 insertions(+), 32 deletions(-) create mode 100644 app/services/analytics_events.rb create mode 100755 scripts/document-analytics-events diff --git a/Gemfile b/Gemfile index 235829577c2..c6b07c6e888 100644 --- a/Gemfile +++ b/Gemfile @@ -68,6 +68,7 @@ gem 'view_component', '~> 2.43.1', require: 'view_component/engine' gem 'webauthn', '~> 2.1' gem 'xmldsig', '~> 0.6' gem 'xmlenc', '~> 0.7', '>= 0.7.1' +gem 'yard' # This version of the zxcvbn gem matches the data and behavior of the zxcvbn NPM package. # It should not be updated without verifying that the behavior still matches JS version 4.4.2. diff --git a/Gemfile.lock b/Gemfile.lock index 0d5bfa95512..70ce9cf0931 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -786,6 +786,7 @@ DEPENDENCIES webmock xmldsig (~> 0.6) xmlenc (~> 0.7, >= 0.7.1) + yard zonebie zxcvbn (= 0.1.7) diff --git a/app/controllers/account_reset/cancel_controller.rb b/app/controllers/account_reset/cancel_controller.rb index 40ffc79b18d..6c71547a1f7 100644 --- a/app/controllers/account_reset/cancel_controller.rb +++ b/app/controllers/account_reset/cancel_controller.rb @@ -26,7 +26,7 @@ def create private def track_event(result) - analytics.track_event(Analytics::ACCOUNT_RESET, result.to_h) + analytics.account_reset(**result.to_h) end def handle_valid_token diff --git a/app/controllers/account_reset/delete_account_controller.rb b/app/controllers/account_reset/delete_account_controller.rb index 1a7aaf52056..6e33dcee820 100644 --- a/app/controllers/account_reset/delete_account_controller.rb +++ b/app/controllers/account_reset/delete_account_controller.rb @@ -4,7 +4,7 @@ def show render :show and return unless token result = AccountReset::ValidateGrantedToken.new(token).call - analytics.track_event(Analytics::ACCOUNT_RESET, result.to_h) + analytics.account_reset(**result.to_h) if result.success? handle_valid_token @@ -16,7 +16,7 @@ def show def delete granted_token = session.delete(:granted_token) result = AccountReset::DeleteAccount.new(granted_token).call - analytics.track_event(Analytics::ACCOUNT_RESET, result.to_h.except(:email)) + analytics.account_reset(**result.to_h.except(:email)) if result.success? handle_successful_deletion(result) diff --git a/app/controllers/account_reset/request_controller.rb b/app/controllers/account_reset/request_controller.rb index f21a5eaef82..1dd9d5f741e 100644 --- a/app/controllers/account_reset/request_controller.rb +++ b/app/controllers/account_reset/request_controller.rb @@ -18,7 +18,7 @@ def create def create_account_reset_request response = AccountReset::CreateRequest.new(current_user).call - analytics.track_event(Analytics::ACCOUNT_RESET, response.to_h.merge(analytics_attributes)) + analytics.account_reset(**response.to_h.merge(analytics_attributes)) end def confirm_two_factor_enabled diff --git a/app/services/account_reset/grant_requests_and_send_emails.rb b/app/services/account_reset/grant_requests_and_send_emails.rb index c44974727f3..07dc7f1eb0f 100644 --- a/app/services/account_reset/grant_requests_and_send_emails.rb +++ b/app/services/account_reset/grant_requests_and_send_emails.rb @@ -23,8 +23,7 @@ def perform(now) notifications_sent += 1 if grant_request_and_send_email(arr) end - analytics.track_event( - Analytics::ACCOUNT_RESET, + analytics.account_reset( event: :notifications, count: notifications_sent, ) diff --git a/app/services/analytics.rb b/app/services/analytics.rb index de21ddd52d8..05729ba1e0c 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Analytics + include AnalyticsEvents + def initialize(user:, request:, sp:, session:, ahoy: nil) @user = user @request = request @@ -120,7 +122,6 @@ def browser_attributes end # rubocop:disable Layout/LineLength - ACCOUNT_RESET = 'Account Reset' ACCOUNT_DELETE_SUBMITTED = 'Account Delete submitted' ACCOUNT_DELETE_VISITED = 'Account Delete visited' ACCOUNT_DELETION = 'Account Deletion Requested' diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb new file mode 100644 index 00000000000..a256eca3fa8 --- /dev/null +++ b/app/services/analytics_events.rb @@ -0,0 +1,55 @@ +module AnalyticsEvents + # @identity.idp.event_name Account Reset + # @param [Boolean] success + # @param ["cancel", "delete", "cancel token validation", "granted token validation", :notifications] event + # @param [String] message_id Request ID from AWS Pinpoint API + # @param [String] request_id Request ID from AWS Pinpoint API + # @param [Boolean] sms_phone + # @param [Boolean] totp does the user have an authentication app as a 2FA option? + # @param [Boolean] piv_cac does the user have PIV/CAC as a 2FA option? + # @param [Integer] count number of notifications sent + # @param [Hash] errors + # @param [String] user_id + # @param [Integer] account_age_in_days + # @param [Hash] mfa_method_counts + # Tracks events related to a user requesting to delete their account during the sign in process + # (because they have no other means to sign in). + def account_reset( + success:, + event: nil, + message_id: nil, + piv_cac: nil, + request_id: nil, + sms_phone: nil, + totp: nil, + count: nil, + errors: nil, + user_id: nil, + account_age_in_days: nil, + mfa_method_counts: nil, + pii_like_keypaths: nil, + error_details: nil, + email_addresses: nil + ) + track_event( + 'Account Reset', + { + success: success, + event: event, + message_id: message_id, + piv_cac: piv_cac, + request_id: request_id, + sms_phone: sms_phone, + totp: totp, + count: count, + errors: errors, + user_id: user_id, + account_age_in_days: account_age_in_days, + mfa_method_counts: mfa_method_counts, + pii_like_keypaths: pii_like_keypaths, + error_details: error_details, + email_addresses: email_addresses, + }.compact, + ) + end +end diff --git a/scripts/document-analytics-events b/scripts/document-analytics-events new file mode 100755 index 00000000000..a1770e722e8 --- /dev/null +++ b/scripts/document-analytics-events @@ -0,0 +1,39 @@ +#!/usr/bin/env ruby + +require 'yard' +require 'json' + +DATABASE_PATH = '.yardoc' + +# figure out how to do this without shelling out +system('bundle', 'exec', 'yard', 'doc', '--db', DATABASE_PATH, 'app/services/analytics_events.rb', out: :err) + +database = YARD::Serializers::YardocSerializer.new(DATABASE_PATH).deserialize('root') + +analytics_methods = database.select do |_k, object| + object.type == :method && object.namespace&.name == :AnalyticsEvents +end.values + +events_json_summary = analytics_methods.map do |method_object| + param_names = method_object.parameters.map { |p| p.first.chomp(':') } + + attributes = method_object.tags('param').map do |tag| + { + name: tag.name, + types: tag.types, + description: tag.text, + } + end.compact + + # could maybe error with these + missing_documentation = attributes.map { |a| a[:name] } - param_names + + + { + event_name: method_object.tag('identity.idp.event_name')&.text, + description: method_object.docstring, + attributes: attributes + } +end + +puts JSON.pretty_generate(events: events_json_summary) diff --git a/spec/controllers/account_reset/cancel_controller_spec.rb b/spec/controllers/account_reset/cancel_controller_spec.rb index 80a35d21018..f8a6bbf20bf 100644 --- a/spec/controllers/account_reset/cancel_controller_spec.rb +++ b/spec/controllers/account_reset/cancel_controller_spec.rb @@ -20,8 +20,7 @@ request_id: 'fake-message-request-id', } - expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, analytics_hash) + expect(@analytics).to receive(:track_event).with('Account Reset', analytics_hash) post :create end @@ -38,8 +37,7 @@ user_id: 'anonymous-uuid', } - expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, analytics_hash) + expect(@analytics).to receive(:track_event).with('Account Reset', analytics_hash) session[:cancel_token] = 'FOO' post :create @@ -55,8 +53,7 @@ user_id: 'anonymous-uuid', } - expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, analytics_hash) + expect(@analytics).to receive(:track_event).with('Account Reset', analytics_hash) post :create end @@ -101,8 +98,7 @@ token: [t('errors.account_reset.cancel_token_invalid', app_name: APP_NAME)], }, } - expect(@analytics). - to receive(:track_event).with(Analytics::ACCOUNT_RESET, properties) + expect(@analytics).to receive(:track_event).with('Account Reset', properties) get :show, params: { token: 'FOO' } diff --git a/spec/controllers/account_reset/delete_account_controller_spec.rb b/spec/controllers/account_reset/delete_account_controller_spec.rb index f4bb35cfa0f..c787b06f0b0 100644 --- a/spec/controllers/account_reset/delete_account_controller_spec.rb +++ b/spec/controllers/account_reset/delete_account_controller_spec.rb @@ -23,7 +23,7 @@ account_age_in_days: 0, } expect(@analytics). - to receive(:track_event).with(Analytics::ACCOUNT_RESET, properties) + to receive(:track_event).with('Account Reset', properties) delete :delete @@ -45,8 +45,7 @@ pii_like_keypaths: [[:mfa_method_counts, :phone]], account_age_in_days: 0, } - expect(@analytics). - to receive(:track_event).with(Analytics::ACCOUNT_RESET, properties) + expect(@analytics).to receive(:track_event).with('Account Reset', properties) delete :delete @@ -68,8 +67,7 @@ pii_like_keypaths: [[:mfa_method_counts, :phone]], account_age_in_days: 0, } - expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, properties) + expect(@analytics).to receive(:track_event).with('Account Reset', properties) delete :delete @@ -98,8 +96,7 @@ pii_like_keypaths: [[:mfa_method_counts, :phone]], account_age_in_days: 2, } - expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, properties) + expect(@analytics).to receive(:track_event).with('Account Reset', properties) travel_to(Time.zone.now + 2.days) do session[:granted_token] = AccountResetRequest.all[0].granted_token @@ -125,8 +122,7 @@ token: [t('errors.account_reset.granted_token_invalid', app_name: APP_NAME)], }, } - expect(@analytics). - to receive(:track_event).with(Analytics::ACCOUNT_RESET, properties) + expect(@analytics).to receive(:track_event).with('Account Reset', properties) get :show, params: { token: 'FOO' } @@ -151,8 +147,7 @@ token: [t('errors.account_reset.granted_token_expired', app_name: APP_NAME)], }, } - expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, properties) + expect(@analytics).to receive(:track_event).with('Account Reset', properties) travel_to(Time.zone.now + 2.days) do get :show, params: { token: AccountResetRequest.all[0].granted_token } diff --git a/spec/controllers/account_reset/request_controller_spec.rb b/spec/controllers/account_reset/request_controller_spec.rb index b9880b3ce25..dd057fa5cc9 100644 --- a/spec/controllers/account_reset/request_controller_spec.rb +++ b/spec/controllers/account_reset/request_controller_spec.rb @@ -48,8 +48,7 @@ email_addresses: 1, errors: {}, } - expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, attributes) + expect(@analytics).to receive(:track_event).with('Account Reset', attributes) post :create end @@ -70,8 +69,7 @@ message_id: 'fake-message-id', errors: {}, } - expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, attributes) + expect(@analytics).to receive(:track_event).with('Account Reset', attributes) post :create end @@ -90,8 +88,7 @@ email_addresses: 1, errors: {}, } - expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, attributes) + expect(@analytics).to receive(:track_event).with('Account Reset', attributes) post :create end diff --git a/spec/support/fake_analytics.rb b/spec/support/fake_analytics.rb index 151faf91220..af4939d15da 100644 --- a/spec/support/fake_analytics.rb +++ b/spec/support/fake_analytics.rb @@ -1,6 +1,8 @@ class FakeAnalytics PiiDetected = Class.new(StandardError) + include AnalyticsEvents + module PiiAlerter def track_event(event, original_attributes = {}) attributes = original_attributes.dup From 78a13b04a49ed90596eae08cc85ca03e943dddea Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Tue, 1 Mar 2022 17:48:00 -0800 Subject: [PATCH 02/22] optional --- app/services/analytics_events.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index a256eca3fa8..b155aa9a104 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -15,7 +15,7 @@ module AnalyticsEvents # Tracks events related to a user requesting to delete their account during the sign in process # (because they have no other means to sign in). def account_reset( - success:, + success: nil, event: nil, message_id: nil, piv_cac: nil, From 1085051518d23af074ca44a415e84d5d7f1cd583 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 09:26:28 -0800 Subject: [PATCH 03/22] Migrate ACCOUNT_DELETE_SUBMITTED --- app/controllers/users/delete_controller.rb | 4 ++-- app/services/analytics.rb | 1 - app/services/analytics_events.rb | 7 +++++++ spec/controllers/users/delete_controller_spec.rb | 4 ++-- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/controllers/users/delete_controller.rb b/app/controllers/users/delete_controller.rb index aedbdab73d9..0a6f3da40d0 100644 --- a/app/controllers/users/delete_controller.rb +++ b/app/controllers/users/delete_controller.rb @@ -12,7 +12,7 @@ def delete delete_user sign_out flash[:success] = t('devise.registrations.destroyed') - analytics.track_event(Analytics::ACCOUNT_DELETE_SUBMITTED, success: true) + analytics.account_delete_submitted(success: true) redirect_to root_url end @@ -29,7 +29,7 @@ def confirm_current_password return if valid_password? flash[:error] = t('idv.errors.incorrect_password') - analytics.track_event(Analytics::ACCOUNT_DELETE_SUBMITTED, success: false) + analytics.account_delete_submitted(success: false) render :show end diff --git a/app/services/analytics.rb b/app/services/analytics.rb index 05729ba1e0c..8f8ecd20726 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -122,7 +122,6 @@ def browser_attributes end # rubocop:disable Layout/LineLength - ACCOUNT_DELETE_SUBMITTED = 'Account Delete submitted' ACCOUNT_DELETE_VISITED = 'Account Delete visited' ACCOUNT_DELETION = 'Account Deletion Requested' ACCOUNT_RESET_VISIT = 'Account deletion and reset visited' diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index b155aa9a104..f0754e41d64 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -52,4 +52,11 @@ def account_reset( }.compact, ) end + + # @identity.idp.event_name Account Delete submitted + # When a user submits a form to delete their account + # @param [Boolean] success + def account_delete_submitted(success:) + track_event('Account Delete submitted', success: success) + end end diff --git a/spec/controllers/users/delete_controller_spec.rb b/spec/controllers/users/delete_controller_spec.rb index b0104a2c526..3fb06b80596 100644 --- a/spec/controllers/users/delete_controller_spec.rb +++ b/spec/controllers/users/delete_controller_spec.rb @@ -41,7 +41,7 @@ stub_signed_in_user expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_DELETE_SUBMITTED, success: false) + with(Account Delete submitted, success: false) delete end @@ -65,7 +65,7 @@ stub_signed_in_user expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_DELETE_SUBMITTED, success: true) + with(Account Delete submitted, success: true) delete end From cbe37c241d6365c8b9dc30a67549d0f9bce308b4 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 13:16:07 -0800 Subject: [PATCH 04/22] Use Makefile to generate the JSON --- .gitignore | 1 + Makefile | 34 ++++++++++++++++++++++++++++++- scripts/document-analytics-events | 7 ++----- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index a74947fceec..19c77fdcac6 100644 --- a/.gitignore +++ b/.gitignore @@ -69,6 +69,7 @@ Vagrantfile /.tmp/ /.vscode/ /vendor/bundle +.yardoc package-lock.json diff --git a/Makefile b/Makefile index 4889facd49f..01f2b8925c4 100644 --- a/Makefile +++ b/Makefile @@ -10,7 +10,30 @@ PORT ?= 3000 GZIP_COMMAND ?= gzip ARTIFACT_DESTINATION_FILE ?= ./tmp/idp.tar.gz -.PHONY: brakeman check check_asset_strings docker_setup fast_setup fast_test help lint lint_country_dialing_codes lint_erb lint_optimized_assets lint_yaml lintfix normalize_yaml optimize_assets optimize_svg run run setup test update_pinpoint_supported_countries build_artifact +.PHONY: \ + analytics_events \ + brakeman \ + build_artifact \ + check \ + check_asset_strings \ + docker_setup \ + fast_setup \ + fast_test \ + help \ + lint \ + lint_country_dialing_codes \ + lint_erb \ + lint_optimized_assets \ + lint_yaml \ + lintfix \ + normalize_yaml \ + optimize_assets \ + optimize_svg \ + run \ + run \ + setup \ + test \ + update_pinpoint_supported_countries help: ## Show this help @echo "--- Help ---" @@ -158,3 +181,12 @@ build_artifact $(ARTIFACT_DESTINATION_FILE): ## Builds zipped tar file artifact --exclude='./vendor/ruby' \ --exclude='./config/application.yml' \ -cf - "." | "$(GZIP_COMMAND)" > $(ARTIFACT_DESTINATION_FILE) + +analytics_events: public/api/analytics_events.json + +public/api/analytics_events.json: .yardoc .yardoc/objects/root.dat + mkdir -p public/api + ./scripts/document-analytics-events $< > $@ + +.yardoc .yardoc/objects/root.dat: app/services/analytics_events.rb + bundle exec yard doc --db $@ -- $< diff --git a/scripts/document-analytics-events b/scripts/document-analytics-events index a1770e722e8..a7e191c8be7 100755 --- a/scripts/document-analytics-events +++ b/scripts/document-analytics-events @@ -3,12 +3,9 @@ require 'yard' require 'json' -DATABASE_PATH = '.yardoc' +database_path = ARGV.first || '.yardoc' -# figure out how to do this without shelling out -system('bundle', 'exec', 'yard', 'doc', '--db', DATABASE_PATH, 'app/services/analytics_events.rb', out: :err) - -database = YARD::Serializers::YardocSerializer.new(DATABASE_PATH).deserialize('root') +database = YARD::Serializers::YardocSerializer.new(database_path).deserialize('root') analytics_methods = database.select do |_k, object| object.type == :method && object.namespace&.name == :AnalyticsEvents From dd7575720a1e06508c3f71401c5e127836124562 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 13:21:31 -0800 Subject: [PATCH 05/22] Migrate ACCOUNT_DELETE_VISITED --- app/controllers/users/delete_controller.rb | 2 +- app/services/analytics.rb | 1 - app/services/analytics_events.rb | 8 +++++++- spec/controllers/users/delete_controller_spec.rb | 3 +-- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/controllers/users/delete_controller.rb b/app/controllers/users/delete_controller.rb index 0a6f3da40d0..545b2693bd6 100644 --- a/app/controllers/users/delete_controller.rb +++ b/app/controllers/users/delete_controller.rb @@ -4,7 +4,7 @@ class DeleteController < ApplicationController before_action :confirm_current_password, only: [:delete] def show - analytics.track_event(Analytics::ACCOUNT_DELETE_VISITED) + analytics.account_delete_visited end def delete diff --git a/app/services/analytics.rb b/app/services/analytics.rb index 8f8ecd20726..6925a1223f1 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -122,7 +122,6 @@ def browser_attributes end # rubocop:disable Layout/LineLength - ACCOUNT_DELETE_VISITED = 'Account Delete visited' ACCOUNT_DELETION = 'Account Deletion Requested' ACCOUNT_RESET_VISIT = 'Account deletion and reset visited' ACCOUNT_VISIT = 'Account Page Visited' diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index f0754e41d64..ad3bea05b19 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -1,7 +1,8 @@ module AnalyticsEvents # @identity.idp.event_name Account Reset # @param [Boolean] success - # @param ["cancel", "delete", "cancel token validation", "granted token validation", :notifications] event + # @param ["cancel", "delete", "cancel token validation", "granted token validation", + # :notifications] event # @param [String] message_id Request ID from AWS Pinpoint API # @param [String] request_id Request ID from AWS Pinpoint API # @param [Boolean] sms_phone @@ -59,4 +60,9 @@ def account_reset( def account_delete_submitted(success:) track_event('Account Delete submitted', success: success) end + + # @identity.idp.event_name Account Delete visited + def account_delete_visited + track_event('Account Delete visited') + end end diff --git a/spec/controllers/users/delete_controller_spec.rb b/spec/controllers/users/delete_controller_spec.rb index 3fb06b80596..47007b2dd44 100644 --- a/spec/controllers/users/delete_controller_spec.rb +++ b/spec/controllers/users/delete_controller_spec.rb @@ -8,8 +8,7 @@ stub_analytics stub_signed_in_user - expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_DELETE_VISITED) + expect(@analytics).to receive(:track_event).with('Account Delete visited') get :show From a28fc6400ff847ead2425bf333cca607f89cf520 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 13:21:58 -0800 Subject: [PATCH 06/22] fixup ACCOUNT_DELETE_SUBMITTED --- spec/controllers/users/delete_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/controllers/users/delete_controller_spec.rb b/spec/controllers/users/delete_controller_spec.rb index 47007b2dd44..a10d7565f86 100644 --- a/spec/controllers/users/delete_controller_spec.rb +++ b/spec/controllers/users/delete_controller_spec.rb @@ -40,7 +40,7 @@ stub_signed_in_user expect(@analytics).to receive(:track_event). - with(Account Delete submitted, success: false) + with('Account Delete submitted', success: false) delete end @@ -64,7 +64,7 @@ stub_signed_in_user expect(@analytics).to receive(:track_event). - with(Account Delete submitted, success: true) + with('Account Delete submitted', success: true) delete end From 0b54508a48f33b3404aff2ca151a4b9506117dbe Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 13:24:40 -0800 Subject: [PATCH 07/22] Makefile help line --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 01f2b8925c4..9677bc03aac 100644 --- a/Makefile +++ b/Makefile @@ -182,7 +182,7 @@ build_artifact $(ARTIFACT_DESTINATION_FILE): ## Builds zipped tar file artifact --exclude='./config/application.yml' \ -cf - "." | "$(GZIP_COMMAND)" > $(ARTIFACT_DESTINATION_FILE) -analytics_events: public/api/analytics_events.json +analytics_events: public/api/analytics_events.json ## Generates a JSON file that documents analytics events for events.log public/api/analytics_events.json: .yardoc .yardoc/objects/root.dat mkdir -p public/api From 2d4f2c6d5f7081146ee80c537fb5071544b39714 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 15:16:12 -0800 Subject: [PATCH 08/22] Add separate lint and output commands, factor into a class --- Makefile | 10 +- lib/analytics_events_documenter.rb | 127 +++++++++++++++++++ scripts/document-analytics-events | 36 ------ spec/lib/analytics_events_documenter_spec.rb | 112 ++++++++++++++++ 4 files changed, 247 insertions(+), 38 deletions(-) create mode 100644 lib/analytics_events_documenter.rb delete mode 100755 scripts/document-analytics-events create mode 100644 spec/lib/analytics_events_documenter_spec.rb diff --git a/Makefile b/Makefile index 9677bc03aac..20e976b806e 100644 --- a/Makefile +++ b/Makefile @@ -21,6 +21,7 @@ ARTIFACT_DESTINATION_FILE ?= ./tmp/idp.tar.gz fast_test \ help \ lint \ + lint_analytics_events \ lint_country_dialing_codes \ lint_erb \ lint_optimized_assets \ @@ -58,6 +59,8 @@ lint: ## Runs all lint tests make lint_erb @echo "--- rubocop ---" bundle exec rubocop --parallel + @echo "--- analytics_events ---" + make lint_analytics_events @echo "--- brakeman ---" bundle exec brakeman @echo "--- zeitwerk check ---" @@ -184,9 +187,12 @@ build_artifact $(ARTIFACT_DESTINATION_FILE): ## Builds zipped tar file artifact analytics_events: public/api/analytics_events.json ## Generates a JSON file that documents analytics events for events.log +lint_analytics_events: .yardoc + @ruby lib/analytics_events_documenter.rb --check $< + public/api/analytics_events.json: .yardoc .yardoc/objects/root.dat mkdir -p public/api - ./scripts/document-analytics-events $< > $@ + @ruby lib/analytics_events_documenter.rb --json $< > $@ .yardoc .yardoc/objects/root.dat: app/services/analytics_events.rb - bundle exec yard doc --db $@ -- $< + bundle exec yard doc --type-tag identity.idp.event_name:"Event Name" --db $@ -- $< diff --git a/lib/analytics_events_documenter.rb b/lib/analytics_events_documenter.rb new file mode 100644 index 00000000000..eb91733795c --- /dev/null +++ b/lib/analytics_events_documenter.rb @@ -0,0 +1,127 @@ +#!/usr/bin/env ruby +require 'yard' +require 'json' +require 'optparse' +require 'stringio' +require 'active_support/core_ext/object/blank' + +# Parses YARD output for AnalyticsEvents methods +class AnalyticsEventsDocumenter + DEFAULT_DATABASE_PATH = '.yardoc' + EVENT_NAME_TAG = 'identity.idp.event_name' + + DOCUMENTATION_OPTIONAL_PARAMS = %w[ + pii_like_keypaths + ] + + attr_reader :database_path + + # @return [(String, Integer)] returns a tuple of (output, exit_status) + def self.run(argv) + exit_status = 0 + output = StringIO.new + check = false + json = false + help = false + + parser = OptionParser.new do |opts| + opts.on('--check', 'Checks that all params are documented, will exit 1 if missing') do + check = true + end + + opts.on('--json') do + json = true + end + + opts.on('--help', 'print this help message') do + help = true + end + end + + parser.parse!(argv) + + documenter = new(argv.first) + + if help || (!check && !json) + output.puts parser + elsif check + missing_documentation = documenter.missing_documentation + output.puts missing_documentation + + exit_status = 1 if missing_documentation.present? + elsif json + output.puts JSON.pretty_generate(documenter.as_json) + end + + [ output.string.presence, exit_status ] + end + + def initialize(database_path) + @database_path = database_path || DEFAULT_DATABASE_PATH + end + + # Checks for params that are missing documentation, and returns a list of + # @return [Array] + def missing_documentation + analytics_methods.flat_map do |method_object| + param_names = method_object.parameters.map { |p| p.first.chomp(':') } + documented_params = method_object.tags('param').map(&:name) + missing_attributes = param_names - documented_params - DOCUMENTATION_OPTIONAL_PARAMS + + error_prefix = "#{method_object.file}:#{method_object.line} #{method_object.name}" + errors = [] + + errors << "#{error_prefix} missing @#{EVENT_NAME_TAG}" if !method_object.tag(EVENT_NAME_TAG) + + missing_attributes.each do |attribute| + errors << "#{error_prefix} #{attribute} (undocumented)" + end + + errors + end + end + + # @return [{ events: Array}] + def as_json + events_json_summary = analytics_methods.map do |method_object| + param_names = method_object.parameters.map { |p| p.first.chomp(':') } + + attributes = method_object.tags('param').map do |tag| + { + name: tag.name, + types: tag.types, + description: tag.text.presence, + } + end.compact + + { + event_name: method_object.tag(EVENT_NAME_TAG)&.text, + description: method_object.docstring.presence, + attributes: attributes + } + end + + { events: events_json_summary } + end + + private + + def database + @database ||= YARD::Serializers::YardocSerializer.new(database_path).deserialize('root') + end + + # @return [Array] + def analytics_methods + database.select do |_k, object| + object.type == :method && object.namespace&.name == :AnalyticsEvents + end.values + end +end + +# rubocop:disable Rails/Output +if $PROGRAM_NAME == __FILE__ + output, status = AnalyticsEventsDocumenter.run(ARGV) + puts output + exit status +end +# rubocop:enable Rails/Output diff --git a/scripts/document-analytics-events b/scripts/document-analytics-events deleted file mode 100755 index a7e191c8be7..00000000000 --- a/scripts/document-analytics-events +++ /dev/null @@ -1,36 +0,0 @@ -#!/usr/bin/env ruby - -require 'yard' -require 'json' - -database_path = ARGV.first || '.yardoc' - -database = YARD::Serializers::YardocSerializer.new(database_path).deserialize('root') - -analytics_methods = database.select do |_k, object| - object.type == :method && object.namespace&.name == :AnalyticsEvents -end.values - -events_json_summary = analytics_methods.map do |method_object| - param_names = method_object.parameters.map { |p| p.first.chomp(':') } - - attributes = method_object.tags('param').map do |tag| - { - name: tag.name, - types: tag.types, - description: tag.text, - } - end.compact - - # could maybe error with these - missing_documentation = attributes.map { |a| a[:name] } - param_names - - - { - event_name: method_object.tag('identity.idp.event_name')&.text, - description: method_object.docstring, - attributes: attributes - } -end - -puts JSON.pretty_generate(events: events_json_summary) diff --git a/spec/lib/analytics_events_documenter_spec.rb b/spec/lib/analytics_events_documenter_spec.rb new file mode 100644 index 00000000000..a62e1c2adab --- /dev/null +++ b/spec/lib/analytics_events_documenter_spec.rb @@ -0,0 +1,112 @@ +require 'spec_helper' +require 'analytics_events_documenter' +require 'tempfile' + +RSpec.describe AnalyticsEventsDocumenter do + around do |ex| + Dir.mktmpdir('.yardoc') do |database_dir| + @database_dir = database_dir + + YARD::Registry.clear + YARD::Tags::Library.define_tag("Event Name", :'identity.idp.event_name') + YARD.parse_string(source_code) + YARD::Registry.save(false, database_dir) + + ex.run + end + end + + subject(:documenter) { AnalyticsEventsDocumenter.new(@database_dir) } + + describe '#missing_documentation' do + context 'when all methods have all documentation' do + let(:source_code) { <<~RUBY } + class AnalyticsEvents + # @identity.idp.event_name Some Event + # @param [Boolean] success + def some_event(success:) + end + end + RUBY + + it 'is empty' do + expect(documenter.missing_documentation).to be_empty + end + end + + context 'when a method is missing the event_name tag' do + let(:source_code) { <<~RUBY } + class AnalyticsEvents + def some_event; end + end + RUBY + + it 'reports the missing tag' do + expect(documenter.missing_documentation.first). + to include('some_event missing @identity.idp.event_name') + end + end + + context 'when a method is missing documentation for a param' do + let(:source_code) { <<~RUBY } + class AnalyticsEvents + # @identity.idp.event_name Some Event + def some_event(success:); end + end + RUBY + + it 'reports the missing tag' do + expect(documenter.missing_documentation.first). + to include('some_event success (undocumented)') + end + end + + context 'when a method is skips documenting an param, such as pii_like_keypaths' do + let(:source_code) { <<~RUBY } + class AnalyticsEvents + # @identity.idp.event_name Some Event + def some_event(pii_like_keypaths:); end + end + RUBY + + it 'allow documentation to be missing' do + expect(documenter.missing_documentation).to be_empty + end + end + end + + describe '#as_json' do + let(:source_code) { <<~RUBY } + class AnalyticsEvents + # @identity.idp.event_name Some Event + # @param [Boolean] success + # @param [Integer] count number of attempts + # The event that does something with stuff + def some_event(success:, count:); end + + # @identity.idp.event_name Other Event + def other_event; end + end + RUBY + + it 'is a JSON representation of params for each event' do + expect(documenter.as_json[:events]).to match_array( + [ + { + event_name: 'Some Event', + description: 'The event that does something with stuff', + attributes: [ + { name: 'success', types: ['Boolean'], description: nil }, + { name: 'count', types: ['Integer'], description: 'number of attempts' }, + ], + }, + { + event_name: 'Other Event', + description: nil, + attributes: [], + }, + ], + ) + end + end +end From b9b57407b9158cffcda9306466646877b3207fb1 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 15:18:48 -0800 Subject: [PATCH 09/22] rubocops --- lib/analytics_events_documenter.rb | 4 +++- spec/lib/analytics_events_documenter_spec.rb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/analytics_events_documenter.rb b/lib/analytics_events_documenter.rb index eb91733795c..da91c60f3f2 100644 --- a/lib/analytics_events_documenter.rb +++ b/lib/analytics_events_documenter.rb @@ -97,7 +97,7 @@ def as_json { event_name: method_object.tag(EVENT_NAME_TAG)&.text, description: method_object.docstring.presence, - attributes: attributes + attributes: attributes, } end @@ -119,9 +119,11 @@ def analytics_methods end # rubocop:disable Rails/Output +# rubocop:disable Rails/Exit if $PROGRAM_NAME == __FILE__ output, status = AnalyticsEventsDocumenter.run(ARGV) puts output exit status end +# rubocop:enable Rails/Exit # rubocop:enable Rails/Output diff --git a/spec/lib/analytics_events_documenter_spec.rb b/spec/lib/analytics_events_documenter_spec.rb index a62e1c2adab..d2b4b06976f 100644 --- a/spec/lib/analytics_events_documenter_spec.rb +++ b/spec/lib/analytics_events_documenter_spec.rb @@ -8,7 +8,7 @@ @database_dir = database_dir YARD::Registry.clear - YARD::Tags::Library.define_tag("Event Name", :'identity.idp.event_name') + YARD::Tags::Library.define_tag('Event Name', :'identity.idp.event_name') YARD.parse_string(source_code) YARD::Registry.save(false, database_dir) From 778eeaa119d5c7f65aabb5ac22d7bb67d3b405e1 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 15:22:38 -0800 Subject: [PATCH 10/22] Update analytics lint command and fix analytics lint errors --- Makefile | 2 +- app/services/analytics_events.rb | 2 ++ lib/analytics_events_documenter.rb | 9 ++++++--- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 20e976b806e..9cb9861ca27 100644 --- a/Makefile +++ b/Makefile @@ -187,7 +187,7 @@ build_artifact $(ARTIFACT_DESTINATION_FILE): ## Builds zipped tar file artifact analytics_events: public/api/analytics_events.json ## Generates a JSON file that documents analytics events for events.log -lint_analytics_events: .yardoc +lint_analytics_events: .yardoc # Checks that all methods on AnalyticsEvents are documented @ruby lib/analytics_events_documenter.rb --check $< public/api/analytics_events.json: .yardoc .yardoc/objects/root.dat diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index ad3bea05b19..6c0c0adcffc 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -10,9 +10,11 @@ module AnalyticsEvents # @param [Boolean] piv_cac does the user have PIV/CAC as a 2FA option? # @param [Integer] count number of notifications sent # @param [Hash] errors + # @param [Hash] error_details # @param [String] user_id # @param [Integer] account_age_in_days # @param [Hash] mfa_method_counts + # @param [Integer] email_addresses number of email addresses the user has # Tracks events related to a user requesting to delete their account during the sign in process # (because they have no other means to sign in). def account_reset( diff --git a/lib/analytics_events_documenter.rb b/lib/analytics_events_documenter.rb index da91c60f3f2..90b3e9caff5 100644 --- a/lib/analytics_events_documenter.rb +++ b/lib/analytics_events_documenter.rb @@ -46,9 +46,12 @@ def self.run(argv) output.puts parser elsif check missing_documentation = documenter.missing_documentation - output.puts missing_documentation - - exit_status = 1 if missing_documentation.present? + if missing_documentation.present? + output.puts missing_documentation + exit_status = 1 + else + output.puts 'All AnalyticsEvents methods are documented! 🚀' + end elsif json output.puts JSON.pretty_generate(documenter.as_json) end From 5cfb74605b4fbec873a2020a9bd423ea879b97ea Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 16:01:40 -0800 Subject: [PATCH 11/22] Bundle exec stuff --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 9cb9861ca27..4ca2f50beaf 100644 --- a/Makefile +++ b/Makefile @@ -188,11 +188,11 @@ build_artifact $(ARTIFACT_DESTINATION_FILE): ## Builds zipped tar file artifact analytics_events: public/api/analytics_events.json ## Generates a JSON file that documents analytics events for events.log lint_analytics_events: .yardoc # Checks that all methods on AnalyticsEvents are documented - @ruby lib/analytics_events_documenter.rb --check $< + bundle exec ruby lib/analytics_events_documenter.rb --check $< public/api/analytics_events.json: .yardoc .yardoc/objects/root.dat mkdir -p public/api - @ruby lib/analytics_events_documenter.rb --json $< > $@ + bundle exec ruby lib/analytics_events_documenter.rb --json $< > $@ .yardoc .yardoc/objects/root.dat: app/services/analytics_events.rb bundle exec yard doc --type-tag identity.idp.event_name:"Event Name" --db $@ -- $< From 75dd072da96593452095d4bc7dbfea5a280bd0c2 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 16:01:58 -0800 Subject: [PATCH 12/22] Add changelog changelog: Internal, Documentation, Add documentation for analytics events in events.log From d5628147a2e83795ce2b829fa378a11d899111e5 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 16:32:42 -0800 Subject: [PATCH 13/22] Add additional code coverage --- spec/lib/analytics_events_documenter_spec.rb | 46 ++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/spec/lib/analytics_events_documenter_spec.rb b/spec/lib/analytics_events_documenter_spec.rb index d2b4b06976f..467d463fddf 100644 --- a/spec/lib/analytics_events_documenter_spec.rb +++ b/spec/lib/analytics_events_documenter_spec.rb @@ -18,6 +18,52 @@ subject(:documenter) { AnalyticsEventsDocumenter.new(@database_dir) } + describe '.run' do + let(:source_code) { <<~RUBY } + class AnalyticsEvents + # @identity.idp.event_name Some Event + # @param [Boolean] success + def some_event(success:) + end + end + RUBY + + subject(:run) { AnalyticsEventsDocumenter.run(args) } + + context 'with --help' do + let(:args) { ['--help'] } + + it 'prints help' do + output, status = run + + expect(output).to include('Usage') + expect(status).to eq(0) + end + end + + context 'with --check' do + let(:args) { ['--check', @database_dir] } + + it 'prints a rocket when there are no errors' do + output, status = run + + expect(output).to include('🚀') + expect(status).to eq(0) + end + end + + context 'with --json' do + let(:args) { ['--json', @database_dir] } + + it 'prints json output' do + output, status = run + + expect(JSON.parse(output, symbolize_names: true)).to have_key(:events) + expect(status).to eq(0) + end + end + end + describe '#missing_documentation' do context 'when all methods have all documentation' do let(:source_code) { <<~RUBY } From cdeca156a6618f0af8be199847075d6a7fd903e6 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 2 Mar 2022 16:38:10 -0800 Subject: [PATCH 14/22] Switch to dasherized /api/analytics-events endpoint, add to CORS allowlist --- Makefile | 4 ++-- config/application.rb | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 4ca2f50beaf..4ef8eb513a7 100644 --- a/Makefile +++ b/Makefile @@ -185,12 +185,12 @@ build_artifact $(ARTIFACT_DESTINATION_FILE): ## Builds zipped tar file artifact --exclude='./config/application.yml' \ -cf - "." | "$(GZIP_COMMAND)" > $(ARTIFACT_DESTINATION_FILE) -analytics_events: public/api/analytics_events.json ## Generates a JSON file that documents analytics events for events.log +analytics_events: public/api/analytics-events.json ## Generates a JSON file that documents analytics events for events.log lint_analytics_events: .yardoc # Checks that all methods on AnalyticsEvents are documented bundle exec ruby lib/analytics_events_documenter.rb --check $< -public/api/analytics_events.json: .yardoc .yardoc/objects/root.dat +public/api/analytics-events.json: .yardoc .yardoc/objects/root.dat mkdir -p public/api bundle exec ruby lib/analytics_events_documenter.rb --json $< > $@ diff --git a/config/application.rb b/config/application.rb index e00485b1284..416aac06e2c 100644 --- a/config/application.rb +++ b/config/application.rb @@ -141,6 +141,7 @@ class Application < Rails::Application origins allowed_origins resource '/api/country-support', headers: :any, methods: [:get] + resource '/api/analytics-events.json', headers: :any, methods: [:get] end end # rubocop:enable Metrics/BlockLength From 13da56d32398016d242d67afb802ce10153f143e Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 3 Mar 2022 08:29:10 -0800 Subject: [PATCH 15/22] gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 19c77fdcac6..51c40b0e462 100644 --- a/.gitignore +++ b/.gitignore @@ -59,6 +59,7 @@ Vagrantfile /kitchen/cookbooks /log/* /postgres-data +/public/api /public/system /public/user_flows /pwned_passwords/* From 531b95d2f89f147297e5fbd42cf959fc7710ee39 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 3 Mar 2022 11:16:26 -0800 Subject: [PATCH 16/22] eslint ignore doc dir --- .eslintignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.eslintignore b/.eslintignore index 4e5e9406277..7dc0e1a58e5 100644 --- a/.eslintignore +++ b/.eslintignore @@ -3,3 +3,4 @@ node_modules public vendor coverage +doc From 24ba2bcae4fa938ce0937b6dbaa386f9cc455ae5 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 3 Mar 2022 12:15:18 -0800 Subject: [PATCH 17/22] build analytics_events as part of artifact compilation --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 4ef8eb513a7..aa5da66f2ab 100644 --- a/Makefile +++ b/Makefile @@ -161,7 +161,7 @@ lint_country_dialing_codes: update_pinpoint_supported_countries ## Checks that c check_asset_strings: ## Checks for strings find ./app/javascript -name "*.js*" | xargs ./scripts/check-assets -build_artifact $(ARTIFACT_DESTINATION_FILE): ## Builds zipped tar file artifact with IDP source code and Ruby/JS dependencies +build_artifact $(ARTIFACT_DESTINATION_FILE): analytics_events ## Builds zipped tar file artifact with IDP source code and Ruby/JS dependencies @echo "Building artifact into $(ARTIFACT_DESTINATION_FILE)" bundle config set --local cache_all true bundle package From 1c8a239bf81b2d849876099d114703be451f8e1f Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 3 Mar 2022 12:23:25 -0800 Subject: [PATCH 18/22] Move JSON building to build-post-config --- Makefile | 2 +- deploy/build-post-config | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index aa5da66f2ab..4ef8eb513a7 100644 --- a/Makefile +++ b/Makefile @@ -161,7 +161,7 @@ lint_country_dialing_codes: update_pinpoint_supported_countries ## Checks that c check_asset_strings: ## Checks for strings find ./app/javascript -name "*.js*" | xargs ./scripts/check-assets -build_artifact $(ARTIFACT_DESTINATION_FILE): analytics_events ## Builds zipped tar file artifact with IDP source code and Ruby/JS dependencies +build_artifact $(ARTIFACT_DESTINATION_FILE): ## Builds zipped tar file artifact with IDP source code and Ruby/JS dependencies @echo "Building artifact into $(ARTIFACT_DESTINATION_FILE)" bundle config set --local cache_all true bundle package diff --git a/deploy/build-post-config b/deploy/build-post-config index e7866f88ce1..e0341ce6eee 100755 --- a/deploy/build-post-config +++ b/deploy/build-post-config @@ -22,6 +22,8 @@ bundle exec rake assets:precompile bundle exec bin/copy_robots_file +make analytics_events + set +x echo "deploy/build-post-config finished" From 7377ed09f36334987e0ff58624f83f00becad38c Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 3 Mar 2022 14:52:12 -0800 Subject: [PATCH 19/22] Serve the analytics events data from Rails so that the CORS middleware wraps it --- Makefile | 4 +- .../analytics_events_controller.rb | 18 ++++++++ config/application.rb | 2 +- config/routes.rb | 1 + .../analytics_events_controller_spec.rb | 42 +++++++++++++++++++ ..._support_cors_spec.rb => api_cors_spec.rb} | 30 ++++++++++--- 6 files changed, 89 insertions(+), 8 deletions(-) create mode 100644 app/controllers/analytics_events_controller.rb create mode 100644 spec/controllers/analytics_events_controller_spec.rb rename spec/requests/{country_support_cors_spec.rb => api_cors_spec.rb} (80%) diff --git a/Makefile b/Makefile index 4ef8eb513a7..cefe2babe11 100644 --- a/Makefile +++ b/Makefile @@ -185,12 +185,12 @@ build_artifact $(ARTIFACT_DESTINATION_FILE): ## Builds zipped tar file artifact --exclude='./config/application.yml' \ -cf - "." | "$(GZIP_COMMAND)" > $(ARTIFACT_DESTINATION_FILE) -analytics_events: public/api/analytics-events.json ## Generates a JSON file that documents analytics events for events.log +analytics_events: public/api/_analytics-events.json ## Generates a JSON file that documents analytics events for events.log lint_analytics_events: .yardoc # Checks that all methods on AnalyticsEvents are documented bundle exec ruby lib/analytics_events_documenter.rb --check $< -public/api/analytics-events.json: .yardoc .yardoc/objects/root.dat +public/api/_analytics-events.json: .yardoc .yardoc/objects/root.dat mkdir -p public/api bundle exec ruby lib/analytics_events_documenter.rb --json $< > $@ diff --git a/app/controllers/analytics_events_controller.rb b/app/controllers/analytics_events_controller.rb new file mode 100644 index 00000000000..69df1573fe7 --- /dev/null +++ b/app/controllers/analytics_events_controller.rb @@ -0,0 +1,18 @@ +# Serve a static file from Rails so that the CORS middleware can add the correct headers +class AnalyticsEventsController < ApplicationController + prepend_before_action :skip_session_load + prepend_before_action :skip_session_expiration + skip_before_action :disable_caching + + JSON_FILE = Rails.root.join('public', 'api', '_analytics-events.json') + + def index + if File.exist?(JSON_FILE) + expires_in 15.minutes, public: true + + send_file JSON_FILE, type: 'application/json', disposition: 'inline' + else + render_not_found + end + end +end diff --git a/config/application.rb b/config/application.rb index 416aac06e2c..4e4743d59f3 100644 --- a/config/application.rb +++ b/config/application.rb @@ -140,8 +140,8 @@ class Application < Rails::Application end origins allowed_origins + resource '/api/analytics-events', headers: :any, methods: [:get] resource '/api/country-support', headers: :any, methods: [:get] - resource '/api/analytics-events.json', headers: :any, methods: [:get] end end # rubocop:enable Metrics/BlockLength diff --git a/config/routes.rb b/config/routes.rb index 9ab7a834b16..dff9442502b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,5 +1,6 @@ Rails.application.routes.draw do # Non i18n routes. Alphabetically sorted. + get '/api/analytics-events' => 'analytics_events#index' get '/api/country-support' => 'country_support#index' get '/api/health' => 'health/health#index' get '/api/health/database' => 'health/database#index' diff --git a/spec/controllers/analytics_events_controller_spec.rb b/spec/controllers/analytics_events_controller_spec.rb new file mode 100644 index 00000000000..1557323afdd --- /dev/null +++ b/spec/controllers/analytics_events_controller_spec.rb @@ -0,0 +1,42 @@ +require 'rails_helper' + +RSpec.describe AnalyticsEventsController do + describe '#index' do + subject(:action) { get :index } + + context 'when the JSON file exists' do + let(:json_content) { { events: [] }.to_json } + + around do |ex| + Tempfile.create do |json_file| + @json_file = json_file + json_file.rewind + json_file << json_content + json_file.close + + ex.run + end + end + + before do + stub_const('AnalyticsEventsController::JSON_FILE', @json_file.path) + end + + it 'renders the file' do + action + + expect(response).to be_ok + expect(response.body).to eq(json_content) + expect(response.content_type).to eq('application/json') + end + end + + context 'when the JSON file does not exist' do + it '404s' do + action + + expect(response).to be_not_found + end + end + end +end diff --git a/spec/requests/country_support_cors_spec.rb b/spec/requests/api_cors_spec.rb similarity index 80% rename from spec/requests/country_support_cors_spec.rb rename to spec/requests/api_cors_spec.rb index 0323e7d509e..804faa4188c 100644 --- a/spec/requests/country_support_cors_spec.rb +++ b/spec/requests/api_cors_spec.rb @@ -1,11 +1,7 @@ require 'rails_helper' RSpec.describe 'CORS headers for OpenID Connect endpoints' do - describe '/api/country-support' do - before do - get api_country_support_path, headers: { 'HTTP_ORIGIN' => http_origin } - end - + shared_examples_for 'static API with correct CORS headers' do context 'origin is www.login.gov' do let(:http_origin) { 'https://www.login.gov' } @@ -72,4 +68,28 @@ end end end + + describe '/api/country-support' do + before do + get api_country_support_path, headers: { 'HTTP_ORIGIN' => http_origin } + end + + it_behaves_like 'static API with correct CORS headers' + end + + describe '/api/analytics-events' do + before do + Tempfile.create do |json_file| + json_file.rewind + json_file << '{}' + json_file.close + + stub_const('AnalyticsEventsController::JSON_FILE', json_file.path) + + get api_analytics_events_path, headers: { 'HTTP_ORIGIN' => http_origin } + end + end + + it_behaves_like 'static API with correct CORS headers' + end end From e872498932399e2cac4cbe5b10ab80019077d953 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 3 Mar 2022 18:03:33 -0800 Subject: [PATCH 20/22] Clean up documentation --- app/services/analytics_events.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 6c0c0adcffc..5c1eef105aa 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -2,9 +2,9 @@ module AnalyticsEvents # @identity.idp.event_name Account Reset # @param [Boolean] success # @param ["cancel", "delete", "cancel token validation", "granted token validation", - # :notifications] event - # @param [String] message_id Request ID from AWS Pinpoint API - # @param [String] request_id Request ID from AWS Pinpoint API + # "notifications"] event + # @param [String] message_id from AWS Pinpoint API + # @param [String] request_id from AWS Pinpoint API # @param [Boolean] sms_phone # @param [Boolean] totp does the user have an authentication app as a 2FA option? # @param [Boolean] piv_cac does the user have PIV/CAC as a 2FA option? From fb3a914e5280d847fc78748ca71a6d079659a42a Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Fri, 4 Mar 2022 11:33:40 -0800 Subject: [PATCH 21/22] Clean up documentation --- app/services/analytics_events.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 5c1eef105aa..764a08489c1 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -57,13 +57,14 @@ def account_reset( end # @identity.idp.event_name Account Delete submitted - # When a user submits a form to delete their account # @param [Boolean] success + # When a user submits a form to delete their account def account_delete_submitted(success:) track_event('Account Delete submitted', success: success) end # @identity.idp.event_name Account Delete visited + # When a user visits the page to delete their account def account_delete_visited track_event('Account Delete visited') end From 698e2c7feee455a5217d7324df6c5ba7259968d2 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Fri, 4 Mar 2022 11:36:11 -0800 Subject: [PATCH 22/22] Migrate ACCOUNT_DELETION event --- app/controllers/users_controller.rb | 2 +- app/services/analytics.rb | 1 - app/services/analytics_events.rb | 7 +++++++ spec/controllers/users_controller_spec.rb | 9 +++------ 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 3709c921c3e..10992662a80 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -13,7 +13,7 @@ def destroy def track_account_deletion_event properties = ParseControllerFromReferer.new(request.referer).call - analytics.track_event(Analytics::ACCOUNT_DELETION, properties) + analytics.account_deletion(**properties) end def destroy_user diff --git a/app/services/analytics.rb b/app/services/analytics.rb index e3cbe76db09..23fb22b2695 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -122,7 +122,6 @@ def browser_attributes end # rubocop:disable Layout/LineLength - ACCOUNT_DELETION = 'Account Deletion Requested' ACCOUNT_RESET_VISIT = 'Account deletion and reset visited' ACCOUNT_VISIT = 'Account Page Visited' ADD_EMAIL = 'Add Email: Email Submitted' diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 764a08489c1..002d0dff589 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -68,4 +68,11 @@ def account_delete_submitted(success:) def account_delete_visited track_event('Account Delete visited') end + + # @identity.idp.event_name Account Deletion Requested + # @param [String] request_came_from the controller/action the request came from + # When a user deletes their account + def account_deletion(request_came_from:) + track_event('Account Deletion Requested', request_came_from: request_came_from) + end end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 401272dda26..46ed9699905 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -52,7 +52,7 @@ stub_analytics properties = { request_came_from: 'no referer' } - expect(@analytics).to receive(:track_event).with(Analytics::ACCOUNT_DELETION, properties) + expect(@analytics).to receive(:track_event).with('Account Deletion Requested', properties) delete :destroy end @@ -62,16 +62,13 @@ request.env['HTTP_REFERER'] = 'http://example.com/' properties = { request_came_from: 'users/sessions#new' } - expect(@analytics).to receive(:track_event).with(Analytics::ACCOUNT_DELETION, properties) + expect(@analytics).to receive(:track_event).with('Account Deletion Requested', properties) delete :destroy end it 'calls ParseControllerFromReferer' do - parser = instance_double(ParseControllerFromReferer) - - expect(ParseControllerFromReferer).to receive(:new).and_return(parser) - expect(parser).to receive(:call).and_return({}) + expect_any_instance_of(ParseControllerFromReferer).to receive(:call).and_call_original delete :destroy end