OpenURI: open file with write access when requested

[grulja]

When user specifies to open a file with write access, we need to open it with specified permission, otherwise xdg-desktop-portal will fail as it checks for write access.

Also follow glib and do not use O_PATH, because since xdg-desktop-portal 1.0.1, regular file descriptors are also accepted.

For reference:

1. Glib change
2. Portal bug

[grulja]

The check for write access in xdg-desktop-portal is here.

[hadess]

The check for write access in xdg-desktop-portal is here.

Should that check also be reversed? !writable && fd_is_writable
If the check was also made, then libportal would trigger a warning, which this MR would fix.

If you can check that that's the case, then I'm happy with this change.

[grulja]

The check for write access in xdg-desktop-portal is here.

Should that check also be reversed? !writable && fd_is_writable
If the check was also made, then libportal would trigger a warning, which this MR would fix.

That check is correct in my opinion. It checks whether we specified the file should be writable (you can request that with a flag in libportal) and whether the passed file descriptor is writable. If we specify it is writable, but we don't open the file with write access, then it obviously fails the check and you are not allowed to open the file.

[hadess]

That check is correct in my opinion.

I meant we should have both checks, not that this one was incorrect

eg:

Should that check also be reversed?

[grulja]

I meant we should have both checks, not that this one was incorrect

I misunderstood before what you meant.

We should have both:
if (!writable && fd_is_writable || writable &&!fd_is_writable)
This would also disallow a fd with write access which we didn't request, this however doesn't need any change on libportal side, while the condition writable &&!fd_is_writable needs this change, otherwise it will silently fail.

[hadess]

This would also disallow a fd with write access which we didn't request, this however doesn't need any change on libportal side, while the condition writable &&!fd_is_writable needs this change, otherwise it will silently fail.

I'd like xdg-desktop-portal to throw a warning for the current libportal code. Then the patch in this MR becomes obviously correct.

[grulja]

I will try to write a test case for xdg-desktop-portal. I had planned this anyway.

[grulja]

I discussed this with @matthiasclasen yesterday, the test case for this requires a sandbox environment and we don't do the tests this way in xdg-desktop-portal yet and I don't want to spend that much time on it since it shouldn't be mandatory for this fix, because it's clear and obvious. Can we merge it then?

[hadess]

I didn't ask for a test case to be written, I said:

I'd like xdg-desktop-portal to throw a warning for the current libportal code. Then the patch in this MR becomes obviously correct.

So, one commit to add the warning. One commit that fixes that warning with some details like a backtrace, or at least the error message.

[hadess]

So, one commit to add the warning. One commit that fixes that warning with some details like a backtrace, or at least the error message.

Can you try reproducing the issue with this patch and running xdg-desktop-portal with G_MESSAGES_DEBUG=all:
flatpak/xdg-desktop-portal#474

It should print a warning about Rejecting open request as opening... which you can add in the commit message for this patch. Thanks!

[hadess]

The error message is:

DEBUG: 23:48:22.586: Rejecting open request for /tmp/xdp-test-263CJ0/test.txt as opening not writable but fd is writable

[grulja]

I already had an error message there as I put it there myself when I was debugging this issue, based on that I wrote this patch.

Anyway, I built xdg-desktop-portal from your branch, dropped this patch from my local Dolphin build and I got your error message:
** (/usr/libexec/xdg-desktop-portal:55877): DEBUG: 21:32:54.753: Rejecting open request for /home/jgrulich/xdp-kde.patch as opening writable but fd is not writable

[hadess]

Can you update the commit message to include the error message as we discussed, so we can merge this?

[grulja]

Can you update the commit message to include the error message as we discussed, so we can merge this?

Done.