launchpad-dev team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

On Thu, Jul 23, 2009 at 10:42 AM, Julian
Edwards<julian.edwards@xxxxxxxxxxxxx> wrote:
> On Thursday 23 July 2009 14:11:44 Celso Providelo wrote:
>> Right, in the same way we implicitly check packagesets when querying
>> permission for a given package name, we could also check against the
>> source *current* component if we knew the context distroseries, no
>> need to callsites to call the canUpload() again.
>
> Ah, very good point, I hadn't thought of doing it right there in canUpload(),
> because my vision is clouded by the mess in NascentUpload and its
> peculiarities regarding different error messages (as you mentioned below).
>
>> Based on the discussion we had on #lp-dev yesterday we need to
>> consolidate all this logic in a single point, something like:
>>
>> can_upload(user, archives, source_name, distroseries, pocket) -> Yes or
>> None
>
> Are you suggesting to move this away from IArchive.canUpload()?  I don't think
> that's a good idea if so. Also, is archives (plural) a typo?

Typo, my bad.

>>   * check upload right to the pocket (archive, distroseries, pocket)
>
> I think we need to get more clever about pocket here.  We should be able to
> default it to RELEASE or UPDATES at least, depending on the distroseries
> state.  Jono would still need to pass it of course, since he has an object
> referring to a suite.

Right, the pocket information is implicit in the package-branch
(ubuntu/karmic-updates/foobar), IIRC
We don't need to assume it, it will be provided and we can check it
explicitly, before allowing the upload and/or push

>>   * check upload rights on the source_name/packageset (archive,
>> source_name)
>> * figure out the component where the source is currently
>> published (archive, distroseries)
>
> What about pocket?  I don't think I've seen the component change between
> pockets but it's possible, right?  Or we can just take all info from the most
> recent publication.

Yes, it is possible and nasty because it causes unexpected changes
when building other SRUs.

BACKPORTS are allowed to build-depend on any component in order to
avoid most of the cases when we need move binaries around in
post-Release pockets, but it probably doesn't cover all the cases.

To be safe, we can use component and replicated the mechanism used in
NU.getSourceAncestry() and lookup the target pocket and fallback to
the RELEASE pocket if necessary.

Since we have the pocket value, it shouldn't be a big problem.

>>   * check upload rights on the component (archive, component)
>
> Perfect!  But, we only need to do this if distroseries < karmic soon, unless
> Colin needs us to use both components and packagesets still.

We've talked about it already, component-based ACLs (and component
themselves) are not going to suddenly die, there will be a transition
period, and it's not clear at all if they will be entirely replaced at
some point.

Anyway, the component-ACL check it done *after* the name/packageset
check for this reason.

>> This function can immediately replace NascentUpload.verify_acl() and
>> UploadPolicy.checkUpload() in archiveuploader/ if it respects some
>> specialties (checks for users with no permissions at all ate treated
>> differently).
>
> We need to think about the ancestry checks as well and avoid doing them twice.

That's a good point, indeed, but it seems to be a problem specifically
in the NascentUpload workflow. PackageBranches do not know about
components until this point.

Maybe it's silly, but even if we execute the component lookup twice in
NU domain, it doesn't seem to be a blocker for the refactoring the way
it was proposed.

>> I think refactoring the existing code in archiveuploader/ domain is
>> the easier and path to get what we need for package-branches without
>> introducing any, possibly unneeded, extra abstraction to the system.
>>
>> We can think about benefiting of the check in the zope permission domain
>> later.
>
> +1
>

Thanks.

-- 
Celso Providelo <celso.providelo@xxxxxxxxxxxxx>
IRC: cprov,  Jabber: cprov@xxxxxxxxxx, Skype: cprovidelo
1024D/681B6469 C858 2652 1A6E F6A6 037B  B3F7 9FF2 583E 681B 6469