Skyspark needs post requests instead of get requests for his_read

[christiantremblay]

Skyspark needs post requests instead of get requests for his_read

[ChristianTremblay]

@sjlongland and I worked really hard and we finally found something that works the way we wanted.

Develop branch have just been updated and all the tests I have made are successful with nhaystack and Skyspark.

For now, only his_read is treated as POST. Next step will be to convert write to POST... but for now, let's celebrate his_read working on skyspark.

@clarsen thanks for your contribution

This will be merge to master ASAP so PyPI version works for Skyspark. So please give any feedback.

Thanks

[sjlongland]

On 21/8/20 3:04 am, Christian Tremblay wrote: From what I remember, all implementations were using the same base (get requests). Question will be do we need to rewrite everything or create something else exclusive for Skyspark...

I think we can safely do POST requests for all… so maybe an option is to replace the implementation of `_get_grid` with code that does a POST with a single-row grid containing what was once query arguments. Been a long time since I looked at `pyhaystack` code, and work seems to be moving away from Python as a language in favour of JavaScript.

[ChristianTremblay]

Basic Idea here.
I just override _on_his_read for skyspark session.
Why breaking every other implementation for now ? :)

def _on_his_read(self, point, rng, callback, **kwargs):
        """
        Skyspark will not accept GET request for his_read by default
        [ref : https://project-haystack.org/forum/topic/787#c6]
            The default behavior of SkySpark is now to disallow GET requests 
            non-idempotent operations. So its still allowed on certain operations 
            such as about, formats, read. However as Chris said it can be toggled 
            back on using Settings|API for backward compatibility.

            However as a recommendation I think we should always be using POST as 
            a safer alternative. Using GET for ops with side-effects is against 
            the HTTP spec. Plus it is an attack vector if cookies are involved. 
            And it provides a more precise way to pass the request payload.

            Its not really from a theoretical perspective. But in SkySpark 
            we allow customers to generate histories using their own custom 
            functions. So from a security perspective we took the safest route 
            and consider it to potentially have side effects.
            If your code is all using GET, then just have the customer set 
            Settings|API allowGetWithSideEffects flag to false and it should all work.
        """
        if isinstance(rng, slice):
            str_rng = ",".join([hszinc.dump_scalar(p) for p in (rng.start, rng.stop)])
        elif not isinstance(rng, string_types):
            str_rng = hszinc.dump_scalar(rng)
        else:
            # Better be valid!
            str_rng = rng

        col_list = [('id',[]),('range',[])]
        his_grid = hszinc.grid.Grid(columns=col_list)
        his_grid.insert(0,{"id": self._obj_to_ref(point), "range": str_rng})
        print(his_grid)
        return self._post_grid(
            "hisRead",
            his_grid,
            callback,
            **kwargs
        )

I think I got it right (not sure my Grid is number 1 but it seem to work) ... but server throws :
HTTPError: 400 Client Error: SkyArc-Attest-Key required for url: https://demo.skyfoundry.com/api/demo/hisRead
This confirms what @clarsen was seing in issue #84

Reading through forums and doc, i see this

If you are not using a browser, then don't use cookies! Use a bearer token with authorization header

Then my knowledge is not strong enough to overpass this. I don't get it.
@sjlongland if you can bring some light it would be awesome.

I'm also poking people that took part of issue #84
@clarsen you were near something in april, maybe you can help
@gmanohar25
@San42
@justindarcy
@dawcurious

[clarsen]

The patch I submitted has been running successfully for the last months.

…

On Sat, Aug 22, 2020 at 8:18 PM Christian Tremblay ***@***.***> wrote: Basic Idea here. I just override _on_his_read for skyspark session. Why breaking every other implementation for now ? :) def _on_his_read(self, point, rng, callback, **kwargs): """ Skyspark will not accept GET request for his_read by default [ref : https://project-haystack.org/forum/topic/787#c6] The default behavior of SkySpark is now to disallow GET requests non-idempotent operations. So its still allowed on certain operations such as about, formats, read. However as Chris said it can be toggled back on using Settings|API for backward compatibility. However as a recommendation I think we should always be using POST as a safer alternative. Using GET for ops with side-effects is against the HTTP spec. Plus it is an attack vector if cookies are involved. And it provides a more precise way to pass the request payload. Its not really from a theoretical perspective. But in SkySpark we allow customers to generate histories using their own custom functions. So from a security perspective we took the safest route and consider it to potentially have side effects. If your code is all using GET, then just have the customer set Settings|API allowGetWithSideEffects flag to false and it should all work. """ if isinstance(rng, slice): str_rng = ",".join([hszinc.dump_scalar(p) for p in (rng.start, rng.stop)]) elif not isinstance(rng, string_types): str_rng = hszinc.dump_scalar(rng) else: # Better be valid! str_rng = rng col_list = [('id',[]),('range',[])] his_grid = hszinc.grid.Grid(columns=col_list) his_grid.insert(0,{"id": self._obj_to_ref(point), "range": str_rng}) print(his_grid) return self._post_grid( "hisRead", his_grid, callback, **kwargs ) I think I got it right (not sure my Grid is number 1 but it seem to work) ... but server throws : HTTPError: 400 Client Error: SkyArc-Attest-Key required for url: https://demo.skyfoundry.com/api/demo/hisRead This confirms what @clarsen <https://github.com/clarsen> was seing in issue #84 <#84> Reading through forums and doc, i see this If you are not using a browser, then don't use cookies! Use a bearer token with authorization header Then my knowledge is not strong enough to overpass this. I don't get it. @sjlongland <https://github.com/sjlongland> if you can bring some light it would be awesome. I'm also poking people that took part of issue #84 <#84> @clarsen <https://github.com/clarsen> you were near something in april, maybe you can help @gmanohar25 <https://github.com/gmanohar25> @San42 <https://github.com/San42> @justindarcy <https://github.com/justindarcy> @dawcurious <https://github.com/dawcurious> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#91 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALZFP2ZCPP5GLZUY5DO6TSCCDB7ANCNFSM4QGL7UUQ> .

[sjlongland]

Okay, so the cookie stuff… that could be tricky as I tried very hard to round-trip cookies set by the server, and now they want us to do the opposite?

[ChristianTremblay]

@clarsen Submitted where ?
I missed something ?

[sjlongland]

https://github.com/ChristianTremblay/pyhaystack/blob/master/pyhaystack/client/http/base.py#L160-L163

Okay, so we need to set exclude_cookies=True in requests made to SkySpark.

[sjlongland]

Okay, so python-requests is too clever for its own good I think… it was ignoring me when told to not round-trip cookies.

https://github.com/sjlongland/pyhaystack/tree/skyspark-his-read-fix

I actually have converted all GET requests to POSTs, things seem to work. I don't see it being a problem for the other implementations either. We should give that a shot and see what happens.

[clarsen]

@clarsen Submitted where ?
I missed something ?
Sorry i only shared 1/2 of the code involved. #93 has the modifications to strip out cookies.

[sjlongland]

@ChristianTremblay hisWrite should be a POST request already… as it's often large blocks of data being written, more than what would fit in a GET request. :-)

[ChristianTremblay]

I was thinking about point_write

[ChristianTremblay]

CLosing this as this is working in the develop branch. New release soon