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: cluster member role user should not be able to delete node on Harvester Hosts page #83

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

houhoucoop
Copy link
Collaborator

@houhoucoop houhoucoop commented Jan 13, 2025

Summary

For security concern, cluster member role user should not be able to delete node on Harvester Hosts page.

PR Checklists

  • Do we need to backport this PR change to the Harvester Dashboard?
    • Yes, the relevant PR is at:
  • Are backend engineers aware of UI changes?
    • Yes, the backend owner is:

Related Issue

[BUG] Cluster member role user should not be able to delete node on Harvester Hosts page on Rancher managed Harvester #7255

Test screenshot/video

if current user is cluster-owner
Screenshot 2025-01-13 at 1 47 33 PM (2)

if current user is cluster-member
Screenshot 2025-01-13 at 1 48 35 PM (2)

Extra technical notes summary

Test with API=https://rancher.192.168.0.141.sslip.io yarn dev

Signed-off-by: Yi-Ya Chen <[email protected]>
@houhoucoop houhoucoop self-assigned this Jan 13, 2025
@a110605
Copy link
Collaborator

a110605 commented Jan 13, 2025

Could you also check this PR works well on harvester standalone mode with multiple nodes env ?

@a110605
Copy link
Collaborator

a110605 commented Jan 13, 2025

Could you also check this PR works well on harvester standalone mode (only admin) with multiple nodes env ?

Looks great with 3 nodes (default / witness /default) 👍

image

return node;
});
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if there's a more elegant way to handle this, any suggestions would be helpful. 🙏

Copy link
Collaborator

@torchiaf torchiaf left a comment

Choose a reason for hiding this comment

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

I think we should move the logic in the node's model.
I any case, I think the delete action should be blocked by the backend. @Yu-Jack WDYT ?

});

// keep availableActions non-enumerable
Object.defineProperty(node, 'availableActions', {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, availableActions should be defined in the model, in this case it would be pkg/harvester/models/harvester/node.js.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, but the trade-off is that the model will have a broader impact, and using async within the getter could lead to infinite recursion, so we need the _initialized flag to prevent repetition.
update in commit 43b10b2.

@Yu-Jack
Copy link
Contributor

Yu-Jack commented Jan 14, 2025

Hi @torchiaf Actually, backend has blocked this request. So, I think the point here is to hide the Delete button.

cluster-member

demo.mov

cluster-owner

demo-02.mov

@houhoucoop houhoucoop requested a review from torchiaf January 15, 2025 07:17
@torchiaf
Copy link
Collaborator

Hi @torchiaf Actually, backend has blocked this request. So, I think the point here is to hide the Delete button.

Sorry, what I actually mean is: should the server remove the DELETE method from node's schema depending on user's roles ?
I'd calculate the delete action availability in the backend if possible.

@Yu-Jack
Copy link
Contributor

Yu-Jack commented Jan 16, 2025

@torchiaf But, I don't find the CollectionMethods in the response with cluster-member role. I only saw this. Where is CollectionMethods from?

"links": {
    "self": "https://xxxx/k8s/clusters/c-m-5vhzgjw9/v1/harvester/nodes/node1",
    "view": "https://xxxx/k8s/clusters/c-m-5vhzgjw9/api/v1/nodes/node1"
},
"actions": {
    "disableCPUManager": "https://xxxx/k8s/clusters/c-m-5vhzgjw9/v1/harvester/nodes/node1?action=disableCPUManager",
    "enableCPUManager": "https://xxxx/k8s/clusters/c-m-5vhzgjw9/v1/harvester/nodes/node1?action=enableCPUManager",
    "listUnhealthyVM": "https://xxxx/k8s/clusters/c-m-5vhzgjw9/v1/harvester/nodes/node1?action=listUnhealthyVM",
    "maintenancePossible": "https://xxxx/k8s/clusters/c-m-5vhzgjw9/v1/harvester/nodes/node1?action=maintenancePossible",
    "powerActionPossible": "https://xxxx/k8s/clusters/c-m-5vhzgjw9/v1/harvester/nodes/node1?action=powerActionPossible"
},

Besides, there is no "remove" in links map. How does it work? And it seems dashboard has checked canDelete.

https://github.com/harvester/dashboard/blob/b78181df8695b69177c2efcf185eba54303a98ff/shell/plugins/dashboard-store/resource-class.js#L885-L894

@torchiaf
Copy link
Collaborator

The UI gets the actions list from norman & steve schema API response

For instance, node is a norman schema -> https://localhost:8005/v3/schemas
The payload is:

image

collectionMethods and resourceMethods are the list of actions allowed for my user.

The mechanism should be the same for Harvester's schemas
Harvester is already preventing standard users from deleting nodes, so I think the same logic could be used to remove the action from resourceMethods in the node's schema.

@houhoucoop
Copy link
Collaborator Author

I think I found the root cause, this.canDelete is being set to true here.
Ref: https://github.com/harvester/harvester-ui-extension/blob/main/pkg/harvester/models/harvester/node.js#L470-L474

@torchiaf @Yu-Jack Thanks for stepping in here. It helped me gain a better understanding of the Harvester architecture.

@torchiaf
Copy link
Collaborator

torchiaf commented Jan 17, 2025

I think I found the root cause, this.canDelete is being set to true here. Ref: https://github.com/harvester/harvester-ui-extension/blob/main/pkg/harvester/models/harvester/node.js#L470-L474

@torchiaf @Yu-Jack Thanks for stepping in here. It helped me gain a better understanding of the Harvester architecture.

Thanks Yiya, so I would change the function like this:

  get canDelete() {
    const nodes = this.$rootGetters['harvester/all'](NODE) || [];

    return nodes.filter((n) => n.canDelete()).length > 1;
  }

and test the action as cluster member user

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.

4 participants