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

Add static redirect page to sso and then to requested page #438

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

aneelac22
Copy link

[RHCLOUD-32115]

Copy link

openshift-ci bot commented Jun 24, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aneelac22
Once this PR has been reviewed and has the lgtm label, please assign mfrancisc for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

openshift-ci bot commented Jun 24, 2024

Hi @aneelac22. Thanks for your PR.

I'm waiting for a codeready-toolchain member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@aneelac22 aneelac22 force-pushed the feature/RHCLOUD-32115 branch 3 times, most recently from 5727a5a to 93c5126 Compare August 13, 2024 14:06
@aneelac22 aneelac22 marked this pull request as ready for review August 14, 2024 07:26
@aneelac22 aneelac22 force-pushed the feature/RHCLOUD-32115 branch 3 times, most recently from 35a935a to 99c96c2 Compare September 3, 2024 09:45
@alexeykazakov
Copy link
Contributor

/ok-to-test

</head>
<body>
</body>
<script src="redirectpage.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem to be working...

This is what I tried: {baseURL}/redirectpage.html?link="import"
And I expect to get redirected to {consoleURL}/import/ns/alexeykazakov-dev
But the JS failed with:

redirectpage.js:19 Uncaught SyntaxError: Unexpected token 'export'
jquery.min.js:14 [Deprecation] Listener added for a 'DOMSubtreeModified' mutation event. Support for this event type has been removed, and this event will no longer be fired. See https://chromestatus.com/feature/5083947249172480 for more information.
add @ jquery.min.js:14

`${baseUrl}?link="catalogName"&keyword="keyword"&selectedId="selectedId"`
2. If the user is not authenticated, it redirects to sso page for user to login
3. Once the user is authenticated it redirects to the page user wants to access which is in the format
`${consoleUrl}/${link}/ns/${defaultUserNamespace}?keyword="keyword"&selectedId="selectedId"` with
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm...
What URL should I use if I want to get redirected to

{consoleURL}/catalog/ns/alexeykazakov-dev?category=databases&selectedId=8ef58d8f-edcc-4284-96bb-5006360b6e73

Where alexeykazakov-dev is my default namespace.
What should I use for keyword to get category=databases as the result?

Copy link
Author

@aneelac22 aneelac22 Sep 16, 2024

Choose a reason for hiding this comment

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

In the google doc there is no URL with category as a param, Am I missing something?

Copy link
Contributor

@alexeykazakov alexeykazakov Sep 18, 2024

Choose a reason for hiding this comment

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

Yeah, you are rigth.
But could you please clarify what URL should be used to access

{consoleURL}/catalog/ns/alexeykazakov-dev?keyword=jboss&selectedId=openshift-helm-charts--https%3A%2F%2Fgithub.com%2Fopenshift-helm-charts%2Fcharts%2Freleases%2Fdownload%2Fredhat-eap8-1.1.2%2Feap8-1.1.2.tgz

?
Should we use

{baseURL}/redirectpage.html?link="catalog"&keyword=jboss&selectedId=openshift-helm-charts--https%3A%2F%2Fgithub.com%2Fopenshift-helm-charts%2Fcharts%2Freleases%2Fdownload%2Fredhat-eap8-1.1.2%2Feap8-1.1.2.tgz

?

Copy link
Author

Choose a reason for hiding this comment

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

Yes!

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that is correct

Copy link
Contributor

@alexeykazakov alexeykazakov left a comment

Choose a reason for hiding this comment

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

Still not working for me :(
See my comments below. Please let me know if you need any help with setting up dev environment (see this comment in JIRA) so you can properly test it localy.

const link = urlParams.get("link");
const keyword = urlParams.get("keyword");
const selectedId = urlParams.get("selectedId");
const consoleUrlMock = "https://console.redhat.com/openshift/sandbox";
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of hardcoding this consleURL (and why it's called consoleUrlMock btw?) let's load it from the config (/api/v1/authconfig)
So let's maybe do the following:

  • rename consoleUrlMock to something like signupURL.
  • try to load it from configURL JSON as signup-url.
  • if there is no signup-url defined in the config payload JSON then fall back to the default value (which is just the base URL: /)
  • We, the dev sandbox team, will add https://console.redhat.com/openshift/sandbox to the config API for our production sandbox.

Copy link
Contributor

Choose a reason for hiding this comment

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

We, the dev sandbox team, will add https://console.redhat.com/openshift/sandbox to the config API for our production sandbox.

See #464

async function getRedirectData() {
const xhr = new XMLHttpRequest();

xhr.open("GET", registrationURL, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... I don't actually see in my browser any requests to "/api/v1/signup". This is what I see instead:

  • redirectpage.html redirects to sso (expected)
  • then it redirects to console.openshift.com/sandbox even if I already have an account in dev sandobx. So it doesn't seem to actually check /api/v1/signup unless I'm missing something. But anyway, no matter what I do I always end up in console.openshift.com

handleError();
}
} else {
handleError();
Copy link
Contributor

Choose a reason for hiding this comment

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

If the signup api returns 404 it's not an error actually. It just means that user has to sign up first. So I wouldn't mix it with other cases when something went wrong.
And yes, in case of 404 we should redirect to the signup URL but we also need to redirect back to the redirectpage after user signs up successfully. See https://issues.redhat.com/browse/RHCLOUD-32115?focusedId=24554951&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-24554951 for details.
The redirect param is not currently supported in HCC but we can add this to our internal landing page for now and later add it to HCC too.

Copy link

sonarcloud bot commented Oct 1, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants