Develop/feature/td 5862 add an internal bff api endpoint 2 #1288
Conversation
| /// It may be extended in the future with user-specific functionality or properties. | ||
| /// </para> | ||
| /// </summary> | ||
| public interface IOpenApiHttpClient : IAPIHttpClient |
There was a problem hiding this comment.
The introduction of marker interface is so we can standardised the apis. This allows the blazor client and other projects share them and implement them differently. However another facade has been introduced, for openapi, during these tasks being created. it is probably worth at this point whether we should (i think so) we standardised and define the apis, and interfaces and how they are hit. or if its all already thought through share the outcome.
| @@ -0,0 +1,25 @@ | |||
| namespace LearningHub.Nhs.WebUI.Configuration | |||
There was a problem hiding this comment.
this is the configuration file for mapping appsettings which as a test example looks like this
ive included the line above so you can find where it sits in appsettings
"SupportUrlExcludedFiles": "https://localhost/resource/excludedfiles",
"BFFPathValidation": {
"AllowedPathPrefixes": [
"search/",
"catalogue/",
"Resource/GetArticleDetails",
"Resource/GetAudioDetails",
"Resource/GetFileTypes"
],
"BlockedPathSegments": [
"Admin",
"User/Delete",
"Sensitive"
]
},
| /// <summary> | ||
| /// Gets the section name for BFF path validation options. | ||
| /// </summary> | ||
| public const string SectionName = "BFFPathValidation"; |
There was a problem hiding this comment.
i like this and how it removes hard coding in service mapping, i'd even consider interfacing it or something, unsure how it would work with nesting etc but i like i
| ILearningHubHttpClient learningHubClient, | ||
| IUserApiHttpClient userAPIClient, | ||
| IOpenApiHttpClient openAPIClient, | ||
| IOptions<BFFPathValidationOptions> bffPathValidationOptions) |
There was a problem hiding this comment.
I didnt put the name public in the interface. i didnt know i should in the other pull request i wanted to use interfacing to split appsettings into public and not. this is consumed serverside so is fine but i am unsure if it should signal public or not ...
| if we are redirected the client may not handle it as it isnt the token holder so we need to continue using the bff until we get the outcome | ||
| if the BFF caller is not expecting redirects but only data they should handle the 302 response and redirect themselves. | ||
| E.g. A compontent that uses the BFF to fetch data may not be appropriate for redirecting to a specific page so the consuming client may need to have a way of handling page redirects. | ||
| */ |
There was a problem hiding this comment.
the comments: Would like any points of view on this
| { | ||
| var normalizedPath = path?.Trim('/').ToLowerInvariant() ?? string.Empty; | ||
|
|
||
| // Check blacklist first |
There was a problem hiding this comment.
black list and white list may have cracks but seemed a sensible start. specifically defining all endpoints getting used is not very flexible. it seems a good approach but would be swayed by any strong opinons on it.
| ServerCertificateCustomValidationCallback = | ||
| HttpClientHandler.DangerousAcceptAnyServerCertificateValidator, | ||
| }); | ||
|
|
There was a problem hiding this comment.
This comes from the RC branch what is it. what is the rational
|
|
||
| // Config | ||
| services.Configure<OpenAthensScopes>(configuration.GetSection("OpenAthensScopes")); | ||
| services.Configure<BFFPathValidationOptions>(configuration.GetSection(BFFPathValidationOptions.SectionName)); |
There was a problem hiding this comment.
I really like this rather than hard coding the appsettings name its a tiny thing but i like it
Phil-NHS
merged commit 6ac95a8
into
Develop/Feature/TD-5829-Introduce-and-Integrate-a-Shared-Project-into-the-LH-Solution
Phil-NHS
changed the title
1 participant
JIRA link
TD-5862
**I pulled a more recent version of RC, i dont know if i should be pulling on the recent rc into 5829, first at the moment alot of the changes are from RC version mismatch between first and second task **
Description
OpenApi and SearchService are affected this is actually due to merge conflicts with TD-5829
The main thrust of this task is creating a same site cookie authenticated bff to enable api calls that would go through the internal apis that are getting removed to go through instead.
There are appsettings changes as well.
The bff wont be in use until we use it with Blazor or if we want to use it for other calls.
There is discussion of the BFF in the confluence and further information in the ticket.
Some meeting notes and discussion of architecture
I have created a task that may or may not be done in future depending if it is needed. We use a same site cookie with Authentication on the BFF this keeps security concerns server-side rather than having tokens in client storage for example.
It is same site so safe it is like how csrf? on forms work. We do have on the openapi a policy for unauthenticated there are solutions to navigate this if we find we cannot hit them via the bff due to the bff refusing the initial request. (I have not confirmed any limitation of the same site cookie and there are multiple approaches to solutions so it is a little out of scope for now)
Another complexity is I believe that the apis return 3xx response that are automatically redirected by the base apihttpclient which is fine for MVC but not for blazor where it is a component post not a page post. The blazor version of the httpclient will be expected to handle this. The correct way to handle it will have options.
Screenshots
N/A
Developer checks
(Leave tasks unticked if they haven't been appropriate for your ticket.)
I have:
Either:
Or: