Dev log user #263
Dev log user #263
Conversation
tabpy-server/tabpy_server/app/app.py
Outdated
| 'keyfile': self.settings[SettingsParameters.KeyFile] | ||
| }) | ||
| else: | ||
| log_and_raise('Unsupported transfer protocol.', RuntimeError) |
There was a problem hiding this comment.
nit - we should log the transfer protocol in use here
There was a problem hiding this comment.
Good idea! Added protocol to the message.
| ConfigParameters.TABPY_LOG_DETAILS, | ||
| default_val='false') | ||
| self.settings[SettingsParameters.LogRequestContext] = ( | ||
| self.settings[SettingsParameters.LogRequestContext].lower() != |
There was a problem hiding this comment.
It may be me, but this might read better if it was
self.settings[SettingsParameters.LogRequestContext] = (
self.settings[SettingsParameters.LogRequestContext].lower() ==
'true')
There was a problem hiding this comment.
I thought about this and implemented it for the cases where the setting is anything but 'false', e.g. 'enable' or 'on'. However, I still need to updated documentation for the new setting so changing the logic as well.
| return | ||
|
|
||
| if protocol != 'https': | ||
| log_and_raise('Unsupported transfer protocol: {}.'.format( |
There was a problem hiding this comment.
this looks like dead code after running the first check in this method.
There was a problem hiding this comment.
Check above for HTTP and this one is for HTTPS
|
|
||
| protocol = self.settings['transfer_protocol'] | ||
| protocol = self.settings[SettingsParameters.TransferProtocol] | ||
|
|
There was a problem hiding this comment.
we should log the protocol being set here
| str | ||
| Name of authentication method used by client. | ||
| If empty no authentication required. |
There was a problem hiding this comment.
these return value comments need clarification. For instance, we can return True, empty string - that means the auth method is found, but not used?
There was a problem hiding this comment.
With (True, '') it means there's no error in processing request and auth is not needed. Updating the documentation.
|
@nmannheimer Could you check documentation updates on this change? |
|
Discussed all the @jnegara feedback with her, reached an agreement - no changes there :) |
| For extended logging (e.g. for auditing purposes) additional logging can be turned | ||
| on with setting `TABPY_LOG_DETAILS` configuration file parameter to `true`. | ||
|
|
||
| With the feature on additional information is logged for HTTP requests: caller ip, |
There was a problem hiding this comment.
I think it would be useful to say specifically that the client is Tableau/Tableau Server to make users aware that this will log usernames from Tableau Server.
There was a problem hiding this comment.
Okay, I'll update the text together with the example below.
|
|
||
| ### Request Context Logging | ||
|
|
||
| For extended logging (e.g. for auditing purposes) additional logging can be turned |
There was a problem hiding this comment.
I would maybe say "additional logging details"
Add new config param to turn on logging of request context: caller ip, client info, Tableau user name, TabPy user name.