Remove the remaining bits of the authenticated middleware #11266

codyrancher · 2024-06-18T19:25:07Z

Summary

Remove the remaining bits of the authenticated middleware

Technical notes summary

Moving the remaining bits to navigation guards.

Areas which could experience regressions

Harvester clusters, products and cluster loading

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

codyrancher · 2024-06-20T03:53:59Z

shell/initialize/entry-helpers.js

-    // Call the authenticated middleware. This used to attempt to load the error layout but because it was missing it would:
-    // 1. load the default layout instead
-    // 2. then call the authenticated middleware
-    // 3. Authenticated middleware would then load plugins and check to see if there was a valid route and navigate to that if it existed
-    // 4. This would allow harvester cluster pages to load on page reload
-    // We should really make authenticated middleware do less...
-    await callMiddleware.call(this, [{ options: { middleware: ['authenticated'] } }], app.context);


This could cause problems with loading harvester cluster pages. I haven't been able to test yet.

I think we should do that, so we do not risk rollback.
@torchiaf is it something we can try?

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

codyrancher · 2024-06-20T03:58:02Z

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

@@ -151,9 +111,7 @@ export default async function({
      })
    ]);

-    if (localCheckResource) {


This check doesn't appear to be needed any longer.

It looked like it was previously needed because some of the code we only wanted to be executed once while others we wanted on every page with authenticated.

shell/initialize/entry-helpers.js

shell/utils/auth.js

codyrancher · 2024-06-20T04:07:52Z

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

-  const backTo = window.localStorage.getItem(BACK_TO);
-
-  if (backTo) {
-    window.localStorage.removeItem(BACK_TO);


I didn't port backTo over. It's non-functional in master and Neil seems keen to remove it. You can view a thread in team-container-ui where this was discussed.

I added an issue to track the removal of the rest of this feature #11268

Seen the conversation already 👍
UI/UX should be approved by Ken though and not Neil.
As he did not express any opinion, I take it as a tacit agreement.

cnotv

Thanks for finally implementing routing guards as replacement of the middleware.

I added some side notes.
Trivial question outside of this PR scope: should not the config folder contain just static configuration?

shell/initialize/entry-helpers.js

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

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

codyrancher · 2024-06-20T13:46:49Z

Trivial question outside of this PR scope: should not the config folder contain just static configuration?

I don't feel too strongly about this, I've seen a fair amount of config which included executable code. I'd be comfortable moving all of this to the initialize code though. I can understand the desire to keep it static.

codyrancher · 2024-07-04T00:38:13Z

I'll wait to merge after we branch for the 2.9 release.

cnotv

I think is pretty much fine. There are some comments to clarify some choices and so on.

cnotv · 2024-07-04T13:02:04Z

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

-      }, 1);
-    });
-  }
+  const route = to;


Why this assignment? Are we trying to clone or what? Also, the name in such cases should have more specificity and a comment with an explanation.

I only wanted to make this easier to diff with the original implementation so it was obvious the logic wasn't being changed. I change change all references.

cnotv · 2024-07-04T13:02:52Z

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

