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

Password hardening test fix #6453

Merged
merged 3 commits into from
Oct 16, 2022

Conversation

davidpil2002
Copy link
Contributor

@davidpil2002 davidpil2002 commented Oct 2, 2022

Description of PR

Summary:
In continue of password-hardening sonic-mgmt tests. Link: #5503
found 2 test issues:

  1. one related to internal loop from fixture when calling function from tests module, fixed by moving the functions involved in the loop to a new module named password_hardening_utils.py
  2. second issue is that the test set an invalid value to the expiration attribute, fixed this test issue by setting a legal value.

Fixes # (issue)
Issue link:
#6428

Type of change

  • [X ] Bug fix
  • Testbed and Framework(new/improvement)
  • [X ] Test case(new/improvement)

Back port request

  • 201911
  • 202012
  • [ X] 202205

Approach

What is the motivation for this PR?

Fix the issue #6428

How did you do it?

explained in the summary.

How did you verify/test it?

Run the test with the sonic-builimage build from branch 202205
command line:
py.test /sonic-mgmt/tests/passw_hardening/test_passw_hardening.py

Any platform specific information?

no

Supported testbed topology if it's a new test case?

no

Documentation

… loop when calling fixtures

Change-Id: I43cf89ad498405521723e7fb164ef46929095b6b
…ng explicity the feature disable

Change-Id: Iea5b589290a7900b0585286ff3ef336f612e1b27
@lgtm-com
Copy link

lgtm-com bot commented Oct 2, 2022

This pull request introduces 1 alert and fixes 3 when merging 3899b9a into a8170d7 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 3 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Oct 4, 2022

This pull request fixes 3 alerts when merging 5533309 into 66aaf8d - view on LGTM.com

fixed alerts:

  • 3 for Unused local variable

@ZhaohuiS
Copy link
Contributor

ZhaohuiS commented Oct 9, 2022

@davidpil2002 could you please add a new fixture to address teardown issue?
Is it possible to disable passw-harden or clean related config whenever case passed or failed, in case of blocking access to testbed and blocking other test cases?

@davidpil2002
Copy link
Contributor Author

@ZhaohuiS This fixture already exist
if you look in the context file, there is a a fixture named clean_passw_policies, and this fixture is cleaning all the configuration done to the default values.
this should cover your concern.

@ZhaohuiS
Copy link
Contributor

@davidpil2002 I am not sure if it's enough, because last time, when I tried to run this script, it raised command error, it configured some commands on the testbed, it reminded me to change the original password, because it's too simple, otherwise, I can't login the testbed. Could you make the clean_passw_policies more robust to handle any exception in case of blocking the access to testbed? Much appreciated!

@davidpil2002
Copy link
Contributor Author

this is meaning that the expiration of the user admin remained 0 and the feature remained enabled as well.
Now, I modified the expiration value in the test, so it will never be 0. So I think it will be ok to approved anyway.

In addition, we can in parallel debug the connection testbed issue.
the testbed connection issue is actually not expected because the fixture clean_passw_policies is calling the function "set_default_passw_hardening_policies" (desc below), and the function is setting default values including disabling the feature to state="disabled".

This not happened to me, can you share a full log, to see if the clean_passw_policies was called?

`def set_default_passw_hardening_policies(duthosts, enum_rand_one_per_hwsku_hostname):
duthost = duthosts[enum_rand_one_per_hwsku_hostname]

passw_hardening_ob_dis = test_passw_hardening.PasswHardening(state='disabled',
passw_hardening_ob_dis = passw_hardening_utils.PasswHardening(state='disabled',
                                        expiration='100',
                                        expiration_warning='15',
                                        history='12',
                                        len_min='8',
                                        reject_user_passw_match='true',
                                        lower_class='true',
                                        upper_class='true',
                                        digit_class="true",
                                        special_class='true')`

@ZhaohuiS
Copy link
Contributor

@davidpil2002 Sure, I will verify it then give you feedback in this week.

Copy link
Contributor

@ZhaohuiS ZhaohuiS left a comment

Choose a reason for hiding this comment

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

Verify on physical testbed, all testcases run to success.

tests/passw_hardening/conftest.py Show resolved Hide resolved
@ZhaohuiS ZhaohuiS merged commit 8cc2eb3 into sonic-net:master Oct 16, 2022
wangxin pushed a commit that referenced this pull request Oct 17, 2022
What is the motivation for this PR?
Fix the issue #6428

How did you do it?
explained in the summary.

How did you verify/test it?
Run the test with the sonic-builimage build from branch 202205
command line:
py.test /sonic-mgmt/tests/passw_hardening/test_passw_hardening.py
allen-xf pushed a commit to allen-xf/sonic-mgmt that referenced this pull request Oct 28, 2022
What is the motivation for this PR?
Fix the issue sonic-net#6428

How did you do it?
explained in the summary.

How did you verify/test it?
Run the test with the sonic-builimage build from branch 202205
command line:
py.test /sonic-mgmt/tests/passw_hardening/test_passw_hardening.py

common_password_diff = [li for li in difflib.ndiff(command_password_stdout, common_password_expected) if
li[0] != ' ']
pytest_assert(len(common_password_diff) == 0, common_password_diff)
Copy link
Contributor

Choose a reason for hiding this comment

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

common_password_diff

The 2nd parameter should be a string, not an array.

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

Successfully merging this pull request may close these issues.

5 participants