Superfluous HTTP header validation

[guidovranken]

This isn't a bug but an architectural issue. Various HTTP headers supplied by the client are verified multiple times at various places in the code:

base_taxii_handlers.py -> class MessageHandler -> validate_headers():

• Checks existence of HTTP_X_TAXII_SERVICES, CONTENT_TYPE, HTTP_X_TAXII_CONTENT_TYPE, HTTP_X_TAXII_PROTOCOL
• Checks validity of HTTP_X_TAXII_SERVICES, CONTENT_TYPE, HTTP_X_TAXII_CONTENT_TYPE, HTTP_X_TAXII_PROTOCOL, HTTP_ACCEPT, HTTP_X_TAXII_ACCEPT

middleware.py -> class StatusMessageExceptionMiddleware -> process_exception():

• Checks existence/validity of HTTP_ACCEPT, HTTP_X_TAXII_ACCEPT (but checking this latter header is necessary to generate an appropriate response)

views.py

• Checks if request.method == POST
• Checks/validates HTTP_X_TAXII_CONTENT_TYPE

I think it's probably better to reorganize this so that an invalid request is intercept at the view layer, triggers a StatusMessageException, after which the StatusMessageExceptionMiddleware class deals with determining and constructing an appropriate response. Move the checks in base_taxii_handlers.py to views.py service_router(), since that is the function through which all relevant incoming traffic flows (as far as I know).

Guido

[MarkDavidson]

@guidovranken, I'll explain my current thinking on how things got to the way they are.

• base_taxii_handlers.py and taxii_handlers.py have been moved into the message_handlers module, but your point still stands. The headers are validated here because each Message Handler might have a different supported set of headers, and that set of supported headers will not be known until the Message Handler is loaded.
• middleware.py has a slightly different role. I want developers who extend django-taxii-services to be able to detect an error condition and use the raise StatusMessageException(...) idiom, and have django-taxii-services just do the "right thing".
• views.py checks the most basic of aspects about the request

Thinking about your suggestion a bit, I agree with your main point that the code architecture can probably be improved, possibly by leveraging django.request's session property. I could see a workflow like:

1. Request is received. Relevant request headers are parsed and stored in request.session. At this point, the only checking is to make sure that the headers are supported by django-taxii-services.
2. Once the Message Handler is loaded, the parsed and stored headers are checked again to make sure that the Message Handler supports the headers as specified in the request
3. If a StatusMessageException is raised, the parsed and stored headers can be used to perform the logic in process_exception()
4. Remember to invalidate the session at the end of the request/response

I'm trying to get a release out today, and I won't be able to fix this issue in time, but I'll try to get it for the next one.

[jasenj1]

I looked through this and don't think it is a problem.

In views.py service_router() the code is configuring the parser and handler class to be used; it uses the 'x-taxii-content-type' header to determine the parser & XML validator.

In validate_headers() the code is ensuring that the incoming headers and message are well formed, understandable, and that the chosen handler object really can handle the message. Again, 'x-taxii-content-type' is used, but it is used for different reasons (it is confirming that the chosen handler can handle the message).

In process_exception() the code is determining what sort of error response to send back to the client. So it checks a couple of the incoming headers again to make that decision.

So while it looks like there's a bunch of redundancy, there really isn't. A case could be made that there is redundancy in the 'x-taxii-content-type' checking, since service_router() ensures it is good, and validate_headers() does again. But that makes the assumption that validate_headers() will only ever be called after service_router(), keeping the redundancy keeps the functions decoupled and they both do what they need to do for completeness.

I touched up the comments in my local version to make it clearer what is going on. We'll decide whether that is worth pushing up to the repo.