mblarsen / vue-browser-acl

Easy user access control in Vue for better UX. Build on top of the browser-acl package.
https://blog.cheesefi.com/blog/vue-user-permissions-through-directives/
MIT License
214 stars 27 forks source link

How to make the directives react on state changes? #35

Open thapar opened 4 years ago

thapar commented 4 years ago

I am using vue-browser-acl's custom directive as such:

<template>
  <div>
      <BaseSideNavigationMenu v-role:authenticated />
      <PageContent />
  </div>
</template>

With the following setup and rule:

Vue.use(Acl, store.state.currentUser, (acl) => acl.rule("authenticated", () => store.getters.isAuthenticated));

And the following in my store:

export default new Vuex.Store({
  state: {
    currentUser: JSON.parse(window.localStorage.getItem("currentUser") || "{}"),
  },
  mutations: {
    SET_CURRENT_USER(state, user) {
      state.currentUser = user;
      window.localStorage.setItem("currentUser", JSON.stringify(user));
    },
    SET_NO_CURRENT_USER(state) {
      state.currentUser = {};
      window.localStorage.setItem("currentUser", "{}");
    },
  },
  actions: {
    async login({ commit }, login_data) {
      try {
        const response = await axios.post("/api/login", login_data);
        const currentUser = response.data.currentUser
        commit("SET_CURRENT_USER", currentUser);
        return response;
      }
    },
    async logout({ commit }) {
      const response = await axios.post("/api/logout");
      commit("SET_NO_CURRENT_USER");
    },
  },
  getters: {
    isAuthenticated(state) {
      return Object.keys(state.currentUser).length > 0;
    }
  },

However, when a user does a login, the <BaseSideNavigationMenu v-role:authenticated /> element does not show unless the page is refreshed. Likewise, upon logout, the BaseSideNavigationMenu element remains visible until the page is refreshed.

How can vue-browser-acl's directive be made to react on state changes?

mblarsen commented 4 years ago

Check the index file in the example folder.

The user parameter must be a function if it is loaded dynamically.

// Since the user is asynchronously fetch we
// provide a function to fetch it in this case
// we access it through the Vuex store.
const user = () => {
  return store.getters.user
}

Vue.use(Acl, user, (acl) => { ... })
thapar commented 4 years ago

@mblarsen Upon further tinkering, I noticed that certain elements are reactive with v-role:authenticated tacked on, and others are not. I made the following changes, as per your suggestion, but the outcome of which elements are reactive still hasn't changed:

Vue.use(Acl, () => store.getters.currentUser, (acl) => acl.rule("authenticated", () => store.getters.isAuthenticated));

And I updated the getters portion of the store to include the following:

getters: {
    isAuthenticated(state) {
      return Object.keys(state.currentUser).length > 0;
    },
    currentUser(state) {
      return Object.keys(state.currentUser).length > 0 ? state.currentUser : {};
    }
}

The following elements are responsive as expected:

<span v-role:authenticated.not>Login</span>
<span v-role:authenticated>{{ currentUser.name }}</span>

But the following elements are not responsive:

<LoginForm v-role:authenticated.not />
<LoggedinMenu v-role:authenticated />

The full project code can be seen by following the links above or here.

mblarsen commented 4 years ago

Can you please test if this works:

<LoginForm v-if="$can('authenticated')" />
mblarsen commented 4 years ago

I looked at the repo you shared.

All of the following isn't really about the issue you've raised. Just some observations. Hope you don't mind :)


When you add () => store.getters.currentUser you provide a function that Acl uses to pass as the first parameter to the rules you define.

So your rule should ideally be:

-  acl.rule("authenticated", () => store.getters.isAuthenticated)
+  acl.rule("authenticated", (user) => Boolean(user))

If the user is present you are authenticated. Although your currentUser-getter is returning an "empty" object if the user is not there. Maybe null would be better?

What you've done isn't wrong. Just a bit confusing. They idea of the Acl is to ask "can the current user do x" or "what is true about the current user" - so ideally you should resolve your logic from the user's perspective as I've shown above.

Also:

<LoginForm v-role:authenticated.not />

Is kind of misuse of the semantics. The authenticated isn't really a role. You could perhaps instead create a rule called login.

<LoginForm v-can:login />

But really when it comes to UI flow I'd use the store getters directly in your case:

<LoginForm v-if="!authenticated" />
<LoggedinMenu v-if="authenticated" />

For me it is all about how the code reads. If "authenticated do this if not do that".

thapar commented 4 years ago

Can you please test if this works:

<LoginForm v-if="$can('authenticated')" />

This works reactively as expected. Can the v-role syntax not be reliably used reactively?

thapar commented 4 years ago

I looked at the repo you shared.

All of the following isn't really about the issue you've raised. Just some observations. Home you don't mind :)

When you add () => store.getters.currentUser you provide a function that Acl uses to pass as the first parameter to the rules you define.

So your rule should ideally be:

-  acl.rule("authenticated", () => store.getters.isAuthenticated)
+  acl.rule("authenticated", (user) => Boolean(user))

If the user is present you are authenticated. Although your currentUser-getter is returning an "empty" object if the user is not there. Maybe null would be better?

What you've done isn't wrong. Just a bit confusing. They idea of the Acl is to ask "can the current user do x" or "what is true about the current user" - so ideally you should resolve your logic from the user's perspective as I've shown above.

Also:

<LoginForm v-role:authenticated.not />

Is kind of misuse of the semantics. The authenticated isn't really a role. You could perhaps instead create a rule called login.

<LoginForm v-can:login />

But really when it comes to UI flow I'd use the store getters directly in your case:

<LoginForm v-if="!authenticated" />
<LoggedinMenu v-if="authenticated" />

For me it is all about how the code reads. If "authenticated do this if not do that".

"authenticated" is only a simplified example. In the final application, there will be "moderator", "admin", "superadmin", etc. But as per your last example with v-if="authenticated" it seems like you're saying not to use vue-browser-acl šŸ˜­. I like the syntax of separating ACL permissions for components and routes from the business logic (the code will have plenty of other v-ifs to deal with that aren't related to ACL permissions). Also, this blogpost makes excellent points for using your vue-browser-acl library (and I like the syntax and flexibility of this library).

mblarsen commented 4 years ago

This works reactively as expected. Can the v-role syntax not be reliably used reactively?

I haven't had any issues with it previously. When you work Vuex and async in general things change a bit. Anyway, I'll have to look more into to your issue. Thanks for testing the $can part.

Can you try this too:

+<div v-if="authenticated || !authenticated">
    <LoginForm v-role:authenticated.not />
    <LoggedinMenu v-role:authenticated />
+</div>

Where authenticated is the getter from your store.

Never mind the logic. This is just to see what happens if it works after being forced to re-render by a parent. This is to exclude the issue being something else.

thapar commented 4 years ago

This works reactively as expected. Can the v-role syntax not be reliably used reactively?

I haven't had any issues with it previously. When you work Vuex and async in general things change a bit. Anyway, I'll have to look more into to your issue. Thanks for testing the $can part.

Can you try this too:

+<div v-if="authenticated || !authenticated">
    <LoginForm v-role:authenticated.not />
    <LoggedinMenu v-role:authenticated />
+</div>

Where authenticated is the getter from your store.

Never mind the logic. This is just to see what happens if it works after being forced to re-render by a parent. This is to exclude the issue being something else.

Yes, it is reactive with the v-if wrapping.

thapar commented 4 years ago

Hey @mblarsen, just wondering if you may have had a chance to look into the reactivity issue?

mblarsen commented 4 years ago

Hey @mblarsen, just wondering if you may have had a chance to look into the reactivity issue?

I have not. Don't expect any quick response. I cannot put so much time into it at the moment.

josefsabl commented 4 years ago

Can you please test if this works:

<LoginForm v-if="$can('authenticated')" />

This works reactively as expected. Can the v-role syntax not be reliably used reactively?

I am having exactly the same issue. Thanks for raising this up and coming to this workaround. I would however also appreciate if the better syntax would work reactively ;-)

mblarsen commented 4 years ago

Hi @thapar @josefsabl

I've spend a little time updating the example (changes have been pushed).

Would you mind checking it out and try this flow:

  1. Log in as 'jane' in 'admin' group
  2. See the Admin paragraph appear at the top
  3. Log out and see
  4. See the Admin paragraph disappear again

I'm wondering how this (working) example is different from yours. When I log in and out the user object in the state is replace completely. Could that be what makes the difference? That you don't replace your state object.