-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(ui): support base HREF in dev environment #14894
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
Conversation
This updates the dev environment to allow testing the [base HREF option](https://argo-workflows.readthedocs.io/en/stable/argo-server/#base-href), since some users have reported issues with that (argoproj#14668 (comment)). Tested by running the following commands to start up the environment, then verified I can access the workflows page and login: 1. `make start UI=true` 2. `make start UI=true PROFILE=sso BASE_HREF=/test` 3. `make start UI_SECURE=true PROFILE=sso BASE_HREF=/test/argo-workflows` 4. `make start UI_SECURE=true BASE_HREF=test` Signed-off-by: Mason Malone <[email protected]>
This fixes two bugs when the [base HREF option](https://argo-workflows.readthedocs.io/en/stable/argo-server/#base-href) is set: 1. The "Logout" button doesn't delete the `Authorization` cookie properly. 2. If you visit the login page directly and click the SSO button, you'll be redirected to the `/workflows` page without the base HREF, which will likely result in a 404. The first issue is because logging out `document.cookie = 'authorization=;Max-Age=0';`, which means the `path` attribute defaults to `/`. However, when the base HREF option is set, the cookie will have a different `path` attribute, which means it won't match per [RFC 6265](https://httpwg.org/specs/rfc6265.html#sane-set-cookie-semantics). The second issue is because the default value of the `redirect` query parameter was hardcoded to `'/workflows'`, when it should be `uiUrl('/workflows')`. The `uiUrl()` function handles prepending the base HREF. I had to do some refactoring to make this easily testable using [React Testing Library](https://testing-library.com/docs/react-testing-library/intro/). Tested by following these steps: 1. Merge these changes with argoproj#14894 2. Run `make start UI_SECURE=true PROFILE=sso BASE_HREF=/test/argo-workflows` 3. Visit https://localhost:8080/test/argo-workflows/login 4. Click the first "Login" button 5. Click "Log in with Example" at Dex 6. Click "Grant Access" 7. Verify I'm redirected to https://localhost:8080/test/argo-workflows/workflows 8. Go back to https://localhost:8080/test/argo-workflows/login and click "Logout" 9. Verify the `Authorization` cookie is deleted Signed-off-by: Mason Malone <[email protected]>
This fixes two bugs when the [base HREF option](https://argo-workflows.readthedocs.io/en/stable/argo-server/#base-href) is set: 1. The "Logout" button doesn't delete the `Authorization` cookie properly. 2. If you visit the login page directly and click the SSO button, you'll be redirected to the `/workflows` page without the base HREF, which will likely result in a 404. The first issue is because logging out `document.cookie = 'authorization=;Max-Age=0';`, which means the `path` attribute defaults to `/`. However, when the base HREF option is set, the cookie will have a different `path` attribute, which means it won't match per [RFC 6265](https://httpwg.org/specs/rfc6265.html#sane-set-cookie-semantics). The second issue is because the default value of the `redirect` query parameter was hardcoded to `'/workflows'`, when it should be `uiUrl('/workflows')`. The `uiUrl()` function handles prepending the base HREF. I had to do some refactoring to make this easily testable using [React Testing Library](https://testing-library.com/docs/react-testing-library/intro/). Tested by following these steps: 1. Merge these changes with argoproj#14894 2. Run `make start UI_SECURE=true PROFILE=sso BASE_HREF=/test/argo-workflows` 3. Visit https://localhost:8080/test/argo-workflows/login 4. Click the first "Login" button 5. Click "Log in with Example" at Dex 6. Click "Grant Access" 7. Verify I'm redirected to https://localhost:8080/test/argo-workflows/workflows 8. Go back to https://localhost:8080/test/argo-workflows/login and click "Logout" 9. Verify the `Authorization` cookie is deleted Signed-off-by: Mason Malone <[email protected]>
This fixes two bugs when the [base HREF option](https://argo-workflows.readthedocs.io/en/stable/argo-server/#base-href) is set: 1. The "Logout" button doesn't delete the `Authorization` cookie properly. 2. If you visit the login page directly and click the SSO button, you'll be redirected to the `/workflows` page without the base HREF, which will likely result in a 404. The first issue is because logging out `document.cookie = 'authorization=;Max-Age=0';`, which means the `path` attribute defaults to `/`. However, when the base HREF option is set, the cookie will have a different `path` attribute, which means it won't match per [RFC 6265](https://httpwg.org/specs/rfc6265.html#sane-set-cookie-semantics). The second issue is because the default value of the `redirect` query parameter was hardcoded to `'/workflows'`, when it should be `uiUrl('/workflows')`. The `uiUrl()` function handles prepending the base HREF. I had to do some refactoring to make this easily testable using [React Testing Library](https://testing-library.com/docs/react-testing-library/intro/). Tested by following these steps: 1. Merge these changes with argoproj#14894 2. Run `make start UI_SECURE=true PROFILE=sso BASE_HREF=/test/argo-workflows` 3. Visit https://localhost:8080/test/argo-workflows/login 4. Click the first "Login" button 5. Click "Log in with Example" at Dex 6. Click "Grant Access" 7. Verify I'm redirected to https://localhost:8080/test/argo-workflows/workflows 8. Go back to https://localhost:8080/test/argo-workflows/login and click "Logout" 9. Verify the `Authorization` cookie is deleted Signed-off-by: Mason Malone <[email protected]>
This fixes two bugs when the [base HREF option](https://argo-workflows.readthedocs.io/en/stable/argo-server/#base-href) is set: 1. The "Logout" button doesn't delete the `Authorization` cookie properly. 2. If you visit the login page directly and click the SSO button, you'll be redirected to the `/workflows` page without the base HREF, which will likely result in a 404. The first issue is because logging out `document.cookie = 'authorization=;Max-Age=0';`, which means the `path` attribute defaults to `/`. However, when the base HREF option is set, the cookie will have a different `path` attribute, which means it won't match per [RFC 6265](https://httpwg.org/specs/rfc6265.html#sane-set-cookie-semantics). The second issue is because the default value of the `redirect` query parameter was hardcoded to `'/workflows'`, when it should be `uiUrl('/workflows')`. The `uiUrl()` function handles prepending the base HREF. I had to do some refactoring to make this easily testable using [React Testing Library](https://testing-library.com/docs/react-testing-library/intro/). Tested by following these steps: 1. Merge these changes with argoproj#14894 2. Run `make start UI_SECURE=true PROFILE=sso BASE_HREF=/test/argo-workflows` 3. Visit https://localhost:8080/test/argo-workflows/login 4. Click the first "Login" button 5. Click "Log in with Example" at Dex 6. Click "Grant Access" 7. Verify I'm redirected to https://localhost:8080/test/argo-workflows/workflows 8. Go back to https://localhost:8080/test/argo-workflows/login and click "Logout" 9. Verify the `Authorization` cookie is deleted Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
|
Neat, thank you. Tested locally. |
|
❌ Cherry-pick failed for 3.7. Please check the workflow logs for details. |
Co-authored-by: Alan Clucas <[email protected]> (cherry picked from commit 5d75b62) Signed-off-by: Mason Malone <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Signed-off-by: Alan Clucas <[email protected]>
|
|
… 3.7) (#14953) Signed-off-by: Mason Malone <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Co-authored-by: Mason Malone <[email protected]>
Wrong PR to comment on, sorry. |
Signed-off-by: Mason Malone <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Co-authored-by: Alan Clucas <[email protected]>
Motivation
This updates the dev environment to allow testing the base HREF option, which is necessary for testing the fix in #14909.
Modifications
Previously, we used
hack/update-sso-redirect-url.shto update the SSO redirect URL whenUI_SECUREis set, but that didn't account forBASE_HREF. Updating it to do so would be possible, but it gets messy because we now need to update two differentConfigMaps, not justworkflow-controller-configmap:argo-workflows/manifests/components/sso/overlays/workflow-controller-configmap.yaml
Line 15 in cb7ebd9
argo-workflows/manifests/components/sso/dex/dex-cm.yaml
Lines 20 to 23 in cb7ebd9
Instead, this removes that script and moves the logic to
Makefile, where it does a globalsedon the output ofkustomizeto replace all instances of the redirect URL. Obviously, this is rather hacky, but I couldn't think of a better way of doing this.Update
ui/webpack.config.jsto respectARGO_BASE_HREFand use it for both populating the<base href="...">tag and proxying connections to the API server. This required removing the tag fromui/src/index.html, since Webpack will populate it now.Update
tasks.yamlto restart Webpack ifwebpack.config.jschanges, to simplify developmentVerification
Tested by running the following commands to start up the environment, then verified I can access the workflows page and login:
make start UI=truemake start UI=true PROFILE=sso BASE_HREF=/testmake start UI_SECURE=true PROFILE=sso BASE_HREF=/test/argo-workflowsmake start UI_SECURE=true BASE_HREF=testDocumentation
I added a "Proxying" section to the "Running Locally" page to document this: https://argo-workflows--14894.org.readthedocs.build/en/14894/running-locally/#proxying