-
Notifications
You must be signed in to change notification settings - Fork 295
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
Omitting authentication tokens from the request text while logging to the trace database files #912
base: main
Are you sure you want to change the base?
Conversation
… the trace databases when no_tokens_in_logs setting is set to true
@@ -120,6 +120,7 @@ def to_dict(self, omit_request_text=None): | |||
tags.update(self.tags) | |||
tags.update(self.sequence_tags) | |||
request_text = None if omit_request_text == True else self.request | |||
request_text = logger.remove_tokens_from_logs(request_text) if Settings().no_tokens_in_logs else request_text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Swamy, thanks for the fix!
The check-in tests did not run because we need to migrate them to GitHub actions (this will be done soon), so you will need to run unit tests locally. For this change, you can just run 'test_trace_database*' tests for validation.
The Settings() object can't be used directly here - could you please restructure the code as follows, which should fix the issue:
def to_dict(self, omit_request_text=None, remove_tokens_from_logs=True):
from utils.logger import remove_tokens_from_logs as remove_tokens
tags = {}
...
request_text = None if omit_request_text == True else self.request
request_text = remove_tokens(request_text) if remove_tokens_from_logs else request_text
then just one place in trace_db.py line 202 needs to be updated with the new parameter set to the setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for detailed code suggestions.
Somehow I saw the usage of Settings() in the file and assumed it will work.
I did run unit tests locally and they failed, but I ran all of those and it took some time.
Now the tests are all succeeding.
Is there a place I should add verification in the unit tests to check if auth tokens are actually getting replaced or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
@microsoft-github-policy-service agree company="Microsoft" |
When no_tokens_in_logs is set to true, the RESTler traditional logger detects the authorization tokens in the requests and replaces the same with OMITTED_AUTH_TOKEN. The same is not happening when logging is enabled to trace logger files using the setting use_trace_database.
The changes in this pull request aim to copy the functionality to trace logger and make trace logger also omit the auth headers during logging.