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

Missing user in logs bug #7282

Merged
merged 13 commits into from
Jan 27, 2023
Merged

Conversation

kdamichie
Copy link
Contributor

@kdamichie kdamichie commented Jan 10, 2023

This bug fix logs user when requests are made to Traffic_Ops when different authentication methods are used.


Which Traffic Control components are affected by this PR?

  • Traffic Ops

What is the best way to verify this PR?

To test this change, you will need to test an API call to TO using 3 different authentication methods(mojolicious, access token, and bearer token).

  • Login to TO and obtain mojolicious cookie and access token.
  • Observe access.logs while making next requests
  • Create request with mojolicious cookie.
    • Ex: curl -k --cookie "mojolicious=mojolicious cookie" -H "Content-Type: application/json" "your TO endpoint"
  • Create request with access token.
    • Ex: curl -k --cookie "access_token=access token" -H "Content-Type: application/json" "your TO endpoint"
  • Create request with access token.
    • Ex: curl -k --cookie "Authorization: Bearer access token" -H "Content-Type: application/json" "your TO endpoint"
  • Ensure with each request that in access.logs logs the correct user you logged in with, and does not equal "-"

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

Master

PR submission checklist

@kdamichie kdamichie marked this pull request as ready for review January 11, 2023 15:51
@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Merging #7282 (1eb1f57) into master (0a30f45) will increase coverage by 0.00%.
The diff coverage is 28.57%.

@@            Coverage Diff            @@
##             master    #7282   +/-   ##
=========================================
  Coverage     26.11%   26.11%           
  Complexity       98       98           
=========================================
  Files           648      648           
  Lines         76057    76080   +23     
  Branches         90       90           
=========================================
+ Hits          19862    19872   +10     
- Misses        54362    54373   +11     
- Partials       1833     1835    +2     
Flag Coverage Δ
golib_unit 53.01% <ø> (-0.01%) ⬇️
grove_unit 4.60% <ø> (ø)
t3c_unit 5.32% <ø> (ø)
traffic_monitor_unit 20.43% <ø> (ø)
traffic_ops_integration 69.34% <ø> (ø)
traffic_ops_unit 19.69% <28.57%> (+0.01%) ⬆️
traffic_stats_unit 10.41% <ø> (ø)
unit_tests 22.55% <28.57%> (+<0.01%) ⬆️
v3 57.79% <ø> (ø)
v4 79.09% <ø> (ø)
v5 78.53% <ø> (ø)

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

Impacted Files Coverage Δ
lib/go-rfc/http.go 34.09% <ø> (ø)
traffic_ops/traffic_ops_golang/tocookie/cookie.go 61.81% <ø> (+5.45%) ⬆️
.../traffic_ops_golang/routing/middleware/wrappers.go 62.50% <28.57%> (-3.31%) ⬇️
lib/go-atscfg/serverunknown.go 77.96% <0.00%> (-1.70%) ⬇️

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

@srijeet0406 srijeet0406 marked this pull request as draft January 13, 2023 19:44
…erToken, MojoCookie. Reorged import. Added logging for case where user is not found from cookie.
… unt it test for getCookieToken in wrappers_test.go.
@kdamichie kdamichie marked this pull request as ready for review January 25, 2023 22:14
Copy link
Contributor

@srijeet0406 srijeet0406 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 great! All automated tests pass.
Manual verification passed as well.

@srijeet0406 srijeet0406 merged commit 4ae5117 into apache:master Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants