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

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

Merged
merged 1 commit into from
Jun 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions shell/config/router/navigation-guards/attempt-first-login.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import { SETUP } from '@shell/config/query-params';
import { SETTING } from '@shell/config/settings';
import { MANAGEMENT, NORMAN } from '@shell/config/types';
import { tryInitialSetup } from '@shell/utils/auth';
import { routeNameMatched } from '@shell/utils/router';
import { routeRequiresAuthentication } from '@shell/utils/router';

export function install(router, context) {
router.beforeEach((to, from, next) => attemptFirstLogin(to, from, next, context));
}

export async function attemptFirstLogin(to, from, next, { store }) {
if (routeNameMatched(to, 'unauthenticated')) {
if (!routeRequiresAuthentication(to)) {
return next();
}

Expand Down
63 changes: 63 additions & 0 deletions shell/config/router/navigation-guards/authentication.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import { routeRequiresAuthentication } from '@shell/utils/router';
import { isLoggedIn, notLoggedIn, noAuth, findMe } from '@shell/utils/auth';

export function install(router, context) {
router.beforeEach((to, from, next) => authenticate(to, from, next, context));
}

export async function authenticate(to, from, next, { store }) {
if (!routeRequiresAuthentication(to)) {
return next();
}

if ( store.getters['auth/enabled'] !== false && !store.getters['auth/loggedIn'] ) {
// `await` so we have one successfully request whilst possibly logged in (ensures fromHeader is populated from `x-api-cattle-auth`)
await store.dispatch('auth/getUser');

const v3User = store.getters['auth/v3User'] || {};

if (v3User?.mustChangePassword) {
return next({ name: 'auth-setup' });
}

// In newer versions the API calls return the auth state instead of having to make a new call all the time.
const fromHeader = store.getters['auth/fromHeader'];

if ( fromHeader === 'none' ) {
noAuth(store);
} else if ( fromHeader === 'true' ) {
const me = await findMe(store);

isLoggedIn(store, me);
} else if ( fromHeader === 'false' ) {
notLoggedIn(store, next, to);

return;
} else {
// Older versions look at principals and see what happens
try {
const me = await findMe(store);

isLoggedIn(store, me);
} catch (e) {
const status = e?._status;

if ( status === 404 ) {
noAuth(store);
} else {
if ( status === 401 ) {
notLoggedIn(store, next, to);
} else {
store.commit('setError', { error: e, locationError: new Error('Auth Middleware') });
}

return;
}
}
}

store.dispatch('gcStartIntervals');
Copy link
Member

Choose a reason for hiding this comment

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

How is gcStartIntervals related to authentication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

// 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);

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue to track per our conversation #11205

}

next();
}
3 changes: 2 additions & 1 deletion shell/config/router/navigation-guards/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { install as installLoadInitialSettings } from '@shell/config/router/navigation-guards/load-initial-settings';
import { install as installAttemptFirstLogin } from '@shell/config/router/navigation-guards/attempt-first-login';
import { install as installAuthentication } from '@shell/config/router/navigation-guards/authentication';

/**
* Install our router navigation guards. i.e. router.beforeEach(), router.afterEach()
Expand All @@ -8,7 +9,7 @@ export function installNavigationGuards(router, context) {
// NOTE: the order of the installation matters.
// Be intentional when adding, removing or modifying the guards that are installed.

const navigationGuardInstallers = [installLoadInitialSettings, installAttemptFirstLogin];
const navigationGuardInstallers = [installLoadInitialSettings, installAttemptFirstLogin, installAuthentication];

navigationGuardInstallers.forEach((installer) => installer(router, context));
}
5 changes: 5 additions & 0 deletions shell/config/router/routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export default [
{
path: '/',
component: () => interopDefault(import('@shell/components/templates/default.vue')),
meta: { requiresAuthentication: true },
children: [
{
path: '',
Expand All @@ -27,12 +28,14 @@ export default [
path: '',
component: () => interopDefault(import('@shell/components/templates/blank.vue')),
name: 'blank',
meta: { requiresAuthentication: true },
children: [
]
},
{
path: '',
component: () => interopDefault(import('@shell/components/templates/home.vue')),
meta: { requiresAuthentication: true },
children: [
{
path: '/home',
Expand All @@ -50,6 +53,7 @@ export default [
path: '',
component: () => interopDefault(import('@shell/components/templates/plain.vue')),
name: 'plain',
meta: { requiresAuthentication: true },
children: [
{
path: '/about',
Expand Down Expand Up @@ -145,6 +149,7 @@ export default [
path: '',
component: () => interopDefault(import('@shell/components/templates/default.vue')),
name: 'default',
meta: { requiresAuthentication: true },
children: [
{
path: '/clusters',
Expand Down
4 changes: 3 additions & 1 deletion shell/initialize/entry-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,9 @@ export async function mountApp(appPartials, VueClass) {
// Add beforeEach router hooks
router.beforeEach(render.bind(vueApp));
router.afterEach((from, to) => {
updatePageTitle(getVendor());
if (from?.name !== to?.name) {
updatePageTitle(getVendor());
}
Comment on lines +268 to +270
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

});

// First render on client-side
Expand Down
51 changes: 2 additions & 49 deletions shell/middleware/authenticated.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ import dynamicPluginLoader from '@shell/pkg/dynamic-plugin-loader';
import { AFTER_LOGIN_ROUTE, WORKSPACE } from '@shell/store/prefs';
import { BACK_TO } from '@shell/config/local-storage';
import { NAME as FLEET_NAME } from '@shell/config/product/fleet.js';
import {
validateResource, setProduct, isLoggedIn, notLoggedIn, noAuth, findMe
} from '@shell/utils/auth';
import { validateResource, setProduct } from '@shell/utils/auth';
import { getClusterFromRoute, getProductFromRoute, getPackageFromRoute } from '@shell/utils/router';

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

Choose a reason for hiding this comment

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

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

// `await` so we have one successfully request whilst possibly logged in (ensures fromHeader is populated from `x-api-cattle-auth`)
await store.dispatch('auth/getUser');

const v3User = store.getters['auth/v3User'] || {};

if (v3User?.mustChangePassword) {
return redirect({ name: 'auth-setup' });
}

// In newer versions the API calls return the auth state instead of having to make a new call all the time.
const fromHeader = store.getters['auth/fromHeader'];

if ( fromHeader === 'none' ) {
noAuth(store);
} else if ( fromHeader === 'true' ) {
const me = await findMe(store);

isLoggedIn(store, me);
} else if ( fromHeader === 'false' ) {
notLoggedIn(store, redirect, route);

return;
} else {
// Older versions look at principals and see what happens
try {
const me = await findMe(store);

isLoggedIn(store, me);
} catch (e) {
const status = e?._status;

if ( status === 404 ) {
noAuth(store);
} else {
if ( status === 401 ) {
notLoggedIn(store, redirect, route);
} else {
store.commit('setError', { error: e, locationError: new Error('Auth Middleware') });
}

return;
}
}
}

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

const backTo = window.localStorage.getItem(BACK_TO);
Expand Down
4 changes: 2 additions & 2 deletions shell/utils/auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

} else {
return redirect(302, `/auth/login?${ TIMED_OUT }`);
return redirect(`/auth/login?${ TIMED_OUT }`);
}
}

Expand Down
11 changes: 5 additions & 6 deletions shell/utils/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,13 @@ export const routeMatched = (to, fn) => {
};

/**
* Given a route and a name it will look through the matching parent routes to see if any have the specified name
*
* Checks to see if the route requires authentication by taking a look at the route and it's parents 'meta' to see if it
* contains { requiresAuthentication: true }
* @param {*} to a VueRouter Route object
* @param {*} routeName the name of a route you're checking to see if it was matched.
* @returns true if a matching route was found, false otherwise
* @returns true if the route requires authentication, false otherwise
*/
export const routeNameMatched = (to, routeName) => {
return routeMatched(to, (matched) => (matched?.name === routeName));
export const routeRequiresAuthentication = (to) => {
return routeMatched(to, (matched) => matched.meta?.requiresAuthentication);
Comment on lines +97 to +98
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

};

function findMeta(route, key) {
Expand Down
Loading