Passing of authenticated user in query lifecycle callbacks in GraphManager.

[andrii0lomakin]

Our database (YouTrackDB) works with records only on a session basis and requires information about the authenticated user for real-time authorisation of user operations for each operation on a specific resource: schema, record, sequence ... and so on.

So I have introduced one additional lifecycle method and added an authenticated user parameter to the one that is called before the request.

An additional method is called at the end of request processing once the transaction is rolled back if it is managed by session or traversal.

[andreachild]

In order to account for evolving lifecycle requirements it would be more flexible for the overloaded method to accept a single query context parameter that can be adapted to contain more information as needed in the future. This way if more data is needed, it need only be added to the query context and no additional overloaded method is needed.

Something like:

 /**
   * This method will be called before a script or query is processed by the
   * gremlin-server.
   *
   * @param msg the {@link RequestMessage} received by the gremlin-server.
   * @deprecated replaced by {@link #beforeQueryStart(QueryContext)}
   */
  default void beforeQueryStart(final RequestMessage msg) {
      beforeQueryStart(new QueryContext(msg, null));
  }

  default void beforeQueryStart(final QueryContext context) {
  }
  
  public static class QueryContext {
      private RequestMessage msg;
      private AuthenticatedUser user;
      // other fields can be added in the future without breaking backwards compatibility
      
      public QueryContext(final RequestMessage msg, final AuthenticatedUser user) {
          this.msg = msg;
          this.user = user;
      }
      public RequestMessage getMessage() {
          return msg;
      }
      public AuthenticatedUser getUser() {
          return user;
      }
  }

[andrii0lomakin]

Makes sense, will update code soon.

[andreachild]

I am wondering where the best place to unit or integration test the availability of the 'user' to the lifecycle. Perhaps AbstractEvalOpProcessorTest.java (requires a lot of mocking and perhaps stubbing).

[andrii0lomakin]

I am not sure that I understood your observation.

[xiazcy]

I would also suggest to include some form of testing for this addition, and mocking is probably the best way, though it can get rather complex. I believe Andrea is suggesting to place a set of tests inside of AbstractEvalOpProcessorTest.java?

[andrii0lomakin]

Do you suggest adding a test that checks that the processor redirected the context parameter correctly?

[andreachild]

I was imagining the test would check that the AuthenticatedUser can be accessed by the beforeQueryStart method as part of the lifecycle.

[andrii0lomakin]

@andrii0lomakin Yes, but that is very high level in what scenario? In a scenario, when does it already exist in the channel as an attribute? Or should this test cover all cases of authentication? I believe that such a test case should either exist or be added as a separate change, as that is not the point of this PR to test the correctness of authentication mechanics. As that is a very resource-consuming task, it should be handled separately. I do not mind providing such testing, as security is paramount for us, but if such tests already exist in this case, I will only add checking of the parameters of this callback there. Otherwise, I will test authentication mechanics separately, as we for sure will not go in prod with authentication holes. Do you have any objections?

[andreachild]

@andrii0lomakin

that is not the point of this PR to test the correctness of authentication mechanics.

I agree testing authentication mechanics are not in scope of this PR. It was not my intent to suggest that.

I will only add checking of the parameters of this callback there

Agree, that is all you should have to do for this change. My initial comment was that I could not immediately identify the best existing test to add this check to.