I'd like to propse the following (untested) code as how isApproved() and isGods() should behave. The idea is to make isApproved() more flexible by allowing it to take a string as the second argument, its adds a third argument to allow accessrules to operate on a "per-node" basis. It also sets up a return set of data in the $HTMLVARS{isApproved} that can be used by the caller to obtain information about why the user was not approved.
The most important thing is the per session caching of isGods() and isApproved() lookups. I've noticed that when you review the code these two routines get called many many times. Caching them per session will mean that this overhead is considerably reduced. Despite the way the caches look (HoH and HoHoH) I beleive that per session they will have relatively little data in them. And since %HTMLVARS is reset every session I dont beleive we have data synchornization problems or the like.
Anyway, comments appreciated. Also, i say the code is untested insofar as I havent actually put it into play and tested it on the pmdev server. It does compile, and seems to behave as expected in the dumper prompt and should be pretty close to bug free.
BTW, i was considering a fourth argument to isApproved(), a flag that would determine if gods should really always be approved. If the flag was true then the gods would only be approved if the gods were in the group, otherwise the behaviour would remain unchanged. IE when this flag was set the result would be a true membership/rule check and not implicitly put the gods in every group.
Update: Testing on the pmdev server indicates there is a flaw in the code below. I need to figure out what it is. /gah Ok, all fixed now. This code is running on the test server right now. Also there is some instrumnetation code in there to determine how often the cache is used, and how often the two subs are called. On the dev server there is code to see the results, and in my testing around 85% or more of the calls were resolvable from the session-cache. The sample total was 42 calls, with 36 in total resolved from cache. When you look to the two seperately, the numbers were 1:7 for isGods and 4:30 for isApproved. IMO this makes it clear this is an improvement. I have to say the feel of the site on pmdev seems to be more responsive too.
###################################################################### +####### # Sub # isApproved # # Purpose # Checks to see if the given user is approved within a given gro +up # # Parameters # $USER - reference to a user node hash (-1 if super user) # $GROUP - reference to a nodegroup that the user might be in # $NODE - optional reference to the item being tested against. # its prescence allows rules to be applied against the # the item. # $NotGod - If the last parameter is the string 'notgod' # then isApproved will not consider gods to # to be automatically approved. # # Returns # true if the user is authorized, false otherwise # # Uses %Everything::HTML::HTMLVARS as a per-session cache # of the results as well as populating # $Everything::HTML::HTMLVARS{isApproved} # with a hash of the results. This hash should be treated # as read only, and may contain the following keys: # group => the last $GROUP checked # user => the last $USER checked # node => the last $NODE # rule => if a rule was responsible for a failure # it will be provided here # error => if an error occured during processing # a rule the value will be here # ret => the actual return value (0/1) # # Note this cache is per session, which means on average # it will be for one $USER and most likely three or four # groups. It wont be large. sub isApproved { my( $this, $USER, $GROUP, $NODE, $notgod )= @_; ($notgod,$NODE)=($NODE,$notgod) if $NODE eq 'notgod'; my $vars=\%Everything::HTML::HTMLVARS; $vars->{isApproved_calls}++; my $cachekey = join "|", map { ref $_ ? $_->{node_id} : $_ } $USER,$GROUP,$NODE?$NODE:0,$notgod?1:0 ; if (my $hash=$vars->{'isApproved.Lookup'}{$cachekey}) { $vars->{isApproved_calls_cachedret}++; $vars->{isApproved}=$hash; return $hash->{ret}; } else { $vars = $vars->{'isApproved.Lookup'}{$cachekey} = $vars->{isApproved} = {}; } $vars->{user}=$USER; $vars->{group}=$GROUP; $vars->{node}=$NODE; $vars->{notgod}=$notgod; return $vars->{isApproved}=0 if ! $USER || ! $GROUP; if ( !ref $GROUP ) { if ($GROUP=~/^\d+$/) { $GROUP=$this->getNodeById( $GROUP ); } else { my ($name,$type)=split /\s*\|\s*/,$GROUP; return $vars->{isApproved}=0 if ! $name; if ($type) { $GROUP=$this->getNode($name,$type); } else { $GROUP=undef; } $GROUP||=$this->getNode($name,'usergroup'); $GROUP||=$this->getNode($name,'accessrule'); } } return $vars->{isApproved}=0 if ! $GROUP; # We always say that gods are approved, # regardless of their true membership. # Is this really the right thing to do? # It maintains legacy behaviour, but its unclear # if this really is the best idea. (demerphq) return $vars->{ret}=1 if !$notgod && $this->isGod($USER); my $user_id = $this->getId($USER); #You're always approved if it's yourself... return $vars->{ret} = 1 if $user_id == $this->getId($GROUP); foreach my $node ( @{ $this->selectNodegroupFlat($GROUP) } ) { return $vars->{ret} = 1 if $user_id == $this->getId($node); if( $node->{type}{title} =~ /accessrule$/i ) { my $RULE= $node; my $res= do { package Everything::HTML; eval $node->{code}; }; if ($@ or (!$res and defined $res)) { $vars->{isApproved}{rule} = $node; } if( $@ ) { Everything::printLog( "Access Rule eval error in $node->{node_id}\n" . "$@\n$node->{code}" ); $vars->{error}=$@; return $vars->{ret} = 0; } return $vars->{ret}= $res if defined $res; } } return $vars->{ret} = 0; } ###################################################################### +####### # Sub # isGod # # Purpose # Checks to see if a user is in the gods group. This includes r +oot # and '-1' as gods. This also checks sub groups so you can have # other usergroups in the gods group. # # Parameters # $USER - an id or HASH ref to a user node. # # Returns # 1 if the user is a god, 0 otherwise # # Uses %Everything::HTML::HTMLVARS as a per-session cache # sub isGod { my ($this, $USER) = @_; my $user_id; my $usergroup; my $GODS; my $godsgroup; my $god; # he's my god too... return 1 if($USER == -1); $this->getRef($USER); my $vars=\%Everything::HTML::HTMLVARS; $vars->{isGod_calls}++; if (defined $vars->{'isGod.Lookup'}{$USER->{node_id}}) { $vars->{isGod_calls_cachedret}++; return $vars->{'isGod.Lookup'}{$USER->{node_id}} } $user_id = $this->getId($USER); $usergroup = $this->getType("usergroup"); ($GODS) = $this->getNode("gods", $usergroup); $godsgroup = $$GODS{group}; #$this->selectNodegroupFlat($GODS); my $is_god=0; foreach $god (@$godsgroup) { if ($user_id == $this->getId($god)) { $is_god=1; last; } } $vars->{'isGod.Lookup'}{$USER->{node_id}}=$is_god; return $is_god; }
|
|---|