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

ENH Tidy up permissions #141

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Dec 18, 2023

Issue #103

This PR does a few things:

  • Checks canView() for related data on SiteTreeLink and FileLink
  • Add unit tests for existing logic to make the link form readonly if cannot canEdit() or canCreate()
  • Makes the react 'clear' (delete) button display conditional on Link.canDelete()
  • Makes the react 'create' "button" or "cannot create" div display conditional on Link.canCreate()
  • Uncomments a bunch of mistakenly commented out dataProvider items which all passed when uncommented

@emteknetnz emteknetnz force-pushed the pulls/4/permissions branch 4 times, most recently from 9d81374 to 3c11fad Compare December 19, 2023 02:29
@emteknetnz emteknetnz marked this pull request as ready for review December 19, 2023 02:37
@@ -230,15 +231,15 @@ public function provideLinkFormPost(): array
// note: not duplicating code paths already tested with provideLinkFormGetSchema()
// e.g. Reject Invalid ID
return [
// 'Valid update existing record' => [
Copy link
Member Author

Choose a reason for hiding this comment

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

A lot of these items in the data provider were previously merged while commented out, I've uncommented them

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

canCreate

With canCreate returning false, it still looks like I can add links:
image

The modal is set to readonly, but I shouldn't even be able to get to the modal.

canView for file and sitetree

It feels a bit weird that the "File Link:" and "SiteTree Link:" are just left blank - maybe we should have a "Cannot view file/page" or similar message there in red instead?

Also, do we have opinions about how the modal should treat those?
i.e:

  • the treedropdown is still editable and shows the name of the page
    • Should it be editable if the user doesn't have view access to the page currently stored there?
    • Should the page title be visible in that field?
  • The file upload field is still editable, but shows nothing (as though no file was saved)
    • Should it be editable?
    • Should it provide some feedback that there is a file saved there, but the current user just isn't allowed to see it?

babel.config.json Show resolved Hide resolved
client/src/components/LinkPicker/LinkPicker.js Outdated Show resolved Hide resolved
client/src/components/LinkPicker/LinkPicker.js Outdated Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/4/permissions branch 2 times, most recently from e11043b to 08a8862 Compare December 20, 2023 22:45
@emteknetnz
Copy link
Member Author

  • Have added a trait to LinkField/MultiLinkField that gets the Owner object from the Form, this allows us to check canCreate() on the Owner rather than using $field->Readonly()
  • Have added a 'Cannot view file/page' styles messages instead of empty string
  • Re the 'editable' bits for treedropdown and file upload - This PR adds a 'Cannot create link' if it fails a canCreate() check - this means you can't open the modal

@emteknetnz emteknetnz force-pushed the pulls/4/permissions branch 2 times, most recently from 163c11c to 0559f50 Compare December 20, 2023 23:33
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

If I set Link::canCreate() to return false, I still get the option to open the modal. The changes I've requested in the code should fix this.

Re the 'editable' bits for treedropdown and file upload - This PR adds a 'Cannot create link' if it fails a canCreate() check - this means you can't open the modal

This isn't about whether I can create a link. The scenarios is this:

  • I already have a link to a file or to a page
  • I do not have canView() permissions for the file/page that is being linked to
  • I click on the link to edit it (which I can do - I do have edit permissions for the link itself)

src/Form/MultiLinkField.php Outdated Show resolved Hide resolved
src/Models/FileLink.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member

I haven't tested this in a gridfield record or in an elemental block yet - just testing on a page until everything is working correctly in that context, then I'll check other contexts.

@emteknetnz emteknetnz force-pushed the pulls/4/permissions branch 2 times, most recently from 929f905 to 5856507 Compare December 21, 2023 03:01
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Nearly there.

I've spun off my UX inconsistency concerns into a separate card: #147

public function getSchemaStateDefaults()
{
$data = parent::getSchemaStateDefaults();
$data['canCreate'] = $this->getOwner()->canEdit() && Link::singleton()->canCreate();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$data['canCreate'] = $this->getOwner()->canEdit() && Link::singleton()->canCreate();
$data['canCreate'] = $this->getOwner()->canEdit();

You made a good point while we were discussing this in person - we have to respect the actual type of link being created - if we can create a PhoneLink but not Link then we need to respect that. You're doing that with the change to AllowedLinkClassesTrait so we should remove the canCreate() check from here.

You can also remove the canEdit (and this entire prop and its functionality) if you want - if owner's canEdit() method returns false then the field will be set to readonly (which is being handled separately in #11) so this prop won't matter in that scenario anyway.

We should probably also add some logic (assuming it's not there already) so that when there are no available link types for the field, it has a nice message instead of a weirdly empty link type menu - though I'd be okay with that being spun off separately.

Copy link
Member

Choose a reason for hiding this comment

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

Please also change this in the other places that pass this prop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

Have added AC to the readonly card to revisit the canEdit() logic, decide what to do with it then

Have created new card to handle the empty list scenario

…emove-menu

ENH Remove LinkFieldController from cms menu
@emteknetnz emteknetnz mentioned this pull request Dec 21, 2023
3 tasks
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM, works well locally

@GuySartorelli GuySartorelli merged commit 3528526 into silverstripe:4 Dec 21, 2023
10 checks passed
@GuySartorelli GuySartorelli deleted the pulls/4/permissions branch December 21, 2023 20:08
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