Skip to content

add REST API interceptors for handling more diverse permissions (identity links, variables, conditional events)#3141

Closed
tiffmaelite wants to merge 10 commits into
flowable:mainfrom
tiffmaelite:feature/rest-api-interceptors-for-identitylinks
Closed

add REST API interceptors for handling more diverse permissions (identity links, variables, conditional events)#3141
tiffmaelite wants to merge 10 commits into
flowable:mainfrom
tiffmaelite:feature/rest-api-interceptors-for-identitylinks

Conversation

@tiffmaelite
Copy link
Copy Markdown
Contributor

@tiffmaelite tiffmaelite commented Dec 10, 2021

I extended the CMMN REST API interceptor and the BPMN REST API interceptor such that it becomes possible to react on a request to access, create, edit or delete identity links for cases, processes and tasks.
In the first draft, I also made the new method generic enough to be able to use it in the future for other kinds of action intercepting, if it were to become a use case (e.g. need to intercept requests to alter entity links, comments or attachments)

Check List:

  • Unit tests: NO
  • Documentation: NA

@tiffmaelite tiffmaelite force-pushed the feature/rest-api-interceptors-for-identitylinks branch 2 times, most recently from b89cd9a to 0fc2b64 Compare December 10, 2021 16:40
@tiffmaelite
Copy link
Copy Markdown
Contributor Author

I now applied the style settings files of the project, which I had forgotten to use.

@filiphr
Copy link
Copy Markdown
Contributor

filiphr commented Dec 13, 2021

Thanks for the PR @tiffmaelite.

I also made the new method generic enough to be able to use it in the future for other kinds of action intercepting, if it were to become a use case (e.g. need to intercept requests to alter entity links, comments or attachments)

I understand where you are coming from. However, we are not using that pattern in the interceptors anywhere else. Therefore, can we please use dedicated methods for each action. e.g.

  • accessTaskIdentityLinks
  • updateTaskIdentityLinks
  • deleteTaskIdentityLinks
  • createTaskIdentityLinks

In addition to that some of the endpoints are invoking the access instance info by id interceptor, e.g. when getting the case instance identity links. Perhaps here we should not invoke that interceptor and only invoke the access identity links one.

@tiffmaelite tiffmaelite marked this pull request as draft December 13, 2021 15:37
@tiffmaelite
Copy link
Copy Markdown
Contributor Author

@filiphr : API was changed as requested
I am not sure whether any additional permission check on the task/process/case is actually necessary (they still can be done in the new methods if needed), so I removed all of them, but let me know if you disagree

@tiffmaelite
Copy link
Copy Markdown
Contributor Author

Thanks for the PR @tiffmaelite.

I also made the new method generic enough to be able to use it in the future for other kinds of action intercepting, if it were to become a use case (e.g. need to intercept requests to alter entity links, comments or attachments)

I understand where you are coming from. However, we are not using that pattern in the interceptors anywhere else. Therefore, can we please use dedicated methods for each action. e.g.

  • accessTaskIdentityLinks
  • updateTaskIdentityLinks
  • deleteTaskIdentityLinks
  • createTaskIdentityLinks

In addition to that some of the endpoints are invoking the access instance info by id interceptor, e.g. when getting the case instance identity links. Perhaps here we should not invoke that interceptor and only invoke the access identity links one.

fixed

@tiffmaelite tiffmaelite marked this pull request as ready for review December 14, 2021 08:21
@tiffmaelite tiffmaelite marked this pull request as draft December 14, 2021 08:35
@tiffmaelite tiffmaelite force-pushed the feature/rest-api-interceptors-for-identitylinks branch 6 times, most recently from 3925e8d to e3040d4 Compare December 14, 2021 11:22
add license headers to new files

fix code style
@tiffmaelite tiffmaelite force-pushed the feature/rest-api-interceptors-for-identitylinks branch 2 times, most recently from 4ba8f16 to 1c74615 Compare December 14, 2021 14:07
keep feature of ensuring existence of entity for which identity links are accessed or modified

apply review comments to ensure the existence check is done before calling the interceptors and to pass all relevant parameters to the interceptors
@tiffmaelite tiffmaelite force-pushed the feature/rest-api-interceptors-for-identitylinks branch from 1c74615 to 4f67263 Compare December 14, 2021 14:11
@tiffmaelite tiffmaelite marked this pull request as ready for review December 14, 2021 15:07
Copy link
Copy Markdown
Contributor

@filiphr filiphr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes @tiffmaelite. I've added some more comments where we can make some things more granular. Can you please have a look at my last comments and let me know what you think

@tiffmaelite tiffmaelite force-pushed the feature/rest-api-interceptors-for-identitylinks branch from fc42e6c to e998ca6 Compare December 20, 2021 16:11
filiphr and others added 2 commits December 20, 2021 18:19
@tiffmaelite tiffmaelite force-pushed the feature/rest-api-interceptors-for-identitylinks branch from d14d395 to eea0403 Compare December 21, 2021 09:50
@tiffmaelite tiffmaelite requested a review from filiphr December 21, 2021 10:28
@tiffmaelite
Copy link
Copy Markdown
Contributor Author

Thanks for the changes @tiffmaelite. I've added some more comments where we can make some things more granular. Can you please have a look at my last comments and let me know what you think

done

@tiffmaelite tiffmaelite changed the title add REST API interceptors for handling permissions on identity links add REST API interceptors for handling more diverse permissions (identity links, variables, conditional events) Apr 19, 2022
@tiffmaelite tiffmaelite force-pushed the feature/rest-api-interceptors-for-identitylinks branch 2 times, most recently from 494c4af to 71a21ab Compare March 3, 2023 10:17
…terceptors-for-identitylinks

# Conflicts:
#	modules/flowable-cmmn-rest/src/main/java/org/flowable/cmmn/rest/service/api/CmmnRestApiInterceptor.java
#	modules/flowable-cmmn-rest/src/main/java/org/flowable/cmmn/rest/service/api/repository/CaseDefinitionResource.java
#	modules/flowable-cmmn-rest/src/main/java/org/flowable/cmmn/rest/service/api/runtime/caze/BaseVariableResource.java
#	modules/flowable-cmmn-rest/src/main/java/org/flowable/cmmn/rest/service/api/runtime/caze/CaseInstanceVariableDataResource.java
#	modules/flowable-cmmn-rest/src/main/java/org/flowable/cmmn/rest/service/api/runtime/caze/CaseInstanceVariableResource.java
#	modules/flowable-cmmn-rest/src/main/java/org/flowable/cmmn/rest/service/api/runtime/task/TaskBaseResource.java
#	modules/flowable-cmmn-rest/src/main/java/org/flowable/cmmn/rest/service/api/runtime/task/TaskVariableResource.java
#	modules/flowable-rest/src/main/java/org/flowable/rest/service/api/BpmnRestApiInterceptor.java
#	modules/flowable-rest/src/main/java/org/flowable/rest/service/api/repository/ProcessDefinitionResource.java
#	modules/flowable-rest/src/main/java/org/flowable/rest/service/api/runtime/process/BaseExecutionVariableResource.java
#	modules/flowable-rest/src/main/java/org/flowable/rest/service/api/runtime/process/BaseProcessInstanceResource.java
#	modules/flowable-rest/src/main/java/org/flowable/rest/service/api/runtime/process/ExecutionVariableResource.java
#	modules/flowable-rest/src/main/java/org/flowable/rest/service/api/runtime/process/ProcessInstanceResource.java
#	modules/flowable-rest/src/main/java/org/flowable/rest/service/api/runtime/process/ProcessInstanceVariableResource.java
#	modules/flowable-rest/src/main/java/org/flowable/rest/service/api/runtime/task/TaskVariableResource.java
@tiffmaelite tiffmaelite force-pushed the feature/rest-api-interceptors-for-identitylinks branch from 71a21ab to 72615b8 Compare March 3, 2023 10:19
@tiffmaelite
Copy link
Copy Markdown
Contributor Author

replaced by 022b6ac

@tiffmaelite tiffmaelite closed this May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants