1550 platform api receive [WIP]

[willdoran]

This pull request makes the following changes:

• Allows user to associate inbound datasources with specific surveys
• Allows user to associate inbound data from datasources with specific survey fields, not just content
• Generates and regenerates API keys - currently per platform deployment but will allow for in future app specific apikey/client_id/client_secret
• Adds apikey table
• Adds 3rd party webhook route to update specific posts
• Adds check for api key
• Adds check for signature of inbound data using established secret
• Adds check for webhook_uuid
• Adds webhook_uuid
• Adds destination, and source fields to outbound webhook data.

Test these changes by:

Fixes #1550

Ping @ushahidi/platform

---

This change is 

[rjmackay]

Why not just use the UpdatePostUsecase? Whats different in this instance?

[willdoran]

It uses a different update method since it's coming through the additional api

[rjmackay]

indenting?

[rjmackay]

This isn't used?

[willdoran]

No, I added it but then never used it, will remove

[rjmackay]

TBH this is a little too big to review well on a friday afternoon. It seems a bit like it could have been a couple of separate PRs.
However I think it looks good.

The API for managing api keys is straightforward - and we could easily migrate it to be based around oauth at a future date anyway.

The API for updating posts looks sane. Though I wonder if 1. it should just use the standard usecase? 2. it could just be a modification of the Post controller that allows a different auth method?

[willdoran]

So, I was going to use the standard use case but the essentially difference is the removal of the authorizer which checks for a logged in user which we don't have, that could become a switch. I'll look into changing that so there is just the one, in part I also wanted an explicit usecase to make sure that there couldn't be accidental data revealed if something changed on the main post usecase.

[rjmackay]

removal of the authorizer which checks for a logged in user which we don't have, that could become a switch.

make sense. One way I've achieved this for console tasks is to pass in a custom authorizer. Though TBH it sounds like this is a distinct usecase.. at least for now so leave it as is.

I think what we may need to do with our usecases in future is make them less generalized so they're more understandable. Then review and figure out what the appropriate abstraction is.. right now they're hard to understand because we generalized too early.

[willdoran]

This is waiting on resolving the tag changes that are causing problems with saving

[rjmackay]

Reviewed. There are some concerning things for this code going into core still

1. the posts update API having zero validation, and unclear divergence in the repo
2. API keys using UUIDs

The rest is mostly fine

---

Reviewed 11 of 28 files at r1, 36 of 42 files at r2, 8 of 8 files at r3.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

application/classes/Ushahidi/Repository/ApiKey.php, line 49 at r2 (raw file):

		$record = $entity->asArray();
		try {
			$uuid = Uuid::uuid4();

I'm not sure UUIDs are safe to use as API keys https://tools.ietf.org/html/rfc4122#section-6

---

application/classes/Ushahidi/Repository/Post.php, line 1005 at r3 (raw file):

		{
			// Update post-values
			$this->post_value_factory->proxy()->deleteAllForPost($entity->id);

Why did this

---

application/classes/Ushahidi/Repository/Post.php, line 1029 at r3 (raw file):

	// UpdateRepository
	public function updateFromService(Entity $entity)

Not 100% sure about this... the diverging logic here is a bit risky

---

application/classes/Ushahidi/Repository/Post.php, line 1054 at r3 (raw file):

		{
			// Update post-values
			$this->updatePostValues($entity->id, $values);

Doesn't this end up a bit weird any changed values will end up being duplicated since we don't remove the old values first??

---

src/Core/Tool/Authorizer/ApiKeyAuthorizer.php, line 43 at r3 (raw file):

		// Only logged in users have access if the deployment is private
		if (!$this->hasAccess()) {

This should be canAccessDeployment($user) now

---

src/Core/Traits/DataTransformer.php, line 125 at r3 (raw file):

		if ($value instanceof \DateTimeInterface) {
			$value = clone $value;
		} elseif (is_array($value)) {

Not sure fond of adding yet another date format :(

---

Comments from Reviewable

[willdoran]

API Keys

@rjmackay I can look at changing to something other than UUID, however, that is more practical to change post lumen because it may rely on a specific lib which seems redundant to change and then change again.

What is important to note about this use of UUID is that it is v4, while it is theoretically possible to duplicate these keys, even with knowledge of a fundamental flaw in the random generation there are 2^128 possible combinations of these 32 digits. Assuming that the random number generator underlying the UUID is not compromised then the chances of generating a duplicate(collision) are difficult to write out because I would be typing zeros for a very very long time but the general example that is used is 0.000'000'000'000'000'4 is the probability of generating a duplicate when the space is 2^36, ' here represents orders of magnitude. The facetious example that mathematicians give is the chance you're going to be hit by a meteorite this year. That's roughly 1 in seventeen billion or 0.000'000'000'06.

Assuming current known computing technology that means that it would require about 256 exabytes of data storage and ~100 years generating somewhere around a billion keys per second to gain a 50% of possibility of generating a duplicate. If that is possible then GPG/PGP is basically worthless.

If the random number generator is compromised then we would need to immediately contact the owner, but I don't have any information that this is the case.

Are UUIDs cryptographically secure inherently? No, this is not part of their RFC so it is not a contractual guarantee. If the UUID were the sole form of protection to the data then that would be a problem, however, it is not. The data is signed using a combination of the UUID, a Shared Secret and the content. A compromised UUID would provide a partial opportunity for an attacker to pass data to the API, however, without a registered shared key they will be unable to pass the signature verification.

We do check the API key while also checking the signature, but if an attacker has compromised the system to such an extent that they own the shared secret we have a different problem. The same vulnerability exists for each of the Data Providers except they do not have the additional protection of the signing verification.

For me, this is reasonable security. It's also the pattern used by Twillio, Frontlinesms and I think Nexmo so if this is compromised for us it may be compromised for them too which I'm not sure what to do about as a problem.

The best alternative would be to upgrade this system to using a full GPG lib or something that does elliptic curve - I'd be entirely up for doing that.

In general, API keys on their own are not enough, they must be paired with a secret of some sort to mitigate security issues. Ideally, they would even be tied to IP but we don't have that capability right now.

A serious problem is the underlying openssl system that is used to generate the random bits. The lib we're using is using openssl_random_pseudo_bytes(), we should double check the version of OpenSSL that we're compiling with PHP.

Validation

My belief though I might be completely misremembering was that we discussed this last time this almost merged. The missing piece here is two fold

1. Lumen Validation - I can try to add a validation step here using the existing system but if it's going to change with lumen then it might be better to land lumen then I can merge to that or I can make that change post lumen
2. We need PATCH for updates, I was waiting for Lumen to be able to implement that, we can build that now but it seems more logical to build it after this will positively effect:

• Will remove the divergent service for updates
• Possibility that the service could cause duplication of elements

Datetime

I'm not super sure if this is a different type, I think though it's a while ago, that it was a valid format we weren't handling. I'm investigating it now, hopefully, there is a way to make datetime less weird generally.

Permission

Changed

[rjmackay]

API keys on their own are not enough, they must be paired with a secret of some sort

I think I missed that there was a secret. As long as we have both thats fine.

Lumen Validation - I can try to add a validation step here using the existing system but if it's going to change with lumen then it might be better to land lumen then I can merge to that or I can make that change post lumen

No validation changes very little with lumen - at least for now.

We need PATCH for updates

I think its fine to have a specific purpose Update endpoint as a temporary solution. We just need to validate enough to ensure we're not getting broken data