New Tizen.Security.PrivacyPrivilegeManager#7720
Draft
wiertel wants to merge 4 commits into
Draft
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modernizes the PrivacyPrivilegeManager API by transitioning from an obsolete callback/event-based pattern to a modern async/await-based model, introducing new types like PermissionState and PermissionDeniedException while removing several deprecated classes. The code review identified critical issues that need to be addressed: a potential crash due to garbage collection of the native callback delegate (which requires a GCHandle to keep it alive), namespace-sensitivity when parsing tizen-manifest.xml that could prevent privileges from being loaded, and strict equality checks on PermissionState.Granted that fail to account for other granted states like GrantedSession and GrantedInUse. Additionally, it is recommended to optimize permission checking by utilizing the native bulk API instead of looping over individual checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+294
to
348
| private static async Task<IDictionary<string, PermissionState>> RequestPermissionsInternal(List<string> privileges) | ||
| { | ||
| var tcs = new TaskCompletionSource<IDictionary<string, PermissionState>>(); | ||
|
|
||
| Interop.PrivacyPrivilegeManager.RequestMultipleResponseCallback callback = | ||
| (cause, results, requestedPrivileges, privilegesCount, userData) => | ||
| { | ||
| add | ||
| try | ||
| { | ||
| if (_ResponseFetched == null) | ||
| var states = new Dictionary<string, PermissionState>(); | ||
|
|
||
| for (int i = 0; i < privilegesCount; i++) | ||
| { | ||
| if (!s_responseMap.ContainsKey(_privilege)) | ||
| var state = results[i] switch | ||
| { | ||
| s_responseMap[_privilege] = this; | ||
| } | ||
| Interop.PrivacyPrivilegeManager.RequestResult.AllowForever => PermissionState.Granted, | ||
| Interop.PrivacyPrivilegeManager.RequestResult.DenyForever => PermissionState.Denied, | ||
| Interop.PrivacyPrivilegeManager.RequestResult.DenyOnce => PermissionState.DeniedOnce, | ||
| Interop.PrivacyPrivilegeManager.RequestResult.AllowSession => PermissionState.GrantedSession, | ||
| Interop.PrivacyPrivilegeManager.RequestResult.DenySession => PermissionState.DeniedSession, | ||
| Interop.PrivacyPrivilegeManager.RequestResult.AllowInUse => PermissionState.GrantedInUse, | ||
| _ => PermissionState.Denied | ||
| }; | ||
| states[requestedPrivileges[i]] = state; | ||
| } | ||
| _ResponseFetched += value; | ||
| } | ||
|
|
||
| remove | ||
| { | ||
| _ResponseFetched -= value; | ||
| if (_ResponseFetched == null) | ||
| if (cause == Interop.PrivacyPrivilegeManager.CallCause.Error) | ||
| { | ||
| if (s_responseMap.ContainsKey(_privilege)) | ||
| { | ||
| s_responseMap.Remove(_privilege); | ||
| } | ||
| tcs.SetException(new IOException("Permission request failed")); | ||
| } | ||
| else | ||
| { | ||
| tcs.SetResult(states); | ||
| } | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Log.Error(LogTag, $"Exception in callback: {ex.Message}"); | ||
| tcs.SetException(ex); | ||
| } | ||
| }; | ||
|
|
||
| private event EventHandler<RequestResponseEventArgs> _ResponseFetched; | ||
| int ret = (int)Interop.PrivacyPrivilegeManager.RequestPermissions( | ||
| privileges.ToArray(), | ||
| (uint)privileges.Count, | ||
| callback, | ||
| IntPtr.Zero); | ||
|
|
||
| internal void FireEvent(CallCause _cause, RequestResult _result) | ||
| if (ret != (int)Interop.PrivacyPrivilegeManager.ErrorCode.None) | ||
| { | ||
| _ResponseFetched?.Invoke(this, new RequestResponseEventArgs { cause = _cause, result = _result, privilege = _privilege }); | ||
| throw new IOException($"Failed to request permissions, error: {ret}"); | ||
| } | ||
|
|
||
| return await tcs.Task; | ||
| } |
There was a problem hiding this comment.
The callback delegate is declared as a local variable and passed directly to the asynchronous native method RequestPermissions. Because the native call is asynchronous and returns immediately, the delegate can be garbage collected before the native callback is invoked, leading to a CallbackOnCollectedDelegate crash.
To prevent this, we must keep the delegate alive until the callback is executed. We can achieve this by allocating a GCHandle for the delegate and freeing it inside the callback's finally block (or if the native call fails).
private static async Task<IDictionary<string, PermissionState>> RequestPermissionsInternal(List<string> privileges)
{
var tcs = new TaskCompletionSource<IDictionary<string, PermissionState>>();
System.Runtime.InteropServices.GCHandle handle = default;
Interop.PrivacyPrivilegeManager.RequestMultipleResponseCallback callback =
(cause, results, requestedPrivileges, privilegesCount, userData) =>
{
try
{
var states = new Dictionary<string, PermissionState>();
for (int i = 0; i < privilegesCount; i++)
{
var state = results[i] switch
{
Interop.PrivacyPrivilegeManager.RequestResult.AllowForever => PermissionState.Granted,
Interop.PrivacyPrivilegeManager.RequestResult.DenyForever => PermissionState.Denied,
Interop.PrivacyPrivilegeManager.RequestResult.DenyOnce => PermissionState.DeniedOnce,
Interop.PrivacyPrivilegeManager.RequestResult.AllowSession => PermissionState.GrantedSession,
Interop.PrivacyPrivilegeManager.RequestResult.DenySession => PermissionState.DeniedSession,
Interop.PrivacyPrivilegeManager.RequestResult.AllowInUse => PermissionState.GrantedInUse,
_ => PermissionState.Denied
};
states[requestedPrivileges[i]] = state;
}
if (cause == Interop.PrivacyPrivilegeManager.CallCause.Error)
{
tcs.SetException(new IOException("Permission request failed"));
}
else
{
tcs.SetResult(states);
}
}
catch (Exception ex)
{
Log.Error(LogTag, $"Exception in callback: {ex.Message}");
tcs.SetException(ex);
}
finally
{
if (handle.IsAllocated)
{
handle.Free();
}
}
};
handle = System.Runtime.InteropServices.GCHandle.Alloc(callback);
int ret = (int)Interop.PrivacyPrivilegeManager.RequestPermissions(
privileges.ToArray(),
(uint)privileges.Count,
callback,
IntPtr.Zero);
if (ret != (int)Interop.PrivacyPrivilegeManager.ErrorCode.None)
{
if (handle.IsAllocated)
{
handle.Free();
}
throw new IOException($"Failed to request permissions, error: {ret}");
}
return await tcs.Task;
}
Comment on lines
+231
to
+236
| var doc = XDocument.Load(manifestPath); | ||
| var privileges = doc | ||
| .Descendants("privilege") | ||
| .Select(e => e.Value.Trim()) | ||
| .Where(v => !string.IsNullOrEmpty(v)) | ||
| .ToList(); |
There was a problem hiding this comment.
In Tizen, tizen-manifest.xml typically defines a default XML namespace (e.g., xmlns="http://tizen.org/ns/packages"). Using doc.Descendants("privilege") is namespace-sensitive and will fail to find any elements when a default namespace is present, resulting in an empty list of privileges.
To make this namespace-agnostic and robust, we should filter descendants by their LocalName.
var doc = XDocument.Load(manifestPath);
var privileges = doc
.Descendants()
.Where(e => e.Name.LocalName == "privilege")
.Select(e => e.Value.Trim())
.Where(v => !string.IsNullOrEmpty(v))
.ToList();
Comment on lines
+97
to
101
| public static async Task<bool> CheckIfAllPermissionsAreGranted(IEnumerable<string> privileges) | ||
| { | ||
| string[] privilegesArray = CheckPrivilegesArgument(privileges, "CheckPermissions"); | ||
|
|
||
| Interop.PrivacyPrivilegeManager.CheckResult[] results = new Interop.PrivacyPrivilegeManager.CheckResult[privilegesArray.Length]; | ||
| int ret = (int)Interop.PrivacyPrivilegeManager.CheckPermissions(privilegesArray, (uint)privilegesArray.Length, results); | ||
| if (ret != (int)Interop.PrivacyPrivilegeManager.ErrorCode.None) | ||
| { | ||
| Log.Error(LogTag, "Failed to check permission"); | ||
| throw PrivacyPrivilegeManagerErrorFactory.GetException(ret); | ||
| } | ||
|
|
||
| CheckResult[] checkResults = new CheckResult[results.Length]; | ||
| for (int iterator = 0; iterator < results.Length; ++iterator) | ||
| { | ||
| checkResults[iterator] = (CheckResult)results[iterator]; | ||
| } | ||
| return checkResults; | ||
| var states = await CheckPermissions(privileges); | ||
| return states.All(kvp => kvp.Value == PermissionState.Granted); | ||
| } |
There was a problem hiding this comment.
Checking kvp.Value == PermissionState.Granted ignores other valid granted states such as GrantedSession and GrantedInUse. We should use the IsGranted() extension method defined in PermissionStateExtensions to correctly handle all granted states.
public static async Task<bool> CheckIfAllPermissionsAreGranted(IEnumerable<string> privileges)
{
var states = await CheckPermissions(privileges);
return states.All(kvp => kvp.Value.IsGranted());
}
Comment on lines
+161
to
+164
| var alreadyGranted = states.Where(kvp => kvp.Value == PermissionState.Granted) | ||
| .Select(kvp => kvp.Key).ToList(); | ||
| var needRequest = states.Where(kvp => kvp.Value != PermissionState.Granted) | ||
| .Select(kvp => kvp.Key).ToList(); |
There was a problem hiding this comment.
Using strict equality kvp.Value == PermissionState.Granted will treat GrantedSession and GrantedInUse as not granted, causing them to be requested again. We should use the IsGranted() extension method instead.
var alreadyGranted = states.Where(kvp => kvp.Value.IsGranted())
.Select(kvp => kvp.Key).ToList();
var needRequest = states.Where(kvp => !kvp.Value.IsGranted())
.Select(kvp => kvp.Key).ToList();
Comment on lines
+181
to
+184
| // Check if any required permissions were denied | ||
| var deniedRequired = effectiveRequired | ||
| .Where(p => states[p] != PermissionState.Granted) | ||
| .ToList(); |
There was a problem hiding this comment.
Checking states[p] != PermissionState.Granted will throw a PermissionDeniedException even if the required permission was successfully granted as GrantedSession or GrantedInUse. We should use the IsGranted() extension method here as well.
// Check if any required permissions were denied
var deniedRequired = effectiveRequired
.Where(p => !states[p].IsGranted())
.ToList();
Comment on lines
+57
to
73
| public static async Task<IDictionary<string, PermissionState>> CheckPermissions(IEnumerable<string> privileges) | ||
| { | ||
| Interop.PrivacyPrivilegeManager.CheckResult result; | ||
| int ret = (int)Interop.PrivacyPrivilegeManager.CheckPermission(privilege, out result); | ||
| if (ret != (int)Interop.PrivacyPrivilegeManager.ErrorCode.None) | ||
| var privilegeList = ValidatePrivileges(privileges, nameof(privileges)); | ||
|
|
||
| return await Task.Run(() => | ||
| { | ||
| Log.Error(LogTag, "Failed to check permission"); | ||
| throw PrivacyPrivilegeManagerErrorFactory.GetException(ret); | ||
| } | ||
| return (CheckResult)result; | ||
| var results = new Dictionary<string, PermissionState>(); | ||
|
|
||
| foreach (var privilege in privilegeList) | ||
| { | ||
| var state = CheckSinglePermission(privilege); | ||
| results[privilege] = state; | ||
| } | ||
|
|
||
| return results; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Instead of calling CheckSinglePermission in a loop (which incurs multiple P/Invoke transition overheads), we should use the native bulk API ppm_check_permissions which is already declared in Interop.PrivacyPrivilegeManager.cs.
public static async Task<IDictionary<string, PermissionState>> CheckPermissions(IEnumerable<string> privileges)
{
var privilegeList = ValidatePrivileges(privileges, nameof(privileges));
return await Task.Run(() =>
{
var privilegesArray = privilegeList.ToArray();
var results = new Interop.PrivacyPrivilegeManager.CheckResult[privilegesArray.Length];
int ret = (int)Interop.PrivacyPrivilegeManager.CheckPermissions(privilegesArray, (uint)privilegesArray.Length, results);
if (ret != (int)Interop.PrivacyPrivilegeManager.ErrorCode.None)
{
Log.Error(LogTag, "Failed to check permissions");
throw new IOException("Failed to check permissions");
}
var dict = new Dictionary<string, PermissionState>();
for (int i = 0; i < privilegesArray.Length; i++)
{
dict[privilegesArray[i]] = results[i] switch
{
Interop.PrivacyPrivilegeManager.CheckResult.Allow => PermissionState.Granted,
Interop.PrivacyPrivilegeManager.CheckResult.Deny => PermissionState.Denied,
Interop.PrivacyPrivilegeManager.CheckResult.Ask => PermissionState.DeniedOnce,
Interop.PrivacyPrivilegeManager.CheckResult.AllowSession => PermissionState.GrantedSession,
Interop.PrivacyPrivilegeManager.CheckResult.DenySession => PermissionState.DeniedSession,
Interop.PrivacyPrivilegeManager.CheckResult.AllowInUse => PermissionState.GrantedInUse,
_ => PermissionState.Denied
};
}
return dict;
});
}
Collaborator
Public API ChangedPlease follow the ACR process for the changed API below. Added: 19, Removed: 31, Changed: 2Added+ /// <since_tizen>none</since_tizen
+ Tizen.Security.PermissionDeniedException
+ /// <since_tizen>none</since_tizen
+ System.Collections.Generic.IReadOnlyList`1<System.String> Tizen.Security.PermissionDeniedException::DeniedPrivileges()
+ /// <since_tizen>none</since_tizen
+ System.Void Tizen.Security.PermissionDeniedException::.ctor(System.Collections.Generic.IReadOnlyList`1<System.String>,System.String,System.Exception)
+ /// <since_tizen>none</since_tizen
+ System.Void Tizen.Security.PermissionDeniedException::.ctor(System.Collections.Generic.IReadOnlyList`1<System.String>,System.String)
+ /// <since_tizen>none</since_tizen
+ System.Void Tizen.Security.PermissionDeniedException::.ctor(System.Collections.Generic.IReadOnlyList`1<System.String>)
+ /// <since_tizen>none</since_tizen
+ Tizen.Security.PermissionState
+ /// <since_tizen>none</since_tizen
+ static Tizen.Security.PermissionState Tizen.Security.PermissionState::Denied
+ /// <since_tizen>none</since_tizen
+ static Tizen.Security.PermissionState Tizen.Security.PermissionState::DeniedOnce
+ /// <since_tizen>none</since_tizen
+ static Tizen.Security.PermissionState Tizen.Security.PermissionState::DeniedSession
+ /// <since_tizen>none</since_tizen
+ static Tizen.Security.PermissionState Tizen.Security.PermissionState::Granted
+ /// <since_tizen>none</since_tizen
+ static Tizen.Security.PermissionState Tizen.Security.PermissionState::GrantedInUse
+ /// <since_tizen>none</since_tizen
+ static Tizen.Security.PermissionState Tizen.Security.PermissionState::GrantedSession
+ /// <since_tizen>none</since_tizen
+ Tizen.Security.PermissionStateExtensions
+ /// <since_tizen>none</since_tizen
+ static System.Boolean Tizen.Security.PermissionStateExtensions::IsDenied(Tizen.Security.PermissionState)
+ /// <since_tizen>none</since_tizen
+ static System.Boolean Tizen.Security.PermissionStateExtensions::IsGranted(Tizen.Security.PermissionState)
+ /// <since_tizen>none</since_tizen
+ static System.Boolean Tizen.Security.PermissionStateExtensions::IsTemporary(Tizen.Security.PermissionState)
+ /// <since_tizen>14</since_tizen
+ static System.Threading.Tasks.Task`1<System.Boolean> Tizen.Security.PrivacyPrivilegeManager::CheckIfAllPermissionsAreGranted(System.Collections.Generic.IEnumerable`1<System.String>)
+ /// <since_tizen>14</since_tizen
+ static System.Threading.Tasks.Task`1<System.Collections.Generic.IDictionary`2<System.String,Tizen.Security.PermissionState>> Tizen.Security.PrivacyPrivilegeManager::RequestPermissions(System.Collections.Generic.IEnumerable`1<System.String>,System.Collections.Generic.IEnumerable`1<System.String>)
+ /// <since_tizen>14</since_tizen
+ static System.Threading.Tasks.Task`1<System.Collections.Generic.List`1<System.String>> Tizen.Security.PrivacyPrivilegeManager::LoadPrivilegesFromManifest()
Removed- /// <since_tizen>4</since_tizen
- Tizen.Security.CallCause
- /// <since_tizen>4</since_tizen
- static Tizen.Security.CallCause Tizen.Security.CallCause::Answer
- /// <since_tizen>4</since_tizen
- static Tizen.Security.CallCause Tizen.Security.CallCause::Error
- /// <since_tizen>4</since_tizen
- Tizen.Security.CheckResult
- /// <since_tizen>4</since_tizen
- static Tizen.Security.CheckResult Tizen.Security.CheckResult::Allow
- /// <since_tizen>4</since_tizen
- static Tizen.Security.CheckResult Tizen.Security.CheckResult::Ask
- /// <since_tizen>4</since_tizen
- static Tizen.Security.CheckResult Tizen.Security.CheckResult::Deny
- /// <since_tizen>none</since_tizen
- Tizen.Security.PermissionRequestResponse
- /// <since_tizen>6</since_tizen
- System.String Tizen.Security.PermissionRequestResponse::Privilege()
- /// <since_tizen>6</since_tizen
- Tizen.Security.RequestResult Tizen.Security.PermissionRequestResponse::Result()
- /// <since_tizen>none</since_tizen
- System.Void Tizen.Security.PermissionRequestResponse::.ctor()
- /// <since_tizen>6</since_tizen
- [Obsolete]
- static System.Threading.Tasks.Task`1<Tizen.Security.RequestMultipleResponseEventArgs> Tizen.Security.PrivacyPrivilegeManager::RequestPermissions(System.Collections.Generic.IEnumerable`1<System.String>)
- /// <since_tizen>4</since_tizen
- [Obsolete]
- static System.Void Tizen.Security.PrivacyPrivilegeManager::RequestPermission(System.String)
- /// <since_tizen>4</since_tizen
- [Obsolete]
- static System.WeakReference`1<Tizen.Security.PrivacyPrivilegeManager/ResponseContext> Tizen.Security.PrivacyPrivilegeManager::GetResponseContext(System.String)
- /// <since_tizen>4</since_tizen
- [Obsolete]
- static Tizen.Security.CheckResult Tizen.Security.PrivacyPrivilegeManager::CheckPermission(System.String)
- /// <since_tizen>4</since_tizen
- [Obsolete]
- Tizen.Security.PrivacyPrivilegeManager/ResponseContext
- /// <since_tizen>4</since_tizen
- [Obsolete]
- System.EventHandler`1<Tizen.Security.RequestResponseEventArgs> Tizen.Security.PrivacyPrivilegeManager/ResponseContext::ResponseFetched
- /// <since_tizen>6</since_tizen
- Tizen.Security.RequestMultipleResponseEventArgs
- /// <since_tizen>6</since_tizen
- System.Collections.Generic.IEnumerable`1<Tizen.Security.PermissionRequestResponse> Tizen.Security.RequestMultipleResponseEventArgs::Responses()
- /// <since_tizen>6</since_tizen
- Tizen.Security.CallCause Tizen.Security.RequestMultipleResponseEventArgs::Cause()
- /// <since_tizen>none</since_tizen
- System.Void Tizen.Security.RequestMultipleResponseEventArgs::.ctor()
- /// <since_tizen>4</since_tizen
- Tizen.Security.RequestResponseEventArgs
- /// <since_tizen>4</since_tizen
- System.String Tizen.Security.RequestResponseEventArgs::privilege()
- /// <since_tizen>4</since_tizen
- Tizen.Security.CallCause Tizen.Security.RequestResponseEventArgs::cause()
- /// <since_tizen>6</since_tizen
- Tizen.Security.PermissionRequestResponse Tizen.Security.RequestResponseEventArgs::Response()
- /// <since_tizen>4</since_tizen
- Tizen.Security.RequestResult Tizen.Security.RequestResponseEventArgs::result()
- /// <since_tizen>none</since_tizen
- System.Void Tizen.Security.RequestResponseEventArgs::.ctor()
- /// <since_tizen>4</since_tizen
- Tizen.Security.RequestResult
- /// <since_tizen>4</since_tizen
- static Tizen.Security.RequestResult Tizen.Security.RequestResult::AllowForever
- /// <since_tizen>4</since_tizen
- static Tizen.Security.RequestResult Tizen.Security.RequestResult::DenyForever
- /// <since_tizen>4</since_tizen
- static Tizen.Security.RequestResult Tizen.Security.RequestResult::DenyOnce
Changed- /// <since_tizen>4</since_tizen
[Obsolete]
+ /// <since_tizen>14</since_tizen
Tizen.Security.PrivacyPrivilegeManager
- /// <since_tizen>6</since_tizen
[Obsolete]
static System.Collections.Generic.IEnumerable`1<Tizen.Security.CheckResult> Tizen.Security.PrivacyPrivilegeManager::CheckPermissions(System.Collections.Generic.IEnumerable`1<System.String>)
+ /// <since_tizen>14</since_tizen
static System.Threading.Tasks.Task`1<System.Collections.Generic.IDictionary`2<System.String,Tizen.Security.PermissionState>> Tizen.Security.PrivacyPrivilegeManager::CheckPermissions(System.Collections.Generic.IEnumerable`1<System.String>)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Change
PrivacyPrivilegeManager has been deprecated and it's time to remove it, but there is a request to restore it and expand. I would like to take this opportunity and try to rewrite the API to be more convenient.
API Changes
TBD, in short current state is: