db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dag H. Wanvik (JIRA)" <j...@apache.org>
Subject [jira] Issue Comment Edited: (DERBY-3223) SQL roles: make use of privileges granted to roles in actual privilege checking
Date Tue, 26 Aug 2008 23:26:44 GMT

    [ https://issues.apache.org/jira/browse/DERBY-3223?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12625905#action_12625905
] 

dagw edited comment on DERBY-3223 at 8/26/08 4:24 PM:
---------------------------------------------------------------

Thanks, Rick!

> BaseActivation.makeInvalid(): To my eyes there is only one case in this
> switch statement which does anything. Are the other cases performing some
> useful work?

No, but the two actions that are no-ops will arrive but should be
ignored. Other actions (than these two) should not even arrive, so I added a
sane assert.

The action REVOKE_ROLE is called when a role is revoked, to force invalidation
(i.e. dropping currently) of views, triggers and constraints which depend on a
role, similarly to what is done when permissions are revoked
(REVOKE_PRIVILEGE). Since activations also may now depend on a role, we must
be prepared for the REVOKE_ROLE action to arrive, since the invalidation called (see
e.g. invalidateFor(REVOKE_ROLE) call in RevokeRoleConstantAction) visits all
dependents of the role, not just the ones that need it :)

The action INTERNAL_RECOMPILE_REQUEST is intended to invalidate prepared
statements (which I relied on before this patch), but also to invalidate SPSes
[which are used by triggers and constraints], but it may now be unnecessary to
call this action from RevokeRoleConstantAction/DropRoleConstantAction since
the descriptors do it themselves, cf. ViewDescriptor#drop which
calls invalidateFor(DROP_VIEW), which in turn is heeded by SPSes. I will see if I can
safely omit the invalidateFor(INTERNAL_RECOMPILE_REQUEST) call from
RevokeRoleConstantAction/ DropRoleConstantAction.

> There's a similar, long switch statement in
> ConstraintDescriptor.prepareToInvalidate(), which I noticed you had
> to touch. Was that useful?

Yes. Since constraints can also depend on roles, they would also get signalled
when we issue the RECHECK_PRIVILEGES when the current role is changed (in
addition to the intended activation), but we don't want views, constraints or
triggers to be impacted by this action, hence the no-op.
 
> 
> ViewDescriptor.prepareToInvalidate(). Here's another long, vacuous
> switch statement which puzzles me. Is there some reason that all of
> the cases fall through to the RECHECK_PRIVILEGES case except for the
> INTERNAL_RECOMPILE_REQUEST case? It looks to me as though the work
> of this switch statement could be accomplished by one case
> (REVOKE_PRIVILEGE_RESTRICT), which raises an exception--all other
> cases could exit gracefully without having to bother to name
> themselves.

Right, I think they are enumerated to show they are legal no-ops. I just added
my two new actions here following the existing pattern.

> 
> GenericActivationHolder. I wonder what it means for this object to
> have the same UUID as the Activation it holds. This UUID will not
> uniquely identify an object.

The forwarding implementation in GenericActivationHolder is vacuous; it is presently not
used, but needed to make the class concrete. I will add comments/asserts to make this
clear instead of the redirections (I initially thought I needed them).  Thanks for noticing,
I'll respin the patch. 



      was (Author: dagw):
    Thanks, Rick!

> BaseActivation.makeInvalid(): To my eyes there is only one case in this
> switch statement which does anything. Are the other cases performing some
> useful work?

No, but the two actions that are no-ops will arrive but should be
ignored. Other actions (than these two) should not even arrive, so I added a
sane assert.

The action REVOKE_ROLE is called when a role is revoked, to force invalidation
(i.e. dropping currently) of views, triggers and constraints which depend on a
role, similarly to what is done when permissions are revoked
(REVOKE_PRIVILEGE). Since activations also may now depend on a role, we must
be prepared for the REVOKE_ROLE action to arrive, since the invalidation called (see
e.g. invalidateFor(REVOKE_ROLE) call in RevokeRoleConstantAction) visits all
dependents of the role, not just the ones that need it :)

The action INTERNAL_RECOMPILE_REQUEST is intended to invalidate prepared
statements (which I relied on before this patch), but also to invalidate SPSes
[which are used by triggers and constraints], but it may now be unnecessary to
call this action from RevokeRoleConstantAction/DropRoleConstantAction since
the the descriptors themselves do it themselves, cf. ViewDescriptor#drop which
calls invalidateFor(DROP_VIEW) which is heeded by SPSes. I will see if I can
safely omit the invalidateFor(INTERNAL_RECOMPILE_REQUEST) call from
RevokeRoleConstantAction/ DropRoleConstantAction.

> There's a similar, long switch statement in
> ConstraintDescriptor.prepareToInvalidate(), which I noticed you had
> to touch. Was that useful?

Yes. Since constraints can also depend on roles, they would also get signalled
when we issue the RECHECK_PRIVILEGES when the current role is changed (in
addition to the intended activation), but we don't want views, constraints or
triggers to be impacted by this action, hence the no-op.
 
> 
> ViewDescriptor.prepareToInvalidate(). Here's another long, vacuous
> switch statement which puzzles me. Is there some reason that all of
> the cases fall through to the RECHECK_PRIVILEGES case except for the
> INTERNAL_RECOMPILE_REQUEST case? It looks to me as though the work
> of this switch statement could be accomplished by one case
> (REVOKE_PRIVILEGE_RESTRICT), which raises an exception--all other
> cases could exit gracefully without having to bother to name
> themselves.

Right, I think they are enumerated to show they are legal no-ops. I just added
my two new actions here following the existing pattern.

> 
> GenericActivationHolder. I wonder what it means for this object to
> have the same UUID as the Activation it holds. This UUID will not
> uniquely identify an object.

The forwarding implementation in GenericActivationHolder is vacuous; it is presently not
used, but needed to make the class concrete. I will add comments/asserts to make this
clear instead of the redirections (I initially thought I needed them).  Thanks for noticing,
I'll respin the patch. 


  
> SQL roles: make use of privileges granted to roles in actual privilege checking
> -------------------------------------------------------------------------------
>
>                 Key: DERBY-3223
>                 URL: https://issues.apache.org/jira/browse/DERBY-3223
>             Project: Derby
>          Issue Type: Task
>          Components: Security, SQL
>            Reporter: Dag H. Wanvik
>            Assignee: Dag H. Wanvik
>             Fix For: 10.5.0.0
>
>         Attachments: derby-3223-1a.diff, derby-3223-1a.stat, derby-3223-1b.diff, derby-3223-1b.stat,
derby-3223-1c.diff, derby-3223-1c.stat, derby-3223-1d.diff, derby-3223-1d.stat, derby-3223-activate-roles-1.diff,
derby-3223-activate-roles-1.stat, derby-3223-activate-roles-2.diff, derby-3223-activate-roles-2.stat,
derby-3223-activate-roles-2b.diff, derby-3223-activate-roles-2b.stat, derby-3223-invalidate-activations-1.diff,
derby-3223-invalidate-activations-1.stat, derby-3223-revise-iterator-api-b.diff, derby-3223-revise-iterator-api-b.stat,
derby-3223-revise-iterator-api.diff, derby-3223-revise-iterator-api.stat, derby-3223-revocation-logic-1.diff,
derby-3223-revocation-logic-1.stat, derby-3223-revocation-logic-2.diff, derby-3223-revocation-logic-2.stat,
derby-3223-revocation-logic-2.txt, derby-3223-revocation-logic-3.diff, derby-3223-revocation-logic-3.stat,
derby-3223-revocation-logic-4.diff, derby-3223-revocation-logic-4.stat, derby-3223-revocation-logic-5.diff,
derby-3223-revocation-logic-5.stat, roles.sql, roles2.sql, roles3.sql
>
>
> Pushing out to 10.5

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message