Skip to content

Commit

Permalink
Fix implementation of GovukEnvironment.production?
Browse files Browse the repository at this point in the history
Trello: https://trello.com/c/8HOXkVAT

Previously the implementation relied on the INSTANCE_NAME environment
variable not being set in the production environment. However, since the
replatforming to k8s, that environment variable has incorrectly been set
to "production" [1].

That means that since the replatforming a bunch of logic in the app has
been working incorrectly. This was highlighted by some non-sensical text
in an automated email sent to a user whose account had been locked:

> Production account locks do not lock production accounts.

However, the problem has been a lot more widespread, because we've
effectively treated the production environment as more equivalent to
integration & staging, i.e. GovukEnvironment.production? has always
returned `false`.

I _could_ have fixed the problem by simply removing the value of
INSTANCE_NAME in the production environment. However, I can see why the
mistake was made and it looks as if it has already had to be rectified
once before [2] in the replatforming to AWS! So instead I've chosen to
leave the value of INSTANCE_NAME as "production" and change the
implementation of GovukEnvironment.production? to check for this value.

This commit will fix a bunch of issues:

* Mandating 2SV for users promoted to any admin role

Users promoted to any admin role in production will now be required to
setup 2SV. Since we now mandate 2SV for most users in production, this
is unlikely to be causing a security lapse, but it would be worth
checking that no users have slipped through the cracks while this bug
existed.

* 2SV QR Code Issuer

The issuer in the 2SV QR code will now be set to "GOV.UK Signon" rather
than "Production GOV.UK Signon" in production. The issuer is used by
Authentication Apps to identify which website the secret belongs to.
There _might_ be some confusion for people who originally setup 2SV
while this bug existed who now come back to reset 2SV, but I plan to
investigate that before I merge the changes.

* Email subject line

In production, the subject line of the automated email for 2SV enabled
will now be "2-step verification set up" instead of "[Production] 2-step
verification set up". It's not clear to me why this email is singled out
in this way and unfortunately the commit note [3] & PR [4] where it was
originally introduced doesn't shed any light on the matter.

In production, the subject line of the automated email for batches of
user invitations will now be:

    [SIGNON] Serena Williams created a batch of 37 users

instead of:

    [SIGNON] Serena Williams created a batch of 37 users in production

* Email content

In production, the content for the following automated email types will
no longer include content which was originally only intended to be
displayed in non-production environments. This will resolve the specific
issue which drew our attention to the bug.
    * Suspension reminder
    * Suspension notification
    * Notify reset password disallowed due to suspension
    * 2SV enabled
    * Unlock instructions

In production, automated emails will now refer to "your GOV.UK Signon
account" rather than "your production GOV.UK account".

* Email from address

The from address in all automated emails sent from the production
environment will now be:

    GOV.UK Signon <noreply-signon@digital.cabinet-office.gov.uk>

versus:

    GOV.UK Signon production <noreply-signon-production@digital.cabinet-office.gov.uk>

Note that the reply-to address will not change, so we shouldn't need to
check that we can actually receive emails at the new from address.

[1]: https://github.com/alphagov/govuk-helm-charts/blob/d9a542872a009363f9251d8276e98f1c94c6cd16/charts/app-config/values-production.yaml#L2289-L2290
[2]: alphagov/govuk-puppet#7432
[3]: 89d2801
[4]: #425
  • Loading branch information
floehopper committed Aug 16, 2023
1 parent 40263fe commit fb85a32
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 5 deletions.
2 changes: 1 addition & 1 deletion app/models/govuk_environment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ def self.name
end

def self.production?
name.blank?
name == "production"
end
end
8 changes: 4 additions & 4 deletions test/models/govuk_environment_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,19 @@ class GovukEnvironmentTest < ActionMailer::TestCase
end

context ".production?" do
context "when name is blank" do
context "when name is 'production'" do
setup do
GovukEnvironment.stubs(:name).returns("")
GovukEnvironment.stubs(:name).returns("production")
end

should "return truthy" do
assert GovukEnvironment.production?
end
end

context "when name is not blank" do
context "when name is not 'production'" do
setup do
GovukEnvironment.stubs(:name).returns("not-blank")
GovukEnvironment.stubs(:name).returns("not-production")
end

should "return falsey" do
Expand Down

0 comments on commit fb85a32

Please sign in to comment.