[Opensource] Bug in DBController.stateAllowed()
Ken Chow
kckc at shaw.ca
Tue Nov 18 03:51:32 PST 2003
Mike,
I reviewed the 5.3 code again and you're correct that the problem no longer
exists in the 5.3 code. However, I noticed that the 5.3 code only inserts
the
allowedStates for groups with a match in the cache and continues to iterate
through the remaining groups performing no apparent useful function even
after a match is found.
Wouldn't it be better return true upon finding the first match in
allowedStated
(Option 1) or continue iterating through the remaining groups, but also
inserting the allowedStates for groups with no match in the cache (so that
they can be retrieved from the cache on subsequent invocations of
stateAllowed() instead of retrieving from the database again (Option2).
For example, change the code as follows:
for (int i = 0; i < missingGroupNames.length; i++) {
String oneGroupName = missingGroupNames[i];
if (oneGroupName == null) {
return false;
}
csec.clear();
csec.setField(ControllerSecurity.CONTROLLER_CLASS, className);
csec.setField(ControllerSecurity.GROUP_NAME, oneGroupName);
for (Iterator cse = csec.searchAndRetrieveList().iterator();
cse.hasNext();) {
oneSecurityEntry = (ControllerSecurity) cse.next();
allowedStates =
oneSecurityEntry.getField(ControllerSecurity.STATES);
// *** Original Expresso 5.3 code ***
// if (containsAllowedState(allowedStates, newState)) {
// allowed = true;
// addCachedSecurity(oneGroupName,
// myRequest.getDataContext(),
// className, allowedStates);
// }
// *** Option 1 ***
if (containsAllowedState(allowedStates, newState)) {
addCachedSecurity(oneGroupName,
myRequest.getDataContext(),
className, allowedStates);
return true;
}
// *** Option 2 ***
if (containsAllowedState(allowedStates, newState)) {
allowed = true;
}
addCachedSecurity(oneGroupName,
myRequest.getDataContext(),
className, allowedStates);
} /* for */
} /* for each user group */
Ken
----- Original Message -----
From: "Michael Rimov" <rimovm at centercomp.com>
To: <opensource at jcorporate.com>
Sent: Monday, November 17, 2003 3:13 PM
Subject: Re: [Opensource] Bug in DBController.stateAllowed()
> Ken,
>
> I took a good look at this, and I don't think that Expresso 5.3 code has
> this issue, the biggest reason is that caching is stored by group now
> rather than user and a search is done for any groups that a cached
security
> entry could be found.
>
> Can you take a fresh look at the code and see if I'm missing something?
>
> -Mike
>
>
> At 01:46 AM 11/13/2003, you wrote:
>
> >I think I've found a bug related to a user that is a member of multiple
> >Security Groups. Here's the scenario:
> >
> > - I've defined two Security Groups: Reader and Creator
> > - the group Creator is a member of the group Reader
> > - Reader has access to the states promptUserUpdate and
processUserUpdate in
> >MemberController
> > - Creator has access to the states promptUserSearch, processUserSearch
in
> >MemberController
> > - logged on user is in the Creator group
> >
> > - when DBController.stateAllowed() is invoked with nextState
> >promptUserSearch for the MemberController
> > . for the group Creator with allowedStates="promptUserSearch,
> >processUserSearch", a match
> > is found, and allowedStates="promptUserSearch, processUserSearch"
is
> >saved in the cache
> > . for the group Reader with allowedStates="promptUserUpdate and
> >processUserUpdate", NO match
> > is found, and nothing is saved in the cache
> >
> > - when DBController.stateAllowed() is invoked again with nextState
> >promptUserUpdate for the MemberController
> > . a match is found in the cache with allowedStates="promptUserSearch,
> >processUserSearch"
> > . promptUserUpdate is not contained in the allowedStates retrieved
from
> >the cache
> > . stateAllowed() incorrectly returns false (because not all the
allowed
> >states for the uid|MemberController
> > are stored in the cache)
> >
> >I'm currently working with Expresso 5.0.3, but have also reviewed the
5.0.5
> >and 5.3 RC5 code and they appear to have the same problem.
> >
> >I've attached a revised version of stateAllowed() with the problem
corrected
> >by accumulating all allowed states for the user (from all groups that the
> >user is a member of) before adding to the cache. I suspect that this may
not
> >be the best way to fix the problem since there may be side-effects that
I'm
> >nor aware of, but it appears to be working.
> >
> >Ken Chow
> >
> >-----------------------------------------------------------
> >
> > public synchronized boolean stateAllowed(String newState,
> > ControllerRequest
myRequest)
> > throws ControllerException {
> > try {
> >
> > /* if the security cache is available, use it */
> > if
> >(CacheManager.getInstance().existsCache(myRequest.getDBName(),
cacheName)) {
> > ValidValue secVal = (ValidValue)
> >CacheManager.getInstance().getItem(
> > myRequest.getDBName(), cacheName,
> > myRequest.getUid() + "|" +
> > getClass().getName());
> >
> > /* if there is an entry for this user */
> > if (secVal != null) {
> > String sec = secVal.getDescription();
> >
> >
> > if (containsAllowedState(sec, newState)) {
> > if (log.isDebugEnabled()) {
> > log.debug("User '" + myRequest.getUid() +
> > "' allowed state '" + newState +
> > "' via cache");
> > }
> >
> > return true;
> > } else {
> >
> > /* otherwise they are denied */
> > if (log.isDebugEnabled()) {
> > log.debug("User '" + myRequest.getUid() +
> > "' denied state '" + newState +
> > "' via cache in db '" +
> > myRequest.getDBName() +
> > "'. Cache entry was '" + sec + "'");
> > }
> >
> > return false;
> > }
> > }
> >
> > /* otherwise there was no entry, so check the DB
directly */
> > }
> >
> > /* if security cache is available */
> > User thisUser = new User();
> > thisUser.setDBName(myRequest.getDBName());
> > thisUser.setUid(myRequest.getUid());
> > thisUser.retrieve();
> >
> > ControllerSecurity cs = new ControllerSecurity();
> > cs.setDBName(myRequest.getDBName());
> >
> > String oneGroupName = null;
> > ControllerSecurity oneSecurityEntry = null;
> > String allowedStates = null;
> >// start of inserted code
> > FastStringBuffer accumulatedAllowedStates = new
> >FastStringBuffer();
> >// end of inserted code
> > boolean allowed = false;
> >
> > for (Enumeration eg = thisUser.getGroups().elements();
> > eg.hasMoreElements();) {
> > oneGroupName = (String) eg.nextElement();
> > cs.clear();
> > cs.setField("ControllerClass", getClass().getName());
> > cs.setField("GroupName", oneGroupName);
> >
> > for (Iterator cse =
cs.searchAndRetrieveList().iterator();
> > cse.hasNext();) {
> >
> > oneSecurityEntry = (ControllerSecurity) cse.next();
> > allowedStates = oneSecurityEntry.getField("States");
> >// start of inserted code
> > if ((allowedStates != null) &&
> >(!allowedStates.equals(""))) {
> > if (accumulatedAllowedStates.length() > 0) {
> > accumulatedAllowedStates.append(",");
> > }
> > accumulatedAllowedStates.append(allowedStates);
> > }
> >// end of inserted code
> >
> > if (containsAllowedState(allowedStates, newState)) {
> > allowed = true;
> >// start of deleted code
> >// addCachedSecurity(myRequest.getUid(),
> >// myRequest.getDBName(),
> >// getClass().getName(), allowedStates);
> >// end of deleted code
> > }
> > } /* for */
> >
> > } /* for each user group */
> >
> >// start of inserted code
> > addCachedSecurity(myRequest.getUid(), myRequest.getDBName(),
> > getClass().getName(),
> >accumulatedAllowedStates.toString());
> >// end of inserted code
> >
> > if (!allowed) {
> >// start of deleted code
> >// addCachedSecurity(myRequest.getUid(),
> >myRequest.getDBName(),
> >// getClass().getName(), "");
> >// DelayThread.delay();
> >// end of deleted code
> >
> > if (log.isDebugEnabled()) {
> > log.debug("User " + myRequest.getUid() +
> > " denied all states in " +
getClass().getName()
> >+
> > " in db '" + myRequest.getDBName() + "'");
> > }
> > }
> >
> > return allowed;
> > } catch (DBException de) {
> > throw new ControllerException("Unable to check Controller "
+
> > "security", de);
> > } catch (CacheException ce) {
> > throw new ControllerException("Cache exception checking " +
> > "Controller security", ce);
> > }
> > } /* stateAllowed(String) */
> >
> >
> >
> >_______________________________________________
> >Opensource mailing list
> >Opensource at jcorporate.com
> >http://mail.jcorporate.com/mailman/listinfo/opensource
> >Archives: http://mail.jcorporate.com/pipermail/opensource/
>
>
> _______________________________________________
> Opensource mailing list
> Opensource at jcorporate.com
> http://mail.jcorporate.com/mailman/listinfo/opensource
> Archives: http://mail.jcorporate.com/pipermail/opensource/
>
More information about the Opensource
mailing list