Authorization "bug" in 2.3.5?

[moschlar]

Hi everyone, not sure if it's really a bug or a mistake on my side, but here it is:

While upgrading through TurboGears versions (and sometimes merging stuff from fresh quickstarts) I found that my authorization doesn't work properly anymore in 2.3.5. I'm still using the old authentication layer and in my predicates (https://github.com/moschlar/SAUCE/blob/feature/TG2%2B/sauce/lib/authz.py#L54), I use request.user, which is just request.identity.get('user') and check whether it is equal to or in a relationship attribute on a model object (e.g. request.user in event.teachers). That worked so far, but with 2.3.5, the two objects don't compare equal anymore (both are there and their repr is the same).

Could it be that the two objects are fetched from two separate SQLAlchemy sessions now?

[amol-]

I see that you do not use tgauthmetada in app_cfg so the request.identity[user] is fetched by repoze.who.sqlalchemy.

in tg2.3.5 there are no changes related to user itself. maybe sqlalchemy upgrade? I suggest you follow the upgrade guidelines to turn back on the old middlewares and check that it is not something related to repoze.wjo.sqla together with the new sqla app wrapper

[moschlar]

Hm, well yes, I did read the upgrade guide and was trying to stick with the old authentication layer for now, so I didn't change anything related to auth in app_cfg and these are the packages installed:

repoze.what==1.0.9
repoze.what-pylons==1.0
repoze.what-quickstart==1.0.9
repoze.what.plugins.sql==1.0.1
repoze.who==1.0.19
repoze.who-friendlyform==1.0.8
repoze.who-testutil==1.0.1
repoze.who.plugins.sa==1.0.1

What do you mean by turning on the old middlewares?

[moschlar]

Btw, I kinda fixed it through this workaround:

diff --git a/sauce/lib/base.py b/sauce/lib/base.py
index 8b8afa7..051cb6e 100644
--- a/sauce/lib/base.py
+++ b/sauce/lib/base.py
@@ -77,7 +77,7 @@ class BaseController(TGController):
         request.identity = c.identity = environ.get('repoze.who.identity')

         try:
-            request.user = request.identity.get('user')
+            request.user = model.DBSession.merge(request.identity.get('user'))
         except:
             request.user = None
         finally:

https://github.com/moschlar/SAUCE/commit/91b5b9b401b2e2f880dd71d3576cd339e6029294#diff-e12debc3e7ced5534117e1927c17ccdcR80

[amol-]

Sorry, have been a bit confused in my answer :)

What do you mean by turning on the old middlewares?

In TG 2.3.5 most WSGI Middleware (like Transaction Manager) got replace with TurboGears Application Wrappers. This mixed with the old Repoze.who + repoze.who.plugins.sa might lead to unexpected behaviours.

You might want to try to reenable the old behaviour: http://turbogears.readthedocs.org/en/latest/cookbook/upgrading.html#transaction-manager-is-now-an-application-wrapper if that fixes the issue is probably some odd behaviour enabled by the order the metadata retrieval and the transaction happen.

[amol-]

Found the reason for this :)

As suspected the cause is that the transaction manager starts the transaction after the User has been retrieved by repoze.who. Whenever the transaction manager starts a new transaction it expunges all the objects from the session, so your user gets expunged.

This can be solved by changing inside model/__init__.py:

maker = sessionmaker(autoflush=True, autocommit=False,
                     extension=ZopeTransactionExtension())

to:

maker = sessionmaker(autoflush=True, autocommit=False,
                     extension=ZopeTransactionExtension(keep_session=True))

keep_session=False on TurboGears actually doesn't make much sense as TurboGears itself expunges the objects at the end of the request. It makes sense to me that keep_session=True should be the default in newly quickstarted projects (don't know why currently it isnt't).

Can you try adding the option and let me know if everything works as expected without any side-effect?

[moschlar]

Well yes, that seems to be fixing it! ;)

My test suite is currently failing elsewhere, but I highly doubt that it is linked to this issue. Will maybe have time to fix that tomorrow.

[amol-]

With the occasion of this problem I decided to take the chance to tackle a few problems with the user identity retrieval due to how repoze.who works.

Currently the identity metadata is retrieved by a repoze.who metadata provider, while this was working, has several limits due to being executed outside TurboGears itself. Request is not available, cache is not available, transaction is not available and so on. You are actually working in plain WSGI as that is how repoze.who works.

So I introduced the IdentityApplicationWrapper which is now in charge of metadata retrieval in place of repoze.who. It doesn't actually require any change in people code if they were already using sa_auth.authmetadata but the metadata retrieval is now performed inside TurboGears request flow instead of outside.

This has some major benefits, the first is that your issue is automatically solved as the metadata is now retrieved when the transaction is available. The second is that being better integrated with TG itself it is now possible to leverage all the TG features, for example implementing cached authentication got much easier! (From http://turbogears.readthedocs.org/en/latest/cookbook/advanced_caching.html#caching-authentication to just http://turbogears.readthedocs.org/en/development/cookbook/advanced_caching.html#caching-authentication )

And if people face compatibility issues with their application is actually pretty straightforward to go back to old repoze.who metadataprovider: http://turbogears.readthedocs.org/en/development/cookbook/upgrading.html#identity-provider

[moschlar]

Oh!

Now that I fixed the rest of my test suite, one failure persists: Sprox setters (http://turbogears.readthedocs.org/en/latest/cookbook/Crud/index.html?highlight=setters#customizing-easycrudrestcontroller) don't work anymore with keep_session=True. They are simply not setting the value they are supposed to set.