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 Add permission methods based on owner #134

Merged

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Dec 5, 2023

Note

The behaviour of the link field itself is out of scope - see #103

Issue

@GuySartorelli GuySartorelli marked this pull request as draft December 5, 2023 22:06
@GuySartorelli GuySartorelli mentioned this pull request Dec 5, 2023
@GuySartorelli GuySartorelli marked this pull request as ready for review December 12, 2023 03:50
Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

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

Could you provide an example of how I could test these changes, it seems to me that I am testing it incorrectly.
Steps I took for testing:

  • I have a Page
  • The page has all can* methods that return true.
  • I have an ExternalLinkExtension which has a canView which returns false.
  • When I create a new link, no matter what type, I do not see the new link in the LinkField.
  • If I change the return value of ExternalLinkExtension::canView to true then everything works.

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Dec 13, 2023

  • I have a Page
  • The page has all can* methods that return true.
  • I have an ExternalLinkExtension which has a canView which returns false.
  • When I create a new link, no matter what type, I do not see the new link in the LinkField.
  • If I change the return value of ExternalLinkExtension::canView to true then everything works.

Seeing the permissions be respected in the LinkField is out of scope - you should call the can* method directly and check the returned value is what you think it should be.
But in this case, what you're seeing is the expected behaviour.

Here's what should happen, in a variety of scenarios:

If there are no extensions and no Owner

  • canView(), canEdit(), and canDelete() should use the default value as defined in DataObject
  • canCreate() should return true

If there are no extensions but there is an Owner

  • canView() should return the same as $this->Owner()->canView()
  • canEdit() and canDelete() should return the same as $this->Owner()->canEdit()
  • canCreate() should return true

If there are extensions

Regardless of the value on the owner, or even if there is an owner or not, if the extension's can* method returns true or false, that value will be returned.

If the extension's can* method returns null, it is as though the extension isn't there, so use the appropriate heading above for whether there is an owner or not.

Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

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

LGTM. Test locally.

@sabina-talipova sabina-talipova merged commit 55208f8 into silverstripe:4 Dec 14, 2023
10 checks passed
@sabina-talipova sabina-talipova deleted the pulls/4/permission-model branch December 14, 2023 19:00
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