Add form_post response mode support for system browser authentication #868
Conversation
| parsed_url = urlparse(self.path) | ||
| qs = parse_qs(parsed_url.query) | ||
|
|
||
| if qs.get('code'): # Auth code via GET is a security risk - reject it |
There was a problem hiding this comment.
How about having a grace period that both GET and POST are supported? Otherwise if an app can't just switch to form post it'll be stuck with an older version of MSAL
There was a problem hiding this comment.
I am not sure if anyone really uses this. I also don't see a scenario where the developer would be specifically preferring one method over another. Overall, I am asking for an approval for this approach for all public MSALs with Bogdan's team. Let me conclude that conversation and update here.
There was a problem hiding this comment.
Actually, we shall complete remove the code branch and error branch from do_GET(), based on the reasons described in the context above.
rayluo
left a comment
rayluo
left a comment
There was a problem hiding this comment.
Thanks for the PR which communicated the intention clearly. The implementation needs some change.
MSAL Python code base serves for both public clients and confidential clients. The current architecture is:
- oauth2.py handles all OAuth2 interactions which does NOT include hosting a web server to receive the auth code response. The web server part is controlled by whatever web framework the app developer chooses for their confidential application. We cannot forcefully override our response_mode to form_post, otherwise their existing query-based app would be broken (sev 1). What we can do (in oauth2.py) is to issue a warning when the response_mode value is not "form_post".
- application.py defines the higher-level API that utilizes oauth2.py. So it will automatically trigger that warning in
#1. The only thing needs to be done is to modify its documentation. (More on this below.) - authcode.py is the mini web server implementation that is used by Public Clients (when not using broker). We fully control how this web server is implemented, so, we can simply remove the "auth code via GET" mode, and refactor it to use form_post mode only.
With that context, now my following inline comments shall be easy to understand.
| parsed_url = urlparse(self.path) | ||
| qs = parse_qs(parsed_url.query) | ||
|
|
||
| if qs.get('code'): # Auth code via GET is a security risk - reject it |
There was a problem hiding this comment.
Actually, we shall complete remove the code branch and error branch from do_GET(), based on the reasons described in the context above.
| self._send_full_response(template.safe_substitute(**safe_data)) | ||
| self.server.auth_response = auth_response # Set it now, after the response is likely sent | ||
| self._process_auth_response(auth_response) | ||
| elif not qs and parsed_url.path == '/': |
There was a problem hiding this comment.
Do we really need this branch? Wouldn't the following else: in line 140 catch it properly?
| self._process_auth_response(auth_response) | ||
| else: | ||
| self._send_full_response(self.server.welcome_page) | ||
| # NOTE: Don't do self.server.shutdown() here. It'll halt the server. |
There was a problem hiding this comment.
This line was valuable. In your new implementation, I think we better also move this line to the end of the new do_POST().
| try: | ||
| from urllib.parse import parse_qs as parse_qs_post | ||
| except ImportError: | ||
| from urlparse import parse_qs as parse_qs_post |
There was a problem hiding this comment.
Why is this necessary? Didn't we already import it at the very beginning of the file?
| # OAuth2 successful and error responses contain state when it was used | ||
| # https://www.rfc-editor.org/rfc/rfc6749#section-4.2.2.1 | ||
| self._send_full_response("State mismatch") # Possibly an attack | ||
| # Don't set auth_response for security, but mark as done to avoid hanging |
There was a problem hiding this comment.
The hanging effect was kind of a feature. The rationale was that normally the state shall match and there would be no hanging at all.
When the state mismatches, it usually happens when I/we were developing this, so keeping the http server running and waiting for the next response (thus "hanging") was helpful during development.
I think keeping that typically intangible "hanging" is better than introducing a new state done in the code path.
| # Provide default values for common OAuth2 response fields | ||
| # to avoid showing literal placeholder text like "$error_description" | ||
| safe_data.setdefault("error", "") | ||
| safe_data.setdefault("error_description", "") |
There was a problem hiding this comment.
Did we actually observe those placeholder text in error code path during manual testing? The error shall have already be available in the error scenario, according to the specs. error_description is typically also available, but yes, you can use this treatment for error_description and error_uri.
| "For your security: Do not share the contents of this page, the address bar, or take screenshots.") | ||
| self._server.error_template = Template(error_template or | ||
| "Authentication failed. $error: $error_description. ($error_uri)") | ||
| "Authentication failed. $error: $error_description.\n\n" |
There was a problem hiding this comment.
If you already add the setdefault treatment, whey do we still need to remove the error_uri from the error output?
Implements support for OAuth 2.0 response_mode=form_post in the system browser authentication flow, aligning with modern OAuth 2.0 best practices (RFC 6749).