[isf-wifidog] Re: [wirelesstoronto-discuss] code contribution to wifidog auth server

[Benoit Grégoire]

On November 15, 2005 06:19 am, Rob Janes wrote:
> this is long.  as requested, here's my thoughts ...
>
> what i'd like is the ability (authorization) to maintain a branch of the
> wifidog source repository.  that would make it much easier for me/us to
> merge isf changes made at the head of cvs with wireless toronto's branch
> of customizations.  the bonus for you isf guys is that unlike bcwireless
> we would be contributing code to the wifidog effort.  you could review
> the customizations we had to make, and come up with some generalization
> so that we would be able to make our customizations less intrusive.
>
> the fact is that we have made changes to wifidog.  i don't like it that
> we haven't posted our changes back to isf.  i don't like it that our
> changes are not in the main repositiory.  i didn't like it that our
> changes were not in any repository whatsoever until recently.
>
> we have yet to discuss it as a group in person, but we should.  there
> are two threads of the email discussion.
> 1. the changes patrick made are good.  we want patrick to overlay his
> changes onto the latest wifidog.
> 2. we don't want to branch the source since that will be hard to maintain.
>
> these two points are mutually exclusive, ie they contradict each other.
> we as a group need to resolve this, since patrick's changes are a
> defacto branch of the source.

I believe you trying to solve two different problem with a single solution, 
and that is the source of the disconnect you see.  There are usually four 
types of branches (I'm sure there is some sort of official terminology for 
this, but I don't have time to look it up).  

Main (or head) branch:  The branch that will eventually become, or branch 
into, the latest official version.

Ad-hoc developpement branch: when you want to develop major changes away from 
the mainline, but still collapse it back into the mainline at some future 
date.  It allows to work as a group on major changes without disrupting the 
mainline.  This may be usefull for some of your changes, but you must be 
certain it's worth the extra overhead, and it has to be a different branch 
from the following.

Customised branch:  when you need a customised version of the main (or some 
other) branch.  Youy regularly sync with the main branch, but you do not 
intend to ever merge it back.  That the case for your cosmetic changes, but 
only when you can't wait for (or contribute) the necessary flexibility in the 
mainline.

Stable release branch.  Used to allow changes to stable release that are not 
relevent to the main branch.

So to summarise:  You are going to need you own branch (wirelesstoronto-custom 
(or some such) for the near future, with the goal of eventually not needing 
it anymore.  All that will be there are cosmetic changes.  Having that branch 
on our CVS will not by itself make it easier to share your changes back (but 
there should not be any changes to share back, and having it on our server 
will make it much easier for you to sync it to the mainline).

Most of your changes should go directly in head, and occasionally in a 
development branch (not the same as your custom branch above).

> for myself, i started working on two things in wifidog-auth.
>
> 1. language support doesn't work out of the box on any server I have set
> it up on.

That work that should clearly be done on the head branch.

> 2. the install process is not smooth.  i believe it's an impediment to
> bringing wifidog up in new locations.
>
> generally, i like the idea of an install/customization script.  so,
> rather than cleaning up the installation instructions, i put some effort
> into refactoring the install script.  it has become a bit of an
> avalanche, but i personally am happy with where it's going.  i didn't
> start out wanting to make changes to the core of wifidog-auth, far from
> it.  however, my install web pages look like a frankenstein monster
> (patchwork).  when i tried to smooth that out it was logical to plug in
> to the existing wifidog-auth UI.  that started me on small changes,

Oups, you didn't know, but that page didn't use the main wifidog UI on 
purpuse, because it can't initially depend on any of the wifidog 
dependencies, not even the mandatory ones (smarty, db abstraction, etc.)

> fixups really, to the UI.  i think you know what those are.  when I got
> to the database setup stuff, i found myself writing a database
> abstraction layer at the same time as wanting to use existing
> wifidog-auth functions for sniffing the database and upgrading the
> database.  then when i wanted to work out some way to display the
> database errors (of which there were a lot), i found myself writing a
> logging abstraction layer since there is none in wifidog-auth.
>
> all this stuff seemed to me to be a "useful thing" (to paraphrase winnie
> the pooh), so i made these pieces so that they integrated with
> wifidog-auth.
>
> clearly i've crossed over the rubicon of simple customizations.  these
> are intrusive changes to the core.  if you think they are useful too,
> i'd like to see them checked in at the head.  these are
>
> to wifidog-auth/wifidog:
> cvs add classes/DBObject.php
> cvs add classes/DBODriver_mysql.php
> cvs add classes/DBODriver_postgres.php
> cvs delete classes/AbstractDbMySql.php
> cvs delete classes/AbstractDbPostgres.php
> replacement for classes/AbstractDb.php

At first glance, I don't get it.  We already have a custom DB abstraction 
layer.  We are open to replacing it with an off the shelf one or improving 
it, but I don't see what replacing it with different but still custom one 
would buy anyone?  But I guess I'll understand when I see the code.

> cvs add classes/EventLogging.php

That's definitely sorely needed!

> then there are changes to common.php to use these two things.  as near
> as i can tell, it's a seamless integration.  however, it is still a work
> in progress.  it is missing some features i want to have for the
> installation script.  it is missing much documentation, making it very
> difficult for others to use.

It's seems unlikely that the only changes needed in the db abstraction layer 
are in common.php, unless you kept the exact same API (but then why change 
the abstraction layer?). 

> if you folks on the other hand find that these changes are not useful,
> then i'll have to find a way to use wifidog-auth's UI from the install
> script without making changes to wifidog-auth's internals.

See above

> however, i've made some security improvements to the database stuff i
> hope you'll accept -
>
> * database connections using the unix socket (more secure)

?!?!? That was already supported and documented in the INSTALL file.

> * distinguish DBA connections from regular user connections so you can
> lock down permissions on the regular wifidog-auth database userid

Aside from being able to create new database, the wifidog user needs no 
special privileges, and should not have any.

> why didn't i use PEAR for a db abstraction or a logging abstraction?  i
> looked in to that.  nothing i found on pear did what i wanted it to do,
> and the stuff on pear was not easy to integrate with wifidog-auth.
> also, i needed the install script to use this stuff from the get-go.  i
> didn't want to deal with writing the install script so as to fail
> gracefully if it's own prerequisites were not present.  i want the
> install script to be self-contained.

Excellent.

> as well, i made intrusive changes to include/schema_validate.php.  i
> wanted to use existing routines to check the status of the database.
> however, i most emphatically did not want any changes made to the
> database until the install script was ready.  BUT that's just not how
> schema_validate out of the box works.  on a deeper level, i object to
> running such application intrusive stuff from a user level account.
> schema_validate assumes that the default postgres account for wifidog is
> a dba level account.  that's a security thing.  secondly, i don't like

It should not have any privileges except creating database.

> the idea that database maintenance tasks show up on user's sessions.
> these sorts of things should only be done by the "installer person",
> regular users should not be abused with this kind of stuff on their
> screens.  I am hoping you will find these changes a useful thing.

Well, I may be missing something, but I find what you propose extremely 
dangerous.  When you upgrade a live server, the schema update must happen as 
soon as the code is updated, no matter what.  

Otherwise, you have new code running that may or may not work with the old 
schema (and may even accidentally corrupt data in a way that will break the 
assumptions of the upgrade script).  If you instead checkout in a sandbox 
(but with your real database) to have your schema update before update the 
code on the live server, you have the opposite problem:  old code running 
that may or may not be compatible with the new schema.  That's why schema 
validate is always called (not just from the install script), and why the 
wifidog user must have table creation privileges.

> finally, there is the rewritten install script itself.  i would like to
> contribute that as well.  to be nice, i can put together the install
> script so that it is not intrusive to wifidog-auth and plays by the
> rules.  i would like to work with someone in isf to make sure it works
> well and not just for me.  later this week i hope to have it installed
> on our devauth server so that you'll be able to see it and consider
> whether it too is a useful thing.

I'm sure it's usefull, we must simply make sure we handle the dependencies 
properly (which will affect when it can start to call into the "normal" UI).

> with regards maintenance or responsibility for these changes, i'm happy
> to take on a "maintainers hat" for these things.  i would however need
> write access to the cvs head.

Sure! I'll need your sourceforge username however.

-- 
Benoit Grégoire, http://benoitg.coeus.ca/
-------------- section suivante --------------
Une pièce jointe non texte a été nettoyée...
Nom: non disponible
Type: application/pgp-signature
Taille: 189 octets
Desc: non disponible
Url: http://listes.ilesansfil.org/pipermail/wifidog/attachments/20051115/a9f5ed86/attachment.pgp