-
Notifications
You must be signed in to change notification settings - Fork 829
Passing of authenticated user in query lifecycle callbacks in GraphManager. #3137
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
base: 3.7-dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -131,13 +131,15 @@ protected AbstractEvalOpProcessor(final boolean manageTransactions) { | |
|
||
/** | ||
* Provides an operation for evaluating a Gremlin script. | ||
* | ||
* @return | ||
*/ | ||
public abstract ThrowingConsumer<Context> getEvalOp(); | ||
|
||
/** | ||
* A sub-class may have additional "ops" that it will service. Calls to {@link OpProcessor#select(Context)} that are not | ||
* handled will be passed to this method to see if the sub-class can service the requested op code. | ||
* | ||
* @return | ||
*/ | ||
public abstract Optional<ThrowingConsumer<Context>> selectOther(final Context ctx) throws OpProcessorException; | ||
|
@@ -201,12 +203,13 @@ protected Optional<ThrowingConsumer<Context>> validateEvalMessage(final RequestM | |
* iteration timeouts, metrics and building bindings. Note that result iteration is delegated to the | ||
* {@link #handleIterator(Context, Iterator)} method, so those extending this class could override that method for | ||
* better control over result iteration. | ||
* @param ctx The current Gremlin Server {@link Context}. This handler ensures that only a single final | ||
* response is sent to the client. | ||
* | ||
* @param ctx The current Gremlin Server {@link Context}. This handler ensures that only a single final | ||
* response is sent to the client. | ||
* @param gremlinExecutorSupplier A function that returns the {@link GremlinExecutor} to use in executing the | ||
* script evaluation. | ||
* @param bindingsSupplier A function that returns the {@link Bindings} to provide to the | ||
* {@link GremlinExecutor#eval} method. | ||
* @param bindingsSupplier A function that returns the {@link Bindings} to provide to the | ||
* {@link GremlinExecutor#eval} method. | ||
*/ | ||
protected void evalOpInternal(final Context ctx, final Supplier<GremlinExecutor> gremlinExecutorSupplier, | ||
final BindingSupplier bindingsSupplier) { | ||
|
@@ -233,15 +236,23 @@ protected void evalOpInternal(final Context ctx, final Supplier<GremlinExecutor> | |
|
||
final GremlinExecutor.LifeCycle lifeCycle = GremlinExecutor.LifeCycle.build() | ||
.evaluationTimeoutOverride(seto) | ||
.afterFailure((b,t) -> { | ||
.afterFailure((b, t) -> { | ||
graphManager.onQueryError(msg, t); | ||
if (managedTransactionsForRequest) attemptRollback(msg, ctx.getGraphManager(), settings.strictTransactionManagement); | ||
if (managedTransactionsForRequest) | ||
attemptRollback(msg, ctx.getGraphManager(), settings.strictTransactionManagement); | ||
graphManager.afterQueryEnd(msg); | ||
}) | ||
.afterTimeout((b, t) -> { | ||
graphManager.onQueryError(msg, t); | ||
graphManager.onQueryError(msg, t); | ||
graphManager.afterQueryEnd(msg); | ||
}) | ||
.beforeEval(b -> { | ||
graphManager.beforeQueryStart(msg); | ||
AuthenticatedUser user = ctx.getChannelHandlerContext().channel().attr(StateKey.AUTHENTICATED_USER).get(); | ||
if (null == user) { | ||
// This is expected when using the AllowAllAuthenticator | ||
user = AuthenticatedUser.ANONYMOUS_USER; | ||
} | ||
graphManager.beforeQueryStart(msg, user); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am wondering where the best place to unit or integration test the availability of the 'user' to the lifecycle. Perhaps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure that I understood your observation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you suggest adding a test that checks that the processor redirected the context parameter correctly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was imagining the test would check that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree testing authentication mechanics are not in scope of this PR. It was not my intent to suggest that.
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. |
||
try { | ||
b.putAll(bindingsSupplier.get()); | ||
} catch (OpProcessorException ope) { | ||
|
@@ -268,13 +279,16 @@ protected void evalOpInternal(final Context ctx, final Supplier<GremlinExecutor> | |
handleIterator(ctx, itty); | ||
graphManager.onQuerySuccess(msg); | ||
} catch (Exception ex) { | ||
if (managedTransactionsForRequest) attemptRollback(msg, ctx.getGraphManager(), settings.strictTransactionManagement); | ||
|
||
graphManager.onQueryError(msg, ex); | ||
if (managedTransactionsForRequest) | ||
attemptRollback(msg, ctx.getGraphManager(), settings.strictTransactionManagement); | ||
CloseableIterator.closeIterator(itty); | ||
|
||
// wrap up the exception and rethrow. the error will be written to the client by the evalFuture | ||
// as it will completeExceptionally in the GremlinExecutor | ||
throw new RuntimeException(ex); | ||
} finally { | ||
graphManager.afterQueryEnd(msg); | ||
} | ||
}).create(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, will update code soon.