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

Strong password validation. See: #3227

Closed
wants to merge 3 commits into from

Conversation

lwo
Copy link

@lwo lwo commented Aug 1, 2016

First commit for
#3150
Sensitive Data: Complex Passwords

This covers an SQL update statement and API docs as well. Compare the commit with the develop branch to see all additions. I added a dictionary of one word, because it could do with a small discussion in #3150 what exactly is meant here.


2. Pull Request Checklist

  • Functionality completed as described in FRD
  • Dependencies, risks, assumptions in FRD addressed
  • Unit tests completed
  • Deployment requirements identified (e.g., SQL scripts, indexing)
  • Documentation completed
  • All code checkins completed

3. Review Checklist

After the pull request has been submitted, fill out this section.

  • Code review completed or waived
  • Testing requirements completed
  • Usability testing completed or waived
  • Support testing completed or waived
  • Merged with develop branch and resolved conflicts

#3150

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 7.188% when pulling c446440 on IISH:3150-sensitive-data-complex-passwords into 054cafb on IQSS:develop.

@pdurbin
Copy link
Member

pdurbin commented Aug 1, 2016

Thanks for the pull request, @lwo! I just dragged it to the "Code Review" column at https://waffle.io/IQSS/dataverse

You're going on vacation this month, right? Should we try to give you some feedback before you go or after? My initial feedback is that I'm thrilled by all the tests!

@lwo
Copy link
Author

lwo commented Aug 1, 2016

After I think, it starts from now and I am off to sample some restaurants. So I will see it once I get back.

@pdurbin pdurbin self-assigned this Aug 2, 2016
@pdurbin
Copy link
Member

pdurbin commented Aug 2, 2016

@lwo ok! Enjoy the restaurants!

I'm starting to play around with your tests, adding some println debugging so I understand what's going on. Stuff like this:

4 expected errors for password "one potato".
 - error: Password must contain at least 1 uppercase characters.
 - error: Password must contain at least 1 digit characters.
 - error: Password must contain at least 1 special characters.
 - error: Password matches 1 of 4 character rules, but 3 are required.

Looks great! I'll keep playing and digging. All this new code! It feels like Christmas! 🎄

@lwo
Copy link
Author

lwo commented Aug 3, 2016

Ok... And some things flipped into mind whilst consuming a Pied cochon:

Bug: the hard coded settings like PasswordValidatorServiceBean.MIN_LENGTH are supposed to kick in when nothing was configured via VM argument, admin API or the unit test.
But that would never happen... those defaults - if we want them - should move to SystemConfig.

@pdurbin
Copy link
Member

pdurbin commented Aug 3, 2016

@lwo yeah, in hacking around with your code I added a "saneDefault" to getPVNumberOfCharacteristics in SystemConfig.

The unit tests are fantastic but I think we should add some integration tests. I use REST Assured for this. Basically I'm interested in knowing what the out-of-the-box behavior is of a fresh installation of Dataverse. Is a given password complex enough? If not, why not? You've already provided all the data I need. I just need to expose it over an API endpoint so I can exercise a running system. I put stuff like this under the "Admin" API because it's blocked out-of-the-box. Here's a method I threw together:

@Path("testPassword")
@POST
public Response testPassword(String password) {
    final List<String> errors = passwordValidatorService.validate(password);
    if (errors != null) {
        String messageDetail = PasswordValidatorServiceBean.parseMessages(errors);
        return okResponse(Json.createObjectBuilder()
                .add("password", password)
                .add("errors", messageDetail)
        );
    } else {
        return okResponse("password is ok: " + password);
    }
}

Then I exercise this method with REST Assured like this, perhaps in the BuiltinUsersIT file:

@Test
public void testPasswords() {
    String password = "foobar";
    Response response = given().body(password).when().post("/api/admin/testPassword");
    response.prettyPrint();
    // add assertions here
}

Anyway, this is just throwaway code to show how I think of testing the code... test various levels. Unit tests, integration tests, UI tests. If I'm duplicating existing efforts in the pull request, please let me know! After you visit some more restaurants. 😄

For my part I'm planning on turning my attention to other issues (and I'm out quite a bit in August) but I wanted to at least give you a little feedback. The pull request looks great!

@lwo
Copy link
Author

lwo commented Aug 4, 2016

Okiedokie. I wondered myself how to run integration tests, including a test database and such where I can try out the effect of an API settings call..

Lets pick up things at the end of this month or so, to see what needs further tweaking and what not. Maybe an chat over Google Hangouts to go with it. Thanks for the feedback.

- Added a password validation method to the admin API
- Integration test added to BuiltinUsersIT to check the sane defaults of the out of the box installation using said API.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 7.151% when pulling ebbe423 on IISH:3150-sensitive-data-complex-passwords into 054cafb on IQSS:develop.

@pdurbin
Copy link
Member

pdurbin commented Sep 1, 2016

@lwo hi! I'm back from vacation and I'm hoping you are too. I like your idea of another Google Hangout. Let's pick a time (via email or chat) which will motivate me to get back on your branch ahead of time and continue to look through and play with the code! 😄

@pdurbin
Copy link
Member

pdurbin commented Sep 2, 2016

Per #3150 (comment) @lwo and I don't plan to merge this pull request any time soon. However, there is a ton of value in the pull request so we don't want to lose the code @lwo has written!

@pdurbin pdurbin removed their assignment Sep 2, 2016
new Params(0, "Potatoes on my plate with beef", expired, expirationDays, expirationMinLength, 30, maxLength, minLength, dictionary, numberOfCharacters),
new Params(0, "Potatoes on my plate with pie.", expired, expirationDays, expirationMinLength, 30, maxLength, minLength, dictionary, numberOfCharacters),
new Params(0, "Potatoes on a plate .", expired, expirationDays, expirationMinLength, 30, maxLength, minLength, dictionary, numberOfCharacters),
new Params(0, " ", expired, expirationDays, expirationMinLength, 30, maxLength, minLength, dictionary, numberOfCharacters)
Copy link
Member

Choose a reason for hiding this comment

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

20 character password of just spaces.

@pdurbin
Copy link
Member

pdurbin commented Sep 18, 2017

Closing in favor of pull request #4128. Thanks for all this code, @lwo !

@pdurbin pdurbin closed this Sep 18, 2017
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