Add initial development tools #1097

teneightfive · 2020-12-07T18:34:50Z

Proposed changes

What changed

This adds a sup-app that is only available when in development or enabled using an environment variable.

Why did it change

The first tool within this app allows the permissions to be set based on the roles that exist within the frontend application.

Screenshots

Checklists

Testing

Automated testing

Added unit tests to cover changes
Added end-to-end tests to cover any journey changes

Manual testing

Has been tested in the following browsers:

Chrome
Firefox
Edge
Internet Explorer

Environment variables

Documented in the README
Added to continous integration
Added to staging environment (deployment repository)
Added to preproduction environment (deployment repository)
Added to production environment (deployment repository)

Other considerations

Commit messages with a fix or feat type are automatically used to generate the changelog and
GitHub release notes during the release task. Please make sure they will read well on their own in a
summary of changes and that the commit body gives a more detailed description of those changes if necessary.

No new Personally Identifiable Information (PII) is being sent via analytics
Update README with any new instructions or tasks

nimphal

Nice!

solidgoldpig · 2020-12-08T11:24:37Z

config/index.js

@@ -104,6 +104,8 @@ module.exports = {
  },
  ENABLE_COMPONENTS_LIBRARY:
    IS_DEV || /true/i.test(process.env.ENABLE_COMPONENTS_LIBRARY),
+  ENABLE_DEVELOPMENT_TOOLS:


Is there any way we could ensure that this could never be set on production. (ie. currently, a naughty person with access could add the env var to the k8s config and simply apply it)

Any environment variable is susceptible to that sort of manipulation.

Best option I can think of involves denying this path in the ingress configuration.

Only other two options I can think off are either not including this module in our production builds, which is not a great solution, because then what is staging actually representing. Alternatively, if this was a separate microservice stitched together at the Ingress layer we wouldn't need to worry about this - but that's effectively the same as denying it at that later anyway.

I did also toy with this but the reason I created the env was to actually potentially allow it on some "production-like" environments like Heroku or Staging.

I think some of the tools we may add to this will be useful in those environments, like pre-filling a PER.

However, you're right that this does potentially open up security concerns if things like permissions can be manipulated.

Maybe it is safer at this stage that it can only be done in development environments and we remove support for the environment variable altogether.

@merlinc's suggestion is also something we thought about with the original development only middleware. We could potentially add a npm task that cleans up after a deploy to remove such files?

app/_development-tools/index.js

merlinc · 2020-12-08T11:52:36Z

config/index.js

@@ -104,6 +104,8 @@ module.exports = {
  },
  ENABLE_COMPONENTS_LIBRARY:
    IS_DEV || /true/i.test(process.env.ENABLE_COMPONENTS_LIBRARY),
+  ENABLE_DEVELOPMENT_TOOLS:


Any environment variable is susceptible to that sort of manipulation.

Best option I can think of involves denying this path in the ingress configuration.

Only other two options I can think off are either not including this module in our production builds, which is not a great solution, because then what is staging actually representing. Alternatively, if this was a separate microservice stitched together at the Ingress layer we wouldn't need to worry about this - but that's effectively the same as denying it at that later anyway.

merlinc · 2020-12-08T11:54:18Z

app/_development-tools/controllers.js

@@ -0,0 +1,27 @@
+const { uniq } = require('lodash')


_development-tools does not thrill me, however it does make it obvious that its a "non-functional requirement" module.

Only alternative to this I can think of is ripping this out as a separate npm module and including it that way - but I think it's too early for us to know what that would look like just yet.

Yep, it didn't thrill me either...I also toyed with ./dev, ./development-only, ./_tools....the prefix sort of helped set it out above the rest of the "real" apps. But maybe that's not necessary as we don't do that with the component app which is only available in dev or on certain environments.

What about simple ./tools?

merlinc · 2020-12-08T11:55:25Z

config/nunjucks/globals.js

  FEEDBACK_URL,
  PERSON_ESCORT_RECORD_FEEDBACK_URL,
  SUPPORT_EMAIL,
 } = require('../')
+const {
+  mountpath: toolsUrl,


This is a path, not a full url

merlinc · 2020-12-08T11:57:19Z

app/_development-tools/controllers.js

+  })
+}
+
+function updatePermissions(req, res) {


Future enhancement idea - supporting updating by both form and JSON.

We used this on session injection to give humans an easy form, but then used the JSON updating as a more reliable approach for automated browser tests

Yep, that's a really nice idea! Nothing preventing us extending this to support a JSON body, or in fact just a POST within the e2e tests?

merlinc · 2020-12-08T11:58:02Z

docs/environment-variables.md

@@ -39,7 +39,8 @@
 | LOCATIONS_BATCH_SIZE                | Maximum number of location IDs to send in one request when requesting moves for all locations                                                                                                                                      | 40                                                                                  |
 | FRAMEWORKS_VERSION                  | Current [Book a secure move frameworks](https://github.com/ministryofjustice/hmpps-book-secure-move-frameworks/releases) version that the frontend will use to create new Person Escort Records or any other frameworks being used | latest supported version (see `@hmpps-book-secure-move-frameworks` in package.json) |
 | LOG_LEVEL                           | Level of logs to output                                                                                                                                                                                                            | production: `error`, development: `debug`                                           |
-| ENABLE_COMPONENTS_LIBRARY           | Whether to enable the component library and allowed it to be browsed on this environment                                                                                                                                           | false                                                          |
+| ENABLE_COMPONENTS_LIBRARY           | Whether to enable the component library and allow it to be browsed on this environment                                                                                                                                           | false                                                          |


📚 ❤️

teneightfive · 2020-12-08T12:33:04Z

This could help address #804

This adds an app that is only enabled in development or using a particular environment variable to aid with development and testing.

This adds another link item to the footer to access the permissions switcher.

codeclimate · 2020-12-09T17:00:12Z

Code Climate has analyzed commit 0534347 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (90% is the threshold).

This pull request will bring the total coverage in the repository to 98.0% (0.0% change).

View more on Code Climate.

teneightfive self-assigned this Dec 7, 2020

teneightfive requested review from merlinc, nimphal and solidgoldpig December 7, 2020 18:35

teneightfive temporarily deployed to booksecuremove-pr-1097 December 7, 2020 18:36 Inactive

teneightfive force-pushed the development/update-permissions branch from fbc67b2 to 600b511 Compare December 7, 2020 19:48

teneightfive temporarily deployed to booksecuremove-pr-1097 December 7, 2020 19:48 Inactive

nimphal approved these changes Dec 8, 2020

View reviewed changes

teneightfive force-pushed the development/update-permissions branch from 600b511 to fa52596 Compare December 8, 2020 11:16

teneightfive temporarily deployed to booksecuremove-pr-1097 December 8, 2020 11:16 Inactive

solidgoldpig approved these changes Dec 8, 2020

View reviewed changes

solidgoldpig reviewed Dec 8, 2020

View reviewed changes

merlinc reviewed Dec 8, 2020

View reviewed changes

teneightfive added 2 commits December 9, 2020 16:54

feat: Add support for development tools

cbfb4f4

This adds an app that is only enabled in development or using a particular environment variable to aid with development and testing.

chore: Add permissions dev tool link to footer

0534347

This adds another link item to the footer to access the permissions switcher.

teneightfive force-pushed the development/update-permissions branch from fa52596 to 0534347 Compare December 9, 2020 16:57

teneightfive temporarily deployed to booksecuremove-pr-1097 December 9, 2020 16:57 Inactive

teneightfive merged commit 9a0752c into master Dec 9, 2020

teneightfive deleted the development/update-permissions branch December 9, 2020 17:18

teneightfive linked an issue Apr 30, 2021 that may be closed by this pull request

Allow permissions/locations to be set during development #807

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial development tools #1097

Add initial development tools #1097

teneightfive commented Dec 7, 2020 •

edited

Loading

nimphal left a comment

solidgoldpig Dec 8, 2020

merlinc Dec 8, 2020

teneightfive Dec 8, 2020

merlinc Dec 8, 2020

merlinc Dec 8, 2020

teneightfive Dec 8, 2020

merlinc Dec 8, 2020

merlinc Dec 8, 2020

teneightfive Dec 8, 2020

merlinc Dec 8, 2020

teneightfive commented Dec 8, 2020

codeclimate bot commented Dec 9, 2020

Add initial development tools #1097

Add initial development tools #1097

Conversation

teneightfive commented Dec 7, 2020 • edited Loading

Proposed changes

What changed

Why did it change

Screenshots

Checklists

Testing

Automated testing

Manual testing

Environment variables

Other considerations

nimphal 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

teneightfive commented Dec 8, 2020

codeclimate bot commented Dec 9, 2020

teneightfive commented Dec 7, 2020 •

edited

Loading