-
Notifications
You must be signed in to change notification settings - Fork 256
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
Handle harvester loading in a route guard to better handle the unauthenticated method. #11373
Conversation
b86c891
to
93cd9e5
Compare
} | ||
|
||
export async function loadHarvester(to, from, next, { store }) { | ||
if (!routeRequiresAuthentication(to) || to.name !== '404') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only execute if authenticated and on a 404 page.
path: '*', | ||
name: '404', | ||
component: () => interopDefault(import('@shell/pages/404.vue')), | ||
meta: { requiresAuthentication: true }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We specifically needed this component so we could make sure authentication would be run on 404s.
import Brand from '@shell/mixins/brand'; | ||
|
||
export default { | ||
mixins: [Brand], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this to reduce the flicker in dark mode.
beforeMount() { | ||
this.$store.commit('setError', { error: new Error('404: This page could not be found') }); | ||
this.$router.replace('/fail-whale'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If route guards don't redirect us we should go to failwhale with the error message we used to emit in the render()
method of entry-helpers.js
// If no Components matched, generate 404 | ||
if (!Components.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will no longer ever be true because our 404 page will match if nothing else will.
I'll open for review once tests pass again. Looks like we may have had a node version change. This change was passing all unit and e2e tests on friday. |
93cd9e5
to
e54b0b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing I saw all nav guards start in order, but finish out of order. This makes me very nervous, but i couldn't find any specific bugs. We should pay attention to this, in particular things like multiple api requests to the same endpoint on load, or extra schema missing warnings. Just as a tidy up I added async/await to the beforeEach hooks (though i don't think vue router awaits
them).
I also de-harvester'd the navigation guard names.
Edit: Also re-based to resolve conflict given something merged earlier in the day
try { | ||
// Handle the loading of dynamic plugins (Harvester) because we only want to attempt to load those plugins and routes if we first couldn't find a page. | ||
// We should probably get rid of this concept entirely and just load plugins at the start. | ||
await store.dispatch('loadManagement'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me nervous, we're calling this before some of the normal setup stuff in the auth middleware. It's quite dependent on the order the nav guards run as well, more in PR review comment
I think we need to be really clear what the issue is because the way that you've described it and the commit you added makes me think there's a misunderstanding of how navigation guards work.
You're right, vue-router isn't awaiting them. It's waiting for the the hook passed to
The call to
We will see |
e54b0b8
to
89286c3
Compare
I'll go ahead and merge once master has all tests pass or we decide to skip. |
89286c3
to
a288116
Compare
…enticated method. This is done by providing a 404 catch all page which is authenticated.
…ard to be generic
a288116
to
1664f88
Compare
Summary
This is done by providing a 404l page which is authenticated.
Fixes #11350
Technical notes summary
We will still need the fix from be1e27b for prod.
Areas or cases that should be tested
404 routing.
Harvester routing.
Screenshot/Video
Regular 404:
https://github.com/rancher/dashboard/assets/55104481/5d3d2fc8-0cbd-4d20-99e0-59dda8a0d8e3
Harvester 404:
https://github.com/rancher/dashboard/assets/55104481/310f10e4-6881-402f-8952-a55f9b846d82
I noticed a flicker of the resource page on this one. I can only reproduce this on the harvester cluster but it's in
master
and unrelated to both this PR and harvester itself. If anyone else can reproduce this I'll resolve it as a separate issue.Checklist