Repository navigation
Add conditional permission support - #886
Malika7188 wants to merge 7 commits into
Conversation
8369490 to
d71264f
Compare
d71264f to
a79d0a4
Compare
Previously, conditional permission entries (if:...) were discarded entirely while parsing, to avoid treating them as unsupported and corrupting the overrides file. This now loads the underlying permission through the normal path, the same as any other option, and separately records the raw condition. A new status icon marks a permission row as conditional, with a tooltip stating the condition it depends on
a79d0a4 to
84181ad
Compare
tchx84
left a comment
There was a problem hiding this comment.
Besides the other comments, in order to design this in a future-proof / defensive way, imagine if tomorrow some other permission adds support for conditional but Flatseal doesn't support it, what Flatseal's behavior should be?
c69d942 to
6abc833
Compare
I’ve fixed this. If a future permission adds conditional support that Flatseal doesn’t recognize yet, it now falls into the same unsupported bucket as a normal unrecognized permission and is preserved as it is, including the |
Per-app conditional permissions were silently dropped on unrelated saves instead of being preserved. Preserve a per-app conditional exactly as it was loaded unless the option's current value no longer matches what the conditional's own bare entry implies. In that case, the user's explicit toggle wins, matching how a fresh bare grant clears a conditional in Flatpak. Bump ecmaVersion to 2020 for optional chaining.
96d25cc to
cec8a99
Compare
Recognized conditionals (on shared, sockets, devices, or features permissions) are loaded and shown in the UI, each marked with the condition it depends on. But saveToKeyFile only writes plain on/off overrides, so the condition text is ignored when the override file is saved. Conditional permissions are safely ignored rather than being preserved or modified. Bumped ecmaVersion to 2020 for optional chaining.
cec8a99 to
5c5b9a7
Compare
|
Hi @tchx84 , I've made the changes and removed the optional chaining. The ignore unsupported conditionals case is now its own check at the top of the block. I also reverted Also, I misunderstood your earlier comment about "marking the permission as optional." I thought you meant using optional chaining (?.), that's why I used it. Sorry about that confusion. |
tchx84
left a comment
There was a problem hiding this comment.
I think we can still simplify this while making its scope more clear.
| * guarantee the permission is actually granted at runtime. */ | ||
| markConditional(option, rawValue) { | ||
| this._conditionals.set(option, rawValue); | ||
| } |
There was a problem hiding this comment.
I tested the following scenario:
- Running latest version of this PR.
- Having installed your HelloConditional demo app.
- I launch Flatseal and select HelloConditional.
- I scroll down to the Socket section and negate PulseAudio.
What I see after that is:
I am wondering about the semantics here; it definitely does make sense to display the "conditional" icon when the original permission is still valid but, after it's negated, does it still make sense to display it? Is it adding useful information ?
My first reaction is, probably not. Once we override, the "active" version of that permission is no longer the conditional but an explicit negation (not a conditional negation).
There was a problem hiding this comment.
Agreed. I've changed it so the conditional icon is hidden as soon as the permission is overridden, both by the user and globally. So in your PulseAudio example, only the override icon shows now. If the override is removed, the conditional icon comes back.
| row .conditional { | ||
| padding: 8px; | ||
| background-color: transparent; | ||
| background-image: -gtk-icontheme("dialog-question-symbolic"); |
There was a problem hiding this comment.
Can you explore other icons and colors options for this?
There was a problem hiding this comment.
Here are some of the icon and color options I explored. Let me know what you think about them.
Option 1: The branch-arrow-symbolic choice is because it reads as “this depends on something,” which matches how a conditional permission works.
Option 2: The information icon (dialog-information-symbolic) in the default text color, instead of the current question-mark icon (which Flatseal also uses for help).

Option 3: The information icon (dialog-information-symbolic) in green

| 'if:unsupported-permission:!has-unsupported-permission')).toBe(false); | ||
| expect(has( | ||
| _unsupportedOverride, 'Context', 'unsupported', | ||
| 'unsupported-permission')).toBe(false); |
There was a problem hiding this comment.
Please add a check for the supported "always" permission and double check with a "hasOnly" 1, to explicitly describe the expected outcome and to double check that the unsupported conditional wasn't written back in a different way.
There was a problem hiding this comment.
I tried hasOnly, but it fails here because the saved file also has shared and filesystems lines. Can I use has for always and hasInTotal to make sure nothing else was written, or do I give this test its own fixture so hasOnly works?
| GObject.signal_handler_block(this, this._notifyHandlerId); | ||
|
|
||
| Object.values(MODELS).forEach(model => model.updateStatusProperty(this)); | ||
| CONDITIONAL_MODELS.forEach(model => model.updateConditionalProperty(this)); |
There was a problem hiding this comment.
Another data point regarding conditionals models list; we add a lot of properties that we don't ever use.
There was a problem hiding this comment.
I've updated the implementation so that conditional properties are only created for the four models in CONDITIONAL_MODELS, so we no longer add properties that aren't used.
| permissionsDefault.appId = _conditionalAppId; | ||
|
|
||
| expect(permissionsDefault.features_devel).toBe(true); | ||
| expect(permissionsDefault.features_devel_conditional).toBe(''); |
There was a problem hiding this comment.
Can you remind me what causes this devel conditional not getting marked? I don't see any global-related check in the parsing method.
There was a problem hiding this comment.
I added that to check that conditionals from global overrides are also dropped when they're detected.
Conditionals from an override (per-app or global) are never written back, saveToKeyFile only writes the plain on/off state, so any condition text from an override is silently dropped on save. Displaying a marker for these was misleading, and the user would see a conditional on load, only for it to disappear the next time anything is saved. Only mark conditionals from the app's original metadata, since those are not affected by saving overrides. Added test coverage for both per-app and global override scenarios.
66779b5 to
e996745
Compare
Previously, conditional permission entries (if:...) were discarded
entirely while parsing, to avoid treating them as unsupported and
corrupting the overrides file.
This now loads the underlying permission through the normal path,
the same as any other option, and separately records the raw
condition. A new status icon marks a permission row as conditional,
with a tooltip stating the condition it depends on.
Limitations
Flatpak drops a conditional permission if a bare grant for the same
permission is applied afterward, so the switch showing "on" doesn’t
always mean the permission is actually granted at runtime.
This is Flatpak’s behaviour and not something Flatseal controls.
saveToKeyFileright now only writes from theoverridesSet (the on/off state)and doesn’t look at the conditionals map. So saving can drop an existing
conditionalentry from the override file. Write-back is not implemented yet