ui: Disabling policy form fields from users with 'read' permissions

[kaxcode]

🐛 Description:

User with read permission can edit policies. An error message denying their request appears. The change temporarily appears in policy list page. The changes do not persist. Refreshing the page resets the changes.

💻 Replication Steps:

Using a new Consul cluster with ACLs enabled with default deny.

• Create the a new policy with the following rules:
  acl = "read"
  operator = "read"
  service_prefix "" { policy = "write" }
  node_prefix "" { policy = "write" }
  agent_prefix "" { policy = "write" }
• Create a new Role
• Update the Anonymous Token to use the new Role and Policy
• Log off and log back in with Anonymous Token
• Try to update a Policy name

🎉 Solution:

Fix up a typo in the canWrite() permission for policy.

Now, the user can't edit any policies because form fields are disabled.

🧪 Testing:

Test coverage can be found here.

📝 PR Tasks:

• Backport
• Changelog

[johncowen]

Oh no! 😬 Thanks for spotting this, I'm pretty sure this isn't related to the original issue, if I'm not mistaken the original bug was reported in 1.9.5?

I think this is a fix here, but I don't think it's the whole story. The code with the typo in that you fixed here was only added to 1.10.x so doesn't explain the 1.9.5 bug report. Not sure whether you want to do it all in separate chunks or not? Also could we maybe think of adding a test here? The link you added here is a link to existing tests that test something else, not entirely sure what you meant there?

[kaxcode]

Oh no! 😬 Thanks for spotting this, I'm pretty sure this isn't related to the original issue, if I'm not mistaken the original bug was reported in 1.9.5?

I think this is a fix here, but I don't think it's the whole story. The code with the typo in that you fixed here was only added to 1.10.x so doesn't explain the 1.9.5 bug report. Not sure whether you want to do it all in separate chunks or not? Also could we maybe think of adding a test here? The link you added here is a link to existing tests that test something else, not entirely sure what you meant there?

@johncowen I'll look more into 1.9.5 issue. Is it in a GitHub issue?

As for the tests, what exactly would be tested here? I wanted to point out that there is test coverage. Unfortunately, a typo scenario like we have here can't be caught in a test.

[kaxcode]

@johncowen about splitting this into chunks. I'll have to do that since this can't be back-ported to 1.9.x.

[johncowen]

hey!

I'll look more into 1.9.5 issue. Is it in a GitHub issue?

I think it was reported in the forums, there's a link to it on the top of the issue we are tracking in Asana

https://discuss.hashicorp.com/t/consul-ui-acl-policy-edit-issue/28167

I've just seen a PR from @DingoEatingFuzz which I think is to address the rest of the story here, so we can pick the rest up over there maybe? I'll approve this one in the meantime, thanks again for spotting it! 😅

P.S.

As for the tests, what exactly would be tested here? I wanted to point out that there is test coverage. Unfortunately, a typo scenario like we have here can't be caught in a test.

The existing tests here don't cover what happens when a user only has read only permissions.

At a high level, ideally we'd have a test scenario setup with a user with read-only permissions, then we could assert that the when the user looks at the edit form then it's a read-only form.

If we wanted to go lower we could just test the abilities themselves, which would make it easier to just loop through them all and check that read, write, 'create', 'update' permissions report correctly for each model type we have (plus easily test any future models to prevent any typos like we have here). We could almost test the unit itself if it wasn't for the inheritance chain, so we'd be ever so slightly be blurring into integration, but in this case I think doing some sort of unit testing of the abilities themselves (in a future looking way) might be a good idea 🤔 . What do you think?

[johncowen]

🎉 thanks so much for spotting this!

[hc-github-team-consul-core]

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/433281.

[hc-github-team-consul-core]

🍒✅ Cherry pick of commit a777b0a onto release/1.10.x succeeded!