search

skyscreamer / nevado

A JMS driver for Amazon SQS.
http://nevado.skyscreamer.org/
Apache License 2.0
51 stars 48 forks source link

NevadoConnection assumes admin user access #90

Closed madganti closed 9 years ago

madganti commented 10 years ago

The method is unnecessary because if access to a resource is missing it is still caught downstream when any interaction is attempted with the resource (queue or topic) and AmazonServiceException is thrown with 403 (access denied).

carterpage commented 10 years ago

test() is a public interface method. You can't just remove it. While it is caught downstream, the intention of having the test early is to catch connection problems as soon as possible.

Try changing its implementation to something different that doesn't encounter the same access problems as listQueues().

madganti commented 10 years ago

The only other way would be to test if the user has access to the resource using other basic operations like read/write to queue, but that means the destination would need to be passed into the NevadoConnection constructor. Is that okay?

carterpage commented 10 years ago

Hm, that sounds worse. Maybe we add a boolean flag that forces test to always return true? Someone's probably using the test() method right now, so we still shouldn't change its behavior by default.

On Fri, Jul 18, 2014 at 1:49 PM, madganti notifications@github.com wrote:

The only other way would be to test if the user has access to the resource using other basic operations like read/write to queue, but that means the destination would need to be passed into the NevadoConnection constructor. Is that okay?

— Reply to this email directly or view it on GitHub https://github.com/skyscreamer/nevado/pull/90#issuecomment-49460041.

madganti commented 10 years ago

Yeah, it's a hack, but will work for now. I'll make the change and send out a pull request. Thanks!

madganti commented 10 years ago

Oh, the return type of the method is currently void. it either throws an exception or doesn't. So I can keep the method as it is and just do nothing and return, make it a no-op basically. Is that what you meant?

carterpage commented 10 years ago

Right, that's what I meant.

On Fri, Jul 18, 2014 at 3:51 PM, madganti notifications@github.com wrote:

Oh, the return type of the method is currently void. it either throws an exception or doesn't. So I can keep the method as it is and just do nothing and return, make it a no-op basically. Is that what you meant?

— Reply to this email directly or view it on GitHub https://github.com/skyscreamer/nevado/pull/90#issuecomment-49472584.

madganti commented 10 years ago

I've created/updated my pull request with the new change. Please review

madganti commented 9 years ago

Hi Carter, Just checking back on the pull request...

carterpage commented 9 years ago

Added a comment.

On Tue, Dec 2, 2014 at 4:50 PM, madganti notifications@github.com wrote:

Hi Carter, Just checking back on the pull request...

— Reply to this email directly or view it on GitHub https://github.com/skyscreamer/nevado/pull/90#issuecomment-65334180.

madganti commented 9 years ago

Please review the latest set of commits. I have created a new property/flag that is false by default, but can be set to true by the use in their spring config. Let me know if this is what you intended. Thanks!

carterpage commented 9 years ago

Comments added. Work needed.

madganti commented 9 years ago

I have removed the property from the factory class and the constructors. It is now only in the specific connector class and can be overriden by spring injection (setter). Please review. Thanks!

carterpage commented 9 years ago

Very simple. I'm okay with this. Can you confirm this does what you need?

On Fri, Dec 5, 2014 at 4:27 PM, madganti notifications@github.com wrote:

I have removed the property from the factory class and the constructors. It is now only in the specific connector class and can be overriden by spring injection (setter). Please review. Thanks!

— Reply to this email directly or view it on GitHub https://github.com/skyscreamer/nevado/pull/90#issuecomment-65857205.

madganti commented 9 years ago

Actually it can just be a property of the AmazonAwsSQSConnectorFactory with a setter method and then the same spring config (as in documentation) with a property setter would take care of it. Can you confirm if this is okay? - Moving the property and its setter to AmazonAwsSQSConnectorFactory from AmazonAwsSQSConnector ? Thanks!

madganti commented 9 years ago

Created another pull request. I tested it and it works without being invasive.

carterpage commented 9 years ago

One little style change, and I'll merge.

madganti commented 9 years ago

Done. Thanks !

carterpage commented 9 years ago

Thanks for your patience. I'll try to get the release out this week.