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

Disambiguate debug messages of auth backends #254

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

stephane
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #254 (4218013) into master (4625db8) will decrease coverage by 0.0%.
The diff coverage is 80.0%.

❗ Current head 4218013 differs from pull request most recent head 8b8b3f0. Consider uploading reports for the commit 8b8b3f0 to get more accurate results

@@           Coverage Diff            @@
##           master    #254     +/-   ##
========================================
- Coverage    85.4%   85.3%   -0.1%     
========================================
  Files          11      11             
  Lines         556     561      +5     
========================================
+ Hits          475     479      +4     
- Misses         81      82      +1     
Impacted Files Coverage Δ
django_auth_adfs/backend.py 84.0% <50.0%> (ø)
django_auth_adfs/__init__.py 83.3% <83.3%> (-16.7%) ⬇️
django_auth_adfs/config.py 88.0% <100.0%> (ø)

@tim-schilling
Copy link
Member

I believe the reason this is called "authorization code" is because AD passes back the value with for code querystring key that ends up in the access_token parameter in the backend class.

@stephane
Copy link
Contributor Author

I understand, I saw that when I created my PR but I think it's more important to inform the user about the source of the log[1] so he will be able to check it is using the right backend.
Another way would be to prefix the message with the backend name.

BTW some messages are prefixed with django_auth_adfs , it creates duplicates with the proposed logging config ex. django_auth_adfs django_auth_adfs loaded settings from ADFS server.

[1] without using advanced logger settings to output the function name

@tim-schilling
Copy link
Member

Ah, I better understand your use case. I'm good with this change, but I'd like another maintainer to chime in before merging.

Copy link
Member

@JonasKs JonasKs left a comment

Choose a reason for hiding this comment

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

Good catch!

@JonasKs
Copy link
Member

JonasKs commented Aug 31, 2022

BTW some messages are prefixed wit…

I’m okay fixing these too!

@stephane
Copy link
Contributor Author

BTW some messages are prefixed wit…

I’m okay fixing these too!

Do you want an updated PR to include that?

@JonasKs
Copy link
Member

JonasKs commented Aug 31, 2022

Sure, that would be nice😊 If you’d like to update the version in pyproject.toml and __init__.py it makes releasing a new version easier for us too😊

@tim-schilling
Copy link
Member

@stephane does this have all the changes you intended to make?

@stephane
Copy link
Contributor Author

stephane commented Sep 2, 2022

I tried to use metadata to import version from pyproject.toml w/o success (so the duplicate still there).
Anyway I prefer to leave you bump the version number after merge.

@tim-schilling tim-schilling merged commit de92fa1 into snok:master Sep 2, 2022
@tim-schilling
Copy link
Member

Thank you for the PR and edits!

@stephane stephane deleted the patch-1 branch September 2, 2022 16:59
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.

3 participants