Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: kf_authentication.py fails for some urls #437

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ca-scribner
Copy link
Contributor

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.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/somethingThatDoesNotExist will not result in authentication)

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)
@ca-scribner ca-scribner requested a review from a team as a code owner February 25, 2022 20:45
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?"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use urlunparse ?

Copy link
Member

@beliaev-maksim beliaev-maksim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor messages, otherwise looks fine

@DnPlas
Copy link
Contributor

DnPlas commented Jun 30, 2023

Hi @ca-scribner and @beliaev-maksim I see some recent conversations, but the PR has been opened for a while.
Is this still a relevant change that we want to merge? It seems like this contribution is changing a test file that is not in use.
Please close the PR if the changes do not apply/are relevant anymore, or merge it otherwise.

@beliaev-maksim
Copy link
Member

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

@ca-scribner
Copy link
Contributor Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants