DCC 2: Electric Boogaloo #172

octylFractal · 2017-01-04T06:40:14Z

Back from the dead, it's DCC support!

This closes #93, and replaces #138.

Things to do:

Receive DCC requests
- Fire event when request is received, regardless of potentially malformed content
- Allow clients to accept/deny requests and actually do something if they call the methods
Fire DCCConnectionClosedEvent :P
Timeout if no response
- Should it be configurable?
Refactor the Netty handling
Get input on all TODOs
Implement DCC Send?

codecov-io · 2017-01-04T06:44:31Z

Current coverage is 32.61% (diff: 0.00%)

Merging #172 into master will decrease coverage by 2.32%

@@             master       #172   diff @@
==========================================
  Files           152        161     +9   
  Lines          3818       4093   +275   
  Methods           0          0          
  Messages          0          0          
  Branches        536        567    +31   
==========================================
+ Hits           1334       1335     +1   
- Misses         2431       2705   +274   
  Partials         53         53

Powered by Codecov. Last update 8fc8af2...1b3150f

kashike · 2017-01-04T10:18:18Z

src/main/java/org/kitteh/irc/client/library/element/DccExchange.java

+/**
+ * Represents an exchange using DCC.
+ */
+public interface DccExchange extends Actor, Closeable {


Should be DCCExchange.

kashike · 2017-01-04T10:18:21Z

src/main/java/org/kitteh/irc/client/library/element/DccChat.java

+
+import javax.annotation.Nonnull;
+
+public interface DccChat extends DccExchange {


Should be DCCChat.

kashike · 2017-01-04T10:18:30Z

src/main/java/org/kitteh/irc/client/library/Client.java

+     * a {@link DccConnectedEvent} will be fired. If the connection fails, a
+     * {@link DccFailedEvent} will be fired.</p>
+     */
+    void requestDccChat(@Nonnull User target);


Should be requestDCCChat.

kashike · 2017-01-04T10:19:03Z

src/main/java/org/kitteh/irc/client/library/element/User.java

+     *
+     * @see Client#requestDccChat(User)
+     */
+    default void requestDccChat() {


Should be requestDCCChat.

kashike · 2017-01-04T10:19:16Z

src/main/java/org/kitteh/irc/client/library/event/dcc/DccChatMessageEvent.java

+/**
+ * Fires when a DCC CHAT message is received.
+ */
+public class DccChatMessageEvent extends ActorMessageEventBase<DccChat> implements ReplyableEvent {


Should be DCCChatMessageEvent.

lol768 · 2017-01-04T14:41:36Z

Any objections if I write some tests?

octylFractal · 2017-01-04T15:56:07Z

No, @lol768.

kashike · 2017-01-04T21:09:59Z

src/main/java/org/kitteh/irc/client/library/event/dcc/DCCRequestEvent.java

+        super(client, originalMessages, actor);
+        Sanity.nullCheck(type, "type cannot be null");
+        Sanity.nullCheck(ip, "ip cannot be null");
+        this.type = type;


this.type = Sanity.nullCheck(type, "type cannot be null");

I'd rather follow the existing style:

https://github.com/KittehOrg/KittehIRCClientLib/blob/master/src/main/java/org/kitteh/irc/client/library/event/client/ClientConnectedEvent.java#L55-L56

https://github.com/KittehOrg/KittehIRCClientLib/blob/master/src/main/java/org/kitteh/irc/client/library/event/abstractbase/ClientEventBase.java#L48

https://github.com/KittehOrg/KittehIRCClientLib/blob/master/src/main/java/org/kitteh/irc/client/library/event/capabilities/CapabilitiesAcknowledgedEvent.java#L58

etc.

I've actually meant to migrate to that style for a while now. ^_^

Then change it on master first and I'll follow up :P

kashike · 2017-01-04T21:10:11Z

src/main/java/org/kitteh/irc/client/library/event/dcc/DCCFailedEvent.java

+    }
+
+    /**
+     * @return the readable reason for the failure


Missing full javadocs.

Incoming DCCs will generate events but cannot be accepted yet.

octylFractal · 2017-01-05T06:32:41Z

@mbax @kashike @lol768 It's mostly done, I just need input on the TODOs and anything else you want cleaned up. DCC is happening!

Zarthus · 2017-01-05T23:35:39Z

src/main/java/org/kitteh/irc/client/library/event/dcc/DCCRequestEvent.java

+        return this.type;
+    }
+
+    public String getIp() {


Might be convenient to have this return an InetAddress

InetSocketAddress now, and dropped getPort.

Zarthus · 2017-01-05T23:37:36Z

src/main/java/org/kitteh/irc/client/library/event/dcc/DCCRequestEvent.java

+    /**
+     * Denies the request.
+     */
+    public abstract void deny();


In my mind, the opposite of accept is reject, and the opposite of deny is allow. It's something to consider, although not hugely relevant.

lol768

My thoughts

lol768 · 2017-01-05T23:34:31Z

src/main/java/org/kitteh/irc/client/library/element/DCCExchange.java

+import java.util.Optional;
+
+/**
+ * Represents an exchange using DCC.


immutable exchange snapshot?

All the other immutable snapshots are written this way. Is there anything special about this that sets it apart?

lol768 · 2017-01-05T23:35:34Z

src/main/java/org/kitteh/irc/client/library/event/dcc/DCCFailedEvent.java

+    /**
+     * Returns the exception that caused the failure.
+     *
+     * @return the exception that caused the failure


When is he present?

When is they present?

lol768 · 2017-01-05T23:35:37Z

src/main/java/org/kitteh/irc/client/library/event/dcc/DCCFailedEvent.java

+    /**
+     * Returns the human-readable reason for the failure.
+     *
+     * @return the human-readable reason for the failure


When is he present?

When is they present?

They is always present.

lol768 · 2017-01-05T23:37:42Z

src/main/java/org/kitteh/irc/client/library/implementation/EventListener.java

@@ -996,6 +1002,69 @@ public void ctcp(ClientReceiveCommandEvent event) {
        }
    }

+    private void handleDCCEvent(User user, List<ServerMessage> originalMessages, List<String> parameters) {
+        // TODO should unknown requests or improperly formatted requests get logged? just to clarify to the user that they are still noticed


We throw server exceptions for invalid server messages.. could do the same here but not sure since it's a user originating message..

Not really user but client originating. Still could be maliciously crafted easily.

Discussed on IRC.

lol768 · 2017-01-05T23:39:36Z

src/main/java/org/kitteh/irc/client/library/util/IPUtil.java

+        return addressLong.toString();
+    }
+
+    private static final BigInteger X_FFFF = BigInteger.valueOf(0xFFFF);


Might be better up top?

Zarthus · 2017-01-05T23:41:13Z

src/main/java/org/kitteh/irc/client/library/implementation/EventListener.java

@@ -996,6 +1002,69 @@ public void ctcp(ClientReceiveCommandEvent event) {
        }
    }

+    private void handleDCCEvent(User user, List<ServerMessage> originalMessages, List<String> parameters) {
+        // TODO should unknown requests or improperly formatted requests get logged? just to clarify to the user that they are still noticed


logged in the background? yes. Silently dropping things is bad. will also allow covering for possible exploit attempts

lol768 · 2017-01-06T10:22:54Z

src/main/java/org/kitteh/irc/client/library/util/IPUtil.java

+
+public final class IPUtil {
+    private static final BigInteger X_FFFF = BigInteger.valueOf(0xFFFF);
+    private static final int LENGTH_OF_LARGEST_IPV6_INTEGER = BigInteger.valueOf(2).pow(128).toString().length();


IMO this is missing a -1

I'm not sure I follow?

ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff is
340282366920938463463374607431768211455
2^128 is
340282366920938463463374607431768211456

But that is, of course, not relevant. They're both the same length, which is 39.

Yea, I don't really think that matters too much. We could even just hardcode the 39 and add a comment 😛

It's now just a 39.

kashike · 2017-01-07T01:26:36Z

src/main/java/org/kitteh/irc/client/library/element/DCCChat.java

+
+import javax.annotation.Nonnull;
+
+public interface DCCChat extends DCCExchange {


Missing javadocs.

kashike · 2017-01-08T23:14:39Z

src/main/java/org/kitteh/irc/client/library/util/IPUtil.java

+     * @param ipInteger the IP integer string
+     * @return the corresponding IP address as an InetAddress instance
+     */
+    public static InetAddress getInetAdressFromIntString(String ipInteger) {


Missing a d in address.
getInetAdressFromIntString
getInetAddressFromIntString

Zarthus · 2017-01-09T00:56:01Z

2017 is off to a great start

mbax · 2017-03-01T01:21:44Z

Closing this PR to give the dcc branch's PR the spotlight. #181

kashike suggested changes Jan 4, 2017

View reviewed changes

mbax added DCC IS TERRIBLE proposal work in progress [MINOR] new API / feature labels Jan 5, 2017

mbax assigned octylFractal Jan 5, 2017

mbax added this to the 3.1.0 milestone Jan 5, 2017

octylFractal added 6 commits January 4, 2017 19:54

Support making outgoing DCC CHAT calls

ed72b86

Incoming DCCs will generate events but cannot be accepted yet.

Git didn't catch renames because case-insensitive FS

cd5c959

Small cleanup

b72e2fe

Fix license

a397d54

capslock

f5c6c98

Git didn't catch renames because case-insensitive FS

de2f8e6

octylFractal force-pushed the feature/dcc branch from 4996261 to de2f8e6 Compare January 5, 2017 03:54

octylFractal added 8 commits January 4, 2017 20:53

Support receiving DCC requests and accepting/denying them

a314ada

Send IP in the right format

3514a49

Allow identification of DCC by user-nick

9dc955c

Fix Sanity checks

387eb2e

Add IPv6 support

4f0e999

Off-by-one errors are your friend

6135ead

Really support IPv6

e6f2291

Un-wrap IPv6 address that contain an IPv4 address

bc27b07

Zarthus reviewed Jan 5, 2017

View reviewed changes

AyeNetSocketAddress

636aba4

lol768 reviewed Jan 5, 2017

View reviewed changes

Zarthus reviewed Jan 5, 2017

View reviewed changes

octylFractal added 3 commits January 5, 2017 15:56

lol fixes

3802380

Log DCC failures, improve robustness

bac144d

Fix v

b296564

lol768 suggested changes Jan 6, 2017

View reviewed changes

octylFractal added 3 commits January 6, 2017 13:52

Use a constant for the length of an IPv6 integer

82295a4

Accept/reject your fate

ff5244e

Aquire the outside address in a better way

a125306

kashike suggested changes Jan 8, 2017

View reviewed changes

octylFractal added 5 commits January 8, 2017 16:31

Comply with requests.

d95c111

whitespace.delete()

288779a

public -f DCCRequestEvent

ef8a26e

public getType, getService

769076a

dumbell standards

1b3150f

octylFractal changed the title ~~[WIP] DCC 2: Electric Boogaloo~~ DCC 2: Electric Boogaloo Jan 9, 2017

mbax added DCC IS TRASH and removed work in progress labels Jan 9, 2017

mbax removed the proposal label Jan 14, 2017

mbax closed this Mar 1, 2017

mbax removed this from the 3.1.0 milestone May 28, 2017


		import javax.annotation.Nonnull;

		public interface DccChat extends DccExchange {

DCC 2: Electric Boogaloo #172

DCC 2: Electric Boogaloo #172

Conversation

octylFractal commented Jan 4, 2017 • edited Loading

codecov-io commented Jan 4, 2017 • edited Loading

Current coverage is 32.61% (diff: 0.00%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lol768 commented Jan 4, 2017

octylFractal commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octylFractal commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lol768 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lol768 Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zarthus Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbax Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zarthus commented Jan 9, 2017

mbax commented Mar 1, 2017

octylFractal commented Jan 4, 2017 •

edited

Loading

codecov-io commented Jan 4, 2017 •

edited

Loading

lol768 Jan 5, 2017 •

edited

Loading

Zarthus Jan 5, 2017 •

edited

Loading

mbax Jan 6, 2017 •

edited

Loading