[AIRFLOW-2185] Use state instead of query param for redirect_uri

[samschlegel]

JIRA

• My PR addresses the following Airflow JIRA issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-2185

Description

Both the Google OAuth2 and GHE authentication plugins include the next_url as a query parameter in redirect_uri. This breaks at least Google OAuth2, unless you include the query parameter in the authorized redirection URI. This isn't the most flexible solution, as you would have to do the same for every potential next URL, and seems to go against the OAuth2 spec.

Instead the next_url should be sent via the state parameter which MUST be maintained by all spec compliant OAuth2 implementations, and is not used when comparing redirection URIs.

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:
  This is not easily unit testable, but I have tested it with Google OAuth2. Assuming GHE follows the OAuth2 spec, it should work there as well.

Commits

• My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":

  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

• Passes git diff upstream/master -u -- "*.py" | flake8 --diff

[Fokko]

@samschlegel Would there be any way to test this?

[Fokko]

Please rebase onto master to fix the CI

[samschlegel]

Could use Flask-OAuthlib to create a mock OAuth2 server for all of the OAuth2 related auth backends, special casing for any quirks they have.

But if you wanted to actually test with Google OAuth2, that's a much more involved process that would probably require a headless browser and a GCP account. I'm not sure what the process would be with GHE, but given you need a license to run it I'm not sure it would be much easier.

Could testing this be added as a separate Jira ticket since it's non-trivial?

[samschlegel]

It 100% works with GCP for us, but since I can't test with GHE I can remove that part of the change, unless someone else can test it.

[codecov-io]

Codecov Report

Merging #3103 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3103   +/-   ##
=======================================
  Coverage   73.07%   73.07%           
=======================================
  Files         180      180           
  Lines       12578    12578           
=======================================
  Hits         9191     9191           
  Misses       3387     3387

---

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 0f9f460...a7de7de. Read the comment docs.

[dougblack]

Would love for this to be merged. @Fokko are you blocking this on testing?