Zend Framework

Zend_Service_Twitter authentication mixup

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.7.8
  • Fix Version/s: 1.9.3
  • Component/s: Zend_Service_Twitter
  • Labels:
    None

Description

I have 2 Zend_Service_Twitter objects, both with their own account credentials. After sending an update with one object, an update on the next object will use the wrong useraccount.

E.g.

$a = new Zend_Service_Twitter("userA", "passwordA");
$b = new Zend_Service_Twitter("userB", "passwordB");

$a->status->update("This is a test");
$b->status->update("This is a test too");

// result, both messages get posted in userA's timeline.

Activity

Hide
Ivo Jansch added a comment -

I investigated a bit further, and the problem probably is is that Zend_Service_Abstract, which Zend_Service_Twitter is indirectly derived from, uses a protected static $_httpClient member to handle rest calls. This is probably a design flaw, since this could lead to a number of problems where various different objects could inadvertently get unexpected results, since they share a single common http client object.

Show
Ivo Jansch added a comment - I investigated a bit further, and the problem probably is is that Zend_Service_Abstract, which Zend_Service_Twitter is indirectly derived from, uses a protected static $_httpClient member to handle rest calls. This is probably a design flaw, since this could lead to a number of problems where various different objects could inadvertently get unexpected results, since they share a single common http client object.
Hide
Jon Whitcraft added a comment -

Actually Zend_Service_Twitter does not extend Zend_Service_Abstract. It extends Zend_Rest_Client as it's using their REST API. This might be a design flaw in the Rest Client but with no maintianer of the Rest Service I'm not sure how/if this will ever get fixed. I think the twitter client should get a rewrite and us the Zend_Service_Abstract method instead but that would push it off to 2.0.. I'll send this off to Zend to see what they say.

Show
Jon Whitcraft added a comment - Actually Zend_Service_Twitter does not extend Zend_Service_Abstract. It extends Zend_Rest_Client as it's using their REST API. This might be a design flaw in the Rest Client but with no maintianer of the Rest Service I'm not sure how/if this will ever get fixed. I think the twitter client should get a rewrite and us the Zend_Service_Abstract method instead but that would push it off to 2.0.. I'll send this off to Zend to see what they say.
Hide
Ivo Jansch added a comment -

Actually it is; which is why I said 'indirectly'.

Zend_Service_Twitter extends Zend_Rest_Client, but Zend_Rest_Client in turn inherits from Zend_Service_Abstract. (I think composition would be a better design for the service api than inheritance by the way).

So the problem is still with Zend_Service_Abstract and its static $_httpClient member. Does Zend_Service_Abstract have a maintainer?

Show
Ivo Jansch added a comment - Actually it is; which is why I said 'indirectly'. Zend_Service_Twitter extends Zend_Rest_Client, but Zend_Rest_Client in turn inherits from Zend_Service_Abstract. (I think composition would be a better design for the service api than inheritance by the way). So the problem is still with Zend_Service_Abstract and its static $_httpClient member. Does Zend_Service_Abstract have a maintainer?
Hide
Jon Whitcraft added a comment -

Ahh see if i would have followed the tree all the way i found i found that. My mistake. I have a feeling that it doesn't reset the variables like it should. I ran into this problem with the Ec2 Component where if you did more than one request it would fail as the $_httpClient was tainted with the previous requests items.

I'll look into this further.

Show
Jon Whitcraft added a comment - Ahh see if i would have followed the tree all the way i found i found that. My mistake. I have a feeling that it doesn't reset the variables like it should. I ran into this problem with the Ec2 Component where if you did more than one request it would fail as the $_httpClient was tainted with the previous requests items. I'll look into this further.
Hide
Ivo Jansch added a comment -

Yes, that's indeed another symptom of the same issue. I think the cleanest solution is to replace the static $_httpClient with one that is not shared between instances, in other words a protected member variable.

Show
Ivo Jansch added a comment - Yes, that's indeed another symptom of the same issue. I think the cleanest solution is to replace the static $_httpClient with one that is not shared between instances, in other words a protected member variable.
Hide
Ben Scholzen added a comment -

I just ran into the same problem and after some looking around, I found a workaround which worked for me. Before every statusUpdate() call, I did:

$twitter->setPassword($twitter->getPassword());

That did reset the _authInitialised, so it updated the HTTP client when doiung the next statusUpdate().

Show
Ben Scholzen added a comment - I just ran into the same problem and after some looking around, I found a workaround which worked for me. Before every statusUpdate() call, I did:
$twitter->setPassword($twitter->getPassword());
That did reset the _authInitialised, so it updated the HTTP client when doiung the next statusUpdate().
Hide
Ivo Jansch added a comment - - edited

Thanks Ben. I can confirm that this indeed is a working workaround.

The disadvantage is that this causes a re-authentication upon every update, even if they are from the same account. While this is acceptable in my situation, this is something to take into account if you do high volumes of updates.

Show
Ivo Jansch added a comment - - edited Thanks Ben. I can confirm that this indeed is a working workaround. The disadvantage is that this causes a re-authentication upon every update, even if they are from the same account. While this is acceptable in my situation, this is something to take into account if you do high volumes of updates.
Hide
Pádraic Brady added a comment -

This patch implements some changes in an effort to ensure each Zend_Service_Twitter instance uses a distinct local HTTP client object to prevent the conflcts noted above. The summary of changes:

1. Implement a new Zend_Service_Twitter::getLocalHttpClient() method used in unit tests to test the response.
2. Clones the statically set or retrieved Zend_Http_Client as a local object protected property for the life of the object.
3. Adds four short protected methods partially duplicating REST preparation from Zend_Rest_Client (required since the originals are either final or private). The duplication is minimal.

Note: This allows the setting of a custom client statically, so long as you remember they are cloned once a new object is created (i.e. you cannot change the clients once the object is instantiated which does differ from previously)

Show
Pádraic Brady added a comment - This patch implements some changes in an effort to ensure each Zend_Service_Twitter instance uses a distinct local HTTP client object to prevent the conflcts noted above. The summary of changes: 1. Implement a new Zend_Service_Twitter::getLocalHttpClient() method used in unit tests to test the response. 2. Clones the statically set or retrieved Zend_Http_Client as a local object protected property for the life of the object. 3. Adds four short protected methods partially duplicating REST preparation from Zend_Rest_Client (required since the originals are either final or private). The duplication is minimal. Note: This allows the setting of a custom client statically, so long as you remember they are cloned once a new object is created (i.e. you cannot change the clients once the object is instantiated which does differ from previously)
Hide
Jon Whitcraft added a comment -

Padraic,

It looks good to me. Let me konw when this is in so I can update the block addition for this and get it committed.

Show
Jon Whitcraft added a comment - Padraic, It looks good to me. Let me konw when this is in so I can update the block addition for this and get it committed.
Hide
Ivo Jansch added a comment -

Thanks for the patch, as soon as I have a chance I'll test it in a real world scenario.

Show
Ivo Jansch added a comment - Thanks for the patch, as soon as I have a chance I'll test it in a real world scenario.
Hide
Pádraic Brady added a comment -

Hi Jon,

I can commit the patch this afternoon if it looks fine.

Show
Pádraic Brady added a comment - Hi Jon, I can commit the patch this afternoon if it looks fine.
Hide
Pádraic Brady added a comment -

Issue fixed by applying patch (with some changes) in r18243

Show
Pádraic Brady added a comment - Issue fixed by applying patch (with some changes) in r18243

People

Vote (1)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: