Attribute based access controll support #499
Conversation
| .POST(BodyPublishers.ofString(requestBody)) | ||
| .build(); | ||
|
|
||
| return HttpClient.newHttpClient().send(request, BodyHandlers.ofString()); |
There was a problem hiding this comment.
It is not directly linked to this task but as a potential (future?) improvement, I would suggest to that we could reuse the HttpClient instance configured through ClientConfiguration and ClientProperties.
I do not know i EHRbase requires other external accesses than ABAC server and remote terminology servers?
It could be nice to have common configurations for all HTTP clients. For instance, if EHRbase running behind an authenticated proxy or if an ABAC instance also requires two-way authentication.
There was a problem hiding this comment.
I added the foundation for this in the last commit. I does only allow to set a proxy yet, because I'm not sure how real auth requirements would look like. Can you recheck this too, and check if this is the direction you had in mind?
application/src/main/java/org/ehrbase/application/abac/AbacConfig.java
Outdated
Show resolved
Hide resolved
| @@ -30,6 +30,7 @@ | |||
| import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; | |||
There was a problem hiding this comment.
For the readability and maintenance, I think that we should create separate configuration classes like : SecurityConfiguration, BasicSecurityConfigucation, OAuth2SecurityConfiguration
And we could enable the correct one using @ConditionalOnProperty annotation
There was a problem hiding this comment.
Good idea! I refactored the config classes to be separated like you suggest. Can you check again if that looks okay?
application/src/main/java/org/ehrbase/application/abac/MethodSecurityConfig.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/ehrbase/application/abac/CustomMethodSecurityExpressionRoot.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/ehrbase/application/abac/CustomMethodSecurityExpressionRoot.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/ehrbase/application/abac/CustomMethodSecurityExpressionRoot.java
Outdated
Show resolved
Hide resolved
...-openehr/src/main/java/org/ehrbase/rest/openehr/controller/OpenehrCompositionController.java
Outdated
Show resolved
Hide resolved
rest-openehr/src/main/java/org/ehrbase/rest/openehr/controller/OpenehrDirectoryController.java
Outdated
Show resolved
Hide resolved
rest-openehr/src/main/java/org/ehrbase/rest/openehr/controller/OpenehrEhrStatusController.java
Outdated
Show resolved
Hide resolved
rest-openehr/src/main/java/org/ehrbase/rest/openehr/controller/OpenehrQueryController.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/ehrbase/rest/openehr/controller/OpenehrVersionedEhrStatusController.java
Outdated
Show resolved
Hide resolved
...-openehr/src/main/java/org/ehrbase/rest/openehr/controller/OpenehrCompositionController.java
Show resolved
Hide resolved
| import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| @ConditionalOnProperty(name = "abac.enabled") |
There was a problem hiding this comment.
I think it is better to create the @bean in AbacConfig class in that specific case. I would say that @Component should be used for beans that are always created.
It is my own opinion and probably a personal preference .
There was a problem hiding this comment.
Thanks! I refactored it.
| @Configuration | ||
| @EnableConfigurationProperties | ||
| @ConfigurationProperties(prefix = "httpclient") | ||
| public class HttpClientConfig { |
There was a problem hiding this comment.
Proxy settings was just an example but I am sad that you do not want to use mine ? 😄
https://github.com/ehrbase/ehrbase/blob/87bc85527da3eca9b3d135c38c0f94114731690a/service/src/main/java/org/ehrbase/configuration/client/HttpClientConfiguration.java
There was a problem hiding this comment.
Oh interesting, didn't knew it was there. I'm not sure now. Why is yours in the service package? In the end two different http client config are obviously not a good practice. But it feels wrong to pull that client from the service layer into my ABAC stuff.
There was a problem hiding this comment.
Since there was no activity for 7 days I extracted that into the following issue, so I can continue with this PR here.
https://github.com/ehrbase/project_management/issues/522
|
Kudos, SonarCloud Quality Gate passed! |
Changes
A lot of configuration possible. See
application.ymlfor information.For a meta-perspective see
AbacIntegrationTest.java.For the implementation itself the
CustomMethodSecurityExpressionRoot.The whole feature is rather huge and complex, so see https://wiki.vitagroup.ag/display/ETHERCIS/ABAC+Integration for a better overview.
Related issue
closes https://github.com/ehrbase/project_management/issues/505
Additional information and checks