Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Block users #3164

Open
wants to merge 5 commits into
base: jcbrand/block-users
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
213 changes: 61 additions & 152 deletions src/headless/plugins/blocking/api.js
Original file line number Diff line number Diff line change
@@ -1,157 +1,66 @@
import log from '@converse/headless/log.js';
import { _converse, api, converse } from '@converse/headless/core.js';
import { _converse, api, converse } from "@converse/headless/core.js";
import { sendBlockingStanza } from './utils.js';

const { Strophe, $iq, sizzle, u } = converse.env;
const { Strophe } = converse.env;

export default {
/**
* Retrieves the blocklist held by the logged in user at a JID by sending an IQ stanza.
* Saves the model variable _converse.blocked.set
* @private
* @method api.refreshBlocklist
*/
async refreshBlocklist () {
const features = await api.disco.getFeatures(_converse.domain);
if (!features?.findWhere({ 'var': Strophe.NS.BLOCKING })) {
return false;
}
if (!_converse.connection) {
return false;
}

const iq = $iq({
'type': 'get',
'id': u.getUniqueId('blocklist'),
}).c('blocklist', { 'xmlns': Strophe.NS.BLOCKING });

const result = await api.sendIQ(iq).catch((e) => {
log.fatal(e);
return null;
});
if (result === null) {
const err_msg = `An error occured while fetching the blocklist`;
const { __ } = converse.env;
api.alert('error', __('Error'), err_msg);
log(err_msg, Strophe.LogLevel.WARN);
return false;
} else if (u.isErrorStanza(result)) {
log.error(`Error while fetching blocklist`);
log.error(result);
return false;
}

const blocklist = sizzle('item', result).map((item) => item.getAttribute('jid'));
_converse.blocked.set({ 'set': new Set(blocklist) });
return true;
},

/**
* Handle incoming iq stanzas in the BLOCKING namespace. Adjusts the global blocked_set.
* @private
* @method api.handleBlockingStanza
* @param { Object } [stanza] - The incoming stanza to handle
*/
handleBlockingStanza (stanza) {
if (stanza.firstElementChild.tagName === 'block') {
const users_to_block = sizzle('item', stanza).map((item) => item.getAttribute('jid'));
users_to_block.forEach(_converse.blocked.get('set').add, _converse.blocked.get('set'));
} else if (stanza.firstElementChild.tagName === 'unblock') {
const users_to_unblock = sizzle('item', stanza).map((item) => item.getAttribute('jid'));
users_to_unblock.forEach(_converse.blocked.get('set').delete, _converse.blocked.get('set'));
} else {
log.error('Received blocklist push update but could not interpret it.');
}
// TODO: Fix this to not use the length as an update key, and
// use a more accurate update method, like a length-extendable hash
_converse.blocked.set({ 'len': _converse.blocked.get('set').size });
},

/**
* Blocks JIDs by sending an IQ stanza
* @method api.blockUser
*
* @param { Array } [jid_list] - The list of JIDs to block
*/
async blockUser (jid_list) {
if (!_converse.disco_entities.get(_converse.domain)?.features?.findWhere({ 'var': Strophe.NS.BLOCKING })) {
return false;
}
if (!_converse.connection) {
return false;
}

const block_items = jid_list.map((jid) => Strophe.xmlElement('item', { 'jid': jid }));
const block_element = Strophe.xmlElement('block', { 'xmlns': Strophe.NS.BLOCKING });

block_items.forEach(block_element.appendChild, block_element);

const iq = $iq({
'type': 'set',
'id': u.getUniqueId('block'),
}).cnode(block_element);

const result = await api.sendIQ(iq).catch((e) => {
log.fatal(e);
return false;
});
const err_msg = `An error occured while trying to block user(s) ${jid_list}`;
if (result === null) {
api.alert('error', __('Error'), err_msg);
log(err_msg, Strophe.LogLevel.WARN);
return false;
} else if (u.isErrorStanza(result)) {
log.error(err_msg);
log.error(result);
return false;
}
return true;
},

/**
* Unblocks JIDs by sending an IQ stanza to the server JID specified
* @method api.unblockUser
* @param { Array } [jid_list] - The list of JIDs to unblock
*/
async unblockUser (jid_list) {
if (!_converse.disco_entities.get(_converse.domain)?.features?.findWhere({ 'var': Strophe.NS.BLOCKING })) {
return false;
}
if (!_converse.connection) {
return false;
}

const unblock_items = jid_list.map((jid) => Strophe.xmlElement('item', { 'jid': jid }));
const unblock_element = Strophe.xmlElement('unblock', { 'xmlns': Strophe.NS.BLOCKING });

unblock_items.forEach(unblock_element.append, unblock_element);

const iq = $iq({
'type': 'set',
'id': u.getUniqueId('block'),
}).cnode(unblock_element);

const result = await api.sendIQ(iq).catch((e) => {
log.fatal(e);
return false;
});
const err_msg = `An error occured while trying to unblock user(s) ${jid_list}`;
if (result === null) {
api.alert('error', __('Error'), err_msg);
log(err_msg, Strophe.LogLevel.WARN);
return false;
} else if (u.isErrorStanza(result)) {
log.error(err_msg);
log.error(result);
return false;
}
return true;
},
export default {

/**
* Retrieved the blocked set
* @method api.blockedUsers
*/
blockedUsers () {
return _converse.blocked.get('set');
},
};
blocking: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This already looks much better thanks. However, I think blocklist would be a better namespace than blocking.

The reason being that it allows us to write the API in a more readable way.

I would make the following renamings:

  • api.blocking.block -> api.blocklist.add
  • api.blocking.unblock -> api.blocklist.remove
  • api.blocking.blocklist -> api.blocklist.get

The supported and refresh methods can stay as they are.

I realize you may have called the namespace blocking because that's the name of the plugin's folder. I don't think we generally follow a convention of keeping the plugin folder and api namespace names the same (although an argument could be made we should), at least not explicitly. In any case, if such conformity is desired, I'd rather rename the plugin folder to blocklist as well. I don't think it's necessary though.

/**
* Checks if XEP-0191 is supported
*/
async supported () {
const has_feature = await api.disco.features.has(Strophe.NS.BLOCKING, _converse.domain);
if (!has_feature) {
log.info("XEP-0191 not supported, no blocklist available");
return false;
}
log.debug("XEP-0191 available");
return true;
},
/**
* Retrieves the blocklist held by the logged in user at a JID by sending an IQ stanza.
* @private
* @method api.blocking.refresh
*/
async refresh () {
if (!_converse.connection) {
return false;
}
log.debug("refreshing blocklist");
const available = await this.supported();
if (!available) {
log.debug("XEP-0191 NOT available, not refreshing...");
return false;
}
log.debug("getting blocklist...");
return sendBlockingStanza( 'blocklist', 'get' );
},
/**
* Blocks JIDs by sending an IQ stanza
* @method api.blocking.block
*
* @param { Array } [jid_list] - The list of JIDs to block
*/
block ( jid_list ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if this first parameter can either be a string (single JID) or an array of JIDs.

Similarly to how it's done here:

* @param { String|String[] } jids - A JID or array of JIDs

Same for the function below.

sendBlockingStanza( 'block', 'set', jid_list );
},
/**
* Unblocks JIDs by sending an IQ stanza to the server JID specified
* @method api.blocking.unblock
* @param { Array } [jid_list] - The list of JIDs to unblock
*/
unblock ( jid_list ) {
sendBlockingStanza( 'unblock', 'set', jid_list );
},
/**
* Retrieve the blocked set
* @method api.blocking.blocklist
*/
blocklist () {
return _converse.blocking._blocked?.get('set') ?? new Set();
},
}
}
11 changes: 8 additions & 3 deletions src/headless/plugins/blocking/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
/**
* @description
* Converse.js plugin which adds support for XEP-0191: Blocking
* Allows users to block other users, which hides their messages.
* Converse.js plugin which adds support for XEP-0191: Blocking.
* Allows users to block communications with other users on the server side,
* so a user cannot receive messages from a blocked contact.
*/
import blocking_api from './api.js';
import { _converse, api, converse } from '@converse/headless/core.js';
Expand All @@ -27,7 +28,11 @@ converse.plugins.add('converse-blocking', {
},

initialize () {
_converse.blocked = new SetModel();
_converse.blocking = {
_blocked: new SetModel()
};
api.promises.add(["blockListFetched"]);

Object.assign(api, blocking_api);

api.listen.on('discoInitialized', onConnected);
Expand Down
85 changes: 82 additions & 3 deletions src/headless/plugins/blocking/utils.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,87 @@
import log from '@converse/headless/log.js';
import { _converse, api, converse } from "@converse/headless/core.js";
import { __ } from 'i18n';

const { Strophe } = converse.env;
const { Strophe, $iq, sizzle, u } = converse.env;

/**
* Handle incoming iq stanzas in the BLOCKING namespace. Adjusts the global blocking._blocked.set.
* @method handleBlockingStanza
* @param { Object } [stanza] - The incoming stanza to handle
*/
function handleBlockingStanza ( stanza ) {
const action = stanza.firstElementChild.tagName;
const items = sizzle('item', stanza).map(item => item.getAttribute('jid'));
const msg_type = stanza.getAttribute('type');

log.debug(`handle blocking stanza Type ${msg_type} action ${action}`);
if (msg_type == 'result' && action == 'blocklist' ) {
log.debug(`resetting blocklist: ${items}`);
_converse.blocking._blocked.set({'set': new Set()});
items.forEach((item) => { _converse.blocking._blocked.get('set').add(item)});

/**
* Triggered once the _converse.blocking._blocked list has been fetched
* @event _converse#blockListFetched
* @example _converse.api.listen.on('blockListFetched', () => { ... });
*/
api.trigger('blockListFetched', _converse.blocking._blocked.get('set'));
log.debug("triggered blockListFetched");

} else if (msg_type == 'set' && action == 'block') {
log.debug(`adding people to blocklist: ${items}`);
items.forEach((item) => { _converse.blocking._blocked.get('set').add(item)});
api.trigger('blockListUpdated', _converse.blocking._blocked.get('set'));
} else if (msg_type == 'set' && action == 'unblock') {
log.debug(`removing people from blocklist: ${items}`);
items.forEach((item) => { _converse.blocking._blocked.get('set').delete(item)});
api.trigger('blockListUpdated', _converse.blocking._blocked.get('set'));
} else {
log.error("Received a blocklist push update but could not interpret it");
}
return true;
}

export function onConnected () {
api.refreshBlocklist();
_converse.connection.addHandler(api.handleBlockingStanza, Strophe.NS.BLOCKING, 'iq', 'set', null, null);
_converse.connection.addHandler(
handleBlockingStanza, Strophe.NS.BLOCKING, 'iq', ['set', 'result']
);
api.blocking.refresh();
}

/**
* Send block/unblock IQ stanzas to the server for the JID specified
* @method api.sendBlockingStanza
* @param { String } action - "block", "unblock" or "blocklist"
* @param { String } iq_type - "get" or "set"
* @param { Array } [jid_list] - (optional) The list of JIDs to block or unblock
*/
export async function sendBlockingStanza ( action, iq_type = 'set', jid_list = [] ) {
if (!_converse.connection) {
return false;
}

const element = Strophe.xmlElement(action, {'xmlns': Strophe.NS.BLOCKING});
jid_list.forEach((jid) => {
const item = Strophe.xmlElement('item', { 'jid': jid });
element.append(item);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ 'jid' : jid } can be shortened to { jid } and we can make this a one-liner.

jid_list.forEach((jid) => element.append(Strophe.xmlElement('item', { jid }));


const iq = $iq({
'type': iq_type,
'id': u.getUniqueId(action)
}).cnode(element);

const result = await api.sendIQ(iq).catch(e => { log.fatal(e); return false });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to here handle the result stanza by calling handleBlockingStanza(result) instead of doing it via addHandler on line 46 (although we'd still need addHandler for set IQ stanzas).

We can then use await to wait for the result stanza to be processed and then in the API methods we do return sendBlockingStanza so that the promise is returned to the caller.

The advantage of doing it like this is that the caller of the API method can decide to await for the operation to have succeeded and thereby be sure that the blocklist has been updated accordingly.

const err_msg = `An error occured while trying to ${action} user(s) ${jid_list}`;
if (result === null) {
api.alert('error', __('Error'), err_msg);
log(err_msg, Strophe.LogLevel.WARN);
return false;
} else if (u.isErrorStanza(result)) {
log.error(err_msg);
log.error(result);
return false;
}
return true;
}
20 changes: 18 additions & 2 deletions src/headless/plugins/chat/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ const ChatBox = ModelWithContact.extend({
'time_opened': this.get('time_opened') || (new Date()).getTime(),
'time_sent': (new Date(0)).toISOString(),
'type': _converse.PRIVATE_CHAT_TYPE,
'url': ''
'url': '',
'contact_blocked': undefined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is unnecessary, since you'd automatically get undefined when calling model.get('contact_blocked') if it wasn't set to something else.

And yes, I'm aware there are other undefined defaults in this object. Some of this code is so old, when I wrote it I didn't know what I know now ;-)

}
},

Expand Down Expand Up @@ -78,9 +79,24 @@ const ChatBox = ModelWithContact.extend({
* @example _converse.api.listen.on('chatBoxInitialized', model => { ... });
*/
await api.trigger('chatBoxInitialized', this, {'Synchronous': true});

const blocking_supported = await api.blocking?.supported();
if (blocking_supported) {
await api.waitUntil("blockListFetched");
this.checkIfContactBlocked(api.blocking.blocklist());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await api.waitUntil("blockListFetched") should be called inside api.blocking.blocklist().

We don't want consumers of our API to need to know that they have to first wait for the blockListFetched promise. That's an implementation detail that we need to handle inside the API.

api.listen.on('blockListUpdated', this.checkIfContactBlocked, this);
} else this.set({'contact_blocked': undefined});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add curly brackets here.


this.initialized.resolve();
},

checkIfContactBlocked (jid_set) {
if (jid_set.has(this.get('jid')))
return this.set({'contact_blocked': true});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a general rule, we always use curly brackets for if statements, unless it's a single if (no else) one a single line.


this.set({'contact_blocked': false});
},

getMessagesCollection () {
return new _converse.Messages();
},
Expand Down Expand Up @@ -1104,7 +1120,7 @@ const ChatBox = ModelWithContact.extend({
} else if (
this.isHidden() || (
pluggable.plugins['converse-blocking'] &&
api.blockedUsers()?.has(message?.get('from_real_jid'))
api.blocking?.blocklist().has(message?.get('from_real_jid'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will have to be updated once api.blocking.blocklist() is async.

)
) {
this.incrementUnreadMsgsCounter(message);
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/chatview/bottom-panel.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ export default class ChatBottomPanel extends ElementView {
async initialize () {
this.model = await api.chatboxes.get(this.getAttribute('jid'));
await this.model.initialized;
this.listenTo(this.model, 'change:num_unread', this.debouncedRender)
this.listenTo(this.model, 'change:num_unread', this.debouncedRender);
this.listenTo(this.model, 'emoji-picker-autocomplete', this.autocompleteInPicker);
this.listenTo(this.model, 'change:contact_blocked', () => this.render());

this.addEventListener('focusin', ev => this.emitFocused(ev));
this.addEventListener('focusout', ev => this.emitBlurred(ev));
Expand Down
1 change: 1 addition & 0 deletions src/plugins/chatview/message-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export default class MessageForm extends ElementView {
await this.model.initialized;
this.listenTo(this.model.messages, 'change:correcting', this.onMessageCorrecting);
this.listenTo(this.model, 'change:composing_spoiler', () => this.render());
this.listenTo(this.model, 'change:contact_blocked', () => this.render());

this.handleEmojiSelection = ({ detail }) => {
if (this.model.get('jid') === detail.jid) {
Expand Down
Loading