[Opensource] Bug in DBController.stateAllowed()

[Michael Rimov]

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/