singingwolfboy / flask-dance

Doing the OAuth dance with style using Flask, requests, and oauthlib.
https://pypi.python.org/pypi/Flask-Dance/
MIT License
1.01k stars 158 forks source link

Send specific kwargs to Oauth2CustomerBlueprint #353

Closed willpwa closed 3 years ago

willpwa commented 3 years ago

Due to the additional setup in this function, it is preferable to use make_gitlab_blueprint instead of creating a Oauth2CustomerBlueprint directly. However, in the event of a self-hosted gitlab instance using self-signed certificates, these certificates cannot always be verified and this makes the redirection result in an SSL verification error. Thus, one is unable to use make_gitlab_blueprint and must instead use the "raw" Oauth2CustomerBlueprint in order to pass token_url_params=dict(verify=False).

This change is to add the ability to forward kwargs from make_gitlab_blueprint into Oauth2CustomerBlueprint for more customisationof functionality.

codecov[bot] commented 3 years ago

Codecov Report

Merging #353 (291e0c8) into main (0d5507a) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 291e0c8 differs from pull request most recent head 3ab2608. Consider uploading reports for the commit 3ab2608 to get more accurate results Impacted file tree graph

@@           Coverage Diff           @@
##             main     #353   +/-   ##
=======================================
  Coverage   93.49%   93.49%           
=======================================
  Files          33       33           
  Lines        1014     1014           
=======================================
  Hits          948      948           
  Misses         66       66           
Impacted Files Coverage Δ
flask_dance/contrib/gitlab.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0d5507a...3ab2608. Read the comment docs.

singingwolfboy commented 3 years ago

I would rather that we solve specific problems in these pre-set configurations, rather than operating as a catch-all where the developer has to know all the details of the Oauth2CustomerBlueprint in order to understand how to call make_gitlab_blueprint(). If your specific problem is "I need to turn off TLS verification because my GitLab server doesn't have a verifiable TLS certificate", then we can work with that!

Rather than a catch-all **kwargs arg, I suggest that you add an argument to make_gitlab_blueprint() named something like verify. We can then pass that argument down to all the places that it might be needed, including the token_url_params. We do a similar kind of thing in the Google configuration, with the arguments offline, reprompt_consent, and hosted_domain.

Please make sure to update the docstring for these arguments, and add an entry in the changelog as well.

willpwa commented 3 years ago

Is there a way to add a word to that spellchecker's dictionary? It is complaining that Gitlab is not a word.

singingwolfboy commented 3 years ago

Is there a way to add a word to that spellchecker's dictionary? It is complaining that Gitlab is not a word.

Yep! Add it to docs/spelling_wordlist.txt. You probably want to spell it "GitLab", with a capital L.

singingwolfboy commented 3 years ago

Oh, one more thing I forgot -- please also add an automated test for this. You'll probably want to use the responses library to mock out the actual HTTP request, and then assert that verify=False was passed to it. Once we've got that, I think this will be ready to merge!

willpwa commented 3 years ago

Unfortuantely, as far as I can tell, the Responses library can only check the returned object from a requests call. We instead want to check the parameters and this requires mocking.

I've got a test made but it doesn't test the actual request method, just that the variable that gets passed into the blueprint. If the test were to be improved, requests.sessions.Session.request would need to be mocked so that its parameters can be asserted.

The problem is that I don't know the flask-dance library well enough to be able to get that part of the code to execute in a testing environment and as such, have been unable to add it.

singingwolfboy commented 3 years ago

Close enough! I'll merge this as-is, and see if I can fiddle with the test a bit to make it better. Thanks a lot!