Skip to content
This repository has been archived by the owner on Sep 29, 2023. It is now read-only.

Changed info log level to debug log level for debug logs #197

Merged
merged 5 commits into from
Apr 16, 2019
Merged

Changed info log level to debug log level for debug logs #197

merged 5 commits into from
Apr 16, 2019

Conversation

DevSecNinja
Copy link
Contributor

Related to issue #122. I'm only addressing the log entries I think should be debug instead of logging. Not sure about the other logging mentioned in the issue.

@rayluo
Copy link
Collaborator

rayluo commented Mar 20, 2019

Thanks @Cloudenius ! This PR itself looks straightforward. We will take a close look on how many other logs (if any) would also need same treatment.

@DevSecNinja
Copy link
Contributor Author

Thanks @Cloudenius ! This PR itself looks straightforward. We will take a close look on how many other logs (if any) would also need same treatment.

Thanks @rayluo.

Any idea when this PR can be reviewed/approved/merged? Thanks!

@rayluo
Copy link
Collaborator

rayluo commented Apr 1, 2019

@Cloudenius et al: although not (yet?) advertised, we have been working on the cousin of ADAL Python, named MSAL Python since last 6+ months. And we are currently working on a new milestone of it, which will be finished really soon. After that we will come back to take a look at this PR.

PS: In MSAL Python, there is no such info level log in the first place.

@DevSecNinja
Copy link
Contributor Author

DevSecNinja commented Apr 3, 2019

@Cloudenius et al: although not (yet?) advertised, we have been working on the cousin of ADAL Python, named MSAL Python since last 6+ months. And we are currently working on a new milestone of it, which will be finished really soon. After that we will come back to take a look at this PR.

PS: In MSAL Python, there is no such info level log in the first place.

Thanks @rayluo - looking forward to it. As the library you mentioned is still in preview, I'll keep an eye on it. Do note that I've created a PR with Home Assistant to get Azure DNS integrated in their platform by using this library. Being able to address this issue would really help getting the component approved.

@mabeshark
Copy link

@Cloudenius Hi any chance we can squeeze in a modification to the log here and change it to debug also?

self._log.info("Acquiring token with username password.")

It does seem like many of these log statements should be modified. The logging leads the developer to believe it is getting a new token every time, when it may just be returning a cached token. Seems like it should be modified to where if it gets the token from cache it does: self._log.debug("Using token from cache") or if not in cache it logs self._log.debug("Getting token with client credentials.")

Maybe that will be down the road and outside your straight forward pull request

Per request of markmabe: #197 (comment)
@DevSecNinja
Copy link
Contributor Author

@Cloudenius Hi any chance we can squeeze in a modification to the log here and change it to debug also?

azure-activedirectory-library-for-python/adal/token_request.py

Line 266 in 442d859

self._log.info("Acquiring token with username password.")
It does seem like many of these log statements should be modified. The logging leads the developer to believe it is getting a new token every time, when it may just be returning a cached token. Seems like it should be modified to where if it gets the token from cache it does: self._log.debug("Using token from cache") or if not in cache it logs self._log.debug("Getting token with client credentials.")

Maybe that will be down the road and outside your straight forward pull request

Sure, it's in the PR now. :) See commit c988261.

Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

We are merging this now. Please be also aware that this PR is not strictly required to disable ADAL Python logs. Please refer to this conversation for more details.

This PR closes #122. It will be automatically included into our next release.

@rayluo rayluo merged commit c9f3c80 into AzureAD:dev Apr 16, 2019
@DevSecNinja
Copy link
Contributor Author

Thanks @rayluo! 👍 Any idea when the new version will be released?

@rayluo
Copy link
Collaborator

rayluo commented Apr 29, 2019

Yeah I think @abhidnya13 and I can release a patch version 1.2.2 for this one soon.

Meanwhile, I just want to make sure @Cloudenius is aware that, this PR is just a mitigation, but not a full solution. The moment when you would like to enable all the debug logs for YOUR OWN script, all those ADAL debugging log would come back. The ultimate and canonical solution is actually mentioned in this comment inside issue 122.

@DevSecNinja
Copy link
Contributor Author

Yes I understand @rayluo, thanks!

@rayluo rayluo mentioned this pull request Jul 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants