Baidu: signature authentication support #298

jinzhc · 2018-05-16T09:50:26Z

changes for baidu geocoder & pytest
For signature authentication, see
http://lbsyun.baidu.com/index.php?title=lbscloud/api/appendix

changes for baidu geocoder & pytest For signature authentication, see http://lbsyun.baidu.com/index.php?title=lbscloud/api/appendix

jinzhc · 2018-05-16T11:25:50Z

I have done the pytest with my api_keys with/without security key

KostyaEsmukov

Thanks, I've left a couple of notes. Overall it looks good!

One thing I'm wondering about is the status code changes: was the source code incorrect before? I mean, it used to be strings, but now they're integers. Was it a bug, or a change in the API?

KostyaEsmukov · 2018-05-17T07:39:33Z

geopy/geocoders/baidu.py

@@ -78,6 +84,8 @@ def __init__(
        )
        self.api_key = api_key
        self.api = '%s://api.map.baidu.com/geocoder/v2/' % self.scheme
+        self.api_path = self.api.split('baidu.com')[1]


Maybe that would be better?

self.api_path = '/geocoder/v2/' self.api = '%s://api.map.baidu.com%s' % (self.scheme, self.api_path)

KostyaEsmukov · 2018-05-17T07:42:44Z

geopy/geocoders/baidu.py

            raise GeocoderQueryError(
                'IP/SN/SCODE/REFERER Illegal:'
            )
-        elif status == '2xx':
+        elif status in range(200, 300):


In Python2 range is a list, which seems inefficient.
What about this?

elif 200 <= status <= 300:

KostyaEsmukov · 2018-05-17T07:48:48Z

test/geocoders/baidu.py

@@ -27,6 +27,7 @@ class BaiduTestCase(GeocoderTestBase):
    def setUpClass(cls):
        cls.geocoder = Baidu(
            api_key=env['BAIDU_KEY'],
+            security_key=env['BAIDU_SEC_KEY'],


I suggest to create a separate test class instead, which does use the SK, so we could test both setups. I think that having two simple calls (1 geocode and 1 reverse) for this new class would be enough to be sure that SK works.

Also, if I understood correctly, that should be a different BAIDU_KEY (for which the SK auth is enabled), shouldn't it? If so, it should have a different ENV name.

Yes, you're right. Are BAIDU_KEY_REQUIRES_SK and BAIDU_SEC_KEY ok?

Yup, that's fine.

KostyaEsmukov · 2018-05-17T07:55:46Z

geopy/geocoders/baidu.py

+        """
+
+        # Since quoted_params is escaped by urlencode, don't apply quote twice
+        raw = self.api_path + '?' + quoted_params + self.security_key


According to the docs at http://lbsyun.baidu.com/index.php?title=lbscloud/api/appendix , the quote(params, safe="/:=&?#+!$,;'@()*[]") code is used for getting a raw string, while there we have urlencode(params). Wouldn't that cause different hashes for peculiar requests? I think the relevant test cases should be added. For example, a one with a comma: locator.geocode("beijing, china").

The quoted_params in self._url_with_signature was given by urlencode. urlencode has already called quote_plus on parameters internall 8000 y. Except parameters, quote never escape the characters in site/path/key parts, because they are not special characters.

Does the list of safe characters match the one used in urlencode internally? For example, I see that comma is in that list, however urlencode uses percent encoding on commas, IIRC.

A test wouldn't harm in either way.

Yes, this patch code doesn't match the given sample code in docs. But it works.

It sure works, but for some cases it might not work. Could you please add the test with a comma, and fix the code, if that fails?

It's better to stick to docs. I'm going to change urlencode(params) to urlencode(params, safe="!*'();:@&=+$,/?#[]", quote_via=quote). Will you accept this change?

Please add the test first :)

But yeah, that would be better than crafting the query string manually. Even better if all of this is not required and a simple urlencode works.

jinzhc · 2018-05-21T09:03:33Z

Comments for status in http response page.

See 状态码定义 (status definition in English) section (1st section) in http://lbsyun.baidu.com/index.php?title=lbscloud/api/appendix. status has more values than original implementation. The failed signature will have 211 status value, whose inferred type is int. This is why I change status checking from str to int version.

KostyaEsmukov · 2018-05-22T17:37:36Z

I'm not sure if I understood your reply regarding status correctly. My question was specifically about the types, not about checking for more specific error codes. If status in the API response is an int, then it would be never equal to a string (In Python 1 == '1' yields False). And if it's a string in the response, then, again, '1' == 1 would still yield False.

I've just checked, API returns ints. It means that the current implementation (I mean, without this patch) doesn't seem to respect the status at all. It's not covered by tests too. That's pretty sad :(

This also helps improving testcase coverage.

jinzhc · 2018-05-23T08:48:33Z

May the new Invalid AK test case help. It's hard to cover all failure status tests due to the poor Baidu docs.

jinzhc · 2018-05-23T11:58:50Z

For unittest.assertRaisesRegex's compatibility, willpytest.assertRaises be a better option?

KostyaEsmukov · 2018-05-24T12:41:13Z

test/geocoders/baidu.py

+        else:
+            assert_raise_msg = self.assertRaisesRegexp
+
+        with assert_raise_msg(GeocoderAuthenticationFailure, 'Invalid AK'):


Another way to do the same without tinkering around compatibility:

with self.assertRaises(GeocoderAuthenticationFailure) as cm: ... self.assertEqual(str(cm.exception), 'Invalid AK')

To me it looks better than assertRaisesRegex. What do you think?

Also I'm wondering: does the self.reverse_run command ever run? I feel that self.geocode_run causes an exception being thrown, and the self.reverse_run call is never tested. To fix that, each call should be wrapped in their own assertRaises/assertRaisesRegex context.

Thanks, you have a better solution.
self.reverse_run cannot be reached, and I will remove this line afterwards.

KostyaEsmukov · 2018-05-24T12:45:20Z

geopy/geocoders/baidu.py

+        """
+
+        # Since quoted_params is escaped by urlencode, don't apply quote twice
+        raw = self.api_path + '?' + quoted_params + self.security_key


It sure works, but for some cases it might not work. Could you please add the test with a comma, and fix the code, if that fails?

KostyaEsmukov

At the moment this looks good to me, except the urlencode vs quote_plus problem, discussed in the thread #298 (comment)

A test with a comma in the query is absolutely required, because this is an edge case, dividing the two possible implementations. So in order to avoid any bugs for now and in the future, such test must be added.

jinzhc · 2018-05-25T17:00:05Z

Do you think the comma test is still a must?

KostyaEsmukov · 2018-06-05T14:38:02Z

Sorry for the delay.

Yes, a comma test is definitely a must. Do you need any help or a guidance with it? Please don't hesitate to ask. Basically all what's needed here is a simple forward geocoding request, which contains a comma in its query. If signature is wrong, an exception would be thrown and the test would fail, proving that there's a bug in the signature implementation.

The cf9a30e...1009fe2 change is somewhat disturbing to me. I would better to not construct the query string manually by escaping individual parts and joining them with & and =, but make urlencode to do this.

It's important to distinguish between input for the signature algorithm and resulting URL. The former requires the aforementioned quote with a list of safe characters, while the latter should use urlencode, which is safer. Unless the web server uses urlencoded data (before decoding) for verifying the signature on the server side instead of decoded and encoded with the quote F438... In this case a simple urlencode, just like it was before this change, should work correctly (and this is why I wanted to have a test before an implementation).

jinzhc · 2018-06-08T07:26:27Z

The cf9a30e...1009fe2 change is somewhat disturbing to me. I would better to not construct the query string manually by escaping individual parts and joining them with & and =, but make urlencode to do this.

I append 1009fe2 because cf9a30e failed in py2.7 & py 3.4. No safe parameter in py2.7; New quote_via parameter since 3.5.

KostyaEsmukov · 2018-06-08T13:59:29Z

Would you mind if I finished this by myself? :)

