fix: kf_authentication.py fails for some urls#437
Conversation
The previous iteration worked if the url passed to `kubeflow_login` was the kubeflow host domain (eg: `http://my.kubeflow/`), but not if the url was any other valid url under the domain (eg: `http://my.kubeflow/pipelines`). This was because some of the `request.get`s in the authentication flow needed the domain rather than the target url. This was missed because only the domain was tested when this was first written. These updates fix this problem - the authentication now works for all valid urls in the domain. Note that authentication will not work for any page that does not exist (eg: `http://my.kubeflow/somethingThatDoesNotExist` will **not** result in authentication)
| response = requests.post(dex_login_url, data=data, verify=False, allow_redirects=False) | ||
| validate_response_status_code( | ||
| response, [301, 303], f"Failed to log into dex - are your credentials correct?" | ||
| response, [303], f"Failed to log into dex - are your credentials correct?" |
There was a problem hiding this comment.
| response, [303], f"Failed to log into dex - are your credentials correct?" | |
| response, [303], "Failed to log into dex - are your credentials correct?" |
| logging.debug(f"Got dex_approval_url of '{dex_approval_url}") | ||
| approval_endpoint = response.headers['location'] | ||
| dex_approval_url = url_base + approval_endpoint | ||
| logging.debug(f"Logged in with dex_approval_url of '{dex_approval_url}") |
There was a problem hiding this comment.
| logging.debug(f"Logged in with dex_approval_url of '{dex_approval_url}") | |
| logging.debug(f"Logged in with dex_approval_url of '{dex_approval_url}'") |
| """Completes the dex/oidc login flow, returning the authservice_session cookie.""" | ||
| host = host.rstrip('/') | ||
| parsed_url = urlparse(url) | ||
| url_base = f"{parsed_url.scheme}://{parsed_url.netloc}" |
beliaev-maksim
left a comment
beliaev-maksim
left a comment
There was a problem hiding this comment.
minor messages, otherwise looks fine
|
Hi @ca-scribner and @beliaev-maksim I see some recent conversations, but the PR has been opened for a while. |
|
Actually, it will be interesting to test authentication via API Not sure that PR with current state will work on 1.7,need to retest |
|
I bet this needs refactoring to be relevant, but that it will actually be useful in the next few weeks as our bundle tests come back online |
|
Closing this as obsolete, we 're not using those tests anymore. This could be during #911. |
4 participants
The previous iteration worked if the url passed to
kubeflow_loginwas the kubeflow host domain (eg:http://my.kubeflow/), but not if the url was any other valid url under the domain (eg:http://my.kubeflow/pipelines). This was because some of therequest.gets in the authentication flow needed the domain rather than the target url. This was missed because only the domain was tested when this was first written.These updates fix this problem - the authentication now works for all valid urls in the domain. Note that authentication will not work for any page that does not exist (eg:
http://my.kubeflow/somethingThatDoesNotExistwill not result in authentication)