Merge lp://staging/~benoit.pierre/sloecode/incubation into lp://staging/sloecode

Revision 129 & 130 are fine.

Revision 131 looks fine, but I'm a bit concerned about revision 132. I understand that you need to add new users to the sloecode database once they're authenticated, but surely there's a hook in the repoze PAM authentication plugin that can be used for this? In fact, I think it can be easily done by specialising the PAMAuthenticatorPlugin and overriding the authenticate(...) method such that successful authentications are then inserted into the database, if they don't already exist. See:

http://docs.repoze.org/who/1.0/narr.html#writing-an-authenticator-plugin

for more information. Also, while you're doing this, I think it makes sense to set the password in the database correctly - that way one can switch away from PAM authentication in the future and existing accounts will continue to work. Once you're making the change listed above, this should be trivial to implement.

Revisions 133, 134, 135 look fine.

Revision 136: the public_projects() function in sloecode/model/project.py should really use a SQLAlchemy query to get all the public projects, rather than getting all projects and iterating over them. You should be able to do something similar to:

def public_projects():
    return Project.get(public=True)

Unfortunately, whenever we make a database schema change we need to also write a database patch file that is able to alter a live database without deleting the current data. These are simple SQL scripts that make the appropriate changes, and are run as part of the debian package's upgrade scripts. See the existing ones in 'dbpatches/patches/*/' - you need to write one for mysql and one for SQLite. SQLite is a bit more tricky because it doesn't let you alter tables much, but you should be able to adapt an existing patch file for your needs. I'd suggest adding the 'public' column, and setting all projects to private by default.

Also, it would be nice to have some text explaining what a public project is in the template - either as a short string next to the 'Public' checkbox, or as a help page. Something that explains that public projects are readable by anyone with access to the sloecode server, but write access is still restricted to developers and managers. I also wonder whether we should not allow the addition of 'Observers' to a public project - then again, that UI needs to be re-done anyway, so maybe we'll deal with that later.

Finally, this isn't a bug that you've introduced, but in the nav.html file, projects should be listed alphabetically, in a case-insensitive fashion. If you're able to make that change without too much trouble that would be superb!

Rev 137 is great, and has already been merged with trunk.

Rev 138 needs more thought. I'd like users to have some control over whether their branches are public or private.... or maybe this is a config setting?

Revision 139 looks OK, but maybe thumper can comment on this?

Revision 140 - I'd like some more information on the change - it looks potentially good, but I can't see any change in output on my test system... In any case we'd need to get rid of the 'if False' thing ;)

Revision 141 looks good.

Revision 142: Same comment as rev 140 - I'd l...

On Thu, 29 Sep 2011 00:45:28 -0000, Thomi Richards <email address hidden> wrote:
> Review: Needs Fixing
>
> Revision 129 & 130 are fine.

OK, I'll create a branch/merge proposal for this.

>
> Revision 131 looks fine, but I'm a bit concerned about revision 132. I
> understand that you need to add new users to the sloecode database
> once they're authenticated, but surely there's a hook in the repoze
> PAM authentication plugin that can be used for this? In fact, I think
> it can be easily done by specialising the PAMAuthenticatorPlugin and
> overriding the authenticate(...) method such that successful
> authentications are then inserted into the database, if they don't
> already exist. See:
>
> http://docs.repoze.org/who/1.0/narr.html#writing-an-authenticator-plugin
>
> for more information.

OK.

> Also, while you're doing this, I think it makes sense to set the
> password in the database correctly - that way one can switch away from
> PAM authentication in the future and existing accounts will continue
> to work. Once you're making the change listed above, this should be
> trivial to implement.

I'm really not sure mirroring the password should be done, or if it's
even possible at all. And how do you handle password changes, where
there are not done through the sloecode web interface?

AFAIK, redmine or reviewboard don't do it when using an external
authentication mechanism (like PAM, LDAP, or ActiveDirectory).

I used PAM because it was easier to setup, but I started by looking at
the LDAP plugin, I think some of the authentication configuration could
be moved to the configuration file.

> Finally, if you're using PAM authentication, how do you change the
> admin password from it's initial, insecure setting?

This is a real problem :) Ideally, the admin account should be special:
always using the internal authentication mechanism, especially if at
some point, LDAP/PAM configuration is available through the web
interface, so you can always login to fix an invalid configuration.

> Revisions 133, 134, 135 look fine.
>
> Revision 136: the public_projects() function in
> sloecode/model/project.py should really use a SQLAlchemy query to get
> all the public projects, rather than getting all projects and
> iterating over them. You should be able to do something similar to:
>
> def public_projects():
> return Project.get(public=True)
>
> Unfortunately, whenever we make a database schema change we need to
> also write a database patch file that is able to alter a live database
> without deleting the current data. These are simple SQL scripts that
> make the appropriate changes, and are run as part of the debian
> package's upgrade scripts. See the existing ones in
> 'dbpatches/patches/*/' - you need to write one for mysql and one for
> SQLite. SQLite is a bit more tricky because it doesn't let you alter
> tables much, but you should be able to adapt an existing patch file
> for your needs. I'd suggest adding the 'public' column, and setting
> all projects to private by default.
>
> Also, it would be nice to have some text explaining what a public
> project is in the template - either as a short string next to the
> 'Public' checkbox, or as...

> On Thu, 29 Sep 2011 00:45:28 -0000, Thomi Richards <email address hidden> wrote:
> > Also, while you're doing this, I think it makes sense to set the
> > password in the database correctly - that way one can switch away from
> > PAM authentication in the future and existing accounts will continue
> > to work. Once you're making the change listed above, this should be
> > trivial to implement.
>
> I'm really not sure mirroring the password should be done, or if it's
> even possible at all. And how do you handle password changes, where
> there are not done through the sloecode web interface?
>
> AFAIK, redmine or reviewboard don't do it when using an external
> authentication mechanism (like PAM, LDAP, or ActiveDirectory).
>
> I used PAM because it was easier to setup, but I started by looking at
> the LDAP plugin, I think some of the authentication configuration could
> be moved to the configuration file.
>

Good point.