Paging with authorization interceptor doesn't seem to validate rulelist on every page fetched #590

Open
eevaturkka opened this Issue Mar 13, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@eevaturkka

eevaturkka commented Mar 13, 2017

We have implemented authrozation interceptor approximately like in the Hapi examples:
public class PhrAuthorizationInterceptor extends AuthorizationInterceptor {

and then implemented the method
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {

where last rule of the rulelist is

        } else if (auth == null) {
                LOGGER.info("No token so denying all");
                builder.denyAll("No valid token found").build();
       }

It now seems that once you've made the first search call with applicable and valid token then next link could just be copied to a browser and the contents of that page are returned. Should we implement the rule enforcing and checking the token somehow differently so that each and every call goes through the rule list?

We store the information about the token in the userData:
Object auth = theRequestDetails.getUserData().get(PhrConstants.KEY_AUTH_TOKEN);
Is that stored with the paging and could be causing this?

Which operation the next page actually is?

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Apr 20, 2017

Owner

Hmm, so I tried to reproduce this and I had somewhat different results from you: Paging results are definitely handled incorrectly, but they are always denied for me because the "read" rule examines the outer bundle instead of the inner contents.

I'm going to check in a fix- we now have unit tests confirming that paging requests work with the security interceptor. Can you try the latest 2.5 snapshot build and see if you still see this issue?

Owner

jamesagnew commented Apr 20, 2017

Hmm, so I tried to reproduce this and I had somewhat different results from you: Paging results are definitely handled incorrectly, but they are always denied for me because the "read" rule examines the outer bundle instead of the inner contents.

I'm going to check in a fix- we now have unit tests confirming that paging requests work with the security interceptor. Can you try the latest 2.5 snapshot build and see if you still see this issue?

@eevaturkka

This comment has been minimized.

Show comment
Hide comment
@eevaturkka

eevaturkka Apr 21, 2017

We'll try this and if we cannot get it to work I'll try to to create an unit test to show what we are doing. Thanks!

We'll try this and if we cannot get it to work I'll try to to create an unit test to show what we are doing. Thanks!

@mattiuusitalo

This comment has been minimized.

Show comment
Hide comment
@mattiuusitalo

mattiuusitalo May 29, 2017

Ok, we had a go at 2.5-SNAPSHOT and the problem still occurs. Debugging, we can see that public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) is never called.

Ok, we had a go at 2.5-SNAPSHOT and the problem still occurs. Debugging, we can see that public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) is never called.

@eevaturkka

This comment has been minimized.

Show comment
Hide comment
@eevaturkka

eevaturkka May 29, 2017

The rulelist should be evaluated on each call because the access token might change between calls and its and its scopes need to be checked for every and each call.

The rulelist should be evaluated on each call because the access token might change between calls and its and its scopes need to be checked for every and each call.

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew May 29, 2017

Owner

Very weird. The rule list definitely is (or at least should be) checked for each call, there is no caching of rule evaluation between pages or anything like that.

Would you be able to try putting a breakpoint here: https://github.com/jamesagnew/hapi-fhir/blob/master/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/BaseResourceReturningMethodBinding.java#L263

Essentially for a paging request, that should loop through the interceptors including the authorization one. When I try this locally for page request, it goes into the interceptor and builds the rule list almost right away (I actually notice it builds the list once for each resource in the response bundle, which should definitely be optimized)

There's a unit test that verifies this functionality here FYI: https://github.com/jamesagnew/hapi-fhir/blob/master/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptorDstu2Test.java#L1170

Owner

jamesagnew commented May 29, 2017

Very weird. The rule list definitely is (or at least should be) checked for each call, there is no caching of rule evaluation between pages or anything like that.

Would you be able to try putting a breakpoint here: https://github.com/jamesagnew/hapi-fhir/blob/master/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/BaseResourceReturningMethodBinding.java#L263

Essentially for a paging request, that should loop through the interceptors including the authorization one. When I try this locally for page request, it goes into the interceptor and builds the rule list almost right away (I actually notice it builds the list once for each resource in the response bundle, which should definitely be optimized)

There's a unit test that verifies this functionality here FYI: https://github.com/jamesagnew/hapi-fhir/blob/master/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptorDstu2Test.java#L1170

@mattiuusitalo

This comment has been minimized.

Show comment
Hide comment
@mattiuusitalo

mattiuusitalo May 30, 2017

I followed your suggestion to put the debug breakpoint there. The looping starts fine but what happens is we have registered a ResponseHighlighterInterceptor which processes the request before the AuthorizationInterceptor. ResponseHighlighterInterceptor returns false and the processing ends there.

We have registered our AuthorizationInterceptor before other interceptors so it handles the incomingRequestPreHandled event before others. In this case incomingRequestPreHandled is not triggered at all. Instead Hapi goes straight to outgoingResponse event and calls the interceptors in reverse order of registration.

I confirmed things further by removing the ResponseHighlighterInterceptor. In that case AuthorizationInterceptor gets called. I suppose we could register the Interceptors in a different order, but we have to check if the business logic allows this. We have assumed Hapi calls the Interceptors every time as pictured here http://hapifhir.io/doc_rest_server_interceptor.html It would be much easier if that were the case here.

I followed your suggestion to put the debug breakpoint there. The looping starts fine but what happens is we have registered a ResponseHighlighterInterceptor which processes the request before the AuthorizationInterceptor. ResponseHighlighterInterceptor returns false and the processing ends there.

We have registered our AuthorizationInterceptor before other interceptors so it handles the incomingRequestPreHandled event before others. In this case incomingRequestPreHandled is not triggered at all. Instead Hapi goes straight to outgoingResponse event and calls the interceptors in reverse order of registration.

I confirmed things further by removing the ResponseHighlighterInterceptor. In that case AuthorizationInterceptor gets called. I suppose we could register the Interceptors in a different order, but we have to check if the business logic allows this. We have assumed Hapi calls the Interceptors every time as pictured here http://hapifhir.io/doc_rest_server_interceptor.html It would be much easier if that were the case here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment