tornadoweb
Google OAuth2: change oauth config provision from hardcoded to develo…
There is a use case to use 2 different set of google oauth credentials, due to different app branding. I ran into the "HTTP 401: Unauthorized" error, and it was too vague to tell the root causing. After tracing into the tornado.auth code, it turns out that GoogleOAuth2Mixin has hardcoded assumption that the oauth credentials is read from handler.settings['google_oauth']. Two possible solutions:
-
Override GoogleOAuth2Mixin._OAUTH_SETTINGS_KEY. Pros: a simple fix on developers side, and no code change to tornado framework. Cons: the error message is too vague and it can waste another developers hours to realize it.
-
Apply this patch. Pros: fail loud and easier to notice missing puzzle, and it's backwards compatible. Cons: More code changes to tornado framework.
I'd recommend to the #2 and it's preferred.
Below is a copy of the error message:
[E 220504 14:49:11 web:1789] Uncaught exception GET /app/login/google?state=nonce%3D15560EC1F92442C3A1281CA66FD7278F&code=4%2F0AX4XfSidsdiDkFcscmedNEk_gudM9iXsqW14PPfNs4Fyogc-xX2a8YAbluVWq3ATA&scope=email+profile+openid+https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fuserinfo.email+https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fuserinfo.profile&authuser=0&prompt=none (127.0.0.1) HTTPServerRequest(protocol='https', host='frankdu.com', method='GET', uri='/app/login/google?state=nonce%3D15560EC1F92442C3A1281CA66FD7278F&code=4%2F0AX4XfSidsdiDkFcscmedNEk_gudM9iXsqW14PPfNs4Fyogc-xX2a8YAbluVWq3ATA&scope=email+profile+openid+https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fuserinfo.email+https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fuserinfo.profile&authuser=0&prompt=none', version='HTTP/1.1', remote_ip='127.0.0.1') Traceback (most recent call last): File "/Users/frankdu/.virtualenvs/ab3/lib/python3.9/site-packages/tornado/web.py", line 1704, in _execute result = await result File "/Users/frankdu/Documents/work/ABRepo/Web/tornado/spark/auth/login_app_handlers.py", line 36, in get await self.process_google_auth( File "/Users/frankdu/Documents/work/ABRepo/Web/tornado/spark/auth/login_handlers.py", line 157, in process_google_auth user_auth_data = await self.get_authenticated_user( File "/Users/frankdu/.virtualenvs/ab3/lib/python3.9/site-packages/tornado/auth.py", line 915, in get_authenticated_user response = await http.fetch( tornado.httpclient.HTTPClientError: HTTP 401: Unauthorized [E 220504 14:49:11 web:2239] 500 GET /app/login/google?state=nonce%3D15560EC1F92442C3A1281CA66FD7278F&code=4%2F0AX4XfSidsdiDkFcscmedNEk_gudM9iXsqW14PPfNs4Fyogc-xX2a8YAbluVWq3ATA&scope=email+profile+openid+https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fuserinfo.email+https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fuserinfo.profile&authuser=0&prompt=none (127.0.0.1) 115.61ms
I agree that this is nicer, but we do have precedent in this module for overriding the class attributes. It's documented on several of the other mixin classes, although not in this case. Even though this module isn't very internally consistent, I'd prefer not to make it worse. I think I'd start by just documenting the option to override the class attribute, and then we can consider adding regular instance variables that can be overridden instead of the class attributes (for all the ones that are documented).
API nit: I think it's better to not use @property here. That needs to be repeated any time the method is overridden, and if it's missing the error messages can be confusing.
I agree that this is nicer, but we do have precedent in this module for overriding the class attributes. It's documented on several of the other mixin classes, although not in this case. Even though this module isn't very internally consistent, I'd prefer not to make it worse.
@bdarnell thank you for looking into this! I agree with your concern. The backward compatibility is important. My original suggestion doesn't address it completely. After your feedback, I have a better idea. Let me throw up the code.