-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable and config authentication as default #466
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://eaflood.atlassian.net/browse/WATER-4085 https://eaflood.atlassian.net/browse/WATER-4155 As the final step of supporting pass-through authentication from the legacy UI app and before we can implement our first page we need to enable authentication on all routes as the default. This means in future, when we add a new route requests will be authenticated unless we disable authentication in the route's config. The change involves telling happy to use auth by default then going through our existing routes and disabling it! 😁 We'll also use the opportunity to do a little tidying up of the route files as we go.
We have to set it in the plugin to ensure because we found that if set _after_ the routes have been added (in our case the RouterPlugin) none of them get registered as routes requiring authentication/authorisation.
All our routes were built without the need to authenticate. Arguably, some like changing a billing account address or creating a new bill run should require it. So, we've double-checked the [water-abstraction-service](https://github.com/DEFRA/water-abstraction-service) and have applied the same scopes used on its create bill run and invoice account endpoints. Also as we are having to touch every route file, we also take the opportunity to - ensure the option properties are ordered alphabetically - ensure they are consistent
In the previous change we added config to say that a requests had to be authenticated to access `/bill-runs` and `/billing-accounts`. On top of that the user has to have a matching scope. This broke our existing controller tests because they were not setup to include authentication in the test requests. Thankfully Hapi allows you a way to bypass the authentication by including an `auth` object in the options you pass to [server.inject()](https://hapi.dev/api/?v=21.3.2#-await-serverinjectoptions). To be honest, we wasted a lot of time trying to find examples of how to handle auth in tests many of which led us completely astray. They just don't seem to be out there. Thankfully, an old Stackoverflow post (https://stackoverflow.com/a/31955942/6117745) pointed us in the right direction. It seems if you include an `auth.credentials` object Hapi will just 'pretend' that was the result of the authentication strategy. This also means `validate()` and therefore `AuthService.go()` never get called (so no need to stub them). But you still need to provide a matching scope for the route being tested to prevent a `403` being thrown.
In this case it wasn't about adding credentials to the requests, but configuring the test routes created in the unit tests to not require authentication.
Added when trying something and didn't spot it was left in. Reverting the change reduces the number of files being touched.
Jozzey
approved these changes
Oct 19, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beckyrose200
approved these changes
Oct 19, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cruikshanks
added a commit
to DEFRA/water-abstraction-ui
that referenced
this pull request
Nov 1, 2023
https://eaflood.atlassian.net/browse/WATER-4085 When we [enabled authentication by default](DEFRA/water-abstraction-system#466) in **water-abstraction-system** we just had in mind requests proxied through or redirected from **water-abstraction-ui**. We overlooked requests the UI was generating itself using `ServiceClient` from [water-abstraction-helpers](https://github.com/DEFRA/water-abstraction-helpers). Doh! 🤦 So, we've inadvertently broken SROC supplementary billing and changing a billing account address because the requests we're sending to **water-abstraction-system** are being redirected to the `/signin` page. The 'quick' fix would have been to remove auth off those routes but that would expose a vulnerability if we didn't then try to block them in `src/internal/modules/system-proxy`. What **water0abstraction-system** needs is the cookie the UI has set once someone has been authenticated. It gets passed automatically when requests are proxied or redirected. So, this change grabs that cookie of the [Hapi request](https://hapi.dev/api/?v=21.3.2#request) and passes it through to the HTTP request generated by **water-abstraction-helpers**. SROC supplementary billing and changing a billing account address work again!
Cruikshanks
added a commit
to DEFRA/water-abstraction-ui
that referenced
this pull request
Nov 1, 2023
https://eaflood.atlassian.net/browse/WATER-4085 When we [enabled authentication by default](DEFRA/water-abstraction-system#466) in **water-abstraction-system** we just had in mind requests proxied through or redirected from **water-abstraction-ui**. We overlooked requests the UI was generating itself using `ServiceClient` from [water-abstraction-helpers](https://github.com/DEFRA/water-abstraction-helpers). Doh! 🤦 So, we've inadvertently broken SROC supplementary billing and changing a billing account address because the requests we're sending to **water-abstraction-system** are being redirected to the `/signin` page. The 'quick' fix would have been to remove auth off those routes but that would expose a vulnerability if we didn't then try to block them in `src/internal/modules/system-proxy`. What **water0abstraction-system** needs is the cookie the UI has set once someone has been authenticated. It gets passed automatically when requests are proxied or redirected. So, this change grabs that cookie of the [Hapi request](https://hapi.dev/api/?v=21.3.2#request) and passes it through to the HTTP request generated by **water-abstraction-helpers**. SROC supplementary billing and changing a billing account address work again!
Cruikshanks
added a commit
to DEFRA/water-abstraction-ui
that referenced
this pull request
Nov 1, 2023
https://eaflood.atlassian.net/browse/WATER-4085 When we [enabled authentication by default](DEFRA/water-abstraction-system#466) in **water-abstraction-system** we just had in mind requests proxied through or redirected from **water-abstraction-ui**. We overlooked requests the UI was generating itself using `ServiceClient` from [water-abstraction-helpers](https://github.com/DEFRA/water-abstraction-helpers). Doh! 🤦 So, we've inadvertently broken SROC supplementary billing and changing a billing account address because the requests we're sending to **water-abstraction-system** are being redirected to the `/signin` page. The 'quick' fix would have been to remove auth off those routes but that would expose a vulnerability if we didn't then try to block them in `src/internal/modules/system-proxy`. What **water0abstraction-system** needs is the cookie the UI has set once someone has been authenticated. It gets passed automatically when requests are proxied or redirected. So, this change grabs that cookie of the [Hapi request](https://hapi.dev/api/?v=21.3.2#request) and passes it through to the HTTP request generated by **water-abstraction-helpers**. SROC supplementary billing and changing a billing account address work again!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eaflood.atlassian.net/browse/WATER-4085
https://eaflood.atlassian.net/browse/WATER-4155
As the final step of supporting pass-through authentication from the legacy UI app and before we can implement our first page, we need to enable authentication on all routes as the default.
This means in future, when we add a new route requests will be authenticated unless we disable authentication in the route's config.
The change involves telling happy to use auth by default then going through most of our routes and disabling it! 😁
Where it is applicable we update the route config to include the 'scope' authenticated users must have to be authorised to access it.
We'll also use the opportunity to do a little tidying up of the route files as we go.