search

sampottinger / co_opencampaigndata

API service for serving Colorado TRACER data for opencampaigndata.org
GNU General Public License v3.0
10 stars 1 forks source link

Added implementation of account_manager. #17

Closed sampottinger closed 11 years ago

sampottinger commented 11 years ago

Follow up to rejection of #14. This adds some unit test fixes with regards to time-dependent testing and asynchronous error handling.

sampottinger commented 11 years ago

@trinary, sorry for the confusion but, if you don't mind another code review, that would be great.

trinary commented 11 years ago

Nitpicky recommendation: use crypto.randomBytes for cryptographically secure random generation. I've used crypto.randomBytes(n).toString('hex') for API keys in the past. http://stackoverflow.com/questions/9407892/how-to-generate-random-sha1-hash-to-use-as-id-in-node-js/14869745#14869745

More as I see it. :smile:

trinary commented 11 years ago

This gets a :+1: from me. I think Math.random is seeded at millisecond resolution so API key collision is really unlikely to happen, but I like using crypto.randomBytes when possible. Merge it!

sampottinger commented 11 years ago

Thanks! Actually I think I check for duplicate keys. I agree with randomBytes though... good suggestion. As per your comment in the other pull request, we can come back to it. Thanks :)

sampottinger commented 11 years ago

(unless we are thinking of duplicates in different ways... possible)