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

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

Merged
merged 3 commits into from
Aug 8, 2023

Conversation

shamrickus
Copy link
Member

@shamrickus shamrickus commented Aug 3, 2023

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

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

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

@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 a published api but fixing a bug seems like it would be fair game.

@shamrickus
Copy link
Member Author

@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

@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

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
42 of 43 checks passed
cybertunnel pushed a commit to cybertunnel/trafficcontrol that referenced this pull request Aug 16, 2023
…pache#7697)

* Remove priv checks for secure server fields

* Handle api version 4 also

* Forgot a file + changelog

---------

Co-authored-by: Steve Hamrick <shamrick@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants