You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2021/03/03 03:34:19 UTC

[GitHub] [gobblin] jack-moseley opened a new pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

jack-moseley opened a new pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238


   Dear Gobblin maintainers,
   
   Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
   
   
   ### JIRA
   - [x] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
       - https://issues.apache.org/jira/browse/GOBBLIN-1402
   
   
   ### Description
   - [x] Here are some details about my PR, including screenshots (if applicable):
   Make it possible to update the owner/requester list in a FlowConfig in some circumstances, and also refactoring/fixing some other related issues.
   
   - In `FlowConfigResourceLocalHandler`, don't overwrite the previous requester list property (to allow it to be updated if it was specified by a user)
   - Move whitelisted requester checking to an API in `RequesterService` to allow it to be used by handlers
   - Add some checks to deprecated "flowconfigs" endpoint, since it was possible to set owning group to a group you were not part of through that endpoint
   - Add checks as well to "flowconfigsV2" endpoint to avoid a similar way of updating the owning group, and to include the logic for when requester list is allowed to be updated
   
   ### Tests
   - [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   Updated unit tests and tested locally
   
   ### Commits
   - [x] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
       1. Subject is separated from body by a blank line
       2. Subject is limited to 50 characters
       3. Subject does not end with a period
       4. Subject uses the imperative mood ("add", not "adding")
       5. Body wraps at 72 characters
       6. Body explains "what" and "why", not "how"
   
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] jack-moseley commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586816881



##########
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);
+      }
+
+      if (!updatedRequesterList.equals(requesterList)) {
+        throw new FlowConfigLoggedException(HttpStatus.S_401_UNAUTHORIZED, RequesterService.REQUESTER_LIST + " property may "
+            + "only be updated to yourself. Requesting user: " + requesterList + ", updated requester: " + updatedRequesterList);
+      }
+    }
+  }
+
   /**
    * Check that all {@link ServiceRequester}s in this request are contained within the original service requester list
    * or is part of the original requester's owning group when the flow was submitted. If they are not, throw a {@link FlowConfigLoggedException} with {@link HttpStatus#S_401_UNAUTHORIZED}.
    * If there is a failure when deserializing the original requester list, throw a {@link FlowConfigLoggedException} with
    * {@link HttpStatus#S_400_BAD_REQUEST}.
-   * @param requesterService the {@link RequesterService} used to verify the requester
    * @param originalFlowConfig original flow config to find original requester
    * @param requesterList list of requesters for this request
    */
-  public void checkRequester(
-      RequesterService requesterService, FlowConfig originalFlowConfig, List<ServiceRequester> requesterList) {
-    if (requesterList == null) {
+  public void checkRequester(FlowConfig originalFlowConfig, List<ServiceRequester> requesterList) {
+    if (requesterList == null || this.requesterService.isRequesterWhitelisted(requesterList)) {

Review comment:
       We are using an internal implementation of `RequesterService` which will never return null. Although, it looks like even the open source ones (`TestRequesterService` and `NoopRequesterService`) never return null either, so the null check could probably be removed.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] jack-moseley commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586865437



##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)
+  public void testGroupUpdateRejected() throws Exception {
+   ServiceRequester testRequester = new ServiceRequester("testName", "USER_PRINCIPAL", "testFrom");
+   _requesterService.setRequester(testRequester);
+   Map<String, String> flowProperties = Maps.newHashMap();
+
+   FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_8))
+       .setTemplateUris(TEST_TEMPLATE_URI)
+       .setProperties(new StringMap(flowProperties))
+       .setOwningGroup("testGroup");
+
+   _client.createFlowConfig(flowConfig);
+
+   flowConfig.setOwningGroup("testGroup2");
+   _client.updateFlowConfig(flowConfig);

Review comment:
       Actually it looks like that test also deletes the original group (testGroup1), which was causing some confusing behaviour, and I think could affect the already existing tests as well depending on what order they run in. I made that one depend on all the ones that rely on that group being there.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] jack-moseley commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586815404



##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)
+  public void testGroupUpdateRejected() throws Exception {
+   ServiceRequester testRequester = new ServiceRequester("testName", "USER_PRINCIPAL", "testFrom");
+   _requesterService.setRequester(testRequester);
+   Map<String, String> flowProperties = Maps.newHashMap();
+
+   FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_8))
+       .setTemplateUris(TEST_TEMPLATE_URI)
+       .setProperties(new StringMap(flowProperties))
+       .setOwningGroup("testGroup");
+
+   _client.createFlowConfig(flowConfig);
+
+   flowConfig.setOwningGroup("testGroup2");
+   _client.updateFlowConfig(flowConfig);

Review comment:
       The point is to check that a user cannot update a flow's owningGroup to a group that that user is not a part of (the test user is not part of the group `testGroup2`). Previously we only checked this condition on flow creation, but you could still update it to a group you are not part of.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] codecov-io edited a comment on pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#issuecomment-789411147


   # [Codecov](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=h1) Report
   > Merging [#3238](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=desc) (51d618d) into [master](https://codecov.io/gh/apache/gobblin/commit/5a2fb0d1453ab425c5c379325d680d085372482a?el=desc) (5a2fb0d) will **decrease** coverage by `0.01%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/gobblin/pull/3238/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3238      +/-   ##
   ============================================
   - Coverage     46.36%   46.35%   -0.02%     
   - Complexity     9929     9933       +4     
   ============================================
     Files          2030     2030              
     Lines         78721    78774      +53     
     Branches       8755     8765      +10     
   ============================================
   + Hits          36502    36516      +14     
   - Misses        38816    38855      +39     
     Partials       3403     3403              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...obblin/service/FlowConfigResourceLocalHandler.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnUmVzb3VyY2VMb2NhbEhhbmRsZXIuamF2YQ==) | `18.18% <0.00%> (-0.21%)` | `2.00 <0.00> (ø)` | |
   | [...rg/apache/gobblin/service/FlowConfigsResource.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnc1Jlc291cmNlLmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [.../apache/gobblin/service/FlowConfigsV2Resource.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnc1YyUmVzb3VyY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...a/org/apache/gobblin/service/RequesterService.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9SZXF1ZXN0ZXJTZXJ2aWNlLmphdmE=) | `50.00% <0.00%> (-2.95%)` | `3.00 <0.00> (ø)` | |
   | [...pache/gobblin/cluster/JobConfigurationManager.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSm9iQ29uZmlndXJhdGlvbk1hbmFnZXIuamF2YQ==) | `73.58% <0.00%> (-7.82%)` | `11.00% <0.00%> (+1.00%)` | :arrow_down: |
   | [...in/java/org/apache/gobblin/cluster/HelixUtils.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhVdGlscy5qYXZh) | `34.71% <0.00%> (-3.31%)` | `13.00% <0.00%> (-1.00%)` | |
   | [.../org/apache/gobblin/cluster/GobblinTaskRunner.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpblRhc2tSdW5uZXIuamF2YQ==) | `63.46% <0.00%> (-0.33%)` | `35.00% <0.00%> (ø%)` | |
   | [...lin/hive/metastore/HiveMetaStoreBasedRegister.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1oaXZlLXJlZ2lzdHJhdGlvbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9oaXZlL21ldGFzdG9yZS9IaXZlTWV0YVN0b3JlQmFzZWRSZWdpc3Rlci5qYXZh) | `10.21% <0.00%> (-0.03%)` | `5.00% <0.00%> (ø%)` | |
   | [...in/java/org/apache/gobblin/util/DownloadUtils.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvRG93bmxvYWRVdGlscy5qYXZh) | `0.00% <0.00%> (ø)` | `0.00% <0.00%> (ø%)` | |
   | [...org/apache/gobblin/azkaban/AzkabanJobLauncher.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tYXprYWJhbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9hemthYmFuL0F6a2FiYW5Kb2JMYXVuY2hlci5qYXZh) | `35.46% <0.00%> (ø)` | `5.00% <0.00%> (ø%)` | |
   | ... and [6 more](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=footer). Last update [5a2fb0d...51d618d](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] codecov-io commented on pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#issuecomment-789411147


   # [Codecov](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=h1) Report
   > Merging [#3238](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=desc) (51d618d) into [master](https://codecov.io/gh/apache/gobblin/commit/5a2fb0d1453ab425c5c379325d680d085372482a?el=desc) (5a2fb0d) will **decrease** coverage by `37.33%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/gobblin/pull/3238/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #3238       +/-   ##
   ============================================
   - Coverage     46.36%   9.03%   -37.34%     
   + Complexity     9929    1738     -8191     
   ============================================
     Files          2030    2030               
     Lines         78721   78774       +53     
     Branches       8755    8765       +10     
   ============================================
   - Hits          36502    7116    -29386     
   - Misses        38816   70961    +32145     
   + Partials       3403     697     -2706     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...obblin/service/FlowConfigResourceLocalHandler.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnUmVzb3VyY2VMb2NhbEhhbmRsZXIuamF2YQ==) | `0.00% <0.00%> (-18.40%)` | `0.00 <0.00> (-2.00)` | |
   | [...rg/apache/gobblin/service/FlowConfigsResource.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnc1Jlc291cmNlLmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [.../apache/gobblin/service/FlowConfigsV2Resource.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnc1YyUmVzb3VyY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...a/org/apache/gobblin/service/RequesterService.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9SZXF1ZXN0ZXJTZXJ2aWNlLmphdmE=) | `0.00% <0.00%> (-52.95%)` | `0.00 <0.00> (-3.00)` | |
   | [...c/main/java/org/apache/gobblin/util/FileUtils.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvRmlsZVV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-4.00%)` | |
   | [...n/java/org/apache/gobblin/fork/CopyableSchema.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2ZvcmsvQ29weWFibGVTY2hlbWEuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
   | [...java/org/apache/gobblin/stream/ControlMessage.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc3RyZWFtL0NvbnRyb2xNZXNzYWdlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
   | [...va/org/apache/gobblin/dataset/DatasetResolver.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YXNldC9EYXRhc2V0UmVzb2x2ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
   | [...va/org/apache/gobblin/converter/EmptyIterable.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbnZlcnRlci9FbXB0eUl0ZXJhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
   | [...org/apache/gobblin/ack/BasicAckableForTesting.java](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vYWNrL0Jhc2ljQWNrYWJsZUZvclRlc3RpbmcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
   | ... and [1074 more](https://codecov.io/gh/apache/gobblin/pull/3238/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=footer). Last update [5a2fb0d...51d618d](https://codecov.io/gh/apache/gobblin/pull/3238?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] aplex commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586770996



##########
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);
+      }
+
+      if (!updatedRequesterList.equals(requesterList)) {
+        throw new FlowConfigLoggedException(HttpStatus.S_401_UNAUTHORIZED, RequesterService.REQUESTER_LIST + " property may "
+            + "only be updated to yourself. Requesting user: " + requesterList + ", updated requester: " + updatedRequesterList);
+      }
+    }
+  }
+
   /**
    * Check that all {@link ServiceRequester}s in this request are contained within the original service requester list
    * or is part of the original requester's owning group when the flow was submitted. If they are not, throw a {@link FlowConfigLoggedException} with {@link HttpStatus#S_401_UNAUTHORIZED}.
    * If there is a failure when deserializing the original requester list, throw a {@link FlowConfigLoggedException} with
    * {@link HttpStatus#S_400_BAD_REQUEST}.
-   * @param requesterService the {@link RequesterService} used to verify the requester
    * @param originalFlowConfig original flow config to find original requester
    * @param requesterList list of requesters for this request
    */
-  public void checkRequester(
-      RequesterService requesterService, FlowConfig originalFlowConfig, List<ServiceRequester> requesterList) {
-    if (requesterList == null) {
+  public void checkRequester(FlowConfig originalFlowConfig, List<ServiceRequester> requesterList) {
+    if (requesterList == null || this.requesterService.isRequesterWhitelisted(requesterList)) {

Review comment:
       When can requesterList be null? Looks like if it is not set, we'll allow all operations.

##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)
+  public void testGroupUpdateRejected() throws Exception {
+   ServiceRequester testRequester = new ServiceRequester("testName", "USER_PRINCIPAL", "testFrom");
+   _requesterService.setRequester(testRequester);
+   Map<String, String> flowProperties = Maps.newHashMap();
+
+   FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_8))
+       .setTemplateUris(TEST_TEMPLATE_URI)
+       .setProperties(new StringMap(flowProperties))
+       .setOwningGroup("testGroup");
+
+   _client.createFlowConfig(flowConfig);
+
+   flowConfig.setOwningGroup("testGroup2");
+   _client.updateFlowConfig(flowConfig);
+  }
+
+  @Test (expectedExceptions = RestLiResponseException.class)
+  public void testRequesterUpdateRejected() throws Exception {

Review comment:
       Do we have a test that will check that requester can be updated?

##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)
+  public void testGroupUpdateRejected() throws Exception {
+   ServiceRequester testRequester = new ServiceRequester("testName", "USER_PRINCIPAL", "testFrom");
+   _requesterService.setRequester(testRequester);
+   Map<String, String> flowProperties = Maps.newHashMap();
+
+   FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_8))
+       .setTemplateUris(TEST_TEMPLATE_URI)
+       .setProperties(new StringMap(flowProperties))
+       .setOwningGroup("testGroup");
+
+   _client.createFlowConfig(flowConfig);
+
+   flowConfig.setOwningGroup("testGroup2");
+   _client.updateFlowConfig(flowConfig);

Review comment:
       Why would we get exception here? From what I see, user creates a flow and then sets the owning group for the flow that he owns.

##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)
+  public void testGroupUpdateRejected() throws Exception {
+   ServiceRequester testRequester = new ServiceRequester("testName", "USER_PRINCIPAL", "testFrom");
+   _requesterService.setRequester(testRequester);
+   Map<String, String> flowProperties = Maps.newHashMap();
+
+   FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_8))
+       .setTemplateUris(TEST_TEMPLATE_URI)
+       .setProperties(new StringMap(flowProperties))
+       .setOwningGroup("testGroup");
+
+   _client.createFlowConfig(flowConfig);
+
+   flowConfig.setOwningGroup("testGroup2");
+   _client.updateFlowConfig(flowConfig);
+  }
+
+  @Test (expectedExceptions = RestLiResponseException.class)

Review comment:
       Can we also check that exception message or http result is the one that we expect, and it is about authorization/permissions? Currently if there is a typo in the test, and service rejects the request for any reason, the test will pass.
   
   TestNG has expectedExceptionsMessageRegExp: https://howtodoinjava.com/testng/testng-expected-exception/

##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)

Review comment:
       The design doc also mentioned that "A user that is part of the owning group can take control of the flow by updating the owner to themselves". Is this implemented/covered with tests?




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] jack-moseley commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586863349



##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)

Review comment:
       Yes, it's covered by the test I just added




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] jack-moseley commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r587826887



##########
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:
       I couldn't think of a good way since it is modifying files that were used by other tests in the same class (this could have been a problem even before this PR).
   
   I decided to refactor and move this test to a separate class. I'm not sure why it was even in this class in the first place, since it seems to be testing that `LocalGroupOwnershipService` is able to pick up changes to the file, which is unrelated to flowConfig client. (And in this class there is already tests that group ownership is working while calling flowConfig client).




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] asfgit closed pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] aplex commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586992976



##########
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);
+      }
+
+      if (!updatedRequesterList.equals(requesterList)) {
+        throw new FlowConfigLoggedException(HttpStatus.S_401_UNAUTHORIZED, RequesterService.REQUESTER_LIST + " property may "
+            + "only be updated to yourself. Requesting user: " + requesterList + ", updated requester: " + updatedRequesterList);
+      }
+    }
+  }
+
   /**
    * Check that all {@link ServiceRequester}s in this request are contained within the original service requester list
    * or is part of the original requester's owning group when the flow was submitted. If they are not, throw a {@link FlowConfigLoggedException} with {@link HttpStatus#S_401_UNAUTHORIZED}.
    * If there is a failure when deserializing the original requester list, throw a {@link FlowConfigLoggedException} with
    * {@link HttpStatus#S_400_BAD_REQUEST}.
-   * @param requesterService the {@link RequesterService} used to verify the requester
    * @param originalFlowConfig original flow config to find original requester
    * @param requesterList list of requesters for this request
    */
-  public void checkRequester(
-      RequesterService requesterService, FlowConfig originalFlowConfig, List<ServiceRequester> requesterList) {
-    if (requesterList == null) {
+  public void checkRequester(FlowConfig originalFlowConfig, List<ServiceRequester> requesterList) {
+    if (requesterList == null || this.requesterService.isRequesterWhitelisted(requesterList)) {

Review comment:
       This is marked as resolved, but the null check is still there in the second commit.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] Will-Lo commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586812283



##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)

Review comment:
       I think it would be a better practice for us to try and catch the exception, and then assert that the error code is as expected. For example, with `testGroupRequesterRejected`
   ```
   try {
   ...
   } catch (RestLiResponseException e) {
     Assert.assertEquals(e.getStatus(), HttpStatus.ORDINAL_401_Unauthorized);
   }
   ```
   
   I was able to catch a bug before in this test suite as it actually returned a different error code than expected. Also these tests would mark 500 errors as passing tests which I think we should avoid.
   Also it allows better granularity in the test too, so we can assert that only the second call here would fail

##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)
+  public void testGroupUpdateRejected() throws Exception {
+   ServiceRequester testRequester = new ServiceRequester("testName", "USER_PRINCIPAL", "testFrom");
+   _requesterService.setRequester(testRequester);
+   Map<String, String> flowProperties = Maps.newHashMap();
+
+   FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_8))
+       .setTemplateUris(TEST_TEMPLATE_URI)
+       .setProperties(new StringMap(flowProperties))
+       .setOwningGroup("testGroup");
+
+   _client.createFlowConfig(flowConfig);
+
+   flowConfig.setOwningGroup("testGroup2");
+   _client.updateFlowConfig(flowConfig);

Review comment:
       We should avoid using testGroup2 as a failing group. There's another test that writes testGroup2 to the file where the group ownership service reads from, to test if the groupownership service will update automatically on reads, and this could lead to flakiness 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] jack-moseley commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586835857



##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)

Review comment:
       Doesn't this have the issue that the test will pass if no exception is thrown? (Unless you do something like `throw new RuntimeException("Shouldn't reach here")` after the point you expect it to fail). Probably better to use the `expectedExceptionsMessageRegExp` mentioned by Alex above.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] aplex commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586838437



##########
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);
+      }
+
+      if (!updatedRequesterList.equals(requesterList)) {
+        throw new FlowConfigLoggedException(HttpStatus.S_401_UNAUTHORIZED, RequesterService.REQUESTER_LIST + " property may "
+            + "only be updated to yourself. Requesting user: " + requesterList + ", updated requester: " + updatedRequesterList);
+      }
+    }
+  }
+
   /**
    * Check that all {@link ServiceRequester}s in this request are contained within the original service requester list
    * or is part of the original requester's owning group when the flow was submitted. If they are not, throw a {@link FlowConfigLoggedException} with {@link HttpStatus#S_401_UNAUTHORIZED}.
    * If there is a failure when deserializing the original requester list, throw a {@link FlowConfigLoggedException} with
    * {@link HttpStatus#S_400_BAD_REQUEST}.
-   * @param requesterService the {@link RequesterService} used to verify the requester
    * @param originalFlowConfig original flow config to find original requester
    * @param requesterList list of requesters for this request
    */
-  public void checkRequester(
-      RequesterService requesterService, FlowConfig originalFlowConfig, List<ServiceRequester> requesterList) {
-    if (requesterList == null) {
+  public void checkRequester(FlowConfig originalFlowConfig, List<ServiceRequester> requesterList) {
+    if (requesterList == null || this.requesterService.isRequesterWhitelisted(requesterList)) {

Review comment:
       yep, let's remove it then (if we can't figure out the reason for it to exist from git blame). In security code it's safer to deny access in unknown or ambiguous situations.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] jack-moseley commented on pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#issuecomment-790040168


   @Will-Lo please review


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] jack-moseley commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586864328



##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)

Review comment:
       Actually I think I prefer it that way, because we can just check the HTTP status code instead, checking error messages seems a bit worse because if we ever want to change the message sent it will affect all the tests.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] jack-moseley commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586863037



##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)
+  public void testGroupUpdateRejected() throws Exception {
+   ServiceRequester testRequester = new ServiceRequester("testName", "USER_PRINCIPAL", "testFrom");
+   _requesterService.setRequester(testRequester);
+   Map<String, String> flowProperties = Maps.newHashMap();
+
+   FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_8))
+       .setTemplateUris(TEST_TEMPLATE_URI)
+       .setProperties(new StringMap(flowProperties))
+       .setOwningGroup("testGroup");
+
+   _client.createFlowConfig(flowConfig);
+
+   flowConfig.setOwningGroup("testGroup2");
+   _client.updateFlowConfig(flowConfig);
+  }
+
+  @Test (expectedExceptions = RestLiResponseException.class)
+  public void testRequesterUpdateRejected() throws Exception {

Review comment:
       Added a test




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] aplex commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586837371



##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws Exception {
     _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
   }
 
+
+  @Test (expectedExceptions = RestLiResponseException.class)

Review comment:
       Try/catch will also work too. I usually add Assert.fail("Expected exception X") at the end of the try block.
   
   Junit has this exception verification streamlined with assertThrows, but we're still on TestNg - https://stackoverflow.com/questions/40268446/junit-5-how-to-assert-an-exception-is-thrown




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [gobblin] aplex commented on a change in pull request #3238: [GOBBLIN-1402] Allow flow's requester list/owner to be updated

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org