Change the default on File Grant

[maxime-rainville]

File::getURL() and getSourceURL() implicitly session grant you access to view the file. This has cause us no ends of grievance over the years. We should change that behaviour in CMS5 either to:

• change to default to require false so you don't get an implicit session grant or
• only give you a session grant if you have read access to the file.

silverstripe-assets/src/File.php

Lines 894 to 914 in db24a78

	public function getURL($grant = true)
	{
	if ($this->File->exists()) {
	return $this->File->getURL($grant);
	}
	return null;
	}
	/**
	* Get URL, but without resampling.
	*
	* @param bool $grant Ensures that the url for any protected assets is granted for the current user.
	* @return string
	*/
	public function getSourceURL($grant = true)
	{
	if ($this->File->exists()) {
	return $this->File->getSourceURL($grant);
	}
	return null;
	}

While the change itself is pretty simple it could have a lot of downstream affect that might not be immediately apparent.

Acceptance criteria

• Default methods doesn't auto grant you session access to view files
• There's a sensible way to session grant you access to the file, but it's not used by default
• Not using session grant out of the box doesn't have a big performance impact.
• Validate that no other methods on File auto session grant you access to file.

Note

• Maybe the default URL methods make a CanView check and grant you session access for later.
• This may break a few things along the way. The obvious problems can be fixed on the spot and/or follow up cards can be created.

PR

• ENH Check canViewFile permissions before automatically grant access #517

[maxime-rainville]

@silverstripe/core-team Any views on this. Auto-grant was very needed early on, but we updater the asset store to do a can view check in later minors.

Do we expect this to cause a lot of headaches for people upgrading?

Do we have a preferences between having dedicated method for sessions granting vs changing the default parameter to false?

[sminnee]

The second option - checking permissions before providing a session grant - seems like a good default as it preserves the intent of the original implementation while clearing out bad cases.

Of course, it assumes that there is a clear notion of view permissions and so might be limited to use on the File object.

If you had no grant at all by default, what would it mean, specifically, for people upgrading projects? That seems to be an important question to answer when determining upgrade impact.

[emteknetnz]

cms-editors would need to have a view permissions on files explicitly set so that they're able to view draft files. There's a couple of ways I can think of doing this:

• Check the FILE_EDIT_ALL permission for a group the cms-editor belongs to, OR
• Add the cms-editor group as a group able to view an ancestor folder of the file (permissions cascade up as intermediate folder 'inherit')

This seems like a fairly minimal upgrade step that would need to be documented

[sminnee]

Yeah I agree that for a major bump that seems fine.

[sabina-talipova]

@maxime-rainville , I implemented "Check canView" option, since if we switch default to $grant = false by default, ADMIN also loses ability to be granted by default. And it requires from users to invert passing argument everywhere they invoke this method as well.
But these changes breaks logic in FileShortcodeProvider and even if user set allow_session_grant to true, he won't be granted access to the file if he doesn't have permission to view file.

[GuySartorelli]

@sabina-talipova
From my understanding of the above comments from Steve and Sam, the acceptance criteria as written still apply - i.e. a grant should not be applied by default.

This is based mostly on this part of the conversation:

If you had no grant at all by default, what would it mean, specifically, for people upgrading projects? That seems to be an important question to answer when determining upgrade impact.

cms-editors would need to have a view permissions on files explicitly set so that they're able to view draft files. ... This seems like a fairly minimal upgrade step that would need to be documented

Yeah I agree that for a major bump that seems fine.

@emteknetnz Can you please confirm that your intention is to say that it would be appropriate to implement this chance as per the acceptance criteria?

[emteknetnz]

Default of the optional param must be $grant = false - that's the primary purpose of this issue