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

Make invalid password message clearer #3057

Merged

Conversation

derek-ho
Copy link
Collaborator

@derek-ho derek-ho commented Jul 26, 2023

Description

Returns the error message instead of the validation message because validation message should already have been shown in above case statements.

Issues Resolved

Fix: #3055

Is this a backport? If so, please add backport PR # and/or commits #

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #3057 (16be66f) into main (bd084c8) will decrease coverage by 0.02%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##               main    #3057      +/-   ##
============================================
- Coverage     62.46%   62.44%   -0.02%     
+ Complexity     3354     3352       -2     
============================================
  Files           254      254              
  Lines         19749    19747       -2     
  Branches       3334     3334              
============================================
- Hits          12337    12332       -5     
+ Misses         5784     5783       -1     
- Partials       1628     1632       +4     
Files Changed Coverage Δ
...curity/dlic/rest/validation/PasswordValidator.java 82.53% <66.66%> (ø)
.../dlic/rest/validation/RequestContentValidator.java 91.61% <100.00%> (-0.10%) ⬇️

... and 6 files with indirect coverage changes

cwperks and others added 2 commits July 29, 2023 16:45
@derek-ho derek-ho force-pushed the change-password-security-message branch from 36a6c4e to 6479513 Compare July 29, 2023 21:26
@derek-ho derek-ho marked this pull request as ready for review July 29, 2023 21:26
@willyborankin
Copy link
Collaborator

willyborankin commented Jul 29, 2023

Checkstyle fails :-). gradle checkstyleMain checkstyleTest should help

Signed-off-by: Derek Ho <[email protected]>
@derek-ho derek-ho force-pushed the change-password-security-message branch 2 times, most recently from 36a6c4e to 2db63a7 Compare July 31, 2023 13:26
willyborankin
willyborankin previously approved these changes Jul 31, 2023
@willyborankin
Copy link
Collaborator

I think test failures are not related to this change.

cwperks
cwperks previously approved these changes Jul 31, 2023
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

I think the previous behavior was better for users to act on, why remove the distinct message?

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

CI failures due to broken tests

@derek-ho derek-ho dismissed stale reviews from cwperks and willyborankin via 19f34c7 August 16, 2023 17:18
peternied
peternied previously approved these changes Aug 16, 2023
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up that code, this makes the documentation align better to how the product behaves.

cwperks
cwperks previously approved these changes Aug 16, 2023
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
@derek-ho derek-ho dismissed stale reviews from cwperks and peternied via 16be66f August 16, 2023 18:59
@cwperks cwperks added the backport 2.x backport to 2.x branch label Aug 17, 2023
@cwperks cwperks merged commit 847f911 into opensearch-project:main Aug 17, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-3057-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 847f91108a4a10c8368fde26a8f79266edf423c3
# Push it to GitHub
git push --set-upstream origin backport/backport-3057-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3057-to-2.x.

@DarshitChanpura
Copy link
Member

@derek-ho Mind creating the backport manually?

derek-ho added a commit to derek-ho/security that referenced this pull request Aug 17, 2023
Returns the error message instead of the validation message because
validation message should already have been shown in above case
statements.

Fix: opensearch-project#3055

Is this a backport? If so, please add backport PR # and/or commits #

[Please provide details of testing done: unit testing, integration
testing and manual testing]

- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Co-authored-by: Craig Perkins <[email protected]>
(cherry picked from commit 847f911)
peternied pushed a commit that referenced this pull request Aug 22, 2023
Backports 847f911 from #3057

Returns the error message instead of the validation message because
validation message should already have been shown in above case
statements.

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Co-authored-by: Craig Perkins <[email protected]>
(cherry picked from commit 847f911)

---------

Signed-off-by: Derek Ho <[email protected]>
@peternied peternied changed the title Change password security message Make invalid password message clearer Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Create user API giving misleading error messages
5 participants