Code review comment for lp://staging/~benoit.pierre/sloecode/incubation

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 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.

OK, I'll look into it, and create another branch/merge proposal for
this, probably after delivering the changes in revision 141 (more
fine-grained control for projects).

> 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!

Sure.

> 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?

Yeah, it's more of a hack for my particular use case:
- user should be authenticated
- most project are public by default
- real-life developers of a project are not developpers of the
  corresponding sloecode project: only integrators are, regular
  developers must use their user repository, and stacking, to
  deliver/backup their work (so only read access to the project)

In fact, it's more like a private launchpad instance with no bzr+http
support.

Trying to stack does lead to another host of problems, though...

Configuring each branch to be public or private is not feasible when
using the same shared repository... You would need 2 repositories, and
different URLs. Maybe mapping sc:~/ to person/private/, and sc:~user/ to
person/public/, where both are shared repositories, since giving a
sc:~/path/to/branch to another person does not make sense.

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

OK.

> 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 ;)

Yeah, the issue is when pushing with create prefix, you end up with a
tree of branches, and more than one branch can share the same nickname.

You can have:

sc:project1/component1/trunk
sc:project1/component2/trunk
sc:project1/user1/fix-ui
sc:project1/user2/fix-ui

And the same is true for a user repository.

Ideally, I think showing the path relative to the project/user base
directory is what should be done.

> Revision 141 looks good.

I'll create a new branch/merge proposal for this (without the support
for public projects).

>
> Revision 142: Same comment as rev 140 - I'd like some more information
> on what this code does...

See comment for revision 140 above.

--
email sent from notmuch.vim plugin