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

Simplify Authenticated Middleware #11096

Merged
merged 2 commits into from
May 24, 2024

Conversation

codyrancher
Copy link
Contributor

@codyrancher codyrancher commented May 22, 2024

Summary

Small rework of two functions and then the extraction of a number of functions to the existing auth util. This is an effort to shrink down the code in authenticated middleware to make it a little easier to address.

It might be helpful to look at the two commits in this PR separately.

Areas or cases that should be tested

  • Logging in
  • Product not found
  • Resource not found

Screenshot/Video

auth-slim.mp4

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

…uthenticated middleware

We didn't actually need to call redirect from the methods since the `loadingError` store action did the redirect for us.

We still need to exit early from the middleware so instead of having to check and execute a return value (that was a boolean so couldn't be executed anyway) we now just throw an exception.
@codyrancher codyrancher force-pushed the de-auth-middleware branch from 1c378ce to 12b6b6b Compare May 22, 2024 19:36
@codyrancher codyrancher added this to the v2.9.0 milestone May 22, 2024
@codyrancher codyrancher requested a review from cnotv May 22, 2024 20:39
@codyrancher codyrancher marked this pull request as ready for review May 22, 2024 20:40
@codyrancher codyrancher changed the title De auth middleware Simplify Authenticated Middleware May 22, 2024
Copy link
Contributor

@cnotv cnotv left a comment

Choose a reason for hiding this comment

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

Glad to see something more readable and with comments 😄
Beside 2 notes, the rest is fine to me.

Thanks for adding the comment about the WIP!


store.dispatch('loadingError', new Error(store.getters['i18n/t']('nav.failWhale.resourceNotFound', { resource }, true)));

return () => redirect(302, '/fail-whale');
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for asking, but I got completely lost from our prior logic and I'm glad you simplified 😅
Is this redirect not necessary anymore, though? I could not find any matching redirect to '/fail-whale' so I just wanted to be sure that was not a mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the loadingError action actually does the redirect.

router.replace('/fail-whale');


// ************************************************************
//
// BELOW ARE METHODS THAT ARE A PART OF THE AUTHENTICATED MIDDLEWARE REMOVAL. THIS IS A TEMPORARY HOME FOR THESE UTILS AND SHOULD BE REWRITTEN, MOVED OR DELETED.
Copy link
Contributor

@cnotv cnotv May 23, 2024

Choose a reason for hiding this comment

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

Thanks for mentioning that this is the first clean round.
If you are not planning to work on it, would it be ok to add an issue with a TODO, so we can track it down?
Otherwise is enough for the next PR as usual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code and added an issue #11111

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks and nice number 😅

@codyrancher codyrancher force-pushed the de-auth-middleware branch from 12b6b6b to a193b53 Compare May 23, 2024 13:37
@codyrancher codyrancher force-pushed the de-auth-middleware branch from a193b53 to 468f296 Compare May 23, 2024 13:44
@codyrancher codyrancher requested a review from cnotv May 23, 2024 14:18
Copy link
Contributor

@cnotv cnotv left a comment

Choose a reason for hiding this comment

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

LGTM


// ************************************************************
//
// BELOW ARE METHODS THAT ARE A PART OF THE AUTHENTICATED MIDDLEWARE REMOVAL. THIS IS A TEMPORARY HOME FOR THESE UTILS AND SHOULD BE REWRITTEN, MOVED OR DELETED.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks and nice number 😅

@codyrancher codyrancher merged commit e7a6fd4 into rancher:master May 24, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants