Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

TO: Only display Server ILO/XMPP Passwords Based on New Permission #7697

Merged
zrhoffman merged 3 commits into from
Aug 8, 2023
Merged

TO: Only display Server ILO/XMPP Passwords Based on New Permission #7697

zrhoffman merged 3 commits into from
Aug 8, 2023

Conversation

@shamrickus
Copy link
Member

@shamrickus shamrickus commented Aug 3, 2023 •
edited
Loading

Currently, the servers endpoints will display the xmppPasswd/iloPassword based only on privlevel. In api v5/v4 this has been changed to check for a new permission SECURE-SERVER:READ instead (if enabled).

Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Ops

What is the best way to verify this PR?

Confirm the TO Tests work. For version api version 3/4 and role_based_permissions is off confirm that Ops/Admin PrivLevel is required to view the two password fields. For Version 4, confirm that with role_based_permissions set, that only users that are admin or have the SECURE-SERVER:READ permission can view the password fields. For version 5, it should only check for the permission.

If this is a bugfix, which Traffic Control versions contained the bug?

  • master
  • 7.0.1

PR submission checklist

@shamrickus shamrickus added bug something isn't working as intended Traffic Ops related to Traffic Ops low impact affects only a small portion of a CDN, and cannot itself break one permissions Dealing with roles/permissions/tenancy labels Aug 3, 2023
@codecov
Copy link

codecov bot commented Aug 3, 2023 •
edited
Loading

Codecov Report

Merging #7697 (2372b21) into master (a52bcd8) will increase coverage by 2.83%.
The diff coverage is 20.00%.

@@             Coverage Diff              @@
##             master    #7697      +/-   ##
============================================
+ Coverage     26.95%   29.79%   +2.83%     
  Complexity       98       98              
============================================
  Files           686      801     +115     
  Lines         80539    85760    +5221     
  Branches         90      952     +862     
============================================
+ Hits          21707    25549    +3842     
- Misses        56760    58071    +1311     
- Partials       2072     2140      +68
Flag Coverage Δ
golib_unit 48.10% <ø> (ø)
grove_unit 4.60% <ø> (ø)
t3c_unit 4.95% <ø> (ø)
traffic_monitor_unit 21.30% <ø> (ø)
traffic_ops_integration 69.39% <ø> (ø)
traffic_ops_unit 22.28% <20.00%> (-0.01%) ⬇️
traffic_portal_v2 73.65% <ø> (?)
traffic_stats_unit 10.14% <ø> (ø)
unit_tests 26.94% <20.00%> (+3.29%) ⬆️
v3 57.79% <ø> (ø)
v4 79.18% <ø> (ø)
v5 78.52% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
traffic_ops/traffic_ops_golang/server/servers.go 27.34% <20.00%> (-0.09%) ⬇️

... and 115 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mitchell852
Copy link
Member

mitchell852 commented Aug 7, 2023

@shamrickus can you also create the SECURE-SERVER:READ permission via a database migration?

@mitchell852
Copy link
Member

mitchell852 commented Aug 7, 2023 •
edited
Loading

@shamrickus is there a reason this can't be "fixed" in api v3 and v4? i know we don't like to change behavior of a published api but fixing a bug seems like it would be fair game.

@shamrickus
Copy link
Member Author

shamrickus commented Aug 7, 2023

@shamrickus is there a reason this can't be "fixed" in api v3 and v4? i know we don't like to change behavior of an api but fixing a bug seems like it would be fair game.

API v3 doesn't have the new role based permissions and this fixes it in v4.

@mitchell852
Copy link
Member

mitchell852 commented Aug 7, 2023

@shamrickus is there a reason this can't be "fixed" in api v3 and v4? i know we don't like to change behavior of an api but fixing a bug seems like it would be fair game.

API v3 doesn't have the new role based permissions and this fixes it in v4.

oh ok. your PR description only mentioned v5

kdamichie
kdamichie approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@kdamichie kdamichie left a comment

Choose a reason for hiding this comment

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

Code looks good, and tested functionality

@zrhoffman zrhoffman merged commit 849d166 into apache:master Aug 8, 2023
cybertunnel pushed a commit to cybertunnel/trafficcontrol that referenced this pull request Aug 16, 2023
@shamrickus
TO: Only display Server ILO/XMPP Passwords Based on New Permission (a…
00890bd 
…pache#7697)

* Remove priv checks for secure server fields

* Handle api version 4 also

* Forgot a file + changelog

---------

Co-authored-by: Steve Hamrick <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@kdamichie kdamichie kdamichie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug something isn't working as intended low impact affects only a small portion of a CDN, and cannot itself break one permissions Dealing with roles/permissions/tenancy Traffic Ops related to Traffic Ops

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shamrickus @mitchell852 @kdamichie @zrhoffman