Add the CHATHISTORY extension specification. #292

Open
wants to merge 16 commits into
from

Projects

None yet

8 participants

@jwheare
Member
jwheare commented Jan 2, 2017

Interesting spec. Can you elaborate why msgid is required?

@prawnsalad

As mentioned on IRC I've been getting this idea into the next version of Kiwi IRC. Something that stands out during implementation:

No acknowledgement by the server of the command is required other then returning the requested content.

It would be tidier if this was required to send an empty batch if there is no content to be returned. Just so the client doesn't sit there waiting for nothing.

@MuffinMedic

@jwheare The draft/msgid is to allow for usage of the draft/reply tag in servers where multiple users may have the same scrollback content and a user wishes to reply to an older message. The draft/msgid would be that which the server originally sent to clients. If a user replies to a scrollback message, other users with the same history can scrollback as well and see the original message replied to.

@prawnsalad Agreed & updated.

@dequis
Contributor
dequis commented Jan 3, 2017 edited

Thanks for taking the time to write a spec for this!

This should probably be together with the other extensions, not just a batch type. In fact, I don't see a strong reason for this to use a new batch type, if it's just chathistory plus event replay.

Please read the previous discussion on why event replay wasn't added as part of the chathistory batch: #156 - the short version is that the way this spec handles it isn't enough.

to allow for usage of the draft/reply tag

While I agree that sounds desirable and that all messages from history should have an ID (also helps with deduplication), there's no need to make that into a requirement.

@MuffinMedic
MuffinMedic commented Jan 3, 2017 edited

@dequis Do you suggest the extension be implemented as an RPL_ISUPPORT 005 command or as the scrollback capability? The original implementation I had was as a CAP but initial feedback suggested to simply send the 005 numeric instead.

While I agree that sounds desirable and that all messages from history should have an ID (also helps with deduplication), there's no need to make that into a requirement.

Agreed. Updated to make draft/msgid optional but recommended.

@dequis
Contributor
dequis commented Jan 3, 2017

Yeah just an ISUPPORT token is fine for this.

@MuffinMedic MuffinMedic changed the title from Add the SCROLLBACK batch type specification. to Add the SCROLLBACK extension specification. Jan 3, 2017
@prawnsalad

The changes to use chathistory makes it more confusing now. Sending the SCROLLBACK command to get a chathistory batch type now uses 2 different terms to refer to the same thing - scrollback.

Would there be anything stopping us from using a CHATHISTORY command instead to keep things consistent?

extensions/scrollback-3.3.md
+ @batch=ID;draft/msgid=UUID;time=YYYY-MM-DDThh:mm:ss.sssZ :nick!ident@host PRIVMSG target :message
+ @batch=XNyDSitp9MvcX;draft/msgid=eb703092-0782-4c28-bf7c-f9e5c45963ca;time=2016-11-19T18:02:01.001Z :foo!bar@example.com PRIVMSG #channel :The SCROLLBACK specification is going to be fantastic!
+### NOTICE
+ @batch=ID;draft/msgid=UUID;time=YYYY-MM-DDThh:mm:ss.sssZ :nick!ident@host NTOICE target :message
extensions/scrollback-3.3.md
+#### Format
+The `scrollback` content can requested using timestamps:
+
+ SCROLLBACK target timestamp=YYYY-MM-DDThh:mm:ss.sssZ message_count
@SaberUK
SaberUK Jan 3, 2017 Contributor

Wouldn't it be better to make it two parameters rather than using one separated with =?

@SaberUK
SaberUK Jan 3, 2017 Contributor

Also:

  • Can message_count be negative to go backwards?
  • Should the max message_count be in the ISUPPORT token?
  • What numerics are sent if message_count is invalid?
@MuffinMedic
MuffinMedic Jan 3, 2017 edited

Wouldn't it be better to make it two parameters rather than using one separated with =?

No preference either way. Any reason two parameters are better?

Can message_count be negative to go backwards?

Backwards in what sense? Send messages in reverse order or else? What is the use case?

Should the max message_count be in the ISUPPORT token?

Added in c4883c0

What numerics are sent if message_count is invalid?

Oversight and not considered. Suggestions?

extensions/scrollback-3.3.md
+## Current Implementations
+A ZNC-module implementation exists as [znc-scrollback](https://github.com/MuffinMedic/znc-scrollback). Client-side support for the ZNC module is in progress. Interest in supporting the batch type and command without ZNC has also been obtained, although specifics are not available at this time.
+
+[batch]: batch-3.2.md
@SaberUK
SaberUK Jan 3, 2017 Contributor

This should link to the website pages for those specs rather than the markdown documents.

@MuffinMedic

@prawnsalad Fixed in d261046 - Use chathistory as both extension and batch type for consistency.

@SaberUK
Contributor
SaberUK commented Jan 3, 2017 edited

I'm wondering if for the purpose of this specification we might be better off introducing a new 'HISTORY' batch type which works similar to chathistory but for all messages instead of just PRIVMSG/NOTICE. The chathistory batch was only really intended for lightweight stuff like ZNC scrollback and InspIRCd's chathistory module.

@MuffinMedic MuffinMedic changed the title from Add the SCROLLBACK extension specification. to Add the CHATHISTORY extension specification. Jan 3, 2017
@dequis
Contributor
dequis commented Jan 3, 2017

a new 'HISTORY' batch type which works similar to chathistory but for all messages instead of just PRIVMSG/NOTICE

A new batch type will have the same problems the chathistory batch had. Batch types by themselves can't introduce the sort of behavior changes we need to stop (for example) a QUIT from the past making someone quit in the present.

We haven't talked much about how to do the event replay thing, maybe it's worth doing it in a new issue. But the ideas i have in mind could reasonably be added retroactively to chathistory.

@Zarthus
Zarthus commented Jan 4, 2017

I have some serious concerns about how this is going to be properly handled security wise.

I think for bouncers this'll be great, but for an ircd I can think of many ways in which this can be abused, and i'm not sure to which point it would be a violation of privacy and where the line'll be drawn. Logs would be very unlikely to have the state of a channel knowledge.

Some of the below points don't necessarily need to be mentioned in the spec, but I'm curious on how some things would be handled nevertheless.


I would consider getting some numerics for errors (to which point it is relevant to IRCv3): What should happen if I do a /CHATHISTORY #channel-i'm-not-in or /CHATHISTORY #secret-channel? Personally, I think access should be rejected, but the specification gives no guidance on how to do this.


  • User1 sends STATUSMSG(@) to #coolchannel
  • User1 ops User2 in #coolchannel
  • User2 does a /CHATHISTORY #coolchannel

Should they receive the opnotice they were not opped for?
I think it'll be hard to keep track of every single edge case.


  • User1 accidentally pastes their password, email, or address. While they change it, do we want to permit a way to delete chathistory? Should the user be able to do it for their own messages?
    • This doesn't have to be the user themselves, plenty of doxxing attempts happen on IRC.

  • Some channels have confidentiality settings (think +mz, or +qz on charybdis)
  • opped User1 talks with deopped User{2,3,4}
  • Spam stops. -mz gets set.
  • Should User2 be able to request chathistory for the logs of when +mz was enabled?

  • coolserver.coolnework.net splits from hub.coolnetwork.net
  • User{1,2,3} chat on coolserver.* whilst split, User{4,5,6} chat on hub.*
  • coolserver.* rejoins hub.*
  • User1 wants to view the chat on hub.*'s chat, do they have to reconnect to hub to see it?

With that, an optional server parameter might be worth considering. Although with different specifications (like server-time) some sort of resync between messages for split servers could also exist in the future.


  • Services are down. My channel is SET RESTRICTED.
  • User2 joins my channel, and requests a CHATHISTORY.

With the above points, I really think that for a bouncer oriented specification, it'll be great. And while I'd like to keep the option for ircds to implement it, I have a hard time seeing how they would cover some of the problems. I imagine some sort of opt-in mechanic could work, but this comment is already getting quite long and it's a bit too much outside of the IRCv3 realm of influence.

@MuffinMedic MuffinMedic referenced this pull request in ircv3/ircv3-ideas Jan 4, 2017
Open

Guidelines for subcommands and/or numerics #6

@jwheare jwheare modified the milestone: Roadmap Jan 7, 2017
@jwheare jwheare added the ids label Jan 7, 2017
extensions/chathistory-3.3.md
+
+The returned `batch` must not be blank. If no content exists to return, the appopriate error message should be sent instead.
+
+Both the `message_count` and `max_message_count` must be integers greater than 0. The client should not request a `message_count` greater than the `max_message_count` parameter sent in the command. If the `message_count` exceeds the `max_message_count`, server should return a number of lines equal to the `max_message_count`.
@DarthGandalf
DarthGandalf Jan 7, 2017 Member

Move this below the CHATHISTORY Command header.
Before reading syntax of the command message_count and max_message_count don't make sense for the reader.

extensions/chathistory-3.3.md
+
+ @draft/label=ID CHATHISTORY target draft/msgid=ID message_count
+
+If no message_count is known, `*` should be used to specify the default value.
@DarthGandalf
DarthGandalf Jan 7, 2017 Member

What example use case for this do you have in mind?

@MuffinMedic
MuffinMedic Jan 10, 2017

It was a leftover from an earlier version of the draft spec - removed in 78b39f1

extensions/chathistory-3.3.md
+#### Errors
+If the server receives an improperly formatted `CHATHISTORY` command, the `CMD_INVALID` error code should be returned.
+
+If no `chathistory` exists to return, the server should return the appropriate error code. `ACCESS_DENIED` should be sent if the user requests content they do not have permission to view. `NOT_FOUND` should be sent if no content matching the request is found and no `ACCESS_DENIED` error exists.
@DarthGandalf
DarthGandalf Jan 7, 2017 Member

What if client requested 40 lines, but server allows access only to 20 (which happened after client JOINed the channel), should it silently decrease size of batch, or report ACCESS_DENIED somewhere?

@MuffinMedic
MuffinMedic Jan 10, 2017

Addressed in 78b39f1 - the max_message_count should be returned an WARN MAX_MESSAGE_COUNT_EXCEEDED returned to let the client know the value was too high.

extensions/chathistory-3.3.md
+### Examples
+The examples below are written with `draft/msgid` tags included. This tag is optional but recommended.
+
+#### Begin
@DarthGandalf
DarthGandalf Jan 7, 2017 Member

Is it all one example? Why does it have the same msgid for two messages?

@MuffinMedic
MuffinMedic Jan 10, 2017

One example with generic values - the IDs are a simple placeholder, not specific values.

@prawnsalad

The latest changes now forbids an empty BATCH response if there is no content to return, and instead forces an error to be returned. However if I request CHATHISTORY for a channel that has no more content then this isn't an error - there's just no more content. An empty BATCH is more descriptive and easier to handle here.

It now also states that draft/labeled-response is required when this could be easily be optional but recommended such as draft/msgid. Relying on any draft/* specs could hold back any clients wanting to implement CHATHISTORY for some time.

@LFP6
LFP6 commented Feb 13, 2017 edited

@Zarthus While a little belated...

I'm not sure to which point it would be a violation of privacy and where the line'll be drawn

I think this is something best determined by the server. What's appropriate on an internal server is potentially much different than on an open network. I would find this to be true for history as a whole as well (I'd find a channel mode for history to be warranted).

@Zarthus
Zarthus commented Feb 13, 2017

@LFP6, indeed, we covered it further in ircv3/ircv3-ideas#7 if I recall correctly.

+## Notes for implementing work-in-progress version
+This is a work-in-progress specification.
+
+Software implementing this work-in-progress specification MUST NOT use the unprefixed `CHATHISTORY` command. Instead, implementations SHOULD use the `draft/CHATHISTORY` command to be interoperable with other software implementing a compatible work-in-progress version.
@Zarthus
Zarthus Feb 13, 2017

in all caps? None other capabilities are capitalized are they?

@MuffinMedic
MuffinMedic Feb 13, 2017 edited

The extension itself is lowercase. Only the command is capitalized for consistency (CHATHISTORY, PRIVMSG, NOTICE, etc...).

@LFP6
LFP6 commented Feb 13, 2017

Could this be extended so that you can request a certain number of messages to either side of a given timestamp/id? I'm imagining ircv3/ircv3-ideas#9 being implemented in a client so that you can jump back to that point in history (as Slack and Discord do).

@Zarthus
Zarthus commented Feb 13, 2017

Extensions to the spec would probably make more sense as a separate specification, rather than to have one do-it-all spec full of "this is all an optional part of the spec".

@LFP6 LFP6 referenced this pull request Feb 13, 2017
Open

Reaction client tag #289

@MuffinMedic

Adding "x lines before and after" would introduce an unneeded level of complexity as it requires an additional parameter for specify the "direction" of the history. I think a better solution is allowing the message_count to accept both positive and negative integers, where a positive value means "x lines after the timestamp" and a negative number represents "x lines before the timestamp".

A search-type spec could simply request content both before and after. This would give a simpler yet more versatile approach I feel, although I'd like to see what others say before implementing it into the spec.

@LFP6
LFP6 commented Feb 13, 2017

That seems good to me. While less concise, it also means that the server doesn't need to parse through the history nor does the client need to parse through the response to determine what comes before and what comes after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment