Project

General

Profile

Feature #1590

Support for Pre-Approving a Subscription Request

Added by Wojciech Kapcia TigaseTeam about 5 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
High
Target version:
Start date:
Due date:
2017-07-13
% Done:

100%

Estimated time:
12.00 h
Database:
n/a
Source Code Disclaimer:

Description


Related issues

Related to Tigase XMPP Server - Feature #1696: Pre approve to roster subscribe requestRejected2017-05-23

Associated revisions

Revision f8800916 (diff)
Added by W Administrator over 1 year ago

#1590 add support for subscription pre-approval in RosterModule and RosterItem

Revision 8e5627ea (diff)
Added by W Administrator over 1 year ago

#1590 add test case for subscription pre-approval

Revision 0aa9638b (diff)
Added by W Administrator over 1 year ago

#1590 add support for subscription pre-approval

Revision 77307354 (diff)
Added by W Administrator over 1 year ago

#1590 add test case for subscription pre-approval

History

#1 Avatar?id=6023&size=24x24 Updated by Artur Hefczyc TigaseTeam about 5 years ago

  • Priority changed from Normal to High

This is a good idea.

#2 Avatar?id=6023&size=24x24 Updated by Artur Hefczyc TigaseTeam about 5 years ago

  • Target version set to tigase-server-5.2.1

#3 Avatar?id=6023&size=24x24 Updated by Artur Hefczyc TigaseTeam almost 5 years ago

  • Target version changed from tigase-server-5.2.1 to tigase-server-7.0.0

I thought this is implemented already, please confirm.

#4 Updated by Wojciech Kapcia TigaseTeam almost 5 years ago

Currently we only support completely automatic subscribing to each other; pre-approving subscription request is a bit different.

#5 Avatar?id=6023&size=24x24 Updated by Artur Hefczyc TigaseTeam almost 4 years ago

  • Target version changed from tigase-server-7.0.0 to tigase-server-7.1.0
  • Source Code Disclaimer set to No

#6 Updated by Daniele Ricci almost 4 years ago

I'd like to contribute to this feature. What approach do you suggest?

I can see that a subscription transition state is 'none' if coming from 'none':

subsToStateMap.put(SubscriptionType.none, StateTransition.none);

Is that right? Could it be enough to forcibly change it to none_pending_out? I'm trying to figure out how it works.

#7 Updated by Daniele Ricci almost 4 years ago

I see that it also needs a stream feature advertisement:

<sub xmlns='urn:xmpp:features:pre-approval'/>

#8 Updated by Daniele Ricci almost 4 years ago

This might be a problem:

3. If the contact is in the user's roster with a state of "To", "None", or "None + Pending Out", the user's server MUST note the
subscription pre-approval by setting the 'approved' flag to a value of "true", then push the modified roster item to all of the
user's interested resources. However, the user's server MUST NOT route the presence stanza of type "subscribed" to the contact.

Because Tigase roster doesn't support a state of pre-approval (that is, something like the 'approved' flag). Do we need a new subscription state for this?

#9 Updated by Daniele Ricci almost 4 years ago

I'm starting to experiment on this feature, any insight that I should know about? :)

#10 Updated by Wojciech Kapcia TigaseTeam almost 4 years ago

I'd say that new subscription state is not needed and most likely inclusion of checking of approved flag in tigase.xmpp.impl.roster.RosterAbstract.StateTransition.getStateTransition() for transition be enough in terms of transition. If that wouldn't be enough then new set of transition states for "To", "None", or "None + Pending Out" would be needed.

#11 Updated by Daniele Ricci almost 4 years ago

I'm afraid using a state transition isn't enough either.

Pre-approval needs "to read the other roster" (that is, from the other contact involved in the subscription flow): presence subscription stanzas (type=subscribe(d)) are kept in storage and not delivered until the other contact comes online - therefore not setting subscription=from in the roster until the type=subscribe is delivered.

#12 Updated by Wojciech Kapcia TigaseTeam over 3 years ago

Daniele Ricci wrote:

Pre-approval needs "to read the other roster" (that is, from the other contact involved in the subscription flow)

How come? This shouldn't be the case and contacts may not be on the same server/installation. And you can extend Roster item to hold any additional data.

#13 Avatar?id=6023&size=24x24 Updated by Artur Hefczyc TigaseTeam over 3 years ago

  • Due date set to 2015-08-27

What is the status of this? Was there any work done for the feature?

#14 Updated by Wojciech Kapcia TigaseTeam over 3 years ago

  • Status changed from New to Feedback
  • Assignee changed from Wojciech Kapcia to Artur Hefczyc

This hasn't been implement yet, no work done

#15 Updated by Daniele Ricci over 3 years ago

I began to tweak something in the roster classes a few months ago but I had no time to proceed on that. It's not worth posting it - it was just some experimentation to understand how the code worked. Sorry I had no chance to go ahead with it.

#16 Avatar?id=6023&size=24x24 Updated by Artur Hefczyc TigaseTeam over 3 years ago

  • Due date changed from 2015-08-27 to 2015-09-12
  • Assignee changed from Artur Hefczyc to Wojciech Kapcia

Please, provide work estimation and work on this for version 7.1.

#17 Updated by Wojciech Kapcia TigaseTeam over 3 years ago

  • Status changed from Feedback to New
  • Estimated time set to 12.00 h

I estimate around 12h to implement this.

#18 Avatar?id=6023&size=24x24 Updated by Artur Hefczyc TigaseTeam almost 3 years ago

  • Due date changed from 2015-09-12 to 2016-04-01
  • Target version changed from tigase-server-7.1.0 to tigase-server-8.0.0

#19 Updated by Wojciech Kapcia TigaseTeam almost 3 years ago

  • Due date deleted (2016-04-01)

#20 Updated by Daniele Ricci almost 3 years ago

Just to let you know that I began working on this on my fork [1].

All I have left is to reply with a subscribed stanza on behalf of the user when the contact requests a subscribe (and remember to not forward that subscribe). It should be all done by tomorrow. In the meantime, you're welcome to inspect the code to check if I did anything stupid so far :)

[1] https://github.com/kontalk/tigase-server/compare/ea37953921bf05998975296cd84d3fcd30c1ab54...master

#21 Updated by Daniele Ricci almost 3 years ago

I finished! I tested it a bit and it seems to be working.

The only thing I couldn't workaround is that when the user sends "subscribe" to a contact that has pre-approved him before sending his initial presence, the incoming pre-approval is not processed yet at that point (state transition hasn't happened yet) so an automatic "unsubscribe" is triggered which causes the incoming pre-approval to be lost (because of the next state transition). This is in contradiction with RFC 6121, par. 4.3.2:

If the contact account does not exist or the user's bare JID is in the contact's roster with a subscription state other than "From", "From + Pending Out", or "Both" (as explained under Appendix A), then the contact's server SHOULD return a presence stanza of type "unsubscribed" in response to the presence probe (this will trigger a protocol flow for canceling the user's subscription to the contact as described under Section 3.2; however, this MUST NOT result in cancellation of a subscription pre-approval as described under Section 3.4).

However, I think sending subscription requests before sending an initial presence is a bit unusual (if not even allowed because you're not an active resource yet, what do you think?).

The URL to access the patch is the same, if you give me the OK I will send you an official patch on top of latest release branch (the code I've been working on is based on commit 81088be7c18e7f1df04f7020301b9c9708d4a008).

Thanks!!

#22 Updated by Daniele Ricci almost 3 years ago

Hello again,

I've just rebased my repository on latest release branch:

https://github.com/kontalk/tigase-server/compare/e77ed362092d461c45d156e9ddc8acd986c339d1...master

As you might already know, you can append .diff or .patch to access a unified diff or a git patch:

https://github.com/kontalk/tigase-server/compare/e77ed362092d461c45d156e9ddc8acd986c339d1...master.diff

https://github.com/kontalk/tigase-server/compare/e77ed362092d461c45d156e9ddc8acd986c339d1...master.patch

Looking forward to your review :)

Thanks

#23 Updated by Daniele Ricci over 2 years ago

Hello! I know you are busy with the upcoming release, is there anything I can do to speed up merging of this patch? Or maybe some feedback to improve or fix it if needed. Thanks for your efforts!!

#24 Updated by Wojciech Kapcia TigaseTeam over 2 years ago

  • Due date set to 2016-05-20

Hi, I'll try to look into the changes within next 7 days.

#25 Updated by Wojciech Kapcia TigaseTeam almost 2 years ago

  • Due date deleted (2016-05-20)

#26 Updated by Daniele Ricci almost 2 years ago

I see you've removed the due date. Not pressuring or anything, but is this going to be merged ever? :)

#27 Avatar?id=6023&size=24x24 Updated by Artur Hefczyc TigaseTeam over 1 year ago

Wojtek, how much time do you think applying Daniele's patch would take? Can we do this for 7.2.0?

#28 Updated by Wojciech Kapcia TigaseTeam over 1 year ago

%kobit

Above patches are simple comparisons to master. There are 6 commits that seems to be related to pre-approval; gathering all changes and reviewing it - 1-3h I guess

#30 Updated by Wojciech Kapcia TigaseTeam over 1 year ago

  • Due date set to 2017-05-23
  • Start date deleted (2013-10-14)
  • Parent task set to #5276

#31 Updated by Wojciech Kapcia TigaseTeam over 1 year ago

  • Status changed from New to In Progress

#32 Updated by Wojciech Kapcia TigaseTeam over 1 year ago

  • Blocks deleted (Feature #1696: Pre approve to roster subscribe request)

#33 Updated by Wojciech Kapcia TigaseTeam over 1 year ago

  • Related to Feature #1696: Pre approve to roster subscribe request added

#34 Updated by Wojciech Kapcia TigaseTeam over 1 year ago

  • Due date changed from 2017-05-23 to 2017-07-13
  • Status changed from In Progress to In QA
  • % Done changed from 0 to 90

I've finished with the implementation (after the review of provided patch, which required changes); added TTS-NG test case as well as modified JaXMPP to match.

@Daniele Ricci, a couple of comments:

  • there wasn't a need for distinct approved_in and approved_out as it's only based on user roster (not contact, i.e. it's local)

  • outgoing subscribed presence wasn't filtered out;

  • "subscribed" presence was not generated automatically.

#35 Updated by Daniele Ricci over 1 year ago

Wojciech Kapcia wrote:

  • there wasn't a need for distinct approved_in and approved_out as it's only based on user roster (not contact, i.e. it's local)

Oh. Ok, thanks.

  • outgoing subscribed presence wasn't filtered out;

My mistake.

  • "subscribed" presence was not generated automatically.

Mmm... it is working on production and it does generate a subscribed.

Anyway, I'll backport this to 7.1.1-SNAPSHOT and test it as soon as possible.

Thanks!!

#36 Updated by Wojciech Kapcia TigaseTeam over 1 year ago

  • Status changed from In QA to Closed
  • % Done changed from 90 to 100

Tests are passing. I'm closing the issue.

@Daniel. If you have any comments or you run into some problems feel free to re-open.

Also available in: Atom PDF