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.

Sign up for GitHub

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

Jump to bottom

Feature: Allow setting client identification name/version manually #1815

Merged
merged 1 commit into from Apr 9, 2023
Merged

Feature: Allow setting client identification name/version manually #1815

merged 1 commit into from Apr 9, 2023

Conversation

H3rnand3zzz
Copy link
Contributor

This privacy enhancing feature allows to change client name and version for each account that user have.

sjaeckel
sjaeckel requested changes Apr 3, 2023
View reviewed changes
Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

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

Basically LGTM

Not sure whether this should really be in /account or whether it'd make more sense in /software!?

src/xmpp/iq.c Outdated Show resolved Hide resolved
@sjaeckel sjaeckel requested a review from jubalh April 3, 2023 17:16
@H3rnand3zzz
Copy link
Contributor Author

Basically LGTM

Not sure whether this should really be in /account or whether it'd make more sense in /software!?

It makes it easier to configure multiple accounts. Each account will have its own client, thus preserving privacy.

sjaeckel
sjaeckel reviewed Apr 3, 2023
View reviewed changes
src/xmpp/iq.c Outdated Show resolved Hide resolved
sjaeckel
sjaeckel reviewed Apr 3, 2023
View reviewed changes
src/xmpp/iq.c Outdated Show resolved Hide resolved
src/xmpp/iq.c Outdated Show resolved Hide resolved
@H3rnand3zzz
Copy link
Contributor Author

Builds failed by unrelated reasons

@jubalh jubalh added the feature label Apr 3, 2023
@jubalh jubalh added this to the next milestone Apr 3, 2023
jubalh
jubalh requested changes Apr 4, 2023
View reviewed changes
Copy link
Member

@jubalh jubalh left a comment

Choose a reason for hiding this comment

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

See comments

src/command/cmd_defs.c Outdated Show resolved Hide resolved
src/command/cmd_ac.c Outdated Show resolved Hide resolved
src/command/cmd_defs.c Outdated Show resolved Hide resolved
@jubalh
Copy link
Member

jubalh commented Apr 4, 2023

I reviewed the code. But apart from this I have some points:

  • I'm not sure whether setting this can cause problems. I'm unsure whether some servers or clients use the client info to decide which features to use. But probably they will only use service discovery?
  • Need to check who actually can query this. If its only users where you allowed this anyways, then it might not be needed.

https://xmpp.org/extensions/xep-0092.html sais:

he jabber:iq:version protocol SHOULD NOT be used to determine the identity of entities from which an application receives presence (e.g., contacts in a user's roster and certain kinds of gateways); [Entity Capabilities (XEP-0115)](https://xmpp.org/extensions/xep-0115.html) [[3](https://xmpp.org/extensions/xep-0092.html#nt-idm45786692116480)] SHOULD be used instead. However, the jabber:iq:version protocol MAY be used to determine the identity of entities from which an application does not receive presence (e.g., servers and many kinds of components). The jabber:iq:version protocol MAY also be used to determine information available only via jabber:iq:version (e.g., operating system information) for contacts from which a user receives presence, but only if the user specifically requests such information for a particular contact.

@H3rnand3zzz
Copy link
Contributor Author

The issues are addressed. As for potential problems - it might theoretically break OMEMO and so warns about ClientSwitcher for Psi+. But the feature is advanced and optional, so users should use it cautiously, thoughtfully and ad hoc.

@jubalh jubalh changed the title Feature/Client Switcher Feature: Allow setting client identification name/version manually Apr 4, 2023
@H3rnand3zzz H3rnand3zzz requested a review from jubalh April 4, 2023 15:43
jubalh
jubalh requested changes Apr 7, 2023
View reviewed changes
Copy link
Member

@jubalh jubalh left a comment

Choose a reason for hiding this comment

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

See comments.
Also you need to squash the fixup commit into the first one.
And change the commit title, you still have Add ClientSwitcher Feature there.

src/command/cmd_funcs.c Outdated Show resolved Hide resolved
@H3rnand3zzz
Allow setting client identification name/version manually
5d3c8ce 
Add changes allowing user to switch client name and version.

Useful for enhancing user privacy.

Minor cleanup.
@jubalh jubalh self-requested a review April 9, 2023 16:23
@jubalh jubalh merged commit e52ca2f into profanity-im:master Apr 9, 2023
5 checks passed
@jubalh
Copy link
Member

jubalh commented Apr 9, 2023

Thanks!
BTW now I saw that PSI also has a plugin with this functionality.

@jubalh
Copy link
Member

jubalh commented Apr 10, 2023

Just for the record: I was not sure if this will lead to problems along the way. I'm not certain whether some clients/servers use this field to cache something or change behaviour.
But I guess not many users will use this anyways. It is hard to cater to all the people. And the privacy enthusiasts will probably like this feature. A note was added to the command description.

@H3rnand3zzz H3rnand3zzz deleted the feature/the-client-switcher branch April 14, 2023 10:57
This was referenced Apr 15, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jubalh jubalh jubalh approved these changes

@sjaeckel sjaeckel Awaiting requested review from sjaeckel

Assignees

@H3rnand3zzz H3rnand3zzz

Labels
feature
Projects
None yet
Milestone
0.14.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@H3rnand3zzz @jubalh @sjaeckel