Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SANDBOX/PRODUCTION API key to context mprovements #73

Closed
DeddoWiersma opened this issue Oct 15, 2018 · 8 comments
Closed

SANDBOX/PRODUCTION API key to context mprovements #73

DeddoWiersma opened this issue Oct 15, 2018 · 8 comments

Comments

@DeddoWiersma
Copy link

See also: https://together.bunq.com/d/4883-api-production-and-sandbox-context-creation-issues

Two small suggestions for improvements for new API users (rookie level):
a) To prevent failing PRODUCTION context creation issues make the quickfix of removing line 315 (CURLOPT_PINNEDPUBLICKEY) in ApiClient.php a definite one. If this balances out the problems which the current SDK causes (spend a lot of time trying to figure out the problem and upgrading cUrl on CentOS/PHP7.0 is not that easy) versus the benefits (which I don't have knowledge about).

b) Update in https://doc.bunq.com/, introduction, 4th bullet point to explain the difference between the production API key obtained through the App and the sandbox API key obtained through (link to article, current link https://doc.bunq.com/#/android-emulator goes to the same page, tipo in link?). Someting like: 'The sandbox API key is different from the production API key. Instructions to get it are found in <page with at least the curl option, maybe more options?>

@OGKevin
Copy link
Contributor

OGKevin commented Oct 15, 2018 via email

@DeddoWiersma
Copy link
Author

ok for a) didn't know the impact, b) is not very clear in the docs, it is hidden under a section in the middle with a link to the page tag which isn't working and if first sight related to Android (and not everybody uses the Bunq API for native Android Apps). Also the curl tip to get a valid Sandbox API would be a good addition. I can understand that documentation is not the highest priority but spending 5 minutes updating the documentation saves n*rookie users as myself several hours trying to figure out how it works.

@kojoru
Copy link
Contributor

kojoru commented Oct 15, 2018

@DeddoWiersma Please rest assured that the issue with steep learning curve for users not using tinker is known, including points that you mention.

Your both points are valid and even if old curl is not recommended for security reasons, that could be communicated more clearly than it is now. Similarly, it's not clear enough that keys for production and sandbox are not the same. We'll be looking into ways to improve that in the future, and I'll leave this issue open and update once that's done.

@kojoru kojoru self-assigned this Oct 15, 2018
@DeddoWiersma
Copy link
Author

@kojoru Thanx, leave it than with you as I assume you can update the documentation. I learned from this discussion that I also have to solve the cUrl issue on the server for safe operational transactions, I will bend my learning curve in that direction now ;-)

@OGKevin
Copy link
Contributor

OGKevin commented Oct 15, 2018

@kojoru i don’t have access to my MacBook atm, while your add it please fix
https://github.com/bunq/sdk_php/blob/f369d3b931ea833e06d40bda85620506a31568a2/composer.json#L24 as well. So that composer will complain if the curl version is to old. For some reason I didn’t think of this before 🤦‍♂️

@OGKevin
Copy link
Contributor

OGKevin commented Oct 15, 2018

@DeddoWiersma just to be clear,
https://github.com/bunq/doc/pull/66/files is not clear enough according to you right ? As sander tried making it more clear in this pr.

@OGKevin OGKevin self-assigned this Oct 15, 2018
@OGKevin
Copy link
Contributor

OGKevin commented Oct 15, 2018

leave it than with you as I assume you can update the documentation

Feel free to create a PR @DeddoWiersma with the changes you please to make it more clear ! 😊 if me or @kojoru didn’t had a chance to do so yet.

@kojoru
Copy link
Contributor

kojoru commented Dec 14, 2018

We won't be removing certificate pinning for reasons outlined above. The documentation should be sufficiently all right after #85. @DeddoWiersma let me know if there's anything else we can do or if the changes look insufficient for any reason.

@kojoru kojoru closed this as completed Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants