Skip to content
This repository has been archived by the owner on Nov 27, 2024. It is now read-only.

adding test which list clocks but show date/time #13

Merged
merged 1 commit into from
Feb 18, 2015

Conversation

davidamitchell
Copy link
Contributor

@pond, I am not 100% sure if I have set these tests up correctly, but in some testing of the calculation creation when hoodoo was attempting to lookup the additional_permissions (in middleware#augment_session_with_permissions_for_action):

additional_permissions = ( interface.additional_permissions() || {} )[ action ]

for the CalculationInterface which have been setup as such:

    additional_permissions_for( :create ) do | p |
      p.set_resource( :Currency, :list, Hoodoo::Services::Permissions::ALLOW )
    end```

the `action` it was looking for was `:create` not `:list`.  

It seems like the `action` is being gotten from the originating call not the inter-resource calls action.

@pond
Copy link
Contributor

pond commented Feb 18, 2015

Yes, very good catch - it was stupid of me to just use "show" for all the chained resources. A very thorough set of tests caught out by one daft oversight! It shouldn't be hard to fix the internal screwup in the local and remote inter-resource calls.

EDIT: SEE BELOW: I misinterpreted what you were saying. The behaviour was correct, but I'm bolstering the tests to call into different action names to make sure there are no bugs of the kind I thought you were actually describing! I'm also correcting the incorrect tests brought in by the merge.

pond added a commit that referenced this pull request Feb 18, 2015
…nstream_call

adding test which list clocks but show date/time
@pond pond merged commit a9fac3e into master Feb 18, 2015
@pond pond deleted the hotfix/different_action_for_downstream_call branch February 18, 2015 09:32
expect_any_instance_of(RSpecAddPermTestTimeImplementation).to receive( :verify ).with( anything(), :show ).and_return( Hoodoo::Services::Permissions::ALLOW )

get '/v1/rspec_add_perm_test_clocks', nil, { 'CONTENT_TYPE' => 'application/json; charset=utf-8' }
expect( last_response.status ).to eq( 200 )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I should've read this more closely before merging. You said the reverse of what I thought you were saying.

Your PR assigns a new action to the Clock resource which is 'list'. It then grants top-level session permissions permitting that. But the action 'list' tries to call 'show' in another resource. You did not grant permissions for Clock's "list" action to do this. Therefore, I would expect the test at line 404 to fail, with the response at line 411 being 403 (correctly), while the test requires 200 (incorrect).

If service A calls service B, then service A must add permissions for THE ACTIONS IN A which call other things. You're saying "hey, someone called me. But for action, I also need to call ".

So without an additional_permissions_for( :list ) do ... inside ...ClockInterface..., Clock should be unable to do #list. Which is exactly what we see.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants