Skip to content
This repository was archived by the owner on Apr 24, 2021. It is now read-only.

HTTP status codes & JSON support #22

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

oriordan
Copy link

Parsing the text output of the KSM server feels unclean. We could rely on HTTP status codes and JSON.

  Add support for JSON response
…r determine if JSON should be returned.

 Adjusted selftest and added two new tests for yielding Database error
@klali
Copy link
Member

klali commented Mar 7, 2016

I'm not sure I see a reason for this change, the current output format is established and will need to remain in working order.
In principle I'm not against adding a different output but I think we need a clear usecase for it.

@oriordan
Copy link
Author

oriordan commented Mar 7, 2016

I do see your point. It feels a bit unclean to parse text output in a HTTP API. Technically speaking the current solution is slightly faster, but I was planning to add JSON support for yubikey-val as well (where it might make even more sense maybe). So the clients and components can talk to each other via JSON. Ideally even the hex would be turned to integer so the output is immediately consumable.

This was the first step, so if you already see you don't want to have JSON support in yubikey-val, it won't make any sense to do it here either.

By the way, I'm working on a python implementation of yubikey-val, yubikey-ksm, and yubico_client bundled in one called yubistack. I'd really welcome any review / comment from you on the project. The aim is to create simple integrated setup, in which components can talk natively to each other, while keeping full compatibility with the original implementation.

@klali
Copy link
Member

klali commented Mar 14, 2016

Sorry for the delay here..

One tough part in refactoring this is that I don't see a way to get rid of the old format (especially in yubikey-val) which means more complex code for the output.

I'll throw some eyes at yubistack.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants