fix(SecureView): hide disfunctional *download* files action #5171
Conversation
github-project-automation
bot
moved this to 🧭 Planning evaluation (don't pick)
in 📝 Office team
blizzz
moved this from 🧭 Planning evaluation (don't pick)
to 🏗️ In progress
in 📝 Office team
| // FIXME: coordinate with Files how a better solution looks like. Maybe by setting it only to 'true' by any provider? To avoid overwriting. Or throwing a dedicated event in just this case? | ||
| $propFind->set(FilesPlugin::SHARE_HIDE_DOWNLOAD_PROPERTYNAME, 'true'); | ||
| // avoid potential race condition with FilesPlugin that may set it to "false" | ||
| $propFind->handle(FilesPlugin::SHARE_HIDE_DOWNLOAD_PROPERTYNAME, 'true'); |
There was a problem hiding this comment.
@susnux this works, but is uncomfortably hacky. Maybe sufficient for 32 and 31, but what do you think of making the dav app emit an event here and stay the whole owner of that attribute? Listeners can then mark a node download-hidden only.
There was a problem hiding this comment.
I am not that sure because for the DAV part probably @salmart-dev or @CarlSchwan would be good to also include in the process.
There was a problem hiding this comment.
@blizzz wasn't the set alone enough? The plugin in core is using handle, which should not override an already set value 🤔
The event solution sounds like a good idea if we need to make sure that the property is not purposefully or inadvertently set back to false and can live with more complex flow/code.
If it's a common case, another idea would be to add some code in Sabre/dav to "declare" a property as "irreversible" once a certain value has been set. So that any subsequent set or handle are not executed any longer. The requirement would be that whichever plugin owns the property has to be the first one setting it.
Another solution to make the above more generic would be to allow declaring a property with a callable parameter which checks if the value should be updatable/settable or not. This way one could implement more variants. I'm not sure which other use-cases we have though, so I'm not sure if this even makes sense, but would avoid the extra event, listener, handling complexity.
blizzz
force-pushed
the
fix/5170/hide-download-button-on-secure-view
branch
from
901762a to
2896f1e
Compare
blizzz
force-pushed
the
fix/5170/hide-download-button-on-secure-view
branch
2 times, most recently
from
58974e7 to
3b27369
Compare
This is achieved by setting a specific DAV attribute. At the moment there is one handler in dav-apps FilesPlugin and it could overwrite the value with "false". We make sure not to downgrade here and prevent downgrade from dav (possible race condition). Signed-off-by: Arthur Schiwon <[email protected]>
blizzz
force-pushed
the
fix/5170/hide-download-button-on-secure-view
branch
from
9a451c7 to
b62b572
Compare
blizzz
added
3. to review
|
As discussed with @juliusknorr we'd take it like this (provided reviews are fine) and follow up later on with a better solution across NC. PR comes with tests :) |
|
/backport to stable32 |
|
/backport to stable31 |
elzody
left a comment
elzody
left a comment
There was a problem hiding this comment.
Seems to work fine as it is after some quick testing locally
This is achieved by setting a specific DAV attribute. At the moment there is one handler in dav-apps FilesPlugin and it could overwrite the value with "false". We make sure not to downgrade here and prevent downgrade from dav (possible race condition).
Summary
TODO
Checklist