Bug 104899 - before/afterSave methods accessing Kernel.getContext().getParty() are potentially broken
Summary: before/afterSave methods accessing Kernel.getContext().getParty() are potenti...
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Red Hat Web Application Framework
Classification: Retired
Component: other
Version: nightly
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Archit Shah
QA Contact: Jon Orris
URL:
Whiteboard:
Depends On:
Blocks: 100952
TreeView+ depends on / blocked
 
Reported: 2003-09-23 13:13 UTC by Daniel Berrangé
Modified: 2007-04-18 16:57 UTC (History)
0 users

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2005-11-10 21:03:26 UTC
Embargoed:


Attachments (Terms of Use)

Description Daniel Berrangé 2003-09-23 13:13:47 UTC
From Bugzilla Helper:
User-Agent: Mozilla/5.0 Galeon/1.2.9 (X11; Linux i686; U;) Gecko/20030314

Description of problem:
There is quiet alot of code in before/afterSave methods that accesses the
Kernel.getContext().getParty() methods, to either record auditing information
(eg creation user) or to validate actions (DML permissions checking). No problem
there you may think. 

However, with the new behaviour of persistence wrt to flushing changes to the DB
we can no longer guarentee the point in time at which the before/after save
methods will be invoked. Specifically, if we are running as a particular
'effective party' there is no guarentee that the session will be flushed before
we switch back to the original effective party. This results in both privilege
escalation & de-escalation on permissions checks & wildly inaccurate auditing data.

The following example JSP illustrates a few of the problems with auditing:

<jsp:root xmlns:jsp="http://java.sun.com/JSP/Page"
          xmlns:define="/WEB-INF/bebop-define.tld" 
          xmlns:show="/WEB-INF/bebop-show.tld"
	  version="1.2"> 

<jsp:directive.page import="com.arsdigita.kernel.*"/>
<jsp:directive.page import="com.arsdigita.auditing.*"/>
<jsp:directive.page import="com.arsdigita.persistence.*"/>

<jsp:declaration>
    AuditedACSObject bar;
    AuditedACSObject foo;
    AuditedACSObject eek;
    AuditedACSObject wizz;
</jsp:declaration>

<jsp:scriptlet>

  KernelExcursion test1 = new KernelExcursion() {
    public void excurse() {
      foo = new AuditedACSObject("com.arsdigita.kernel.ACSObject") {};
      setParty(Kernel.getSystemParty());
      bar = new AuditedACSObject("com.arsdigita.kernel.ACSObject") {};
    }
  };
  test1.run();

  System.err.println("Foo: " + (foo.getCreationUser() == null ? "none" :
foo.getCreationUser().getDisplayName()));
  System.err.println("Bar: " + (bar.getCreationUser() == null ? "none" :
bar.getCreationUser().getDisplayName()));

  KernelExcursion test2 = new KernelExcursion() {
    public void excurse() {
      eek = new AuditedACSObject("com.arsdigita.kernel.ACSObject") {};
      setParty(Kernel.getSystemParty());
      wizz = new AuditedACSObject("com.arsdigita.kernel.ACSObject") {};
      SessionManager.getSession().flushAll();
    }
  };
  test2.run();

  System.err.println("Eek: " + (eek.getCreationUser() == null ? "none" :
eek.getCreationUser().getDisplayName()));
  System.err.println("Wizz: " + (wizz.getCreationUser() == null ? "none" :
wizz.getCreationUser().getDisplayName()));


</jsp:scriptlet>

</jsp:root>


The only solution I can think of is to ensure that Session#flushAll() is called
every time KernelContext#party is changed.

Version-Release number of selected component (if applicable):


How reproducible:
Always

Steps to Reproduce:
1. Save the above JSP
2. Visit it without first logging in
3. Watch stderr.log
    

Actual Results:  Foo: none
Bar: none
Eek: Administrator Account
Wizz: Administrator Account


Expected Results:  Foo: none
Bar: Administrator Account
Eek: none
Wizz: Administrator Account


Additional info:

Comment 1 Archit Shah 2003-12-09 21:29:03 UTC
In principal, this change is correct, however, existing code has
problems, especially in handling of new objects. I don't think we can
effectively hunt down every case. The best plan I can come up with:

- there is a distinction between getEffectiveParty and getParty. For
auditing purposes, getParty should be used. Only login/request
processing code should be calling setParty. This should mitigate some
of the problem.

- the safe thing to do is to force your changes to be saved with
appropriate calls to save()

- I will add appropriate calls to flushAll for the request handling
code that does call getParty

Comment 2 Archit Shah 2003-12-15 20:37:34 UTC
my plan was implemented on 6.0.x (38819) and dev (38822)

Comment 3 Daniel Berrangé 2003-12-16 09:21:51 UTC
Shouldn't we rather add the flushAll() to the KernelExcursion class
itself, otherwise anyone else using KernelExcursion potentially has
the same problems found in BaseServlet & SiteNodeDispatcher.


Comment 4 Archit Shah 2003-12-18 04:51:53 UTC
We would rather do that, but it breaks things (some tests at the very
least). See my first comment. This bug should stay alive so we can
look for better solutions.


Note You need to log in before you can comment on or make changes to this bug.