[Twisted-web] [Nevow] about new guard in sandbox

[Valentino Volonghi aka Dialtone]

On Mon, 31 Jul 2006 21:05:26 +0200, Manlio Perillo <manlio_perillo at libero.it> wrote:
>Hi again.
>
>I have read the draft implementation of guard in Valentino's sandbox and
>I like it.
>
>However I have some questions:
>- why the ISessionMenager interface does not include a name attribute
>  (since the default Session class uses the private _name)?

The reason why it misses the name in the interface is that it's not meant to be accessible from the outside. It's a pretty important internal detail in _this_ SessionManager implementation. There might be many other ways to deal with distributed session rather than simply using name attribute on the sessionmanager and ISessionManager ought to be as generic as possible.

>- why Session.authenticatedAs has to be a property?

Because I wasn't able to find a better name that was somewhat compatible
to using an explicit get/set. And because it's cleaner IMHO.

>- I think that there is no need to store tha password but only the
>  username, so authenticatedAs -> avatarID

Not really... Authentication is rerun for each request because the avatar is not saved and this is the most important detail of that new guard. Also the avatarId is not available in guard because it's a cred implementation detail. And the avatarId might be any object. But I'm open for alternative solutions.

>- what's the use for the guard attribute in Session?

It's explained in the docstring of loggedIn (somehow). It's because it makes
it easier to access the guard that created the session.

>- I think that ISessionManager should not have the loggedIn method.

Why? It allows to change how the application should react to a successfully
logged in session without going through guard code overriding a callback, of a very big method, defined as a closure. IMHO it's a lot cleaner to have it this way unless your reasons are sensible.

>- as I can see the code in SessionManager._tick causes the log
>  "Session %r expired" to be issued two times.

True.

>- what's the use for mind in a web authentication?

I have no idea. There might be a usage, why should you make it impossible
to have a usage of it?

>- why credInterface is a variable? It can be something different from
>  IResource?

iweb2.IResource? Anyway this is absolutely not a valid concern it's simply
a refactoring to allow changing that interface from application level code (again without the need to dig through the code).

>- Session.sessionLifetime is only used on the server side but never set
>  on the cookie.
>  Only persistent cookies have a not null expiration date.

True. In fact sessionLifetime is used for transient sessions that die when you restart your browser and from the serverside they last until sessionLifetime is expired. If the session is logged in the cookies are made persistent by default. If you use the persistent session manager you also have the chance to make the session persistent (by using rememberMe url argument in the login form).

>- As I can see in SessionWrappper.locateChild the code
>  request.session = session is executed twice (the first time in
>  getSession.

I think you might be right also on this. Although it's not that big of a problem it might be worth fixing it. Just by making getSession automatically set the session on the request (and not only with brand new sessions) and removing all the other places where this assignment is done. And then rerunning tests.

Also keep in mind that the most developed version of that code lies in:
http://hg.stiq.it/index.cgi/stiq?f=b9671c61c96b;file=stiq/guard.py;style=gitweb
and
http://hg.stiq.it/index.cgi/stiq?f=eef00c91fd79;file=stiq/session.py;style=gitweb