Skip to content

fix(tables): support invoking hidden actions #112

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

Merged
merged 3 commits into from
Jan 24, 2024

Conversation

owenconti
Copy link
Contributor

This PR creates a failing test for an issue I've discovered. I'm happy to submit a PR for the fix if you could point me in the direction you'd like me to take.

Issue

If you have a conditionally hidden action and the condition depends on part of the request (ie: in my case I need to hide the action by default and then show the action when a specific refinement is applied), then you will receive a 500 when trying to invoke that action.

For example:

  • I use the default TrashedFilter to show active vs trashed records
  • I have an action to restore a record, but this action is only visible when the TrashedFilter is being applied
  • When trying to invoke the action, I receive a 500 because the following chunk of code from HasActions cannot find the specified action since it is hidden by default:
    public function getActions(): Collection
    {
        return $this->cachedActions ??= collect($this->defineActions())
            ->filter(static fn (BaseAction $action): bool => !$action->isHidden());
    }

    public function getInlineActions(): Collection
    {
        return $this->getActions()->filter(static fn (BaseAction $action): bool => $action instanceof InlineAction);
    }

    public function getBulkActions(): Collection
    {
        return $this->getActions()->filter(static fn (BaseAction $action): bool => $action instanceof BulkAction);
    }

Fixing the issue

My initial thought would be to create a getAllActions() method in HasActions and call that within getInlineActions and getBulkActions. One issue with that approach is it would essentially remove any sort of authorization enforcement on actions. You may want to consider a new method on actions for authorization (may be a good idea anyways).

Copy link

netlify bot commented Jan 23, 2024

Deploy Preview for hybridly canceled.

Name Link
🔨 Latest commit 256e2be
🔍 Latest deploy log https://app.netlify.com/sites/hybridly/deploys/65b0e68c5d9d0300086e65ab

@innocenzi innocenzi changed the title Add (failing) test for invoking conditionally hidden actions fix(tables): support invoking hidden actions Jan 24, 2024
@innocenzi innocenzi merged commit 0e89048 into hybridly:0.x Jan 24, 2024
@innocenzi
Copy link
Member

innocenzi commented Jan 24, 2024

Hey, thank you for the failing test! Makes it super easy for me to handle the pull request.

I went the simple route and added a way to bypass the filter when getting all actions. It's not super pretty but I'd rather have that than adding new methods.

As you said, the potential issue with that is that an actor could send a forged request to the InvokedActionController endpoint to call an action that would be hidden.

However, in practice, actions should be guarded with policies so I don't think it's that much of a problem.

Your suggestion to add an authorization handling on the action API is interesting, but for now I'd like to keep it simple.

Thanks for the contribution Owen!

@owenconti
Copy link
Contributor Author

Thanks @innocenzi! Is the guarding of actions something that is built into Hybridly or are you suggesting the actions manually authorize themselves via a gate?

@innocenzi
Copy link
Member

Actions are not guarded at all, because Hybridly can't really know what to guard against. This is a generic endpoint.

The only "guarding" is the verification that the requested table and action exist, so this entrypoint can't just execute arbitrary code.

You may add your own middleware in config/hybridly.php for that endpoint, but the best practice is to add policies to each action, so through the Gate for instance, yes 👍

@owenconti owenconti deleted the hidden-actions-issue branch January 24, 2024 22:31
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