@@ -71,8 +31,12 @@ export default async function({
    const oldPkg = getPackageFromRoute(from);
    const oldProduct = getProductFromRoute(from);

+    // TODO: Replace all references to store.$plugin.


Thanks for clarifying the change in the comment and sorry for asking again, could you please add an issue for this?

Since I'll have to migrate this part, may I ask you where we assign the global $plugin to the store? Or is it something from Vuex?

Here's the issue: #11386

I've added a reference to where $plugin is set in the tech debt issue but here's a reference here as well

dashboard/shell/store/index.js

Line 1149 in 5a00bcc

commit('setPlugin', nuxt.app.$plugin);

I add it to the Nuxt removal bits, since it was listed 😅
#11070

cnotv · 2024-07-04T13:10:16Z

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

+
+export async function loadHistory(to, from, next) {
+  // Clear state used to record if back button was used for navigation
+  // TODO: Investigate if this can be removed. This is only used on the templates/error.vue page and seems hacky.


Do we still use the template/error page?
Because I think it was just on initialization.
Should we maybe open an issue to make an investigation in general on how is handled the 404, so we could merge all to failWhale instead?

I opened two issues, one for this todo and one for the requests 404/failwhale investigation.

#11384

#11385

Thank you! 🍺

cnotv · 2024-07-04T13:21:10Z

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

+  store.dispatch('gcRouteChanged', to);
+
+  await applyProducts(store, store.$plugin);
+  setProduct(store, to);


Sorry for the naive question, would not be better an action in the store for this?

I'm happy to move it if you'd like. Is there a store that you think fits this best?

It seems a bit awkward for our existing stores since it touches the gc stuff, products and plugins.

Let's keep it minimal and think about it in another phase then, e.g. review of the store.

cnotv

LGTM

cnotv · 2024-07-09T09:33:42Z

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

+
+export async function loadHistory(to, from, next) {
+  // Clear state used to record if back button was used for navigation
+  // TODO: Investigate if this can be removed. This is only used on the templates/error.vue page and seems hacky.


Thank you! 🍺

cnotv · 2024-07-09T09:34:41Z

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

+  store.dispatch('gcRouteChanged', to);
+
+  await applyProducts(store, store.$plugin);
+  setProduct(store, to);


Let's keep it minimal and think about it in another phase then, e.g. review of the store.

…ing.

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

codyrancher force-pushed the final-countdown branch 3 times, most recently from d424b68 to 15090eb Compare June 19, 2024 17:10

codyrancher changed the title ~~Removing the back_to code since it's non-functional~~ Remove the remaining bits of the authenticated middleware Jun 20, 2024

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

codyrancher commented Jun 20, 2024

View reviewed changes

shell/config/router/navigation-guards/clusters.js Show resolved Hide resolved

codyrancher commented Jun 20, 2024

View reviewed changes

shell/initialize/entry-helpers.js Show resolved Hide resolved

codyrancher commented Jun 20, 2024

View reviewed changes

shell/utils/auth.js Show resolved Hide resolved

codyrancher commented Jun 20, 2024

View reviewed changes

codyrancher requested a review from cnotv June 20, 2024 04:08

codyrancher marked this pull request as ready for review June 20, 2024 04:08

cnotv reviewed Jun 20, 2024

View reviewed changes

shell/initialize/entry-helpers.js Show resolved Hide resolved

shell/config/router/navigation-guards/history.js Outdated Show resolved Hide resolved

shell/config/router/navigation-guards/clusters.js Show resolved Hide resolved

codyrancher force-pushed the final-countdown branch 2 times, most recently from 8f07fd4 to 41cb67c Compare June 25, 2024 18:11

codyrancher removed this from the v2.9.0 milestone Jun 27, 2024

codyrancher force-pushed the final-countdown branch 3 times, most recently from b3de10c to 41ffbcd Compare July 3, 2024 23:44

codyrancher requested a review from cnotv July 4, 2024 00:37

cnotv reviewed Jul 4, 2024

View reviewed changes

codyrancher force-pushed the final-countdown branch from 41ffbcd to 6fdcc73 Compare July 9, 2024 04:27

cnotv previously approved these changes Jul 9, 2024

View reviewed changes

codyrancher added 3 commits July 10, 2024 11:22

Removing the back_to code since it's non-functional

63f9f43

Migrating the primary product section out of authenticated middleware

314b62b

Removing the rest of authenticated middleware. Primarily cluster load…

fdf7018

…ing.

codyrancher dismissed cnotv’s stale review via fdf7018 July 10, 2024 18:24

codyrancher force-pushed the final-countdown branch from 6fdcc73 to fdf7018 Compare July 10, 2024 18:24

cnotv mentioned this pull request Jul 17, 2024

Router architecture and authentication updates refactoring #11457

Closed

rak-phillip approved these changes Jul 26, 2024

View reviewed changes

codyrancher merged commit 380ece8 into rancher:master Jul 26, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the remaining bits of the authenticated middleware #11266

Remove the remaining bits of the authenticated middleware #11266

codyrancher commented Jun 18, 2024 •

edited

Loading

codyrancher Jun 20, 2024

cnotv Jun 20, 2024

codyrancher Jun 20, 2024 •

edited

Loading

codyrancher Jun 20, 2024

cnotv Jun 20, 2024

cnotv left a comment

codyrancher commented Jun 20, 2024

codyrancher commented Jul 4, 2024

cnotv left a comment

cnotv Jul 4, 2024

codyrancher Jul 9, 2024 •

edited

Loading

codyrancher Jul 9, 2024

cnotv Jul 4, 2024

cnotv Jul 4, 2024

codyrancher Jul 9, 2024

cnotv Jul 9, 2024

cnotv Jul 4, 2024

codyrancher Jul 9, 2024

cnotv Jul 9, 2024

cnotv Jul 4, 2024

codyrancher Jul 9, 2024 •

edited

Loading

cnotv Jul 9, 2024

cnotv left a comment

cnotv Jul 9, 2024

cnotv Jul 9, 2024

Remove the remaining bits of the authenticated middleware #11266

Remove the remaining bits of the authenticated middleware #11266

Conversation

codyrancher commented Jun 18, 2024 • edited Loading

Summary

Technical notes summary

Areas which could experience regressions

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnotv left a comment

Choose a reason for hiding this comment

codyrancher commented Jun 20, 2024

codyrancher commented Jul 4, 2024

cnotv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher Jul 9, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnotv left a 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 18, 2024 •

edited

Loading

codyrancher Jun 20, 2024 •

edited

Loading

codyrancher Jul 9, 2024 •

edited

Loading

codyrancher Jul 9, 2024 •

edited

Loading