[
https://issues.apache.org/jira/browse/GOBBLIN-1402?focusedWorklogId=560796&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-560796
]
ASF GitHub Bot logged work on GOBBLIN-1402:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 04/Mar/21 02:53
Start Date: 04/Mar/21 02:53
Worklog Time Spent: 10m
Work Description: aplex commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586995543
##########
File path:
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigsResource.java
##########
@@ -142,7 +155,7 @@ public UpdateResponse delete(ComplexResourceKey<FlowId,
EmptyRecord> key) {
*/
public static void checkRequester(
RequesterService requesterService, FlowConfig originalFlowConfig,
List<ServiceRequester> requesterList) {
- if (requesterList == null) {
+ if (requesterList == null ||
requesterService.isRequesterWhitelisted(requesterList)) {
Review comment:
Same comment about requester==null as for V2
##########
File path:
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -262,7 +265,7 @@ public void testGroupRequesterRejected() throws Exception {
}
}
- @Test
+ @Test(dependsOnMethods = {"testGroupRequesterRejected",
"testGroupRequesterAllowed", "testGroupUpdateRejected", "testRequesterUpdate"})
Review comment:
Do we have a way to run this without dependencies? Test dependencies
bring a couple of problems:
- We can't run all tests in parallel, which affects build time
- To understand what goes on in the test, developer also need to read all
dependent tests
- People that add new tests will likely be unaware about dependencies, and
will just add their test to the bottom. This can break old tests in unexpected
ways.
Usually, it's good to have tests isolated from each other. If you need a
common setup logic, it can be moved to a separate method and called from all
needed tests.
##########
File path:
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigsV2Resource.java
##########
@@ -208,18 +217,59 @@ private Properties getHeaders() {
return headerProperties;
}
+ /**
+ * Check that this update or delete operation is allowed, throw a {@link
FlowConfigLoggedException} if not.
+ */
+ public void checkUpdateDeleteAllowed(FlowConfig originalFlowConfig,
FlowConfig updatedFlowConfig) {
+ List<ServiceRequester> requesterList =
this.requesterService.findRequesters(this);
+ if (updatedFlowConfig != null) {
+ checkPropertyUpdatesAllowed(requesterList, updatedFlowConfig);
+ }
+ checkRequester(originalFlowConfig, requesterList);
+ }
+
+ /**
+ * Check that the properties being updated are allowed to be updated. This
includes:
+ * 1. Checking that the requester is part of the owningGroup if it is being
modified
+ * 2. Checking if the {@link RequesterService#REQUESTER_LIST} is being
modified, and only allow it if a user is changing
+ * it to themselves.
+ */
+ public void checkPropertyUpdatesAllowed(List<ServiceRequester>
requesterList, FlowConfig updatedFlowConfig) {
+ if (requesterList == null ||
this.requesterService.isRequesterWhitelisted(requesterList)) {
+ return;
+ }
+
+ // Check that requester is part of owning group if owning group is being
updated
+ if (updatedFlowConfig.hasOwningGroup() &&
!this.groupOwnershipService.isMemberOfGroup(requesterList,
updatedFlowConfig.getOwningGroup())) {
+ throw new FlowConfigLoggedException(HttpStatus.S_401_UNAUTHORIZED,
"Requester not part of owning group specified");
+ }
+
+ if (updatedFlowConfig.hasProperties() &&
updatedFlowConfig.getProperties().containsKey(RequesterService.REQUESTER_LIST))
{
+ List<ServiceRequester> updatedRequesterList;
+ try {
+ updatedRequesterList =
RequesterService.deserialize(updatedFlowConfig.getProperties().get(RequesterService.REQUESTER_LIST));
+ } catch (Exception e) {
+ throw new FlowConfigLoggedException(HttpStatus.S_400_BAD_REQUEST,
RequesterService.REQUESTER_LIST + " property was "
+ + "provided but could not be deserialized", e);
Review comment:
It would be good to add an example of the value of this property to the
error message, or add link to the doc showing how it should look like.
One way to do this is to just serialize the sample user name and append json
to the message.
##########
File path:
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigsV2Resource.java
##########
@@ -208,18 +217,59 @@ private Properties getHeaders() {
return headerProperties;
}
+ /**
+ * Check that this update or delete operation is allowed, throw a {@link
FlowConfigLoggedException} if not.
+ */
+ public void checkUpdateDeleteAllowed(FlowConfig originalFlowConfig,
FlowConfig updatedFlowConfig) {
+ List<ServiceRequester> requesterList =
this.requesterService.findRequesters(this);
+ if (updatedFlowConfig != null) {
+ checkPropertyUpdatesAllowed(requesterList, updatedFlowConfig);
+ }
+ checkRequester(originalFlowConfig, requesterList);
+ }
+
+ /**
+ * Check that the properties being updated are allowed to be updated. This
includes:
+ * 1. Checking that the requester is part of the owningGroup if it is being
modified
+ * 2. Checking if the {@link RequesterService#REQUESTER_LIST} is being
modified, and only allow it if a user is changing
+ * it to themselves.
+ */
+ public void checkPropertyUpdatesAllowed(List<ServiceRequester>
requesterList, FlowConfig updatedFlowConfig) {
+ if (requesterList == null ||
this.requesterService.isRequesterWhitelisted(requesterList)) {
+ return;
+ }
+
+ // Check that requester is part of owning group if owning group is being
updated
+ if (updatedFlowConfig.hasOwningGroup() &&
!this.groupOwnershipService.isMemberOfGroup(requesterList,
updatedFlowConfig.getOwningGroup())) {
+ throw new FlowConfigLoggedException(HttpStatus.S_401_UNAUTHORIZED,
"Requester not part of owning group specified");
Review comment:
It would be good to add more details and an action part to the error
message, so that we get fewer support requests asking us what to do about the
error.
For example, we can append it with "User XXX should join the group YYY and
try again".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 560796)
Time Spent: 2h 40m (was: 2.5h)
> Allow flow's requester list/owner to be updated
> -----------------------------------------------
>
> Key: GOBBLIN-1402
> URL: https://issues.apache.org/jira/browse/GOBBLIN-1402
> Project: Apache Gobblin
> Issue Type: Improvement
> Reporter: Jack Moseley
> Priority: Major
> Time Spent: 2h 40m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
