Code review comment for lp:~lifeless/launchpad/private-librarian

Revision history for this message
Stuart Bishop (stub) wrote :

On Wed, Jul 28, 2010 at 5:08 AM, Robert Collins
<email address hidden> wrote:

> I used the session DB because this stuff is transient : memcached might be an answer too but the token *system* has to be available or private file downloads will fail, and memcached is currently (AIUI) a 'when its there use it' system - which is fine. Replicating the tokens isn't terribly useful given we're just going to delete them again, but all of that is a bit beside the point : yes, we do need to commit changes in get requests :).

Right. We should consider promoting memcached to an always-on service
as we have tripped over a few cases where this would be desirable.
That is a separate task though. Stuffing this in the session database
seems the best fit for now.

> Will move the definitions to launchpad_session.sql.

Ta.

> We could store the expiry time, but I figured we'd make the lifetime a config option and thus work from the creation point rather than expiry.
>
> I figured we might want reporting on stuff in the table - thats why I used url, token, rather than a single column. Easy to change if you have a strong opinion any which way - I don't.
>
> I can't answer the url/path segment question yet : once its fully hooked up it should be obvious what is easiest. The current schema is what was easiest based on the attributes of the objects I am using (LibraryFileAlias etc).

I suspect the host and port is just noise and we only want the path.
We have a complex environment, and relying on the URLs seems fragile
so I personally would feel more secure if we just stored the path.

> We could make the librarian offer a redirect *back* to launchpad for expired tokens, rather than a go-away message - adding the source canonical_url to the table would permit that trivially.

That sounds like a good idea.

> Re: exceptions for anonymous users calling TimeLimitedToken.allocate - I can add that in, is there some existing similar safety need I can reference?

If we offer the redirect back on expired tokens, there is no need for
this. My concern was expired tokens getting stuck in caches and Squid
caching Anonymous connections was the first I thought off.

After going over bug lists with Gary last night, this will solve a lot
of outstanding Librarian issues.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

« Back to merge proposal