launchpad-dev team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

On Sat, Jan 16, 2010 at 12:57 AM, Edwin Grubbs <edwin.grubbs@xxxxxxxxxxxxx> wrote:
> On Fri, Jan 15, 2010 at 1:46 AM, Stuart Bishop
> <stuart.bishop@xxxxxxxxxxxxx> wrote:
> > On Fri, Jan 15, 2010 at 1:48 PM, Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>
> > wrote:
> > > On Fri, 2010-01-15 at 13:02 +0700, Stuart Bishop wrote:
> > > > On Fri, Jan 15, 2010 at 12:00 PM, Edwin Grubbs
> > > > <edwin.grubbs@xxxxxxxxxxxxx> wrote:
> > > > > I can't figure out how to test bug 506963 on launchpad.dev. This is in
> > > > > part because I can't understand how this error could have possibly
> > > > > happen. The form is submitted as a POST, so the dbpolicy should choose
> > > > > the master main store. Although the EmailAddress is normally retrieved
> > > > > from the auth store, which appears to replicate that table from the
> > > > > main store, the place where this error occurs should be getting the
> > > > > email address from the store provided by Store.of(person).
> > > > >
> > > > > Does anyone know what is going on here?
> > > >
> > > > You have things backwards. EmailAddress is part of the auth store, and
> > > > available read-only via the main store. Store.of(person) will
> > > > generally give you the main store.
> > > >
> > > > Best way of fixing this sort of thing is if you need to write to an
> > > > object, cast it to the master version - IMasterObject(emailaddress)
> > > > will be a no-op if you already have the writable object and is future
> > > > proof when tables get shuffled around.
> > > >
> > > > lib/canonical/launchpad/doc/storm.txt for more details
> > >
> > > This is the context of the bug that Edwin is fixing.
> > >
> > >   3 InternalError: Slony-I: Table emailaddress is replicated and cannot
> > > be modified on a subscriber node
> > >    Bug: https://launchpad.net/bugs/464161
> > >    POST: 3  Robots: 0  Local: 3
> > >       3 https://launchpad.net/%
> > > 7Ekorhelvtak/+deactivate-account (Person:+deactivate-account)
> > >        OOPS-1475C322, OOPS-1475E193, OOPS-1475H234
> >
> > From https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-1475H234, SQL
> > statement #30 shows attempting to update EmailAddress using the wrong store.
> > Unfortunately the traceback doesn't show the code that actually did this -
> > the traceback is raised later when Storm attempts to flush the database
> > changes.
>
> The real hard part was figuring why our tests for deactivateAccount()
> weren't breaking. Though launchpad.dev doesn't use different dbs, it
> could use different users for the auth store and the main store, and
> after investigating it turns out it does. Then, I found a very
> non-obvious permission problem. Though, launchpad_main only has read
> access to the EmailAddress table, it is also a member of the pg group
> "write", which does have access to insert or update but not delete
> from that table. Removing launchpad_main from "write" caused other
> things to blow up, since launchpad_main doesn't have explicit access
> to the Person table. Revoking insert/append access from "write"
> allowed me to recreate the error and verify that IMasterObject() does
> indeed fix it.
>
> I'm not sure what the correct change to the permissions is for the
> test databases, so that we catch these problems earlier next time.
> Should launchpad_main get explicit access to all the tables it needs
> so that it can be removed from the "write" group?

The write group should not have write permissions to tables not in the lpmain replication set. I agree that this is a non-obvious permission bug.

I fully support all changes that reduce our dependence on the write and read groups in security.cfg. One day I'll assemble the tools needed to properly audit our permission (record what the test suite does, what production does, compare with what permissions are granted). One big problem is many of our tests rely on overgenerous permissions, creating and modifying rows in the database when authenticated as users that really shouldn't need those permissions.

If tests pass with write access to the EmailAddress table removed from the [write] group, please land that. I'm not sure if copying all the permissions from the [write] group to other users such as launchpad_main, revoking their membership in this group, helps much. If you think it does then go for it.

Thankfully, these issues generally only occur on the border. So Person, EmailAddress, Account & MailingListSubscription are where problems like this are most likely to appear. Also, thankfully, we should be able to drop the authdb replication set entirely in a month or three when login.launchpad.net is replaced by login.ubuntu.com bringing some sanity back to the situation.

--
Stuart Bishop <stuart@xxxxxxxxxxxxxxxx>
http://www.stuartbishop.net/

Attachment: signature.asc
Description: OpenPGP digital signature