Implement integration tests for the federation messages receive feature #6539

cmrd-senya · 2015-11-10T18:37:19Z

This is some initial tests, more to come.

diaspora-code-review · 2015-11-10T18:37:28Z

spec/integration/federation/receive_federation_messages.rb

@@ -0,0 +1,93 @@
+require 'spec_helper'


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

cmrd-senya · 2015-11-22T22:23:26Z

This new version of the patch utilizes the latest changes in the diaspora_federation gem and does an integration test on comments receive. More tests to come.

cmrd-senya · 2015-11-23T16:51:39Z

@jhass, this contains fix to the security issue with signatures we discovered before. I put it here, because here I test it using the integration tests.

I propose to merge it all together, what do you think?

There is to major things to do, before we can merge it:

Do something to factories.rb in this patch
Release the diaspora_federation gem with the latest changes

factories.rb here merged from the diaspora_federation gem and has some changes. I think it is bad idea to duplicate code, so it would be nice if we could managed to reuse it somehow. Maybe we can ship this factory definitions with the diaspora_federation gem? What do you think, @SuperTux88?

jhass · 2015-11-23T16:56:45Z

config/initializers/diaspora_federation.rb

@@ -61,5 +35,26 @@

      person_entity.save!
    end
+
+    on :fetch_private_key_by_id do |diaspora_id|
+      Person.find_by(diaspora_handle: diaspora_id).owner.encryption_key


We shouldn't need to allocate AR objects here, the right combination of pluck and joins should do it.

jhass · 2015-11-23T17:11:02Z

spec/integration/federation/receive_federation_messages.rb

+    Workers::ReceiveEncryptedSalmon.new.perform(@user.id, generate_xml(entity, @remote_user, @user))
+
+    expect(@user.contacts.count).to eq(2)
+    new_contact = @user.contacts.last


Not guaranteed to be stable, add an order clause.

cmrd-senya · 2015-11-24T21:33:36Z

spec/integration/federation/receive_federation_messages.rb

+    Workers::ReceiveEncryptedSalmon.new.perform(user.id, generate_xml(entity, remote_user, user))
+    received_entity = klass.find_by(guid: entity.guid)
+    expect(received_entity).not_to be_nil
+    expect(received_entity.author.diaspora_handle).to eq(remote_user.person.diaspora_handle)


@jhass, do I need to avoid AR object here?

I'd say it's okay if you access multiple properties. It's more critical in non test code anyway.

cmrd-senya · 2015-11-25T20:09:32Z

I'm kinda finished, it's left to make a release of the diaspora_federation.

It covers only a few cases of federation receive, but I think it's better to merge it and then add more tests in further commits, so we don't stay with the opened vulnerability.

jhass · 2015-11-25T20:34:03Z

config/initializers/diaspora_federation.rb

+
+    on :post_author_is_local? do |guid|
+      id_array = Post.where(guid: guid).joins(:author).pluck(:owner_id)
+      !id_array.first.nil? unless id_array.empty?


Wouldn't something like Post.joins(:author).where(guid: guid).not(owner_id: nil).exists? work?

Look at the spec. There is three different cases, and I made it to return nil if guid is not found. With what you offered it would be false in that case, which isn't true strictly.

Then this shouldn't be a predicate (foo?), they should only return true or false, but rather post_author_type with the possible outcomes :post_not_found, :local, :remote and theoretically :author_not_found. The third possibility would be to raise in the post not found case.

I think I do prefer to consider the post not found case false.

Ok, that would be simpler then.

cmrd-senya · 2015-11-30T21:50:59Z

I realized that some of situations tested here are artificial (e.g. here). In real life, it's not possible, or just doesn't make sense, that sender makes right signature for Salmon and then fails to make a signature for the payload using the same key. Thus, I'll rewrite tests in consideration that we only test situations where the both signatures (Salmon and in-payload, like in relayables) are correct or both signatures are wrong.

Flaburgan · 2015-11-30T22:08:54Z

Also consider the situation of a malicious user and pod owner ;)

cmrd-senya · 2015-11-30T22:31:00Z

Sure, it is considered, it is the only reason why we need signatures at
all :)

ATM we have a situation where entity in payload, for example comment
gets signed by the key of a user, and then packed in Salmon, and Salmon
gets signed with the same key again. There isn't much sense to check
cases, when one signature is ok and the other is broken (moreover, we
probably just don't need these duplicating signatures at all, see #6566).

The fun is that the only case when a signature in the entity is in fact
useful
(this
is the one where we don't check it now :)

I guess that there maybe was some idea behind it, like they wanted, for
example, to make pods exchange messages in P2P style, like pod A posted
a status, pod B fetched the status and resent it to pod C, which resent
it to pod D. That would explain why they duplicated signatures in the
payloads but not only in signed envelopes, and that would explain why
they wanted to keep signatures in the database. But we don't do that, so
most of this signatures code is redundant for us.

cmrd-senya · 2015-12-02T22:52:08Z

Please, review.

These are some initial tests, more to come. It tests some features of Request, StatusMessage, Comment, Like, Participation, Retraction, SignedRetraction, RelayableRetraction entities receive process.

cmrd-senya · 2015-12-13T05:40:44Z

diaspora_federation 0.0.9 is released, I guess this one is therefore ready.

…ownstream receive of a federated relayable entity, allowing to forge relayables if you are an owner of the pod where a parent object is stored.

jhass · 2015-12-13T11:29:22Z

Merged as 4e41b8d 08b910b 922d26f f0fc62e

Thank you!

cmrd-senya · 2015-12-13T13:33:51Z

Wow, cool!

diaspora-code-review reviewed Nov 10, 2015
View reviewed changes

cmrd-senya force-pushed the receive-tests branch from db14168 to b175543 Compare November 22, 2015 22:21

cmrd-senya mentioned this pull request Nov 22, 2015

Making a 0.0.9 release diaspora/diaspora_federation#15

Closed

cmrd-senya force-pushed the receive-tests branch from b175543 to d557f95 Compare November 23, 2015 16:40

cmrd-senya closed this Nov 23, 2015

cmrd-senya reopened this Nov 23, 2015

jhass reviewed Nov 23, 2015
View reviewed changes

cmrd-senya mentioned this pull request Nov 23, 2015

Introduce diaspora_federation-tests gem. diaspora/diaspora_federation#16

Merged

cmrd-senya reviewed Nov 24, 2015
View reviewed changes

cmrd-senya force-pushed the receive-tests branch from 088a203 to 6df73df Compare November 25, 2015 20:02

jhass reviewed Nov 25, 2015
View reviewed changes

cmrd-senya force-pushed the receive-tests branch from 6df73df to 27d9c67 Compare November 28, 2015 14:54

Allow Retraction to deal with Relayables

b866f22

cmrd-senya force-pushed the receive-tests branch from 27d9c67 to e16694f Compare December 2, 2015 22:48

cmrd-senya added 2 commits December 13, 2015 08:34

bump diaspora_federation

693f568

Implement integration tests for the federation messages receive feature

e92c12e

These are some initial tests, more to come. It tests some features of Request, StatusMessage, Comment, Like, Participation, Retraction, SignedRetraction, RelayableRetraction entities receive process.

cmrd-senya force-pushed the receive-tests branch from e16694f to 17f36b4 Compare December 13, 2015 05:35

cmrd-senya changed the title ~~[WIP] Implement integration tests for the federation messages receive feature~~ Implement integration tests for the federation messages receive feature Dec 13, 2015

Fix a security issue that author_signature is not checked on the to-d…

b066bd1

…ownstream receive of a federated relayable entity, allowing to forge relayables if you are an owner of the pod where a parent object is stored.

cmrd-senya force-pushed the receive-tests branch from 17f36b4 to b066bd1 Compare December 13, 2015 05:47

jhass added this to the 0.5.6.0 milestone Dec 13, 2015

jhass closed this in f0fc62e Dec 13, 2015

cmrd-senya deleted the receive-tests branch December 13, 2015 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement integration tests for the federation messages receive feature #6539

Implement integration tests for the federation messages receive feature #6539

cmrd-senya commented Nov 10, 2015

diaspora-code-review Nov 10, 2015

cmrd-senya commented Nov 22, 2015

cmrd-senya commented Nov 23, 2015

jhass Nov 23, 2015

jhass Nov 23, 2015

cmrd-senya Nov 24, 2015

jhass Nov 24, 2015

cmrd-senya commented Nov 25, 2015

jhass Nov 25, 2015

cmrd-senya Nov 25, 2015

jhass Nov 25, 2015

cmrd-senya Nov 25, 2015

cmrd-senya commented Nov 30, 2015

Flaburgan commented Nov 30, 2015

cmrd-senya commented Nov 30, 2015

cmrd-senya commented Dec 2, 2015

cmrd-senya commented Dec 13, 2015

jhass commented Dec 13, 2015

cmrd-senya commented Dec 13, 2015

Implement integration tests for the federation messages receive feature #6539

Implement integration tests for the federation messages receive feature #6539

Conversation

cmrd-senya commented Nov 10, 2015

Choose a reason for hiding this comment

cmrd-senya commented Nov 22, 2015

cmrd-senya commented Nov 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmrd-senya commented Nov 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmrd-senya commented Nov 30, 2015

Flaburgan commented Nov 30, 2015

cmrd-senya commented Nov 30, 2015

cmrd-senya commented Dec 2, 2015

cmrd-senya commented Dec 13, 2015

jhass commented Dec 13, 2015

cmrd-senya commented Dec 13, 2015