6
\$\begingroup\$

On my free time, I was working on a system that could help some little private companies, like game servers, etc.

The last few days I have been working in such small companies, and I noticed that some of them give direct access to their developers to update game servers, restart them etc., which is insecure, and some does not give access and you have to tell them to manually restart it, which is really annoying.

I thought of creating a middle-man server, you create an user for your developer and give him permissions on actions e.g restart, pull from Git, shut down, etc.

And the user performs these given actions to the remote-server and the remote server commands the main server of these actions, this way the actions are secure and he has no permissions to other stuff, just the given ones.

I started this Netty application, and I am just wondering if I did things the right way, so I'll start with my bootstrapping:

public class RemoteServerBootstrap extends ServerBootstrap {

    /**
     * Event worker
     */
    private EventLoopGroup worker = new NioEventLoopGroup();

    public RemoteServerBootstrap() {
        group(this.worker);
        channel(NioServerSocketChannel.class);
        option(ChannelOption.SO_BACKLOG, 100);
        childHandler(pipeline());

        ChannelFuture future = bind(Configuration.NETWORK_PORT);

        try {
            future.sync();
            System.err.println("Remote Management Server has started on port " + Configuration.NETWORK_PORT);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }

    private ChannelPipeline pipeline() {
        return new ChannelPipeline(new Decoder(new ServerPacketListener()), new Encoder());
    }

}

This is where my server is initialized.

And this is how I manage my clients:

/**
 * @author Jony | Ben
 */
public class ConnectionManager {

    /**
     * The list of client connections
     */
    private List<Connection> connections = new ArrayList<>();

    public void register(Channel c) {
        Connection connection = new Connection(c);
        connections.add(new Connection(c));

        if (!connection.getAuth().authenticated()) {
            connection.getAuth().sendAuthenticationStateMessage();
            System.err.println("New connection registered " + c.remoteAddress() + " | Ready to authenticate");
            return;
        }

        System.err.println("New connection registered " + c.remoteAddress() + " | Authenticated");
    }

    /**
     * Gets the connection instance by given channel
     * @param c The given channel
     * @return The connection's instance
     */
    public Connection get(Channel c) {
        return connections.stream().filter(conn -> conn.equals(c)).findFirst().orElse(null);
    }
}

And then I have an encoder and a decoder:

/**
 * @author Jony | Ben
 */

public class Encoder extends ChannelOutboundHandlerAdapter {

    @Override
    public void write(ChannelHandlerContext ctx, Object o, ChannelPromise p) throws java.lang.Exception {
        System.out.println("sent");
        super.write(ctx, o, p);
    }

}

/**
 * @author Jony | Ben
 */

public class Decoder extends ChannelInboundHandlerAdapter {

    private PacketListener receiver;

    public Decoder(PacketListener receiver) {
        this.receiver = receiver;
    }


    @Override
    public void channelRegistered(ChannelHandlerContext ctx) {
        receiver.connectionRegistered(ctx);
    }

    @Override
    public void channelRead(ChannelHandlerContext ctx, Object data) {
        if (data instanceof ByteBuf) {
            ByteBuf buffer = (ByteBuf) data;

            byte opcode = buffer.readByte();

            Packet packet = new Packet(opcode, buffer);

            receiver.packetReceived(ctx, packet);
        }
    }
}

And a packet receiver which executes packets by given OPCODE:

/**
 * @author Jony | Ben
 */
public class ServerPacketListener extends PacketListener<ServerPacketExecutor> {

    private ConnectionManager connection = new ConnectionManager();

    public ServerPacketListener() {
        super(new ServerPacketExecutor[255]);
        register(1, new AuthenticationInput());

    }

    @Override
    public void connectionRegistered(ChannelHandlerContext ctx) {
        connection.register(ctx.channel());
    }

    @Override
    public void packetReceived(ChannelHandlerContext context, Packet p) {
        ServerPacketExecutor packetExecutor = getPacket(p.getOpcode());
        if (packetExecutor != null) {
            Connection conn = connection.get(context.channel());
            if (conn != null) {
                packetExecutor.execute(conn, p);
            }
            else {
                System.err.println("Connection was not found from address " + context.channel().remoteAddress() + " with OPCODE ID: " + p.getOpcode());
            }
        }
        else {
            System.err.println("Invalid Opcode sent from " + context.channel().remoteAddress() + " OPCODE ID: " + p.getOpcode());
        }
    }
}

You can find the full repository here.

It's not finished, not even close, but I'm just trying to finish the net base. What do you think of my network approach? Is there something I could improve?

\$\endgroup\$

1 Answer 1

6
+200
\$\begingroup\$

Using System.err is "bad" practice

It's bad practice if you use it to log errors and others things to your users. See this question on SO and this question on Progammers to have more information. For a small project it does not matter much, but since you want to use this application for users, I recommend that you get a logging framework. It will be more flexible and will fit the use you currently have for all your System.err calls.

Returning quickly is good, normally

But here it's not necessary!

    if (!connection.getAuth().authenticated()) {
        connection.getAuth().sendAuthenticationStateMessage();
        System.err.println("New connection registered " + c.remoteAddress() + " | Ready to authenticate");
        return;
    }

    System.err.println("New connection registered " + c.remoteAddress() + " | Authenticated");

You're returning early, since if the user was not authenticated you don't want to execute System.err.println("New connection registered " + c.remoteAddress() + " | Authenticated");. That's the wrong way to do it, just use an else block like so :

    if (!connection.getAuth().authenticated()) {
        connection.getAuth().sendAuthenticationStateMessage();
        System.err.println("New connection registered " + c.remoteAddress() + " | Ready to authenticate");
    } 
     else {
        System.err.println("New connection registered " + c.remoteAddress() + " | Authenticated");
    }

Write accurate documentation

This is one thing that is important. Most of the time, I don't read the code of the library I use, instead I read the documentation. I want to have accurate information and I don't want surprise! Take the method public Connection get(Channel c), the Javadoc is not expressing what really is going on :

  /**
     * Gets the connection instance by given channel
     * @param c The given channel
     * @return The connection's instance
     */

The things is, your code return null if the Channel is not registered in your list. That should be in your Javadoc! This is important! I have no mention that you are returning null, so I could get NullPointerException easily!

By the way, you could simply call List.contains() instead of filtering with equals.

Use exception

At some point, you may want to throw exception, instead of using System.err to notify the caller that some things has gone wrong. Take this piece of code for example :

        if (conn != null) {
            packetExecutor.execute(conn, p);
        }
        else {
            System.err.println("Connection was not found from address " + context.channel().remoteAddress() + " with OPCODE ID: " p.getOpcode());
        }

The caller code of this method has no idea that the packet received is from a connection not registered. I'm not used to the library you're using, so maybe throwing an exception could break the flow of the calling method, but some time it's fine to do :

throw new UnregistredExcetpion("Connection was not found from address " + context.channel().remoteAddress() + " with OPCODE ID: " + p.getOpcode());

Where UnregistredExcetpion would be an Exception of your own. The thing is the caller is notified that something has gone wrong.

If you do not want to stop the execution, than don't mind this for this particular method, but keep in mind that you have this option.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.