Re: amavisd-sql driver (was: hi there folks...)

Chuck Hagenbuch wrote:
> Quoting Kendrick Vargas <[email protected]>:
> 
>> The way that sam currently treats policy retrieval and storage doesn't 
>> seem
>> to go well with that idea. I don't know which is right or which is wrong,
>> but I'd like to alter the amavis-sql driver to work well with either.
>>
>> so... am I barking up a really tall tree here for no reason?
> 
> 
> You sound like you're making sense, but I'm not really familiar with 
> amavis at
> all, so I'm not the person to really say. So I encourage you to be 
> persistant
> until the amavisd driver author (Max) responds, or if he's busy, until 
> other
> folks decide you know what you're talking about and think you should 
> maintain
> it. :)

ok... lovely. I guess persistence worked for me on the forwards package, so 
it might work here :-) Is Max terribly active on this list? I think I might 
just get to work on modifying the sources now and stop if I see a negative 
response.

Among things that scare me are:

from lib/Driver/amavisd_sql.php:

     function _store()
     {
         /* Make sure we have a valid database connection. */
         $this->_connect();

         /* Check if the policy already exists. */
         $policyID = $this->_lookupPolicyID();
         if (is_a($policyID, 'PEAR_Error')) {
             return $policyID;
         }


and that's all fine and good, except noone actually checks to see if the 
connection was actually successful before trying anything on the database. 
This also halts the rest of the settings storage if the policy is not found, 
which could happen if the user doesn't have a policy and we're just 
depending on defaulting to the base settings that amavisd-new has. This 
effectively stops the storage of the black and whitelists.

Also, the _lookupPolicyID() is based on searching the policy_name with the 
current username, instead of the actual policy_id stored in the user table. 
Witness the following from README_FILES/README.lookups in the amavisd-new 
sources:

-- local users
CREATE TABLE users (
   id         int unsigned NOT NULL auto_increment,
   priority   int          NOT NULL DEFAULT '7',  -- 0 is low priority
   policy_id  int unsigned NOT NULL DEFAULT '1',
   email      varchar(255) NOT NULL,
   fullname   varchar(255) DEFAULT NULL,    -- not used by amavisd-new
   local      char(1),     -- Y/N  (optional field, see note further down)
   PRIMARY KEY (id),
   KEY email (email)
);
CREATE UNIQUE INDEX users_idx_email ON users(email);

[[[SNIP]]]

CREATE TABLE policy (
   id               int unsigned NOT NULL auto_increment,
   policy_name      varchar(32),     -- not used by amavisd-new

   virus_lover          char(1),     -- Y/N
   spam_lover           char(1),     -- Y/N  (optional field)
   banned_files_lover   char(1),     -- Y/N  (optional field)
   bad_header_lover     char(1),     -- Y/N  (optional field)

   bypass_virus_checks  char(1),     -- Y/N
   bypass_spam_checks   char(1),     -- Y/N
   bypass_banned_checks char(1),     -- Y/N  (optional field)
   bypass_header_checks char(1),     -- Y/N  (optional field)

   spam_modifies_subj   char(1),     -- Y/N  (optional field)

   virus_quarantine_to      varchar(64) default NULL,   -- (optional field)
   spam_quarantine_to       varchar(64) default NULL,   -- (optional field)
   banned_quarantine_to     varchar(64) default NULL,   -- (optional field)
   bad_header_quarantine_to varchar(64) default NULL,   -- (optional field)

   spam_tag_level  float default NULL,  -- higher score inserts spam info 
headers
   spam_tag2_level float default NULL,  -- inserts 'declared spam' header fields
   spam_kill_level float default NULL,  -- higher score activates evasive 
actions, e.g.
                                        -- reject/drop, quarantine, ...
                                      -- (subject to final_spam_destiny setting)
   spam_dsn_cutoff_level float default NULL,            -- (optional field)

   addr_extension_virus      varchar(64) default NULL,  -- (optional field)
   addr_extension_spam       varchar(64) default NULL,  -- (optional field)
   addr_extension_banned     varchar(64) default NULL,  -- (optional field)
   addr_extension_bad_header varchar(64) default NULL,  -- (optional field)

   PRIMARY KEY (id)
);

The policy_id in the users table is an int (not a form of char), and the 
comment on the policy_name field in the policy table clearly says that 
amavisd doesn't even use the field, ie... it's there for cosmetics. Back in 
the sam driver for amavisd_sql, we see this:

     function _lookupPolicyID()
     {
         /* Make sure we have a valid database connection. */
         $this->_connect();

         /* Build the policy lookup query. */
         $query = sprintf('SELECT %s FROM %s WHERE %s = %s',
                          $this->_mapAttributeToField('policies', 'id'),
                          $this->_mapNameToTable('policies'),
                          $this->_mapAttributeToField('policies', 'name'),
                          $this->_db->quote($this->_user));

it's basically looking up the policy by name and not ID. In fact, the only 
reason it seams we ask about the policy_id field in the backend 
configuration is so we can set the policy id in the user table to the policy 
we've forced creation of.

Also, in the _store method, it seems we delete the policy and insert a new 
entry, instead of updating the current policy. This would not bode well if a 
  bunch of users were using the same policy and we were actually loading the 
policy by ID instead of name.

So... I have some gripes that I think are legitimate. The only thing that 
frightens me is that I think I might have to do some serious rewriting of 
the driver. Mostly, there'd have to be some sort of tag in the database for 
the policy to indicate that the policy is private (and updateable) or that 
it's public (and thus should not be updated... any change means creation of 
a new private policy).

Extending this idea, it'd be cool if the GUI had a dropdown which allowed 
users to pick one of the default policies. And going on that idea, it might 
be cool if the GUI were a little more organized regarding the logical 
blocking of the options specific to amavisd-new.

Mostly I'd like to see if I could get some of this in before Horde 4 is 
released. I didn't realize it'd gone to RC-1 the other day. I've been using 
the alphas on my personal (yet, treated like production) server for months 
now and have been thrilled with it. I guess I'll see how this goes.

Thanks, btw, for the lightening quick response. :-)
			-peace

--
Let he who is without clue cast the first scone
-- 
sam mailing list - Join the hunt: http://horde.org/bounties/#sam
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: [email protected]