fix: kf_authentication.py fails for some urls #437
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Suggested change
| 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.
Suggested change
| 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
suggested changes
Apr 11, 2023
|
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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)