prosody: add cloud_notify_encrypted module#213320
prosody: add cloud_notify_encrypted module#213320martinetd wants to merge 2 commits intoNixOS:masterfrom
Conversation
|
hmm tests pass now, I have no idea what went wrong last year with this... So back to my question in OP: what'd be a good way forward for this? Add with default=false and let users pick it? make a 'siskin' config option for all three as the XEP are siskin-specific? also +Cc @pennae as he's done quite a few updates to the prosody modules recently |
pennae
left a comment
There was a problem hiding this comment.
only did doc updates and never used prosody, so all we can do is comment on the docs. also not he. 🙂
probably better to let users choose whether they want to enable these or not. any extension would potentially increase attack surface, right?
There was a problem hiding this comment.
| description = lib.mdDoc "Include encrypted text within cloud notifications for message preview with Siskin IM: https://xeps.tigase.net/docs/push-notifications/encrypt/"; | |
| description = lib.mdDoc '' | |
| Include encrypted text within cloud notifications for message preview with Siskin IM. | |
| See <https://xeps.tigase.net/docs/push-notifications/encrypt/> for details. | |
| ''; |
similar for others.
234f5bc to
50d485e
Compare
woops x2 -- sorry, and thanks for the review!
I've toggled the default to off for now as that's what I also had in mind; it's probably useless for anyone not having Siskin users. |
|
(... and now I'm looking at turning this on in my config I saw the services.prosody.package example overrides libs to add something, so all one had to do would be so this is really only an advantage for documentation, I guess? I hadn't seen lua extra libs was that easy, and it took me a while to discover Siskin needed these modules in the first place...) |
|
if you're no longer convinced that options are needed you could add the info to the prosody manual chapter instead? perhaps with a nod in the extraModules option to look in the manual for more examples. |
|
Can you please rebase this? |
rebased (simple indentation change around lua packages) I'm not sure how it tangles with the prosody big rework PR though, so happy to let this sleep until after that if it's easier |
I think we should merge it before as otherwise everything is blocked on a close to 2-year-old PR which needs a major rebase because of all the formatting changes anyway. |
SuperSandro2000
left a comment
There was a problem hiding this comment.
I think with the 13.0.0 update we do not need any of these changes, do we?
| luabitop | ||
| luadbi-sqlite3 | ||
| luaunbound | ||
| luaossl |
There was a problem hiding this comment.
With prosody 13.0.0 on which I am currently working on seems to not require this.
see https://modules.prosody.im/mod_cloud_notify_encrypted
Also there is withExtraLuaPackages
There was a problem hiding this comment.
Oh, they updated this.. I'll try to remove luaossl in my build and update over the weekend
There was a problem hiding this comment.
master is still at 0.12.5 so we still require this :/
To clarify your poitn about withExtraLuaPackages, you suggest that if any muc's offline_delivery option is set then the nixos module overrides cfg.packages default value to a pkgs.prosody.override with extra lua packages/community modules?
That'd mean the package isn't cached, but prosody isn't too bad so I guess it's fine; I'm just more used to seeing packages provide all the features required by what the nixos module might need and have the module toggle these by config. (that and I'm not sure how to override the default package actually, if you say it's possible I'll look at other modules if I can find an example)
(only ran nixosTests.prosody with/without the option)
diff --git a/nixos/modules/services/networking/prosody.nix b/nixos/modules/services/networking/prosody.nix
index f4d9172bab3f..ad28351007cf 100644
--- a/nixos/modules/services/networking/prosody.nix
+++ b/nixos/modules/services/networking/prosody.nix
@@ -381,6 +381,11 @@ let
default = true;
description = "Adds the ability to set vCard for Multi User Chat rooms";
};
+ offline_delivery = mkOption {
+ type = types.bool;
+ default = false;
+ description = "This module implements support for sending messages in a MUC to affiliated users who are not in the room.";
+ };
# Extra parameters. Defaulting to prosody default values.
# Adding them explicitly to make them visible from the options
@@ -905,6 +910,13 @@ in
)}
${lib.concatStringsSep "\n" (map (x: "${toLua x};") cfg.package.communityModules)}
${lib.concatStringsSep "\n" (map (x: "${toLua x};") cfg.extraModules)}
+ ${lib.optionalString (lib.any (muc: muc.offline_delivery) cfg.muc) ''
+ "cloud_notify_encrypted";
+ "cloud_notify_filters";
+ "cloud_notify_extensions";
+ "cloud_notify_priority_tag";
+ "muc_offline_delivery";
+ ''}
};
disco_items = {
@@ -937,7 +949,7 @@ in
${lib.concatMapStrings (muc: ''
Component ${toLua muc.domain} "muc"
- modules_enabled = { "muc_mam"; ${optionalString muc.vcard_muc ''"vcard_muc";''} ${optionalString muc.allowners_muc ''"muc_allowners";''} }
+ modules_enabled = { "muc_mam"; ${optionalString muc.vcard_muc ''"vcard_muc";''} ${optionalString muc.allowners_muc ''"muc_allowners";''} ${optionalString muc.offline_delivery ''"muc_offline_delivery",''} }
name = ${toLua muc.name}
restrict_room_creation = ${toLua muc.restrictRoomCreation}
max_history_messages = ${toLua muc.maxHistoryMessages}
diff --git a/nixos/tests/xmpp/prosody.nix b/nixos/tests/xmpp/prosody.nix
index 0183e876803e..3b25c8c8036d 100644
--- a/nixos/tests/xmpp/prosody.nix
+++ b/nixos/tests/xmpp/prosody.nix
@@ -87,6 +87,7 @@ import ../make-test-python.nix {
muc = [
{
domain = "conference.example.com";
+ offline_delivery = true;
}
];
uploadHttp = {
diff --git a/pkgs/servers/xmpp/prosody/default.nix b/pkgs/servers/xmpp/prosody/default.nix
index 997716a8928f..f92bafa370f3 100644
--- a/pkgs/servers/xmpp/prosody/default.nix
+++ b/pkgs/servers/xmpp/prosody/default.nix
@@ -30,6 +30,8 @@ let
luabitop
luadbi-sqlite3
luaunbound
+ # no longer needed after 0.13
+ luaossl
]
++ lib.optional withDBI p.luadbi
++ withExtraLuaPackages p
@@ -45,6 +47,12 @@ stdenv.mkDerivation rec {
"cloud_notify"
"vcard_muc"
"http_upload"
+ # These are only enabled if muc.offline_delivery is set
+ "cloud_notify_encrypted"
+ "cloud_notify_filters"
+ "cloud_notify_extensions"
+ "cloud_notify_priority_tag"
+ "muc_offline_delivery"
];
src = fetchurl {
url = "https://prosody.im/downloads/source/${pname}-${version}.tar.gz";What do you think?
There was a problem hiding this comment.
I totally missed this 😅
master is still at 0.12.5 so we still require this :/
It should be on 13.0.2 now.
To clarify your poitn about withExtraLuaPackages, you suggest that if any muc's offline_delivery option is set then the nixos module overrides cfg.packages default value to a pkgs.prosody.override with extra lua packages/community modules?
That'd mean the package isn't cached, but prosody isn't too bad so I guess it's fine; I'm just more used to seeing packages provide all the features required by what the nixos module might need and have the module toggle these by config. (that and I'm not sure how to override the default package actually, if you say it's possible I'll look at other modules if I can find an example)
Yep, then it wouldn't be cached but that is quite easy to do as I imagine most people load at least one community module.
We can probably add an apply to the package option or add a package variable which does that. Something like:
package = cfg.package.override { .... };
And then we use package everywhere.
| # default setup. | ||
| nixosModuleDeps = [ | ||
| "cloud_notify" | ||
| "cloud_notify_encrypted" |
There was a problem hiding this comment.
This is only for the nixos module and will be reduced a lot with the 13.0.0 update
| description = "Push notifications to inform users of new messages or other pertinent information even when they have no XMPP clients online"; | ||
| }; | ||
|
|
||
| cloud_notify_encrypted = mkOption { |
There was a problem hiding this comment.
I only found out today that there is an extraModules option. We can just use that one, right?
There was a problem hiding this comment.
Yes, that's what I'm using in my home configuration, so I haven't actually been using this PR
I wasn't aware of muc_offline_delivery though (it's been a while I last looked at it), that might explain why my siskin user isn't getting notifications reliably; I'll update the PR with your suggestion and test over the weekend.
|
I would be up for one option to enable the entirety of options required for this: Also for https://modules.prosody.im/mod_cloud_notify_extensions.html we need https://modules.prosody.im/mod_muc_offline_delivery.html which would diff to: --- a/nixos/modules/services/networking/prosody.nix
+++ b/nixos/modules/services/networking/prosody.nix
@@ -381,6 +381,16 @@
default = false;
description = "Allow rooms to be moderated";
};
+ offline_delivery = mkOption {
+ type = types.bool;
+ default = false;
+ description = ''
+ This module implements support for sending messages in a MUC to affiliated users who are not in the room.
+ This is a custom extension by Tigase to allow push notifications from MUCs to users who are not currently connected.
+
+ This module is recommend for mod_cloud_notify_extensions.
+ '';
+ };
vcard_muc = mkOption {
type = types.bool;
default = true;
@@ -942,7 +952,7 @@
${lib.concatMapStrings (muc: ''
Component ${toLua muc.domain} "muc"
- modules_enabled = { "muc_mam",${optionalString muc.vcard_muc '' "vcard_muc",''}${optionalString muc.allowners_muc '' "muc_allowners",''}${optionalString muc.muc_moderation '' "muc_moderation";''} }
+ modules_enabled = { "muc_mam",${optionalString muc.vcard_muc '' "vcard_muc",''}${optionalString muc.allowners_muc '' "muc_allowners",''}${optionalString muc.muc_moderation '' "muc_moderation";''}${optionalString muc.offline_delivery '' "muc_offline_delivery",''} }
name = ${toLua muc.name}
restrict_room_creation = ${toLua muc.restrictRoomCreation}
max_history_messages = ${toLua muc.maxHistoryMessages}Please let me know what do you think and what I missed. |
Description of changes
I've been pointed out in tigase/siskin-im#177 that this module exists: modules.prosody.im/mod_cloud_notify_encrypted
regardless of my success with the actual issue linked I think the module would be useful in general.
I'm not sure what to do with new modules, should I add it with default enable=false for a while first?
Alternatively I'd be open to just do this in my configuration, but I wouldn't know how to add
luaosslto the luaEnv cleanly from there.This is take 2 of #162491 -- can't re-open that PR after rebasing the branch...
Things done
./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes