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

fix: #1610 refactor authentication handling and session management #1627

Open
wants to merge 9 commits into
base: feat/frontend-stability-refactor
Choose a base branch
from

Conversation

craigyu
Copy link
Collaborator

@craigyu craigyu commented Oct 10, 2024

This PR only addresses the login flow and auth state.
Some components are broken, please only test out the log in and log out flow.

It's a good idea to clear out the localstorage of this app before testing

@craigyu craigyu self-assigned this Oct 10, 2024
@craigyu craigyu linked an issue Oct 10, 2024 that may be closed by this pull request
4 tasks
Copy link
Collaborator

@ianliuwk1019 ianliuwk1019 left a comment

Choose a reason for hiding this comment

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

There are quite some code commented out and TODO so I didn't look, just leaving some minor questions for my understanding. Thanks for the refactoring!

frontend/src/router/RouteItems.ts Show resolved Hide resolved
frontend/src/router/RouteGuards.ts Show resolved Hide resolved
frontend/src/static/sideNav.json Show resolved Hide resolved
};

// Export the ref-wrapped state for use across components
export const authState = ref<AuthState>(currentAuthState);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you still like to "export" the state If your intention is not giving developers to direct access to this state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The modularization of authState was initially introduced to solve the issue where the authentication state (authState.value) was not being retained or properly updated after a refresh. By creating a separate module to handle the authState as a ref, it ensures that:

  1. The authentication state persists across components.
  2. The state is reactive and updates correctly in Vue’s reactivity system.
  3. Components using the authState can reflect the current authentication status without needing to manage it independently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, maybe I did not state my questions clearly. I understand the usage you build; my question was maybe you would like to have a getter/setter to enforce other component does not directly use the state and only go through setter to modify the state change; if that's the case, maybe there is no need to export it. But it is fine to export it, I was just wondering if that's the intention.

frontend/src/providers/authState.ts Show resolved Hide resolved
frontend/src/composables/useAuth.ts Show resolved Hide resolved
frontend/src/layouts/ProtectedLayout.vue Outdated Show resolved Hide resolved
const logout = async () => {
await Auth.signOut();
stopSilentRefresh();
authState.value = reactive<AuthState>({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something a bit odd, you can directly setting the "authSate.value" with the object you need that I guess vue ref will reflect the change, or is it not? why wrapping into another 'reactive' and not 'ref'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason we wrap the state in reactive is due to the fact that the authState is exposed as readonly to external components through the AuthProvider. Since it’s made readonly, we can’t directly modify it from outside, ensuring that no external mutation happens accidentally. When logging out, we create a new reactive state internally to bypass the readonly restriction and properly update the state. So while Vue does reflect changes through ref, the readonly wrapper necessitates this approach for safe mutation management.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. If after you experiment direct assign object does not work, I only wonder if wrapping in ref() is the suitable one not reactive() (but maybe they have no conflict..).

Copy link

sonarcloud bot commented Oct 11, 2024

Quality Gate Passed Quality Gate passed for 'nr-forests-access-management_admin'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

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.

Refactor Authentication Handling and Session Management
2 participants