Skip to content

Conversation

@XomaDev
Copy link
Contributor

@XomaDev XomaDev commented May 6, 2024

I've been working on extending auth mode, to support dynamic username/password authentication, with some refactoring.

    new SocksServer(determinePort(args))
        .setAuthenticator(new UsernamePasswordAuthenticator(false) {
          @Override
          public boolean validate(String username, String password) {
            return username.equals("mysecureusername") && password.equals("mysecurepassword");
          }
        }).start();

So far it is working good, I've tried with multiple clients.

@XomaDev
Copy link
Contributor Author

XomaDev commented May 6, 2024

This update modifies the constructors, to effectively pass configuration through constructors, rather than methods. So anyone after updating to this code, will have to do a small refactor.

@XomaDev XomaDev changed the title [Draft] Extend Authentication Modes Extend Authentication Modes May 6, 2024
@bbottema
Copy link
Owner

Hey, thanks you so much for helping out!

This update modifies the constructors, to effectively pass configuration through constructors, rather than methods. So anyone after updating to this code, will have to do a small refactor.

Is this still the case? If so, can't you provide a default constructor for backwards compatibility?

Also, in the README changes, you removed the line of stopping servers. You also removed the examples for use in junit testing (although I admit I forgot to update this myself, when I switched to JUnit 5). And also, previously it was demonstrated in the documentation that you can start multiple listeners on different ports. I feel this point got lost in your version.

@bbottema bbottema added documentation Improvements or additions to documentation enhancement New feature or request waiting for input labels May 14, 2024
@XomaDev
Copy link
Contributor Author

XomaDev commented May 14, 2024

Is this still the case? If so, can't you provide a default constructor for backwards compatibility?

Hi yes, it is still the case, yes I can provide backwards compatibility, I'll update the code.

Also, in the README changes, you removed the line of stopping servers.

That's right, I'll add those lines.

You also removed the examples for use in junit testing (although I admit I forgot to update this myself, when I switched to JUnit 5).

I'm not exactly sure what I have to add, could you please help me out there?

And also, previously it was demonstrated in the documentation that you can start multiple listeners on different ports. I feel this point got lost in your version.

IMO, letting one single instance start multiple listeners on different port feels wrong. Each instance of SocksServer should only effectively manage one server, along with customization to each individual server.

@bbottema
Copy link
Owner

You also removed the examples for use in junit testing (although I admit I forgot to update this myself, when I switched to JUnit 5).

I'm not exactly sure what I have to add, could you please help me out there?

That's fine, I'll add the examples shortly.

And also, previously it was demonstrated in the documentation that you can start multiple listeners on different ports. I feel this point got lost in your version.

IMO, letting one single instance start multiple listeners on different port feels wrong. Each instance of SocksServer should only effectively manage one server, along with customization to each individual server.

Perhaps, but it is what it is; this is how the library works right now. I'll let the users decide how to use it, but I don't want to hide it by not documenting it.

@XomaDev
Copy link
Contributor Author

XomaDev commented May 14, 2024

Perhaps, but it is what it is; this is how the library works right now. I'll let the users decide how to use it, but I don't want to hide it by not documenting it.

Hmm, so are we keeping this version, or revert back to the old system where each instance can spawn multiple servers?

@bbottema
Copy link
Owner

Perhaps, but it is what it is; this is how the library works right now. I'll let the users decide how to use it, but I don't want to hide it by not documenting it.

Hmm, so are we keeping this version, or revert back to the old system where each instance can spawn multiple servers?

Oh you changed it. I thought you only changed the documentation :D

@bbottema
Copy link
Owner

Alright, I had a quick look. I'm fine with the change.

@XomaDev
Copy link
Contributor Author

XomaDev commented May 14, 2024

I'll edit the code for backwards compatibility, also resolve the conflict.

@XomaDev
Copy link
Contributor Author

XomaDev commented May 15, 2024

@bbottema can you please review? Can you also update the change history and the version?

@bbottema bbottema merged commit d636535 into bbottema:master May 15, 2024
@bbottema bbottema added this to the 3.1.0 milestone May 15, 2024
@bbottema
Copy link
Owner

Released in v4.1.0! Thanks again!

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

Labels

documentation Improvements or additions to documentation enhancement New feature or request waiting for input

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants