Use password message from /dashboardsinfo #1503

cwperks · 2023-07-06T13:55:33Z

Description

Companion Security PR: opensearch-project/security#2949

This PR uses the message from /_plugins/_security/dashboardsinfo to get the password message from the backend if its configured in opensearch.yml. If the setting is not set, the backend will return the default.

Issues Resolved

#1501

Testing

Jest tests and manual dashboard testing of both password reset and password edit

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.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

DarshitChanpura · 2023-07-06T14:17:58Z

public/apps/configuration/utils/password-edit-panel.tsx

@@ -43,7 +62,7 @@ export function PasswordEditPanel(props: {

  return (
    <>
-      <FormRow headerText="Password" helpText={PASSWORD_INSTRUCTION}>
+      <FormRow data-test-subj="password-form-row" headerText="Password" helpText={passwordHelpText}>


qq. Why add data-test-subj?

I removed that, I was trying to write a test to verify the text after a state update in the password-edit-panel but opted instead to assert that setState with the custom message had been called.

DarshitChanpura · 2023-07-06T14:19:22Z

public/apps/configuration/utils/test/password-edit-panel.test.tsx


  beforeEach(() => {
    useEffect.mockImplementationOnce((f) => f());
    useState.mockImplementation((initialValue) => [initialValue, setState]);
+    (getDashboardsInfo as jest.Mock).mockImplementation(() => {


Is this still need even though it is mocked above?

It is not, I removed this and also removed creating the shallow component in beforeEach and moved it into each individual test. The first test needs mount(...) instead of shallow(...) to test the logic in React.useEffect(...)

DarshitChanpura · 2023-07-06T14:20:04Z

public/apps/configuration/utils/test/password-edit-panel.test.tsx

+  it('renders', (done) => {
+    (getDashboardsInfo as jest.Mock).mockImplementation(() => {
+      return mockDashboardsInfo;
+    });


Is this mock needed?

Looks like this was made redundant by the block above:

jest.mock('../../../../utils/dashboards-info-utils', () => ({ getDashboardsInfo: jest.fn().mockImplementation(() => { return mockDashboardsInfo; }), }));

I removed this redundant mock. Thanks for pointing that out.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

codecov · 2023-07-06T14:33:29Z

Codecov Report

Merging #1503 (c3db9d4) into main (5fd8018) will decrease coverage by 0.06%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main    #1503      +/-   ##
==========================================
- Coverage   66.11%   66.06%   -0.06%     
==========================================
  Files          93       93              
  Lines        2314     2328      +14     
  Branches      310      310              
==========================================
+ Hits         1530     1538       +8     
- Misses        716      722       +6     
  Partials       68       68

Impacted Files	Coverage Δ
...n/panels/internal-user-edit/internal-user-edit.tsx	`80.48% <ø> (ø)`
public/apps/account/password-reset-panel.tsx	`84.61% <28.57%> (-12.26%)`	⬇️
...c/apps/configuration/utils/password-edit-panel.tsx	`95.23% <85.71%> (-4.77%)`	⬇️

Signed-off-by: Craig Perkins <cwperx@amazon.com>

RyanL1997

Hi @cwperks, thank you for this work and it looks good to me in general.

RyanL1997 · 2023-07-10T21:17:19Z

public/apps/account/password-reset-panel.tsx

@@ -32,6 +32,7 @@ import { logout, updateNewPassword } from './utils';
 import { PASSWORD_INSTRUCTION } from '../apps-constants';


For this PASSWORD_INSTRUCTION, should we also update it so that it has the new password rules that introduced by the strong password validation feature?

Just like this password_validation_error_message :

'Password must be minimum 5 characters long and must contain at least one uppercase letter, one lowercase letter, one digit, and one special character.'

That's a good point and needs to be updated on the backend as well. I'll adjust it according to the description on the PR that introduced password strength: opensearch-project/security#2557

plugins.security.restapi.password_min_length - minimum password length, default and minimum is 8

This is already properly set: https://github.com/opensearch-project/security-dashboards-plugin/blob/main/public/apps/apps-constants.tsx

Note: This default is duplicated and the frontend and backend. I decided to keep the value in the frontend, but it could certainly be removed from the frontend codebase in favor of just keeping a single reference on the backend.

Let's do this as a follow up PR to unblock your change.

public/apps/configuration/utils/test/password-edit-panel.test.tsx

* Use password message from /dashboardsinfo Signed-off-by: Craig Perkins <cwperx@amazon.com> * Run eslint --fix Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove unused attribute Signed-off-by: Craig Perkins <cwperx@amazon.com> * Clean up test Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove redundant mock Signed-off-by: Craig Perkins <cwperx@amazon.com> * Removed TODO messages Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> (cherry picked from commit f774ae4)

* Use password message from /dashboardsinfo Signed-off-by: Craig Perkins <cwperx@amazon.com> * Run eslint --fix Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove unused attribute Signed-off-by: Craig Perkins <cwperx@amazon.com> * Clean up test Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove redundant mock Signed-off-by: Craig Perkins <cwperx@amazon.com> * Removed TODO messages Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> (cherry picked from commit f774ae4) Co-authored-by: Craig Perkins <cwperx@amazon.com>

* Use password message from /dashboardsinfo Signed-off-by: Craig Perkins <cwperx@amazon.com> * Run eslint --fix Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove unused attribute Signed-off-by: Craig Perkins <cwperx@amazon.com> * Clean up test Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove redundant mock Signed-off-by: Craig Perkins <cwperx@amazon.com> * Removed TODO messages Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

stephen-crawford · 2023-09-08T14:03:45Z

This went in the 2.9 release not 2.10 so removing the label.

Use password message from /dashboardsinfo

9a2cc26

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks mentioned this pull request Jul 6, 2023

Add password message to /dashboardsinfo endpoint opensearch-project/security#2949

Merged

3 tasks

cwperks marked this pull request as ready for review July 6, 2023 13:55

cwperks requested review from cliu123, DarshitChanpura, davidlago, peternied, RyanL1997 and stephen-crawford as code owners July 6, 2023 13:55

cwperks added 2 commits July 6, 2023 10:09

Run eslint --fix

616f279

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Remove unused attribute

d111e51

Signed-off-by: Craig Perkins <cwperx@amazon.com>

DarshitChanpura reviewed Jul 6, 2023

View reviewed changes

cwperks added 2 commits July 6, 2023 10:29

Clean up test

f3f4148

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Remove redundant mock

6e1162e

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks added the backport 2.x backport to 2.x branch label Jul 6, 2023

Removed TODO messages

d3a6f5e

Signed-off-by: Craig Perkins <cwperx@amazon.com>

stephen-crawford mentioned this pull request Jul 10, 2023

[FEATURE] Use actual password validation message as help text #1501

Closed

RyanL1997 reviewed Jul 10, 2023

View reviewed changes

stephen-crawford approved these changes Jul 11, 2023

View reviewed changes

Merge branch 'main' into password-message

c3db9d4

RyanL1997 approved these changes Jul 11, 2023

View reviewed changes

cwperks merged commit f774ae4 into opensearch-project:main Jul 11, 2023

opensearch-trigger-bot bot mentioned this pull request Jul 11, 2023

[Backport 2.x] Use password message from /dashboardsinfo #1509

Merged

peternied added the v2.10.0 label Aug 9, 2023

stephen-crawford removed the v2.10.0 label Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use password message from /dashboardsinfo #1503

Use password message from /dashboardsinfo #1503

cwperks commented Jul 6, 2023

DarshitChanpura Jul 6, 2023

cwperks Jul 6, 2023

DarshitChanpura Jul 6, 2023

cwperks Jul 6, 2023

DarshitChanpura Jul 6, 2023

cwperks Jul 6, 2023

codecov bot commented Jul 6, 2023 •

edited

Loading

RyanL1997 left a comment

RyanL1997 Jul 10, 2023

RyanL1997 Jul 10, 2023

cwperks Jul 11, 2023

cwperks Jul 11, 2023

stephen-crawford Jul 11, 2023

stephen-crawford commented Sep 8, 2023

		@@ -32,6 +32,7 @@ import { logout, updateNewPassword } from './utils';
		import { PASSWORD_INSTRUCTION } from '../apps-constants';

Use password message from /dashboardsinfo #1503

Use password message from /dashboardsinfo #1503

Conversation

cwperks commented Jul 6, 2023

Description

Category

Issues Resolved

Testing

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 6, 2023 • edited Loading

Codecov Report

RyanL1997 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephen-crawford commented Sep 8, 2023

codecov bot commented Jul 6, 2023 •

edited

Loading