Bug 104899 - before/afterSave methods accessing Kernel.getContext().getParty() are potentially broken
before/afterSave methods accessing Kernel.getContext().getParty() are potenti...
Status: CLOSED WONTFIX
Product: Red Hat Web Application Framework
Classification: Retired
Component: other (Show other bugs)
nightly
All Linux
medium Severity medium
: ---
: ---
Assigned To: Archit Shah
Jon Orris
: Security
Depends On:
Blocks: 100952
  Show dependency treegraph
 
Reported: 2003-09-23 09:13 EDT by Daniel Berrange
Modified: 2007-04-18 12:57 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-11-10 16:03:26 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Daniel Berrange 2003-09-23 09:13:47 EDT
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 16:29:03 EST
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 15:37:34 EST
my plan was implemented on 6.0.x (38819) and dev (38822)
Comment 3 Daniel Berrange 2003-12-16 04:21:51 EST
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-17 23:51:53 EST
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.