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

Implement support for client assertion #458

Closed Exchizz closed 8 months ago

Exchizz commented 3 years ago

Hi, This PR. implements client assertion. The idea is to use certificate/key as an alternative to the client secret.

Description of client assertion: https://docs.microsoft.com/en-us/azure/architecture/multitenant-identity/client-assertion

Assertion format: https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-certificate-credentials (Note, thumbprint is created using SHA1 which Codeclimate complains about)

RFC: https://datatracker.ietf.org/doc/html/rfc7523

I've moved all checks to checks.py which are then loaded from the ready() function. I did this because I read the cert/key files in the ready() method, hence it has to run config-checks before running ready(). Doing like this is suggested in the Django documentation: Lastly, your check function must be registered explicitly with system check registry. Checks should be registered in a file that’s loaded when your application is loaded; for example, in the AppConfig.ready() method https://docs.djangoproject.com/en/3.2/topics/checks/

Codeclimate complains a bit, but I'm not sure how to fix (or ignore) the errors.

AngellusMortis commented 3 years ago

Looks great. Later this week I will try to get Github Actions setup since I never added it before TravisCI shutdown.

AngellusMortis commented 3 years ago

I finally got GHA set up and working. Please sync with master and then fix any CI issues if you would like me to continue with merging the PR.

Exchizz commented 3 years ago

@AngellusMortis There we go. I fixed the merge conflict but codeclimate is still not happy - what should I do about that ?

AngellusMortis commented 3 years ago

I forgot I had to approve runs for others. But Code Climate is basically an extra linter. If you click on the Details link it shows you the linter errors.

AngellusMortis commented 3 years ago

I am also going to have to tweak the secrets to make sure they the CC one can run in PRs. But you differently have some issues from GHA as well. If you just try to get the 3.10/3.2 one to pass minus the last step: https://github.com/AngellusMortis/django_microsoft_auth/runs/4158489151?check_suite_focus=true

AngellusMortis commented 3 years ago

Hopefully got the CC issues fixed if you sync with master again.

AngellusMortis commented 3 years ago

Sorry, because this is your first GH PR, I have to approve every work flow run

AngellusMortis commented 3 years ago

Looks like you had a bad merge. The import of config was moved up to the top of the file for client.py:

https://github.com/AngellusMortis/django_microsoft_auth/blob/master/microsoft_auth/client.py#L17

Exchizz commented 3 years ago

It completes linting and the tests now :).

Do you also want me to fix codeclimate ?

AngellusMortis commented 3 years ago

Fix them if you can. They are all definitely valid, but if they too much of a paint to do, not worries. They are just quality ones.

AngellusMortis commented 3 years ago

It does look like the overall coverage dropped a bit. Can you try to get it back above 90%? Then I will merge it.

Exchizz commented 3 years ago

Yes :)

AngellusMortis commented 3 years ago

It just looks like the code coverage is is a bit dropped. I would like to keep it it above 90% if you can get it back up ~1%. Sorry for all of the trouble and thanks for working through it all to add a new feature!

Exchizz commented 3 years ago

Yeah sure. I'll have a look at it today or tomorrow :)