-
Notifications
You must be signed in to change notification settings - Fork 1
Develop/feature/td 5829 5862 shared architecture and bff 2 #1290
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
base: LHKentia
Are you sure you want to change the base?
Develop/feature/td 5829 5862 shared architecture and bff 2 #1290
Conversation
… into as first code to become shared.
…ributeScreen-AccessDenied TD-5727-Fix for Full user who is not a catalogue editor/admin can access My contribution page by navigating to the link directly
…ursetray-Reorder-resource-tabs TD-5700-Reorder resource tabs on LH home page
/// that secure or private configuration data is not inadvertently exposed to clients. | ||
/// </para> | ||
/// </summary> | ||
public class PublicSettings : IPublicSettings |
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.
If Settings was broken up into more classes/objs in the appsettings we could inject just the bits we are using, -> least knowledge and interface segregation. But I expect there would be alot involved in doing this so I am expect appsetting will stay the same and we will inject PublicSettings and have PublicX per obj, (public may be the wrong word choice).
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.
@binon Do you agree with this approach?
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.
Indeed, naming PublicSettings might be misleading if it implies exposure to external clients; it’s important to be very explicit about who “public” means in this context to avoid confusion among developers. Adapting more granular approach is better.
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.
Agreed, theyre not really public theyre just not secret because they get exposed so they need some naming to signify "Do not put Sensitive Information Here" or something. i couldnt think of anything better
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.
agreeing with binon's opinion-it can create confusion. can we use some FrontendConfig or some other names ?
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.
So I want a consistent word so that i can do Settings : ISettings, IPublicSettings
So that we know whenever we use settings it can provide the implementations to either. so repeating the original name of the interface with an additional word to signify a subset of it I think is frontend ... in the previous meeting it was suggested we can rely on the comments to enforce not putting sensative data where it shoudnt be. But a word that really communicates it i think would be ideal.
FrontendConfig i dont think is right because though the front end does prefer not to expose it actualy the service is being given far more scope that it wants. So frontend i dont think is right.
Config I like better if it was Settings: ISecretSettings, IConfigSettings
but i dont think we will split it like this i think we will be leaving ISettings the same and then interfacing to capture a subset of the same values, (for convenience no other reason).
Other options
Settings: ISettings, IExposableSettings <-- this is my favourate so far?
Settings : ISettings, INonSensitiveSettings
Settings : ISettings, IUnsecuredSettings
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.
Settings: ISettings, IExposableSettings <-- -I think we can go ahead with this naming convention
{ | ||
using LearningHub.Nhs.Models.Enums; | ||
using Microsoft.AspNetCore.Mvc.Rendering; |
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.
If we are not using this, it is only for comments here, i think we shouldnt include it. Or use System.X libraries so we don't pull through unneeded dependencies which arn't support in blazor for example, but generally more generic, more framework-neutral options are better for compatibility.
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.
@AnjuJose011 I would like a meeting about this i think there isnt enough seperation we have infastructure and presentation concerns in models etc the shared project i think needs to be agnostic, no aspnetcore.asp
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.
Agree
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.
@Phil-NHS this is what discussed in today's meeting and seems a real blocker .you can please create a subtask and add more details in that ticket
/// </summary> | ||
public string LearningHubApiUrl { get; set; } | ||
|
||
public IFindwiseSettingsPublic FindwiseSettings { get; set; } |
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.
This is so we map the reduced FindwiseSettings and get just what we have defined as safe. My preference would be not putting PublicSettings into SearchService but inject two separate interface with just the information it needs. this may not be correct.
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.
@binon do you think this is okay, i would continue this naming with everything, do you think its okay?
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.
Injecting two separate interfaces with only the needed information is more in line with the Interface Segregation Principle and results in better encapsulation. I am not sure how many more interfaces we need to add. So be mindful that injecting many small interfaces can also increase complexity in dependency management, so it’s important to find a balance.
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.
However, bundling multiple settings interfaces (like FindwiseSettings inside IPublicSettings) into a single larger interface can lead to unnecessary coupling — it forces consumers to depend on more than they actually need.
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.
Everywhere we inject settings, i think we should inject just the setting group we need ... e.g. findwise, if that means injecting findwise and apiGroupEg then thats fine. So more grouping of appsettings would be great but i am not going to push for it because i know its disruptive. but what will need to happen is for every group there is then a public interface should be created (as needed) because why inject what we dont need, and the private stuff should only be needed in specific places anyway.
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.
looked at this again this is mirroring the existing settings class. which is why it has the nested interface. So this is to mirror current implementation. So agreed but this is the context behind why it is as it is.
However, bundling multiple settings interfaces (like FindwiseSettings inside IPublicSettings) into a single larger interface can lead to unnecessary coupling — it forces consumers to depend on more than they actually need.
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.
we can rename the public setting class name to avoid confusion. agreeing here with binon- bundling multiple settings interfaces can lead to unnecessary coupling and also can increase the complexity .in the current implementation we are using a setting class(Setting.cs) that contains all related nested classes(findwisesetting.cs)
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.
i didnt introduce the findwisesettings
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.
looked at this again this is mirroring the existing settings class. which is why it has the nested interface. So this is to mirror current implementation. So agreed but this is the context behind why it is as it is.
-answering to this comment from your end -findwisesettings is the current setting class file we are using in Leraning Hub .interfaces you have created looks good .but i have the same opinion here with binon regarding coupling .
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.
I agree i dont know what to do about things like this though in term of them being concidered for tasks. let me pm you coz there are few thing i think could be considered but they are not to do with the pr.
/// <summary> | ||
/// Gets the base URL of the API. | ||
/// </summary> | ||
string ApiUrl { get; } |
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.
Other versions of this functionality in other project dont have APIUrl this comes from the baseHttpClient
@@ -20,7 +21,7 @@ | |||
/// <summary> | |||
/// The abstract api http client. | |||
/// </summary> | |||
public abstract class BaseHttpClient | |||
public abstract class BaseHttpClient : IAPIHttpClient |
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.
This will allow blazor to inject clients into services in the same way MVC does by using the interface.
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
This is a neat and clean approach
ILearningHubHttpClient learningHubClient, | ||
IUserApiHttpClient userAPIClient, | ||
IOpenApiHttpClient openAPIClient, | ||
IOptions<BFFPathValidationOptions> bffPathValidationOptions) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comments: Would like any points of view on this
/// Validates the path against allowed and blocked segments. | ||
/// </summary> | ||
private bool IsPathAllowed(string path) | ||
{ |
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.
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.
@@ -81,6 +85,7 @@ public static void AddLearningHubMappings(this IServiceCollection services, ICon | |||
|
|||
// Config | |||
services.Configure<OpenAthensScopes>(configuration.GetSection("OpenAthensScopes")); | |||
services.Configure<BFFPathValidationOptions>(configuration.GetSection(BFFPathValidationOptions.SectionName)); |
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.
I like doing it this way, we may want to then nest stuff. but seems nice maybe could interface or something make this a norm?
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.
@binon should we as standard make our classes have a section name attrinute?
…s/TD-5856_reload_moodle_user Develop/fixes/td 5856 reload moodle user
Hi @AnjuJose011 I pulled from ivory i dont understand why i have conflicts at all, i cant seem to resolve them in the editor. |
To build i had to update to 0.5 models but its not available yet this should stop failing once models update package relaseed |
https://hee-tis.atlassian.net/wiki/spaces/TP/pages/4939153413/LearningHub.Nhs.Shared |
please can we discuss whether we should be using httpfactories, they seem vestigial in the base controllers, facades or just the clients |
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.
If LearningHub.Nhs.Shared is intended for use across multiple solutions, have you considered versioning it to improve dependency management? How are we going to manage dependencies effectively?
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.
Everything looks good overall. Just one suggestion: since each third-party interface (IOpenApiHttpClient, IUserApiHttpClient, ILearningHubHttpClient) involves the use of HttpClient, it might be cleaner and more maintainable to consolidate the configuration into a single, shared HttpClient handler or factory where appropriate. This would help avoid duplication and promote consistency.
Also, just a note—Findwise appears to have a slightly different configuration approach, so we may want to handle that separately or explicitly document the deviation.
@Phil-NHS, could you pleas check the conflicts? |
I liked the centralisation approach. I started thinking about this here And i am starting to think out architecture needs to be better to do what were doing here. And we need much better separation. - i am going to try and rewrite search as an example i think, just to tease out infastructure stuff i can and mvc presentation stuff. Ive interfaced and stubbed some stuff but i am now thinking it needs more. And that DTO and Buisness Services should be the only things in it, or application services but good interfaces for what they orchestrate?. |
Ive previously resolved them they just appear as new thing join ivory i think ... the current ones are now because i am using the not yet released models package version, i dont understand why this is an issue seeing i needed it to resolve the previous clash. So I am not going to until i know i am merging because i keep having to keep up, and also i dont know how to resolve it until the package is released |
…ge we plan not captured by the PR
I don't know. Centralisation will help. And I havent considered versioning LearningHub.Nhs.Shared I think i'd need someone else to lead that discussion as i think the pros and cons would be very much based on experience of the difficulties of doing or not doing this and experience of the consequences in the past in terms of when and how issues caused by this are detected. |
/// that secure or private configuration data is not inadvertently exposed to clients. | ||
/// </para> | ||
/// </summary> | ||
public class PublicSettings : IPublicSettings |
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.
agreeing with binon's opinion-it can create confusion. can we use some FrontendConfig or some other names ?
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.
please correct the name space
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.
Do you mean because we wont be moving it in the scope of this release or that utility helper wouldnt be shared at the top level?
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.
Apologies, in your local branch, the namespace for the utility helper class is currently: namespace LearningHub.Nhs.WebUI.Helpers However, it should be namespace LearningHub.Nhs.Shared.Helpers ?
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.
Ah yes good catch! (didnt see it in chat part of pr but clear in the code bit :) )
/// </summary> | ||
public string LearningHubApiUrl { get; set; } | ||
|
||
public IFindwiseSettingsPublic FindwiseSettings { get; set; } |
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.
we can rename the public setting class name to avoid confusion. agreeing here with binon- bundling multiple settings interfaces can lead to unnecessary coupling and also can increase the complexity .in the current implementation we are using a setting class(Setting.cs) that contains all related nested classes(findwisesetting.cs)
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.
all this files are came from ivory release branch i think .no issues if we are merging these change into LHKentila branch .as per our release pipeline Lh Kentia is after Ivory release
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.
I expect some of them, i think alot of this is moving commonly used file to the shared folder results in lots of services and some helpers needing to be updated so they know where to find the classes they need now theyre in a new location.
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.
can we remove ////qqq comment from here or can you please add a meaningful message?
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.
Yep good catch its because its not going into production. but this is a mistake you may see again if i dont tidy up after myself its how i rememember to comeback to things. here is it is there to mark the breaking library.
@Phil-NHS me and @ArunimaGeorge reviewed this PR As per today's discussion, the plan is to merge structure changes only for the initial scope right , which is LearningHub.Nhs.Shared (containing only folders and empty implementations). Given that, shared concept-related changes—considering future implementation—looks ok . I agree with binon’s comments regarding coupling of multiple interfaces and the complexities involved; Also we need to treat findwise settings separately. these can be addressed in a later version when we complete our first view component implementation. |
Yes. And thanks very very much for going though it I'm less familiar with the branching process so as i understood it.
|
…ure/TD-5888-Make-LH-able-to-Host-Blazor Develop/feature/td 5888 make lh able to host blazor Merging so against one task branch which we will take only certain files from anyway
JIRA link
TD-5829
Description
I've created a Shared Project. This project is for the purpose of sharing coding across project in the solution and is intended to in future be used instead of internal model packages. Changes should not affect the App but should allow and encourage future development to share more code resources across projects. It should also ultimately reduce duplication in the code base.
Interface are used, including marker interfaces, for specifying configurations of services being injected, and to allow injected services to be providers for multiple more specific interfaces enabling services to have less knowledge and access to methods and data they do not use.
Settings and other configuration are being split with an interface whilst maintain the current appsettings structure. This is so if only non secure data is needed in a service then only that is injected, which also supports it being useable clientside safely.
E.g. Something like:
It seems alot of files have been moved but this is due to them all being connected to the SearchService.
I've included in the Shared Project
But I assume ultimately we would migrate from it to shared but that it would not be achievable to start without it.
I recommending starting with the Shared SearchService, then PublicSettings, then HttpClients
Several recommendations and points of discussions come from this task.
Screenshots
NA
Developer checks
(Leave tasks unticked if they haven't been appropriate for your ticket.)
I have:
Either:
Or:
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: