[
https://issues.apache.org/jira/browse/DERBY-3327?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12561421#action_12561421
]
Dag H. Wanvik commented on DERBY-3327:
--------------------------------------
I'll answer in two segments, the first on the specific patch comments,
then on the larger issues.
* Specific patch comments
> For the field 'callers' in GenericLanguageConnectionContext.java I
> think it's holding a stack of Activations. Could you state this in
> its javadoc. (oh for Java 5 :-)
Yes, I thought I did say it ("keeps track of which, if any, stored
procedure activations are active"), but I will make it
clearer. Generics would have been nice here, yes :)
> In the javadoc comment for 'callers' it says:
>
> For the top level, the current role is kept here, cf. 'currentRole'.
>
> Since this is the javadoc for callers, the here applies to callers,
> but I think you really mean the current class. Would this be
> correct?
You are right. I have moved this comment to where it belongs, with
"currentRole".
> In BaseActivation can the comments for the field callActivation be
> javadoc comments, so that IDE's will pick them up.
Agree, will do.
> Can comments be added for nestedCurrentRole.
Agree, will do.
> Even with the comments for BaseActivation.callActivation I became a
> little confused. Looking elsewhere I can see that callActivation is
> the activation of the enclosing CALL statement, that would have been
> useful in the javadoc.
Adding an explicit statement about that.
> I'm somewhat confused by the need for BaseActivation.callActivation
> and the stack in the LCC. It seems that the stack of activations is
> being kept twice, once in LCC and once through the activations.
It's probably redundant, I will try again see if I can get rid of it.
> I wonder about the cost of this stack maintenance, for SQL routines
> that are defined as NO SQL it might be a significant
> overhead. E.g. SIN() function. Avoiding the context manipulation for
> those routines could be a follow on, but I'd be interested in the
> overhead with this change.
Right. I'll run some tests. Moving the pushing to code generation (as
you also) should help us get rid of the overhead for NO SQL functions,
too.
> np - one other item is that according to section 10.4 routine
> invocation the order is:
>
> GR2) Evaluate arguments
> GR5) Push new SQL-session context (RSC)
> GR7/8) Execute the routine
>
> The order in the patch is:
> A) Push new authorization context
> B) Evaluate arguments
> C) Execute the routine
>
> To match the order of the spec the new context would need to be part
> of the generated code. Not sure if it makes any difference or not,
> but something to consider.
I think as long as we run with invoker's right it does not matter, but
if/when we decide to add the ability to run with definer's rights it
would, since for example an argument refering CURRENT_USER would then
evaluate differently depending on whether the authorization stack was
pushed or not, so it would be good to do it in the right order,
> I don't think the patch addresses functions, since the
> lcc.pushCaller() is only set from CallStatementResultSet.
Yes, thanks. I had added a test (buggy!) which blind sided me there
for a while.
* Larger issues
> It seems somewhat strange that this is implemented as a stack of
> activations and because of that we end up needing to store role
> information in two different places, in the LCC if it's the
> top-level and the activation otherwise. Since the activation
> represents an individual statement is its job to hold SQL context
> information?
It is a bit awkward, and it may be the wrong choice. I would have
preferred an explicit representation of the new session context (SQL
10.4's "RSC", perhaps called SQLSessionContext object, as you
suggest), but I did not find one ;) I looked at ConnectionContext and
StatementContext but these classes seems to be available only at the
JDBC level. At execution time, it seems we have to rely on what's
reachable from the activation (lcc, transaction controller, result
set). Somehow, the activation needs to know its dynamic nesting so
that it can access the correct dynamic context. DERBY-1331 would seem
to indicate we need to go this route at some point anyway, so I'll
have a go.
>
> Looking closer at the SQL standard (4.34.1.1 and 4.27.3) I'm not
> sure the patch is implementing exactly what is described there. One
> portion is the SQL-session context, that comes into being with the
> routine invocation. Then within the SQL-session context is an
> authorization stack. The patch seems to be morphing the two into
> one, executing a routine creates just a role stack.
Right, although interpreting the standard literally (GR 5.h: "copying
the authorization stack") seems to be inefficient, so some morphing
may be useful ;)
> Would it make more sense to have objects that followed the model
> described by the SQL specification, even if currently they are not
> fully utilized?
Yes, I think so.
>
> Something like:
> SQLSessionContext - pushed for a routine call, stack maintained in the lcc
> has a field for schema (future for DERBY-1331)
> has a field for authorization stack (stack of AuthorizationCell)
>
> AuthorizationCell (see 4.34.1.1)
> field for user identifier
> field for role name
>
>
> Maybe the existing StatementContext class is already fulfilling the
> purpose of SQLSessionContext?????
It does not seem to be reachable from the activation; being confined
to the JDBC level? A stack of StatementContext is kept in the lcc, but
the activation does not know which one represents it, AFAICT. I'll
have another look, perhaps it could be extended with the information
needed an allow references from the activation? I think it would be
better to add new SQLSessionContext, to keep the separation between
the JDBC and SQL level intact?
Anyway, I'll first fix up the current solution with your comments and
then try to work on a more general solution.
> SQL roles: Implement authorization stack
> ----------------------------------------
>
> Key: DERBY-3327
> URL: https://issues.apache.org/jira/browse/DERBY-3327
> Project: Derby
> Issue Type: New Feature
> Components: Security, SQL
> Reporter: Dag H. Wanvik
> Assignee: Dag H. Wanvik
> Fix For: 10.4.0.0
>
> Attachments: DERBY-3327-1.diff, DERBY-3327-1.stat, DERBY-3327-2.diff,
> DERBY-3327-2.stat, DERBY-3327-3.diff, DERBY-3327-3.stat
>
>
> The current LanguageConnectionContext keeps the user authorization identifier
> for an SQL session.
> The lcc is shared context also for nested connections (opened from stored
> procedures).
> So far, for roles, the current role has been stored in the lcc also. However,
> SQL requires that
> authorization identifers be pushed on a "authorization stack" when calling a
> stored procedure, cf.
> SQL 2003, vol 2, section 4.34.1.1 and 4.27.3.
> This allows a caller to keep its current role after a call even if changed by
> the stored procedure.
> This issue will implement the current role name part ("cell") of the
> authorization stack.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
