[#153072577] First steps to admin app: Base rails app with OAuth #1

chrisfarms · 2017-12-07T16:00:32Z

What

We're building a web UI for the paas.

This is the first steps to that end:

A base Rails app to build upon
Login/Authentication via UAA/OAuth
A basic CF API Client
A page that lists org names as an example
Some tests

How to review

Code review
Ensure the tests make sense and pass (see README)
Run the app and test logging in as a user:
- You will need to first deploy: [#153072577] Add UAA client for paas-admin app paas-cf#1135
- You will then we able to run the app locally (see README) or push to your dev (if you set all the required environment variables manually).

Who can review

Not @chrisfarms nor @henrytk nor @camelpunch nor @carolinegreen

--skip-turbolinks --skip-test --skip-active-record --skip-coffee paas-admin

Choose two instances and 256M for now

Add rspec and pry-byebug We expect that org listing will eventually move to e.g. /orgs

This means that every action (except excluded sessions controller) requires auth.

It's a global configuration, so the aim of this line is to tidy up, not set up.

Capybara follows redirects automatically, behaves more like a user.

This removes a warning about OpenSSL::Digest::Digest being deprecated

Remove logging output from test Reset global state after each example

Fake and real implementations. Fake stores state in memory. Real currently is a no-op. Shared examples to ensure they're in sync.

Without this change, bundler generate bad binstubs that don't accept arguments. Upstream issue: rubygems/bundler#6149

dcarley · 2017-12-08T12:42:53Z

I've enabled Travis for this repo and triggered a build through their admin UI.

chrisfarms · 2017-12-08T13:12:12Z

I have added a .travis.yml that should trigger the (non-integration) tests.

...but travis seems confused and is running against master?

chrisfarms · 2017-12-08T13:23:56Z

Ignore that. Travis seems to have fixed itself.

dcarley

Works 👍

I've left some minor comments and questions.

I like the structure of the commits. Reading them through one-by-one helped me understand it a bit more, even if some code was replaced in later commits. Though as someone that hasn't developed a Rails app for scratch before I would have liked a bit more description of "why" in the commit messages to describe the reason for changes.

I feel a bit nervous that, although I understand the basics of what authentication and validation is being done, I haven't reviewed the code for scenarios where it might be bypassed or broken. Should we? How can we?

dcarley · 2017-12-11T10:53:40Z

spec/features/list_orgs_spec.rb

+        '/oauth/authorize',
+        "?client_id=#{ENV.fetch('OAUTH_CLIENT_ID')}",
+        '&response_type=code',
+        "&redirect_uri=#{CGI.escape("http://example.org/auth/cloudfoundry/callback")}",


Where does http://example.org/ come from?

We discarded this technique for testing the auth once we realized all we were testing was OmniAuth and that it was near imposible to test without mocking UAA too .... we then found OmniAuth has some documented ways of mocking the auth and used that method instead.

Essentially we trust that OmniAuth works and currently have no integration tests to prove that (once we have a CI build pipeline we would probably want to add those sorts of integration/acceptance tests).

To answer @dcarley's question: it's the default 'fake' app host that rack-test uses.

Thanks and thanks 👍

dcarley · 2017-12-11T11:13:49Z

app/controllers/orgs_controller.rb

@@ -1,5 +1,5 @@
 class OrgsController < ApplicationController
  def index
-    redirect_to Rails.configuration.x.oauth.redirect_url


Should we set this config option to '/auth/cloudfoundry' and continue using it?

(this change has been rebased away)

dcarley · 2017-12-11T11:27:17Z

Gemfile

@@ -10,7 +10,7 @@ end
 gem 'rails', '~> 5.1.4'
 gem 'puma', '~> 3.7'
 gem 'uglifier', '>= 1.3.0'
-gem 'omniauth-uaa-oauth2'
+gem 'omniauth-uaa-oauth2', github: 'cloudfoundry/omniauth-uaa-oauth2'


Should we be concerned that subsequent versions (up to 0.0.6) haven't been released to https://rubygems.org/ or that a relatively simple PR to merge a deprecated warning (cloudfoundry/omniauth-uaa-oauth2#9) hasn't been merged?

I think this is concerning. We should complain, or do a spike into using vanilla oauth with https://github.com/omniauth/omniauth-oauth2

I hadn't notice this before, which is more concerning: cloudfoundry/omniauth-uaa-oauth2#10

@alext any thoughts? Should we merge this as-is and have a story to look at changing it afterwards?

@camelpunch @alext and I talked about this: we decided to leave it as-is for this PR and @camelpunch will write a follow-up story to investigate omniauth/omniauth-oauth2.

dcarley · 2017-12-11T13:39:19Z

app/views/layouts/application.html.erb

+  </head>
+
+  <body>
+    <img src="https://static1.squarespace.com/static/59698a7d29687fd47a2a7c52/599a2c9837c581a0f21a7030/5998fbff17bffc5c4b7d0f04/1503700549384/construction_worker.jpg" border="0" width="125">


Should we vendor this rather than hotlink?

👍 We should be hosting that ourselves - makes us better Internet citizens, and also we can't be sure of the longevity of that link etc.

I love your optimism that Construction Duck will last persist long enough to be worried about this.

dcarley · 2017-12-11T13:48:42Z

.envrc

+export CF_AUTH_ENDPOINT=$(cf curl /v2/info | jq -r .authorization_endpoint)
+export CF_TOKEN_ENDPOINT=$(cf curl /v2/info | jq -r .token_endpoint)
+export CF_CLIENT_ID="paas-admin"
+export CF_CLIENT_SECRET=$(aws s3 cp s3://gds-paas-${DEPLOY_ENV}-state/cf-secrets.yml - | awk '/uaa_clients_paas_admin_secret/ { print $2 }')


Should we include things that are so specific to our development environments? Do we want other people to use the app in the future?

I have mixed feelings.

Setting up these variables for running in dev is painful. But I understand this is so specific to us that would be annoying for outsiders.

Henry created some scripts for running (to replace/augment .envrc) ... maybe we should document the "standard" way ie CF_AUTH_ENDPOINT=xx CF_TOKEN_ENDPOINT=yy rails s in the README for all to enjoy ... but then have some additional documentation for paas-specific-ness?

That sounds good. I definitely appreciated having an easy way to run it against my dev environment, but I think it would be easier to understand if we provided generic instructions foremost.

dcarley · 2017-12-11T13:57:52Z

bin/bundle

@@ -1,3 +1,105 @@
 #!/usr/bin/env ruby
-ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../../Gemfile', __FILE__)


Do you know why we need a binstub for Bundler?

something something bug in bundler todo with binstubs .... /cc @camelpunch

I don't know why bundler (or Rails) introduced a binstub for bundler itself. I assume it's to lock the version of bundler for the app.

The bug that @chrisfarms is referring to is here, but not sure if this is related to the question: rubygems/bundler#6149

I'm inclined to delete it rather than worry about when we can update/remove the backported fix. It doesn't seem as important the rails binstub, which appears to be a standard for Rails apps.

The fix was primarily for stubs like rspec, which are used a lot. I think the only risk here is if someone:

a) Decides to run e.g. bundle binstubs rspec-core and
b) Commits the change

Otherwise if we're concerned about bundler versions, we should downgrade to 1.15, as one of the comments on the Rails bug thread suggested, because 1.15 doesn't generate incompatible binstubs by default (1.16 is fine if you also supply --force)

Add scripts and envrc to help with development setup. Document environment configuration

By default rubocop is a tad overzealous. We use the config from the govuk-lint[1] repo to rein it in a bit and we expect to modify it to our liking over time. Rubocop version is pinned in Gemfile as recormended and binstubs are generated. We exclude the 'bin/*' dir since files generated there are generally out of our control. [1] https://github.com/alphagov/govuk-lint fixup: add rubocop/gem/binstubs/exclude-bindir fixup: add rubocop/gem/binstubs/exclude-bindir rubocop binstub

api_endpoint is ambiguous. Rename to cf_api_endpoint to avoid confusion.

The main change here is preventing RSpec monkey-patching the environment which means `describe` blocks must be namespaced correctly. The state file is also configured now so is added to the gitignore.

* Autocorrections Many style fixes applied by executing `./bin/rubocop -a` * Rubocop says: no-class-vars Class vars can be confusing when combined with inheritance[1] so prefur class-instance variables instead. * Rubocop says: use Rails.root.join('path', 'to') The point of path join methods is to avoid hardcoding path seperators which are platform dependent. * Rubocop says: Avoid using Time without time zone Using time without specifying a timezone can lead to confusion, so use ActiveSupport's Time.zone extension by setting config.time_zone to a known value and only requesting/parsing Time values via Time.zone Use Time.now.to_i or Time.zone.now [1] https://github.com/bbatsov/ruby-style-guide#no-class-vars

Adds a new "design" environment mode for when you just want to run the app without requiring a real cloudfoundry deployment with real cloud controller access, uaa clients etc. In this mode: * Authentication is bypassed (OmniAuth test mode enabled) * The FakeClient implementation is used for cf communication * Some fixtures are populated (see design.rb) This is useful if you are just working on styling or other tasks.

Previously we had some .envrc magic and some scripts for running the app against a paas-cf dev environment and the README was geared towards this. We want to try to stay a little more agnostic about who might be running this (even if it is just us for now). So we will favour adding more generic instructions to the README along with outling the scripts for how to get up and running quickly with GOV.UK PaaS

It's a pain to have to set all the environment config to junk values when you only want to run unit tests. This sets all the required vars to "none" unless the CONTRACT_TEST_TOKEN is set (which is what triggers integration test execution)

dcarley

Just one request and then this is good to

dcarley · 2017-12-14T10:50:07Z

Gemfile

  gem 'listen', '>= 3.0.5', '< 3.2'
+  gem 'rubocop', '~> 0.51.0', require: false


We should use govuk-lint as a gem and govuk-lint-ruby as the command, because then we don't need to vendor all of it's .rubocop.yml and it will get updated when we pull in new versions of the gem.

You can refer to alphagov/paas-cf for an example:

https://github.com/alphagov/paas-cf/blob/c11494efc27c03ad61a4a2ced9dce548315b5da2/Gemfile#L5

https://github.com/alphagov/paas-cf/blob/c11494efc27c03ad61a4a2ced9dce548315b5da2/Gemfile#L5

https://github.com/alphagov/paas-cf/blob/c11494efc27c03ad61a4a2ced9dce548315b5da2/.rubocop.yml

I'm not sure if that's the best move tbh. I can see two issues with that:

Anyone who has an editor/plugin that does rubocop linting for them would not understand the "govuk-lint-ruby" command and would expect to find the rubocop.yml file in the root. I find it very useful to get the cop feedback as I'm typing and it would be a real shame to lose that.

The govuk-lint version is not a perfect fit... I had to enable the rails specific cops and had to modify the include/excludes to ensure that certain cops are excluded from some rails specific files (large blocks in environment configs are expected for one) and needed to exclude the vendor dir so travis didnt attempt to lint the entire vendor bundle.

... just talking with @alext ... we think we can have our cake and eat it ... investigating

The README says that there's a -R flag to enable Rails cops:

https://github.com/alphagov/govuk-lint#ruby

I think it's worth trying to find solutions to because people have already done the hard work of maintaining an acceptable config ❤️

🙌 ...sorted!

Found a way to inherit the config from the govuk-lint gem so that standard rubocop commands work as expected and we get the best of both worlds.

By inheriting rubocop configuration from the govuk-lint gem we can stay in sync with upstream changes by updating the gem. We extend the base config to enable the Rails and Rspec cops then rein in the Rspec cops a bit as they are a bit too agressive by default. The large-block cop is excluded from the rails config dir since it is expected to always have large blocks.

Some tweaks at the request of rubocop

dcarley · 2017-12-14T12:27:26Z

Nice!! Thanks 👍

Caroline Green and others added 21 commits December 4, 2017 12:05

rails new

559d794

--skip-turbolinks --skip-test --skip-active-record --skip-coffee paas-admin

Create LICENSE

ee66fb9

Remove coffee-rails

b611a66

Set Ruby version to 2.4.2

6e3d343

CF manifest

13d56e9

Choose two instances and 256M for now

add ./bin to PATH for direnv users

0bf1c4a

Set up / to always redirect to OAUTH_REDIRECT_URL

dfda3e6

Add rspec and pry-byebug We expect that org listing will eventually move to e.g. /orgs

clean Gemfile of comments

776877a

Remove some gems we don't yet need

800b7f6

Use omniauth to redirect to UAA

714612e

Handle response from UAA, display token.

4724cc7

Move before_action to application controller

15c443b

This means that every action (except excluded sessions controller) requires auth.

Tidy up omniauth stuff after spec examples

6f2936a

It's a global configuration, so the aim of this line is to tidy up, not set up.

Switch to capybara

b7757fc

Capybara follows redirects automatically, behaves more like a user.

use latest omniauth-uaa-oauth2

5f15b63

This removes a warning about OpenSSL::Digest::Digest being deprecated

Tidy feature spec

709c020

Remove logging output from test Reset global state after each example

Begin sketching a CF client interface

d0c6896

Fake and real implementations. Fake stores state in memory. Real currently is a no-op. Shared examples to ensure they're in sync.

Fix bundler binstub

cfa8756

Without this change, bundler generate bad binstubs that don't accept arguments. Upstream issue: rubygems/bundler#6149

Skip OmniAuth UAA TLS validation from ENV in dev

7b59785

Add Faraday, an HTTP client

222a005

Use gds-way .editorconfig

231b99e

henrytk mentioned this pull request Dec 7, 2017

[#153072577] Add UAA client for paas-admin app alphagov/paas-cf#1135

Merged

chrisfarms changed the title ~~WIP [#153072577] First steps to admin app: Base rails app with OAuth~~ [#153072577] First steps to admin app: Base rails app with OAuth Dec 7, 2017

dcarley self-assigned this Dec 8, 2017

chrisfarms force-pushed the uaa-auth-153072577 branch from 1a0d443 to a15a838 Compare December 8, 2017 13:18

dcarley reviewed Dec 11, 2017

View reviewed changes

chrisfarms and others added 5 commits December 12, 2017 08:54

Rename environment variables for consistancy with other paas-things

ef885ec

skip integration tests unless CONTRACT_TEST_TOKEN

1b2cc9c

README and helper scripts

2858637

Add scripts and envrc to help with development setup. Document environment configuration

Add instructions for running locally

0cb2c11

add travis.yml

2ec41d5

chrisfarms force-pushed the uaa-auth-153072577 branch 3 times, most recently from f0ac730 to 95f450e Compare December 12, 2017 11:00

chrisfarms added 7 commits December 12, 2017 15:38

Rename Rails.configuration.api_endpoint

b52b35f

api_endpoint is ambiguous. Rename to cf_api_endpoint to avoid confusion.

Enable recormended RSpec defaults

db43048

The main change here is preventing RSpec monkey-patching the environment which means `describe` blocks must be namespaced correctly. The state file is also configured now so is added to the gitignore.

Enable Rubocop in Travis

216d3a1

Ignore vendor dir

02bda56

chrisfarms force-pushed the uaa-auth-153072577 branch from 95f450e to f1608d8 Compare December 12, 2017 15:55

chrisfarms added 2 commits December 12, 2017 16:23

Optional CF_* env variables for unit tests

247d5d5

It's a pain to have to set all the environment config to junk values when you only want to run unit tests. This sets all the required vars to "none" unless the CONTRACT_TEST_TOKEN is set (which is what triggers integration test execution)

chrisfarms force-pushed the uaa-auth-153072577 branch from f1608d8 to 247d5d5 Compare December 12, 2017 16:24

dcarley suggested changes Dec 14, 2017

View reviewed changes

chrisfarms added 2 commits December 14, 2017 12:14

Rubocop fixes after enabling rspec cops.

6d68e91

Some tweaks at the request of rubocop

chrisfarms force-pushed the uaa-auth-153072577 branch from 5a33a5b to 6d68e91 Compare December 14, 2017 12:14

dcarley approved these changes Dec 14, 2017

View reviewed changes

dcarley merged commit 9852b69 into master Dec 14, 2017

paroxp deleted the uaa-auth-153072577 branch March 20, 2018 15:49

paroxp restored the uaa-auth-153072577 branch March 20, 2018 15:49

paroxp deleted the uaa-auth-153072577 branch March 23, 2018 16:31

kr8n3r mentioned this pull request Jan 12, 2022

Move to node v16 and npm v8 #2218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#153072577] First steps to admin app: Base rails app with OAuth #1

[#153072577] First steps to admin app: Base rails app with OAuth #1

chrisfarms commented Dec 7, 2017 •

edited

Loading

dcarley commented Dec 8, 2017

chrisfarms commented Dec 8, 2017 •

edited

Loading

chrisfarms commented Dec 8, 2017

dcarley left a comment

dcarley Dec 11, 2017

chrisfarms Dec 11, 2017

camelpunch Dec 11, 2017

dcarley Dec 12, 2017

dcarley Dec 11, 2017

camelpunch Dec 12, 2017

dcarley Dec 11, 2017

camelpunch Dec 11, 2017

dcarley Dec 12, 2017

dcarley Dec 14, 2017

dcarley Dec 11, 2017

alext Dec 11, 2017

chrisfarms Dec 11, 2017

dcarley Dec 11, 2017

chrisfarms Dec 11, 2017

dcarley Dec 12, 2017

dcarley Dec 11, 2017

chrisfarms Dec 11, 2017

camelpunch Dec 11, 2017

dcarley Dec 12, 2017

camelpunch Dec 12, 2017

dcarley left a comment

dcarley Dec 14, 2017

chrisfarms Dec 14, 2017

chrisfarms Dec 14, 2017

dcarley Dec 14, 2017

chrisfarms Dec 14, 2017

dcarley commented Dec 14, 2017

		@@ -1,3 +1,105 @@
		#!/usr/bin/env ruby
		ENV['BUNDLE_GEMFILE'] \|\|= File.expand_path('../../Gemfile', __FILE__)

		gem 'listen', '>= 3.0.5', '< 3.2'
		gem 'rubocop', '~> 0.51.0', require: false

[#153072577] First steps to admin app: Base rails app with OAuth #1

[#153072577] First steps to admin app: Base rails app with OAuth #1

Conversation

chrisfarms commented Dec 7, 2017 • edited Loading

What

How to review

Who can review

dcarley commented Dec 8, 2017

chrisfarms commented Dec 8, 2017 • edited Loading

chrisfarms commented Dec 8, 2017

dcarley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcarley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcarley commented Dec 14, 2017

chrisfarms commented Dec 7, 2017 •

edited

Loading

chrisfarms commented Dec 8, 2017 •

edited

Loading