[pre production/stable] Digital Signatures for APIs

[matecsaj]

Due to regulatory requirements applicable to our EU/UK sellers, for certain APIs, developers need to add digital signatures to the respective HTTP call. eBay's deadline is January 31st, 2023

https://developer.ebay.com/develop/guides/digital-signatures-for-apis

[gbm001]

I have begun working on this, since we need it...

[gbm001]

I have made some progress with this; added stuff to token.py and api.py to allow key pairs to be stored. The tricky bit is that the digital signature stuff needs to be done at the very last minute really (I think - that might not be completely true) which would imply patching it into the Swagger stuff. I am writing the encryption stuff now, and will then work out how to integrate it.

[gbm001]

68

[matecsaj]

Big change, tricky stuff; well done!

To explain. Initially, I used the custom Error class to encapsulate all possible errors from Swagger API calls. To minimize the number of exception handlers a library user must have, I extended the error class to cover all errors. Should a user want to distinguish been errors, the error number is unique and will never change. If you lack the interest or labour, I'm happy to tweak the errors you raised to adhere to the package convention.

Please add some unit tests, and then I will promptly merge all your code and release a new library version. 100% coverage isn't required; I'm a big believer in the Pareto principle.

[matecsaj]

Oh, also, please add what users need to know, to README.md.

[gbm001]

I have added three (simple) unit tests and updated the README.md as requested.

I have a nightmare getting the sandbox to work; originally I couldn't get the key management API to work on it, so did all my development on production. Now the key management API works, but user tokens don't work for me at all, so I had to do two of these unit tests in production and one in sandbox. Theoretically they should all work in sandbox though, so I've made that the default.

[gbm001]

I have (pending the small pull request where I broke something at the last minute) got this working with my main app now, so at least for me this seems to all be working in anger...

[matecsaj]

I merged all your changes and then did one of my own.

I enjoyed studying your code today; learning new ways to approach things is fun. While doing so, I tweaked things, and I hope you don’t mind.

Would you have time to look at a few oddities, or should I release the package as is?

1. In member test_003_create_signing_key, the second 'key =‘ line overwrites the first.
2. When run with a faulty key, the new unit tests have an unhandled exception. It would be lovely to have one more unit test that intentionally triggers a fault and verifies that the anticipated exception is thrown. An existing example of that is member test_try_except_else_api.
3. Code in a_p_i.py uses the protected members _ensure_key_pair, _has_valid_key, _create_key_pair and _load_key. I am unsure if you meant them to be public, so I didn’t remove the preceding underscores.

[gbm001]

It's your module :) I've generally tried to make the code look vaguely similar rather than enforcing my own style.

The following comments are probably poorly-considered and possibly entirely wrong, so shouldn't be taken with too much concern.

A few things: 1) Yes, the first line doesn't actually need to store the key since it should always fail anyway so could just be self._api.get_digital_signature_key(create_new=False) without the key = part. The line as a whole though is very much not redundant; it should raise an Error and this is tested with with self.assertRaises.

2) Do you mean if someone tries to run the unit tests with a faulty key, it should fail in setupClass or similar before the tests really run? I did think about this, but you'd basically be repeating the first unit test. I think raising an error is reasonable here, although possibly it could provide a helpful 'are you sure you have given valid key details?' or something. The second test verifies that a call works with the provided key and fails with a faulty key.

3) I think single underscore prefixes are also generally to be avoided in Python unless actually justified. I only used underscores on _ensure_key_pair, _has_valid_key etc. to fit with what I thought was the style. Single underscores don't do anything in Python anyway, so developers can and will ignore them... I would have thought that underscores were styled on a 'package' basis anyway i.e. anything with an underscore shouldn't be relied on not to change whereas things without underscores are intended to be used by users of the package. While implementing ebay_rest in our application I have immediately started calling things on the KeyPairToken anyway which involves many underscores.

4) I think handling exceptions in Python is usually a bad thing. The exception is when you are expecting that something might fail e.g. a network request or similar. But for stuff that should work, exception handling that doesn't actually handle anything (e.g. just wraps one error in another) just adds a layer of complexity when something is breaking. It's much easier to debug using something when you get the actual error not just a 'this thing failed somewhere' error and no detailed traceback. You also need to be extremely selective about what happens between 'try' and 'except'. For example, around line 804 in token.py:

        body = CreateSigningKeyRequest(signing_key_cipher='ED25519')
        try:
            key = api.developer_key_management_create_signing_key(body=body)
        except Error as e:
            raise Error(
                    number=96024,
                    reason='Failed to create key pair',
                    detail='Failed to create key pair: {}'.format(e)
                )
        else:
            self._save_key(key)

If something in api.developer_key_management_create_signing_key raises a meaningful 'Error', this gets overwritten with a new 'Error' message and code. You do get the error pasted into the 'detail' part, but you lose the traceback and the original exception (except its str representation) which may make debugging harder. On the other hand the bit of code raising the Error inside the swagger code is probably only doing something similar anyway...

Something that might assist: you can raise one exception 'from' another exception; that way I think you keep the traceback (you get two tracebacks, I think). https://docs.python.org/3/tutorial/errors.html#exception-chaining

Finally, it doesn't really matter but if you are raising an error in all the 'Except' clauses of try/except/else then the 'else' is redundant. For example, in the second test I had:

        try:
            result = self._api.sell_finances_get_transaction_summary(filter="transactionStatus:{PAYOUT}")
        except Error as error:
            self.fail(f'Error {error.number} is {error.reason}  {error.detail}.\n')
        else:
            self.assertTrue('credit_count' in result)

where the else clause is redundant, but I probably left it in as it's effectively the conclusion of the first part of the test and it's then obvious that it is for the 'succeeds' case? Probably I should have dropped it. But in your modification, the whole of the rest of the test is then moved into the else clause which just increases the indentation (and avoiding excessive indentation/nesting is generally a good thing).

5) I'm not sure why, when a unit test case fails during creation, you don't just allow the error to be raised in the setupClass (which will halt the test case entirely) but instead fail in setup? There may be a reason for this :) I also think it's fine for tests to raise an error rather than failing - to me that's a meaningful outcome that in some cases is distinct from a simple fail e.g. 'pass' the test produced the expected result, 'fail' the test did not produce the expected result, 'error' the test failed in an unexpected way...

6) Obviously you have your error codes. The alternative is that you can make a single 'eBayRestError' class derived from Exception and the make further exception classes like 'eBayRestKeyError' or whatever derived from that. Then a third-party application only has to catch 'eBayRestError' and they will also catch all of the subclasses, but they can also catch the more specific errors if they want to.

[matecsaj]

Your points on error handling are constructive; your bang on that it is a package-wide issue with nothing to do with 'Digital Signatures.' I've decided to defer this opportunity for improvement and will start a new issue with your valuable points.

Thank you sincerely for contributing to the project again.

Do a pip upgrade, and you will get the new '.30' version which contains your code.