jinzhc · 2018-06-10T11:42:17Z

Never mind. It's my pleasure.

jinzhc · 2018-06-11T10:06:08Z

@KostyaEsmukov Is new test appropriate?

KostyaEsmukov · 2018-06-13T18:05:28Z

I'm wondering if at one point Baidu changes their search algorithm such as the !*'();:@&=+$,/?[] % part of the query would make nothing returned.

Actually the reason why I wanted to take this over is that there clearly is a confusion between composing a raw body for the signature and urlencoding the params for url. As I said in my previous comment #298 (comment) , urlencode should be used for the latter, instead of manual string construction. But for the raw body, apparently, the string must be constructed manually (unless, as said before, their backend uses urlencoded data for signature verification, where this safe list of characters is actually useless).

It's just easier for me to finish this by myself than trying to explain this in text. Sorry for that. You've done a great job, a little is left. Is that okay if I finish this by myself? 😉Of course you'd still be credited as a contributor and would get a green box. 😀

jinzhc · 2018-06-19T07:17:04Z

It's ok you take it over and go for final shot. :-)

KostyaEsmukov · 2018-06-26T14:31:02Z

Opened #306 which supersedes this PR.

So it looks like the safe list of characters is unnecessary at all! The SN is valid as long as the urlencoded string matches the one in the raw payload for the hash function.

Thanks again! I will merge #306 soon.

tony added 2 commits May 16, 2018 17:36

Baidu: signature authentication support

0119015

changes for baidu geocoder & pytest For signature authentication, see http://lbsyun.baidu.com/index.php?title=lbscloud/api/appendix

Add missing spaces for flake8 checking

c2821cb

KostyaEsmukov requested changes May 17, 2018

View reviewed changes

KostyaEsmukov added this to the 1.15 milestone May 17, 2018

Changes for more efficient impl.

01f9ae1

tony added 6 commits May 23, 2018 09:53

TestCase for BAIDU AK /w SK

0625f35

More accurate exception

3d5ce65

Add testcase for invalid ak

da615f0

This also helps improving testcase coverage.

Remove EOF blank line

786db38

Minor changes to invalid ak testcase

a6b2e97

Using [geocode|reverse]_run in invalid ak testcase

c6ddce7

tony added 2 commits May 23, 2018 19:14

Better invalid-ak testcase

cab7cfd

py2.7 compatible

d653c16

KostyaEsmukov reviewed May 24, 2018

View reviewed changes

tony added 3 commits May 25, 2018 23:59

remove compatibility code in testcase

eb4e5cb

explicit quote along with urlencode

cf9a30e

remove urlencode for compatibility

1009fe2

safe characters SK test

d10b42c

KostyaEsmukov mentioned this pull request Jun 26, 2018

Baidu sk support #306

Merged

KostyaEsmukov closed this Jun 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Baidu: signature authentication support #298

Baidu: signature authentication support #298

Baidu: signature authentication support #298

Baidu: signature authentication support #298

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment