Skip to content
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

Defining endpoint security through JDL #23740

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

SpiralUp
Copy link
Contributor

@SpiralUp SpiralUp commented Oct 4, 2023

This pull request introduces an extension to the generator-jhipster functionality by implementing role-based backend security defined through JDL. The new security feature facilitates the specification of role permissions for different entities directly within the JDL, streamlining the process of securing backend resources.

Key Features:

  • Define security configurations in JDL with new syntax, e.g.,
  secure entity1, entity2, entity3 with roles {
    ROLE_ADMIN allows (get, post, put, delete)
    ROLE_USER allows (get)
  }
  • Auto-generation of Java entity resource files with @Secured({}) annotations reflecting the specified role-based security settings.
  • Extended integration tests to verify the role-based security configurations.

Example Generated Code:

@PostMapping("/customers")
@Secured({ "ROLE_ADMIN" })
public ResponseEntity<Customer> createCustomer(@RequestBody Customer customer) throws URISyntaxException {

Additional Notes:

  • Preliminary grammar and parser definitions have been included for supporting further security types like privilege security, organizational security, parent based security, and relation based security. However, code generators for these additional security types have not been implemented in this commit.
  • This feature enhances the flexibility and control in securing backend resources, aligning with modern security best practices.

Testing:

  • Added test-integration/samples/jdl-entities/app-roles-security.jdl file that enhances provided app.jdl with security definition
  • Manual testing was performed to verify the correct generation of Java code based on various JDL security configurations.

Resolves #19201

This commit introduces a role-based security feature for the backend, allowing users to define security configurations using JDL clauses. Security can now be specified directly within JDL for various entities, where different roles with specified permissions are mapped to the corresponding Java entity resource files. This mapping auto-generates the requisite `@Secured({})` annotations on the endpoints, ensuring the right level of access control based on roles. Examples of the JDL security clauses include:
```
  secure entity1, entity2, entity3 with roles {
    ROLE_ADMIN allows (get, post, put, delete)
    ROLE_USER allows (get)
  }
```
and
```
  secure all except entity3 with roles {
    ROLE_ADMIN allows (get, post, put, delete)
    ROLE_USER allows (get)
  }
```
which would translate to `@Secured({ "ROLE_ADMIN" })` annotations in the generated Java code for specified endpoints. Additionally, the integration tests have been updated to consider these role-based security configurations.

Moreover, the groundwork for supporting other security types has been laid down through the inclusion of grammar and parser definitions. These additional security types encompass privilege security, organizational security, parent based security, and relation based security, although the code generators for these types are not included in this commit.

Resolves jhipster#19201
This commit introduces a role-based security feature for the backend, allowing users to define security configurations using JDL clauses. Security can now be specified directly within JDL for various entities, where different roles with specified permissions are mapped to the corresponding Java entity resource files. This mapping auto-generates the requisite `@Secured({})` annotations on the endpoints, ensuring the right level of access control based on roles. Examples of the JDL security clauses include:
```
  secure entity1, entity2, entity3 with roles {
    ROLE_ADMIN allows (get, post, put, delete)
    ROLE_USER allows (get)
  }
```
and
```
  secure all except entity3 with roles {
    ROLE_ADMIN allows (get, post, put, delete)
    ROLE_USER allows (get)
  }
```
which would translate to `@Secured({ "ROLE_ADMIN" })` annotations in the generated Java code for specified endpoints. Additionally, the integration tests have been updated to consider these role-based security configurations.

Moreover, the groundwork for supporting other security types has been laid down through the inclusion of grammar and parser definitions. These additional security types encompass privilege security, organizational security, parent based security, and relation based security, although the code generators for these types are not included in this commit.

Resolves jhipster#19201
@DanielFran DanielFran requested a review from mraible October 17, 2023 12:03
@OmarHawk
Copy link
Contributor

OmarHawk commented Oct 20, 2023

Nice work as a basic implementation!

Even though this is purely about endpoint security - e.g. what if an entity where there are GET permissions have relationships to other entities where there are no GET permissions... By loading the one where you have permissions on, you could still see parts of the contents of the other ones - allowing to bypass the role check...

@SpiralUp
Copy link
Contributor Author

Nice work as a basic implementation!

Even though this is purely about endpoint security - e.g. what if an entity where there are GET permissions have relationships to other entities where there are no GET permissions... By loading the one where you have permissions on, you could still see parts of the contents of the other ones - allowing to bypass the role check...

Thank you for your valuable feedback!

You rightly pointed out a concern regarding the exposure of related entities through the GET requests, despite the role-based restrictions. In this basic implementation, the focus was primarily on endpoint security to ensure controlled access based on roles.

Regarding the data returned through the GET requests, it's indeed handled by the definition of mappers for each entity type. I have worked on a version of changes where entities have updated mappers returning only basic information for related entities. However, I felt that including this in the current pull request might be an overreach, as it is not directly related to security but more about general entity to DTO mapping.

I am open to creating a separate pull request for the updated mappers if there's interest or it aligns with the project’s roadmap. This way, we can maintain a clear separation of concerns between security enhancements and entity mapping optimizations.

Looking forward to your thoughts and further suggestions!

@mraible mraible added $100 https://www.jhipster.tech/bug-bounties/ $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ labels Oct 24, 2023
@DanielFran
Copy link
Member

@SpiralUp Can you fix the conflicts?

@mraible Since you are the security specialist, any thoughts about this development?

@mshima
Copy link
Member

mshima commented Oct 25, 2023

I think a duplicate _entityClass_secRoles_ResourceIT.java.ejs by itself is a blocking.
That file is too complex to add a copy.

We should think on how to extract as a blueprint.

@SpiralUp
Copy link
Contributor Author

@SpiralUp Can you fix the conflicts?

Certainly! I'll resolve the conflicts and update the pull request shortly. Thank you for bringing this to my attention.

@SpiralUp
Copy link
Contributor Author

I think a duplicate _entityClass_secRoles_ResourceIT.java.ejs by itself is a blocking. That file is too complex to add a copy.

We should think on how to extract as a blueprint.

Thank you for the feedback regarding the duplication of _entityClass_secRoles_ResourceIT.java.ejs. I understand the concern over adding a complex file copy.

The main reason behind creating a duplicate was to ensure the standard functionality remains unaffected without the defined security, as the original file seemed too complex to modify directly. The duplicated file is responsible for generating tests, and it seemed appropriate to have tests generated for all roles with or without access to the entity.

For the initial phase, perhaps keeping these two files separate could be a pragmatic approach, with a view to merging them into one once we have a solid understanding and implementation that caters to both security and standard functionality seamlessly.

Regarding the extraction of this functionality to a blueprint, I'm not sure if JDL can be extended in a blueprint, as this functionality has add additional JDL clause 'secure … with …'?

Looking forward to your guidance and further discussions to improve this implementation.

@mraible
Copy link
Contributor

mraible commented Oct 30, 2023

@SpiralUp Please fix conflicts when you have time.

@mshima
Copy link
Member

mshima commented Oct 30, 2023

What about this structure:

roles ROLE_ADMIN, ROLE_INFRA {
  secure entity Foo allows (get, post)
  secure endpoint '/management/log' allows (get)
}
roles ROLE_MANAGER, ROLE_USER {
  secure entity Bar allows (get, post)
  secure endpoint '/management/log' denies (all)
}

@SpiralUp
Copy link
Contributor Author

@SpiralUp Please fix conflicts when you have time.

Certainly, I will resolve the conflicts this evening. Thank you, Matt.

@SpiralUp
Copy link
Contributor Author

What about this structure:

roles ROLE_ADMIN, ROLE_INFRA {
  secure entity Foo allows (get, post)
  secure endpoint '/management/log' allows (get)
}
roles ROLE_MANAGER, ROLE_USER {
  secure entity Bar allows (get, post)
  secure endpoint '/management/log' denies (all)
}

Thank you Marcelo for the proposed structure. It indeed makes sense as it not only allows for defining roles on entities but also extends the security definition to endpoints, which is a worthwhile direction to consider.

On the other hand, the implementation of security for entities and endpoints differs. Entity security is implemented at the REST controllers, while endpoint security is handled within SecurityConfiguration.java. This distinction might necessitate different approaches or additional considerations in how we structure the JDL definitions and the underlying implementation.

Furthermore, besides roles, I have implemented security on entities defined through privileges, parent entity, and organizational security. Although I haven't submitted a pull request for these yet, I plan to do so in the future if this PR is successfully merged.

I am open to making revisions to this PR, so if upon review, a better approach is identified, I can adapt the JDL definitions accordingly. Your feedback is invaluable, and I look forward to further discussions to refine this implementation.

@DanielFran
Copy link
Member

@SpiralUp I agree with @mshima's approach, and we can focus on this PR for the entities.

Besides that, we should consider roles as a set of ROLE_* and, for each one, have the list of entities/endpoints with affected permissions. It will allow the creation of a list of different roles and not need to use static names...
I did not like to add this logic for the different types of security:
image

@SpiralUp
Copy link
Contributor Author

@SpiralUp I agree with @mshima's approach, and we can focus on this PR for the entities.

Besides that, we should consider roles as a set of ROLE_* and, for each one, have the list of entities/endpoints with affected permissions. It will allow the creation of a list of different roles and not need to use static names... I did not like to add this logic for the different types of security: image

Thank you, Daniel, for your feedback and agreeing with Marcelo's broader approach. It indeed opens up more possibilities compared to the current implementation:

  • It allows for the definition of roles that may not relate to any entity and roles that pertain to endpoints.
  • With such an extended definition of roles, role creation could be facilitated, whereas the current implementation anticipates roles to be created manually.
  • In my present implementation, security defined through a role is associated with an entity and recorded in the entity's .json file. Probably, it would be better to have a separate .json file for roles, which could be stored somewhere within the .jhipster directory.
  • The existing implementation doesn't control role names, allowing any role to be specified regardless of whether it's been created in the system or not. The expanded definition could enforce the validation that a specified role exists in the system.

This approach is focused on security roles. What are your thoughts on other types of entity security? I would appreciate it if, besides role-based security, other types of entity security could also be defined. For instance, allowing each customer to see only the rows in the table that belong to them/their company, or defining access to document items through the access to the document.

Does the role-based security for endpoints pertain only to management endpoints, or are there other endpoints also considered?

@DanielFran
Copy link
Member

@SpiralUp Can you update the branch and fix the conflicts?

@SpiralUp I agree with @mshima's approach, and we can focus on this PR for the entities.
Besides that, we should consider roles as a set of ROLE_* and, for each one, have the list of entities/endpoints with affected permissions. It will allow the creation of a list of different roles and not need to use static names... I did not like to add this logic for the different types of security: image

Thank you, Daniel, for your feedback and agreeing with Marcelo's broader approach. It indeed opens up more possibilities compared to the current implementation:

* It allows for the definition of roles that may not relate to any entity and roles that pertain to endpoints.

* With such an extended definition of roles, role creation could be facilitated, whereas the current implementation anticipates roles to be created manually.

* In my present implementation, security defined through a role is associated with an entity and recorded in the entity's .json file. Probably, it would be better to have a separate .json file for roles, which could be stored somewhere within the .jhipster directory.

* The existing implementation doesn't control role names, allowing any role to be specified regardless of whether it's been created in the system or not. The expanded definition could enforce the validation that a specified role exists in the system.

This approach is focused on security roles. What are your thoughts on other types of entity security? I would appreciate it if, besides role-based security, other types of entity security could also be defined. For instance, allowing each customer to see only the rows in the table that belong to them/their company, or defining access to document items through the access to the document.

Does the role-based security for endpoints pertain only to management endpoints, or are there other endpoints also considered?

I believe we should simplify and focus just on entities here.
We can then do enhancements depending on other scenarios: for exemple, for multi-tenancy, there is some blueprints implemented for that and they might extend the security feature to allow that

@SpiralUp
Copy link
Contributor Author

@SpiralUp Can you update the branch and fix the conflicts?

I believe we should simplify and focus just on entities here. We can then do enhancements depending on other scenarios: for exemple, for multi-tenancy, there is some blueprints implemented for that and they might extend the security feature to allow that

Hi Daniel,

Thank you for your feedback.

  1. Code Conflicts Resolved: I have updated the branch and resolved all conflicts. The latest code should now be conflict-free and in line with the current project state.

  2. Method Removal: As per our discussion, I've removed the resetSecurity method to streamline the implementation.

  3. Role Improvements: Regarding the enhancements related to roles, I plan to address these soon. Currently, I'm juggling a few priorities, but I'll make sure to incorporate these improvements as soon as I can. Your suggestions about role-based security and its implications are certainly on my radar for future updates.

  4. Multi-Tenancy: I'll also take some time to review the existing blueprints that implement multi-tenancy. Interestingly, I've already developed a solution for organizational security, which closely resembles a multi-tenancy approach. It allows each company or organizational unit to access only its relevant data. However, I'd like to present and discuss it with the team first, as I'm not entirely sure if it aligns with the current development direction and standards.

Looking forward to your thoughts on these updates and any further guidance you might have!

Best regards,
Ivan

@github-actions github-actions bot added the Stale label Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
$$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ Stale theme: java theme: jhipster-internals $100 https://www.jhipster.tech/bug-bounties/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defining endpoint security through JDL
5 participants