Fix secure credential loading for nested team-config profiles - #411
Conversation
Signed-off-by: ATorrise <ambertorrise@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #411 +/- ##
==========================================
+ Coverage 84.82% 85.20% +0.38%
==========================================
Files 49 49
Lines 3011 3002 -9
==========================================
+ Hits 2554 2558 +4
+ Misses 457 444 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zFernand0
left a comment
There was a problem hiding this comment.
LGTM! 😋
Very good catch! 🙏
Here is a quick before and after of this fix:
Before
{'rejectUnauthorized': False, 'host': 'lpar.1', 'port': 1234}
HTTP Request has failed with status code 401.
After
{'rejectUnauthorized': False, 'host': 'lpar.1', 'port': 1234, 'user': 'dummy', 'password': 'dummy'}
HTTP Request has failed with status code 401.
I got the 401 because dummy:dummy is not valid on my system
With proper creds:
{'rejectUnauthorized': False, 'host': 'lpar.1', 'port': 1234, 'user': 'REALUSER', 'password': 'PASSWORD'}
//* //FERNAN00 JOB (IZUACCT),FERNANDO,CLASS=B,MSGCLASS=X,TYPRUN=HOLD
...
traeok
left a comment
There was a problem hiding this comment.
I noticed that even with these changes, the failure is never raised from check_missing_props, which is likely why @zFernand0 was seeing a 401 rather than an explicit error. I'd consider it out of scope for this PR, but worth a follow-up issue.
Aside from that, I left a suggestion around the modified code in an attempt to de-duplicate logic.
…into fix/nested-secure-profile-properties Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
- Reduced function complexity, and reuse the `find_profile()` function Co-Authored-by: Claude w/ Model: Sonnet-5/high Modified AI generated code: YES Dev Name: Fernando Rijo Cedeno, Reviewed by: <TBD> Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
0ed44ff to
c9f60d1
Compare
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
|
The missing coverage is just on a line that I fixed a typo 😢 |
There was a problem hiding this comment.
I thoroughly reviewed c9f60d1 in addition to the PR as a whole, no concerns here and code changes LGTM 😋
The only outstanding concern is that patch is at 75% instead of the target 80%, but since its not a required stage for Python SDK I'm not going to hold up the PR over it.
Thanks Fernando!
What It Does
When testing #408 i found that the way i structured my config was returning 401s
ie:
found that
__load_secure_propertieslooked for zosmf as a direct key of the mainframe profile but zosmf actually lives one level deeper, under the profiles key.the fix to config_file.py handles however many levels a config nests
test_custom_file_and_custom_profile_loading_with_nested_profile user/password values came from a plaintext override innested.zowe.config.user.json, not from the vault. So apparently no test ever exercised__load_secure_propertiesfor a profile nested more than one level deep with secure fields on the child! Added a new test (test_nested_profile_with_secure_properties_on_child) that pulls creds from the mocked vaultHow to Test
change your config to match this level of nesting when testing. verify that you get 401s on main and you can run system tests on this branch :)
Review Checklist
I certify that I have:
Additional Comments