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

CreatePassworkSensor: support "character groups" #906

Merged

Conversation

rdowner
Copy link
Contributor

@rdowner rdowner commented Dec 1, 2017

Character groups are groups like "capital letters", "numbers", etc. This option allows a blueprint to require that the generated password contains at least one character from every group.

It is common (e.g. Microsoft default password policy) to see a requirement like "must contain characters from at least 3 groups". Previously, this sensor could not guarantee that this would happen.

Use it like this:

      - type: org.apache.brooklyn.core.sensor.password.CreatePasswordSensor
        brooklyn.config:
          name: my.password
          password.length: 10
          password.character.groups:
           - "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
           - "abcdefghijklmnopqrstuvwxyz"
           - "1234567890"
           - "-_=+[{]};:|`~,<.>/?!@#$%^&*()"

@ahgittin
Copy link
Contributor

ahgittin commented Dec 1, 2017

LGTM.

Is it worth a YAML file illustrating it and a java test case? Probably needs to be in brooklyn-camp, could base on ElectPrimaryTest.testSimpleRebind -- maybe worth checking rebind works and password isn't changed on rebind.

If you think not worth it, I'm happy to merge, but that would be nice to have as "self-documentation".

Character groups are groups like "capital letters", "numbers", etc. This
option allows a blueprint to require that the generated password
contains at least one character from every group.

It is common (e.g. Micrsoft default password policy) to see a requirement
like "must contain characters from at least 3 groups". Previously, this
sensor could not guarantee that this would happen.

Use it like this:

      - type: org.apache.brooklyn.core.sensor.password.CreatePasswordSensor
        brooklyn.config:
          name: my.password
          password.length: 10
          password.character.groups:
           - "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
           - "abcdefghijklmnopqrstuvwxyz"
           - "1234567890"
           - "-_=+[{]};:|`~,<.>/?!@#$%^&*()"
@rdowner rdowner force-pushed the create-password-sensor-character-groups branch from 0bf1520 to d2ecd11 Compare December 1, 2017 15:00
@rdowner
Copy link
Contributor Author

rdowner commented Dec 1, 2017

Thanks for the review @ahgittin. There's already a couple of CreatePasswordSensorTests, so I've added a test for this specific behaviour addition.

@ahgittin
Copy link
Contributor

ahgittin commented Dec 1, 2017

i am finding it really useful where we have yaml examples i can search for, that's why i suggested that, but that's not a requirement so i can't object to this being merged!

curious if you think that's a good idea. (along with being able to embed yaml files from our code when we build docs.)

@rdowner
Copy link
Contributor Author

rdowner commented Dec 1, 2017

I'd be in favour of defining code/doc patterns where we can do this kind of stuff. Initializers are a bit of an odd case - catalog items such as entities, policies etc. are all self-documenting (to a degree; at least it's possible to write the documentation in the code and have it appear in the UI), but initializers tend to fly under the radar - they don't appear in the UI and they don't have the blank spaces to write much documentation. I'd like to see this change!

@aledsage
Copy link
Contributor

aledsage commented Dec 4, 2017

LGTM - merging.

For YAML examples:

  • I like to always include yaml tests if the mapping from yaml to the underlying Java code is non-trivial. In this case it's simple enough.
  • If the dependency order were somehow changed round, so that things could depend on brooklyn-camp and thus yaml-based tests could be written next to the code they relate to, then I think yaml would become much more the default way we write unit tests.
  • Having a yaml example in the docs would certainly be useful (I agree with @rdowner's point about initializers being under-the-radar. Not sure if there's a structure in the docs that makes it obvious where to document new ones. Also the brooklyn.initializers is not an intuitive way to configure things in yaml, so longer term I'd like to see that change.)

@asfgit asfgit merged commit d2ecd11 into apache:master Dec 4, 2017
asfgit pushed a commit that referenced this pull request Dec 4, 2017
@rdowner rdowner deleted the create-password-sensor-character-groups branch December 6, 2017 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants