AngellusMortis / django_microsoft_auth

Simple app to enable Microsoft Account, Office 365 and Xbox Live authentcation as a Django authentcation backend.
MIT License
137 stars 84 forks source link

Instantiate custom auth config class #483

Closed 7oi closed 2 years ago

7oi commented 2 years ago

I was excited to see the option for custom config classes when browsing through the code (it should be documented more) as I'm working on a multi tenant django project in which settings are set via tenant, rather than all in settings.py.

But when experimenting with this I noticed that the custom config class is never instantiated, and therefor fails.

This should remedy that.

7oi commented 2 years ago

I see. That seems a bit confusing, honestly. One would assume that a property called something something class would be an uninstantiated class. Also it doesn’t seem to be consistent with how the default class is instantiated within the init_config method.

Then I may need to reconsider using this module… Well, or perhaps override the getattr method of the custom conf class so it can retrieve the properties for the tenant set to the current thread, as I cannot rely on properties of a class instantiated on compile. I’ll put on the labcoat and test some things out.

AngellusMortis commented 2 years ago

I mean, you can use a different module if you want, I do not care. But this PR as is creating a breaking change.

Adding support for both is really easy, you just need to check if the object is callable (has not be initialized yet) and then initialize it. That way if a pre-initialized one is already passed in, it does not break anything. It is a pattern that Django uses very often.

7oi commented 2 years ago

Sorry, I may have sounded like I was being a Karen of sorts with the whole “I may have to not use this”. I was just thinking out loud, really. I tend to do that. I did not mean it to sound like a Karen expecting everyone and their manager to bend over backwards for their needs.

But I see that I just misunderstood how this was supposed to be used. You raise a good point with testing if the object is callable. That would definitely be a good way to make this change non-breaking and fit this other usecase. I’ll look into that tomorrow as it’s family time now. I’ll also add tests for that usecase as not to lower the coverage.

If there’s anything else that I can do to make this more presentable or if there’s something else I’ve missed, please let me know.

7oi commented 2 years ago

I have updated the code so it checks if settings are callable before calling them. I also added tests to ensure code coverage doesn't go down.

AngellusMortis commented 2 years ago

Imports are not sorted correct: https://github.com/AngellusMortis/django_microsoft_auth/actions/runs/3060059303/jobs/4940073296#step:5:14

If you are using VS Code there is a "Format Code" task you can run. Otherwise, you can run isort . in the root directory of the project if you have the dev extra installed.

7oi commented 2 years ago

Ah, I see I have apparently used my own import habit, where I group native, 3rd party and local imports together. Easily fixed. I’ll get right on it.

EDIT: Sigh, too stuck in my own head here. I just realized that this method is the preffered method and certainly not my own.