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.

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

Added: Random Passwords #48

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Added: Random Passwords #48

wants to merge 1 commit into from

Conversation

123FLO321
Copy link
Contributor

added random passwords if no password is specified (for Sequence and Single)
added "pl" flag to specify length of random passwords (defaults: random)

Copy link
Owner

@drallieiv drallieiv left a comment

Choose a reason for hiding this comment

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

I would rather have user specify their format and split the char pools into named groups with single char alias that could be grouped in a map

A for uppercase letters
b (or a) for lowercase letters
0 for numbers
+ for symbols
x for anything

the same way I give a format for the username i could give :

xxxxxxxx for a 8 char full random password
AbbAbb0000+ for a 100 chars starting with 6 letters, then 4 numbers then a symbol

Also utility class wise the characters set should be able to be customized (no need to go up to the command line for that).

But as far as core could be used as a depency, I should be able to remove the symbols that cannot be typed easily in my language, avoir similar chars such as 0 and O.

Cool idea but not required :

One step further would be to use strings instead of chars, which would let me use phonetic words in order to make a password that can be said or remembered easily. Much like Japanese romanji does.

@@ -13,6 +13,7 @@
EMAIL("m", "mail", true, "account email address"),
SINGLE_USERNAME("u", "username", true, "account username/login"),
PASSWORD("p", "password", true, "account password"),
PASSWORD_LENGTH("pl", "passwordLength", true, "random account password length"),
Copy link
Owner

Choose a reason for hiding this comment

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

Options key should not overlap each other one should not start like the other.
"p" is contained in "pl"


private int length;

public RandomPasswordGenerator(Integer length) {
Copy link
Owner

Choose a reason for hiding this comment

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

There is no need to take an Integer as paramter, primitive int would be better to avoid null checks

@@ -106,22 +109,36 @@ private static Configuration updateConfiguration(Configuration config, Options o

// 3 types of generation : unique, csv and sequence
Copy link
Owner

Choose a reason for hiding this comment

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

Password generator could also be used with csv input, if the password header is not added in file.

This could be part of another PR.

sequenceGenerator.setUsernamePattern(cmd.getOptionValue(CliOptions.SEQ_ACCOUNTS_FORMAT.shortName));
sequenceGenerator.setNbAccounts(Integer.parseInt(cmd.getOptionValue(CliOptions.SEQ_ACCOUNTS_COUNT.shortName)));
sequenceGenerator.setStartFrom(Integer.parseInt(cmd.getOptionValue(CliOptions.SEQ_ACCOUNTS_START.shortName, "0")));
if (cmd.hasOption(CliOptions.PASSWORD.shortName)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Duplicate code should be joined as one.

PasswordGenerator definition could be done before the "3 types of generation" fork

private static final int MIN_LENGTH = 8;
private static final int MAX_LENGTH = 50;

private static Logger LOGGER = LoggerFactory.getLogger(KinanCityCli.class);
Copy link
Owner

Choose a reason for hiding this comment

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

On other classes I currently use instance loggers :
private Logger logger = LoggerFactory.getLogger(getClass());

LOGGER.warn("Password length must be a number! Using a random length!");
this.length = -1;
}
else if (length < 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

style : else if should be on the same line then the closing bracket of previous if


data = generator.next();
assertThat(data).isNotNull();
assertThat(data.getUsername()).isEqualTo("pref01235suf");
assertThat(data.getPassword().length()).isBetween(8, 50);
Copy link
Owner

Choose a reason for hiding this comment

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

Using the same regexp or calling directly isAccountValid would be better.

https://github.com/drallieiv/KinanCity/blob/develop/ptc-api/src/main/java/com/kinancity/api/PtcSession.java

@123FLO321
Copy link
Contributor Author

123FLO321 commented Jun 6, 2017

How about not adding a now flag but adding these formats to the default password flag.
Maybe: "Pa\xswordxxx" xxx gets replaced and \x stays as x

Chars would only be replaced if the char is inside the char pool.
The char pool is empty be default but can be replaced inside the config file (or remove a # if you just use the preset one.

So by defaults Passwordxxx stays Passwordxxx

Config file default:
#pwPool.A = A,B,C,D,...
#pwPool.a = a,b,c,d,...
#pwPool0 = 0,1,2,3,..
#pwPool.+ = (,),&,%,...
#pwPool.x = all
There you could also add custom ones like this:
pwPool.i = Hello, World, ...

@neskk
Copy link

neskk commented Jun 11, 2017

@123FLO321 any chance you can finish the alterations requested by drallieiv? Having the ability to set a pattern for password generation is completely optional for me but it's a nice feature.
Not having to type a new random password for every set of accounts I am about to create is a great idea.
I would take this a step further and define the "template"/string substitution engine and let it be used both in Password and Username fields.
The country code should also be able to be specified in a command line argument for flexibility.
Random complaint to @drallieiv: for some reason not checking if nickname is available before requesting a Captcha is non-sense to me. To create ~1800 accounts 2 captcha reported around 2600 captchas submitted. As I pointed out in Discord the other day, 30% waste on captchas is expensive and all because Kinan doesn't want to make a simple HTTP request to check if username is available for each account being created causing captchas to be requested and going to waste. I can't believe that the number of username checks an IP can make if inferior to the number of accounts that can be registered and because of this there's no reason that for every account created a username check is made to make sure we don't waste a captcha. I haven't had time to test stuff, been busy with my exams, maybe today I take a look into the code.
Good work @123FLO321 👍

@123FLO321
Copy link
Contributor Author

@neskk Yes, I'll continue working on this soon. And I guess also implementing it into the username.

@drallieiv
Copy link
Owner

It would definately be easier to have 2 distinct parameters for fixed password and random password.
current fixed password would become fixed-password, fp and adding random-password, rpfor the new password format.

As for using "pwPool.i" properties file reading can only be done if the key is known

@drallieiv
Copy link
Owner

@neskk I already explained myself on DM, username check is already implemented on the API but was removed from the process. If you want to put it back, create a separate pull request using it, and prove me wrong.

@123FLO321
Copy link
Contributor Author

@drallieiv I don't see any reason to have 2 separate password parameters since random pw/username would be disabled by default.
So:
-pr (password-random) and/or
-ur (username-random) would enable that feature.

@neskk
Copy link

neskk commented Jun 11, 2017

@123FLO321 I think @drallieiv was thinking when you want to distinguish "pure random" from "pattern random"
My opinion is this: --random-password would not accept any parameter and just be a pure random password based on a fixed pattern (maybe variable length, rolling pattern, etc). --password would accept a pattern or, here's the tricky part, a fixed password. Problem comes when you want to distinguish fixed password/username from pattern one, i.e. how do you know if '123abc+' is meant to be a fixed password or random like '123abc%', '123abc!', etc.?
Maybe we actually need an extra flag to disable pattern substitution so we could input a "fixed" password with symbols that are placeholders in pattern mode.

@123FLO321
Copy link
Contributor Author

@neskk "-pw Password+" would not be replaced by default.
only if --random-password is added.
And even then it only gets replaced if "+" is in the set. If you want a "+" you can type "+".

@neskk
Copy link

neskk commented Jun 11, 2017

@123FLO321 ah ok I think I understood now... what you mean is that these three options exist, right?

  1. -pw format + -random-pass - random password with pattern
  2. -pw literal - fixed password
  3. -random-pass - pure random password

I think it would be cool if username could use the same logic as password to generate random usernames.

@123FLO321
Copy link
Contributor Author

@neskk No only 2 (+pool) new ones: enable random password and enable random username.

@tomballgithub
Copy link

Agree with @neskk that random usernames are needed also to avoid Niantic detection from looking for patterns in future. Currently I am creating random usernames/passwords in excel and importing them into KinanCity.

@drallieiv
Copy link
Owner

If anyone wants to work on that, let me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants