LG-460 Display fake banner in lower environments. by gregory-casamento · Pull Request #2418 · 18F/identity-idp

gregory-casamento · 2018-08-07T22:50:45Z

Why: Our lower environment websites (dev, int, qa, staging, etc.)
are public, and sometimes people visit them without realizing they are
not on secure.login.gov. When that happens, we want to make sure they
know they are not on the production site by displaying a banner at the
top of the site that says this is a FAKE site.

Hi! Before submitting your PR for review, and/or before merging it, please
go through the checklists below. These represent the more critical elements
of our code quality guidelines. The rest of the list can be found in
CONTRIBUTING.md

Controllers

When adding a new controller that requires the user to be fully
authenticated, make sure to add before_action :confirm_two_factor_authenticated
as the first callback.

Database

Unsafe migrations are implemented over several PRs and over several
deploys to avoid production errors. The strong_migrations gem
will warn you about unsafe migrations and has great step-by-step instructions
for various scenarios.
Indexes were added if necessary. This article provides a good overview
of indexes in Rails.
Verified that the changes don't affect other apps (such as the dashboard)
When relevant, a rake task is created to populate the necessary DB columns
in the various environments right before deploying, taking into account the users
who might not have interacted with this column yet (such as users who have not
set a password yet)
Migrations against existing tables have been tested against a copy of the
production database. See LG-228 Make migrations safer and more resilient #2127 for an example when a migration caused deployment
issues. In that case, all the migration did was add a new column and an index to
the Users table, which might seem innocuous.

Encryption

The changes are compatible with data that was encrypted with the old code.

Routes

GET requests are not vulnerable to CSRF attacks (i.e. they don't change
state or result in destructive behavior).

Session

When adding user data to the session, use the user_session helper
instead of the session helper so the data does not persist beyond the user's
session.

Testing

Tests added for this feature/bug
Prefer feature/integration specs over controller specs
When adding code that reads data, write tests for nil values, empty strings,
and invalid inputs.

monfresh · 2018-08-08T00:45:34Z

Thanks for your first PR! This looks great so far. I have a few comments:

We have a commit message style guide we like to follow as part of our pull request guidelines. Could we please update the commit message to follow the guide? Apologies if you weren't made aware of this documentation during onboarding. We will make sure to add it to the checklist. One thing I noticed that isn't explicitly spelled out in the documentation (but is enforced if you use overcommit) is that the first line of the commit message should be no longer than 50 characters (to encourage a concise description and so that it all fits in the GitHub title, although it looks like GitHub uses up to 72 characters, so perhaps worth a revisit).
The team often finds it helpful to have screenshots in PRs that include UI changes. Could we please add one?
Apologies if this wasn't communicated, but the root of the issue we're trying to fix is not so much that a banner didn't exist, but that the logic for FeatureManagement#no_pii_mode? is wrong. Currently, it will only show the banner if enable_identity_verification is true, and if profile_proofing_vendor is mock, which will never happen because we don't have a config key called profile_proofing_vendor anymore. What we want is for the banner to show up on all servers except for production (and perhaps except for development), so something like this:

def self.no_pii_mode?
  Rails.env.production? && Figaro.env.domain_name != 'secure.login.gov'
end

Could we add a view spec?
Circle CI failed the build due to some slim-lint errors. They can be viewed locally by running make lint.

monfresh · 2018-08-08T00:59:29Z

app/views/layouts/base.html.slim

      = render 'shared/no_pii_banner' if FeatureManagement.no_pii_mode?
-      = render 'shared/usa_banner'
+      = render 'shared/fake_banner' if FeatureManagement.no_pii_mode?
+      = render 'shared/usa_banner' if !(FeatureManagement.no_pii_mode?)


What do you think about combining these conditionals? Maybe something like this:

- if FeatureManagement.no_pii_mode? = render 'shared/no_pii_banner' = render 'shared/fake_banner' - else = render 'shared/usa_banner'

And then going a step further, what do you think about combining the no_pii_banner into the fake_banner by taking the contents of the former and adding it to the latter, then deleting the former, since those 2 will always be shown together? And finally, perhaps renaming no_pii_mode? to fake_mode? or fake_banner_mode??

monfresh · 2018-08-08T01:02:33Z

config/locales/idv/fr.yml

-        no_pii: N'utilisez pas de véritables données personnelles (il s'agit d'une
-          démonstration seulement)
+        no_pii: FAKE N'utilisez pas de véritables données personnelles (il s'agit
+          d'une FAKE démonstration seulement)


I think the french word we want is TRUQUÉ, and the second instance should come after the closing parenthesis.

monfresh · 2018-08-08T01:02:40Z

config/locales/shared/fr.yml

 fr:
  shared:
+    fake_banner:
+      fake_site: Un site FAKE du gouvernement des États-Unis


s/FAKE/TRUQUÉ

monfresh · 2018-08-10T12:56:54Z

lib/feature_management.rb

-  def self.no_pii_mode?
-    enable_identity_verification? && Figaro.env.profile_proofing_vendor == :mock
+  def self.fake_banner_mode?
+    Rails.env.production? == false && Figaro.env.domain_name != 'secure.login.gov'


Thanks for the updates. This looks great. I failed to clarify this earlier: what we mean by "the banner should show up on all servers except for production" is that the logic is based on the server host, which is defined in Figaro.env.domain_name. The Rails environment is production on all of those servers. When I suggested we use Rails.env.production? as the first condition, I meant the banner should not show up when developing the app locally and in the test environment.

So, what we want is Rails.env.production?, not Rails.env.production? == false.

monfresh · 2018-08-10T12:58:20Z

spec/lib/feature_management_spec.rb

        let(:enable_identity_verification) { false }

-        it { expect(no_pii_mode?).to eq(false) }
+        it { expect(fake_banner_mode?).to eq(true) }


Can we please update all of these tests to match the new conditions? These old scenarios don't apply anymore.

monfresh · 2018-08-10T13:01:42Z

spec/features/users/sign_in_spec.rb

+
+    expect(page).to have_content 'FAKE'
+  end
+


What do you think about using a view spec instead, in spec/views/layouts/application.html.slim_spec.rb? That way, we know this will work on all pages as opposed to just the sign in page.

monfresh · 2018-08-10T13:04:27Z

config/locales/idv/fr.yml

-        no_pii: N'utilisez pas de véritables données personnelles (il s'agit d'une
-          démonstration seulement)
+        no_pii: TRUQUÉ N'utilisez pas de véritables données personnelles (il s'agit
+          d'une TRUQUÉ démonstration seulement)


Can we move the second TRUQUÉ to the end of the string, after the closing parenthesis?

Looks like the second TRUQUÉ is still not at the end of the string. It should be like this:

no_pii: TRUQUÉ N'utilisez pas de véritables données personnelles (il s'agit d'une démonstration seulement) TRUQUÉ

gregory-casamento · 2018-08-12T01:06:23Z

Please review again. I believe I have fixed all concerns.

gregory-casamento · 2018-08-12T01:07:16Z

There is one issue that the CI finds:
bundle exec fasterer
app/models/concerns/user_access_key_overrides.rb
Use attr_reader for reading ivars. Occurred at lines: 30.

This is not due to my changes so I am not sure if I should fix it as it may have been addressed elsewhere

monfresh · 2018-08-13T18:25:42Z

spec/lib/feature_management_spec.rb


-        it { expect(no_pii_mode?).to eq(false) }
+    context 'fake banner mode in production' do
+      it 'returns true' do


I think you meant it 'returns false'? Also, what do you think of a more descriptive test title, such as:

context 'when on secure.login.gov' do it 'does not display the fake banner' do

monfresh · 2018-08-13T18:26:53Z

spec/lib/feature_management_spec.rb

-
-        it { expect(no_pii_mode?).to eq(false) }
-      end
+    context 'fake Banner mode in test'


What do you think of a more descriptive test title, such as:

context 'when the host is not secure.login.gov and the Rails env is production' do it 'displays the fake banner' do

monfresh · 2018-08-13T18:28:11Z

spec/lib/feature_management_spec.rb

+        and_return('test.login.gov')
+      allow(Rails.env).to receive(:production?).
+        and_return(true)
+      expect(FeatureManagement.fake_banner_mode?).to eq(true)


Can we also please add a third test for this scenario?

context 'when the host is not secure.login.gov and Rails env is not production' do it 'does not display the fake banner' do

monfresh · 2018-08-13T18:28:58Z

spec/lib/feature_management_spec.rb

-      context 'with identity verification disabled' do
-        let(:enable_identity_verification) { false }
+  describe '.fake_banner_mode?' do
+    subject(:fake_banner_mode?) { FeatureManagement.fake_banner_mode? }


This subject doesn't seem to be used. Can we remove it?

monfresh · 2018-08-13T18:39:25Z

spec/views/layouts/application.html.slim_spec.rb

+      render
+      expect(rendered).to have_content('FAKE')
+    end
+  end


What do you think about stubbing FeatureManagement here, since that is what the layout is calling, and it is already unit tested separately? Something like this:

context 'when FeatureManagement.fake_banner_mode? is true' do it 'displays the fake banner' do allow(FeatureManagement).to receive(:fake_banner_mode?).and_return(true) render expect(rendered).to have_content('FAKE') end end

and then the negative test as well:

context 'when FeatureManagement.fake_banner_mode? is false' do it 'does not display the fake banner' do allow(FeatureManagement).to receive(:fake_banner_mode?).and_return(false) render expect(rendered).to_not have_content('FAKE') end end

gregory-casamento · 2018-08-15T11:12:56Z

Please review and see if you approve. Thanks. GC

monfresh · 2018-08-15T13:57:47Z

spec/lib/feature_management_spec.rb

+          and_return('test.login.gov')
+        allow(Rails.env).to receive(:production?).
+          and_return(false)
+        expect(FeatureManagement.fake_banner_mode?).to eq(true)


This should be false. If you click on the "Details" link next to ci/circleci at the bottom of the PR, you can see that this test failed.

monfresh · 2018-08-15T13:58:09Z

spec/lib/feature_management_spec.rb

-
-        it { expect(no_pii_mode?).to eq(false) }
+    context 'when the host is not secure.login.gov and the Rails env is not in production' do
+      it 'displays the fake banner' do


this should say it 'does not display the fake banner'

Done.... I apologize. I did these in a hurry this morning and missed it. Should be okay now.

monfresh · 2018-08-15T13:59:53Z

The latest changes look good. Thanks! Just a couple of things to fix:

The French translation still needs to be updated to this:

no_pii: TRUQUÉ N'utilisez pas de véritables données personnelles (il s'agit
  d'une démonstration seulement) TRUQUÉ

and the test that is failing.

gregory-casamento · 2018-08-15T14:25:28Z

Please take a look now.

monfresh

Looks great, thanks! Please squash your commits and update the final commit message to follow our guidelines. Thanks!

**Why**: Our lower environment websites (dev, int, qa, staging, etc.) are public, and sometimes people visit them without realizing they are not on secure.login.gov. When that happens, we want to make sure they know they are not on the production site by displaying a banner at the top of the site that says this is a FAKE site.

monfresh

LGTM

gregory-casamento requested a review from davemcorwin August 7, 2018 22:50

monfresh reviewed Aug 8, 2018

View reviewed changes

monfresh reviewed Aug 10, 2018

View reviewed changes

monfresh reviewed Aug 13, 2018

View reviewed changes

monfresh reviewed Aug 15, 2018

View reviewed changes

monfresh previously approved these changes Aug 15, 2018

View reviewed changes

monfresh dismissed their stale review via 66ee7b6 August 16, 2018 13:46

monfresh force-pushed the gregc-LG460-create-more-visible-banner branch from 46f8a0c to 66ee7b6 Compare August 16, 2018 13:46

monfresh mentioned this pull request Aug 16, 2018

LG460 fake banner #2436

Closed

12 tasks

monfresh changed the title ~~LG-460: Added new banners for when we are not in production and no_pi…~~ LG-460 Display fake banner in lower environments. Aug 16, 2018

monfresh approved these changes Aug 16, 2018

View reviewed changes

monfresh merged commit 31da629 into master Aug 17, 2018

monfresh deleted the gregc-LG460-create-more-visible-banner branch August 17, 2018 22:45

Conversation

gregory-casamento commented Aug 7, 2018 • edited by monfresh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Controllers

Database

Encryption

Routes

Session

Testing

Uh oh!

monfresh commented Aug 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregory-casamento commented Aug 12, 2018

Uh oh!

gregory-casamento commented Aug 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregory-casamento commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh commented Aug 15, 2018

Uh oh!

gregory-casamento commented Aug 15, 2018

Uh oh!

monfresh left a comment

Choose a reason for hiding this comment

Uh oh!

monfresh left a comment

Choose a reason for hiding this comment

Uh oh!

gregory-casamento commented Aug 7, 2018 •

edited by monfresh

Loading