Extracted the authentication portion of the authenticated middleware into a navigation guard #11186

codyrancher · 2024-06-05T21:55:04Z

Summary

Extracted the authentication portion of the authenticated middleware into a navigation guard

Technical notes summary

This is the closest analogue for getting authentication to be executed at the same point in time it was when it was in middleware.

I did verify this works for both local and github authentication providers.

Areas or cases that should be tested

All authenticated area

Areas which could experience regressions

All authenticated area

Checklist

The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
The PR has a Milestone
The PR template has been filled out
The PR has been self reviewed
The PR has a reviewer assigned
The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

…into a navigation guard

codyrancher · 2024-06-10T22:09:55Z

shell/utils/router.js

+export const routeRequiresAuthentication = (to) => {
+  return routeMatched(to, (matched) => matched.meta?.requiresAuthentication);


I decided to update this to use metadata instead of names so that we don't have to maintain a separate list of authenticated pages.

The vue3 router docs also have this exact example for authentication: https://v3.router.vuejs.org/guide/advanced/meta.html#route-meta-fields so it should migrate better.

I like this approach, it's straightforward and the idiomatic way to handle authentication in vue router.

codyrancher · 2024-06-10T22:10:14Z

shell/utils/auth.js

@@ -297,9 +297,9 @@ export function notLoggedIn(store, redirect, route) {
  store.commit('auth/hasAuth', true);

  if ( route.name === 'index' ) {
-    return redirect(302, '/auth/login');
+    return redirect('/auth/login');


navigation guard next() doesn't support passing a status.

codyrancher · 2024-06-10T22:10:51Z

shell/middleware/authenticated.js

@@ -17,52 +15,7 @@ export default async function({
  route, store, redirect, from, $plugin, next
 }) {
  if ( store.getters['auth/enabled'] !== false && !store.getters['auth/loggedIn'] ) {


This is still necessary because we don't always proceed any further in the middleware depending on whether the user was authenticated or not.

codyrancher · 2024-06-10T22:11:41Z

shell/initialize/entry-helpers.js

+    if (from?.name !== to?.name) {
+      updatePageTitle(getVendor());
+    }


When investigating a flaky test I noticed that the page title was flickering on pages where the query or hash in the url was updated (due to tabs for instance)

Could the flashing behavior also have anything to do with the TabTitle (shell/components/TabTitle.vue) component?

rak-phillip

I think that this looks great! I just have one question about initializing garbage collection from within the authentication guard.

rak-phillip · 2024-06-10T22:38:46Z

shell/utils/router.js

+export const routeRequiresAuthentication = (to) => {
+  return routeMatched(to, (matched) => matched.meta?.requiresAuthentication);


I like this approach, it's straightforward and the idiomatic way to handle authentication in vue router.

rak-phillip · 2024-06-10T22:43:55Z

shell/initialize/entry-helpers.js

+    if (from?.name !== to?.name) {
+      updatePageTitle(getVendor());
+    }


Could the flashing behavior also have anything to do with the TabTitle (shell/components/TabTitle.vue) component?

rak-phillip · 2024-06-10T22:46:20Z

shell/config/router/navigation-guards/authentication.js

+      }
+    }
+
+    store.dispatch('gcStartIntervals');


How is gcStartIntervals related to authentication?

It's really not. But it's not related to setting product either and I was going to move it into one of the two lol.

Would you like to put it into a separate navigation guard? I think we can preserve order and timing that way

Would you like to put it into a separate navigation guard? I think we can preserve order and timing that way

Perhaps.. I might also be fine with a comment that explains why we need to start garbage collection intervals at the end of the authentication guard.

If we were to separate this into a new guard, what do we think would happen to this gc action that we also dispatch in the authenticated middleware, would these two end up in closer proximity to each other?

dashboard/shell/middleware/authenticated.js

Lines 76 to 77 in 256ecff

// GC should be notified of route change before any find/get request is made that might be used for that page

store.dispatch('gcRouteChanged', route);

@codyrancher and I synced on this out of band. It looks like there's quite a bit of behavior in the authenticated middleware that isn't related to authentication. We will follow up with an issue to address the garbage collection in the future.

Created an issue to track per our conversation #11205

codyrancher · 2024-06-10T23:06:54Z

Could the flashing behavior also have anything to do with the TabTitle (shell/components/TabTitle.vue) component?

So it can cause flashing if the content changes but there isn't much we can do about that besides debouncing. This was constantly resetting it back to 'Rancher' as the url was modified even if the route hadn't actually changed.

rancher-ui-project-bot bot assigned codyrancher Jun 5, 2024

codyrancher force-pushed the beforeeach-auth branch 7 times, most recently from d962f93 to a043615 Compare June 9, 2024 07:16

Extracted the authentication portion of the authenticated middleware …

b81a1f8

…into a navigation guard

codyrancher force-pushed the beforeeach-auth branch from a043615 to b81a1f8 Compare June 10, 2024 21:04

codyrancher changed the title ~~Extracted the authentication portion of the authenticated middleware …~~ Extracted the authentication portion of the authenticated middleware into a navigation guard Jun 10, 2024

codyrancher added this to the v2.9.0 milestone Jun 10, 2024

codyrancher commented Jun 10, 2024

View reviewed changes

codyrancher requested a review from rak-phillip June 10, 2024 22:12

codyrancher marked this pull request as ready for review June 10, 2024 22:12

rak-phillip reviewed Jun 10, 2024

View reviewed changes

rak-phillip approved these changes Jun 10, 2024

View reviewed changes

codyrancher merged commit 8a4cdc6 into rancher:master Jun 10, 2024
35 checks passed

codyrancher mentioned this pull request Jun 10, 2024

[Vue 3] Review the usage of 'gcStartIntervals' #11205

Open

cnotv mentioned this pull request Jul 17, 2024

Router architecture and authentication updates refactoring #11457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extracted the authentication portion of the authenticated middleware into a navigation guard #11186

Extracted the authentication portion of the authenticated middleware into a navigation guard #11186

codyrancher commented Jun 5, 2024 •

edited

Loading

codyrancher Jun 10, 2024

rak-phillip Jun 10, 2024

codyrancher Jun 10, 2024

codyrancher Jun 10, 2024

codyrancher Jun 10, 2024

rak-phillip Jun 10, 2024

rak-phillip left a comment

rak-phillip Jun 10, 2024

rak-phillip Jun 10, 2024

rak-phillip Jun 10, 2024

codyrancher Jun 10, 2024

rak-phillip Jun 10, 2024

rak-phillip Jun 10, 2024

codyrancher Jun 10, 2024

codyrancher commented Jun 10, 2024

		export const routeRequiresAuthentication = (to) => {
		return routeMatched(to, (matched) => matched.meta?.requiresAuthentication);

	// GC should be notified of route change before any find/get request is made that might be used for that page
	store.dispatch('gcRouteChanged', route);

Extracted the authentication portion of the authenticated middleware into a navigation guard #11186

Extracted the authentication portion of the authenticated middleware into a navigation guard #11186

Conversation

codyrancher commented Jun 5, 2024 • edited Loading

Summary

Technical notes summary

Areas or cases that should be tested

Areas which could experience regressions

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rak-phillip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher commented Jun 10, 2024

codyrancher commented Jun 5, 2024 •

edited

Loading