-
Notifications
You must be signed in to change notification settings - Fork 32
rc/8.0.0 #216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
rc/8.0.0 #216
Changes from 10 commits
6b1d5c8
1058b9e
bd7894b
56487c3
ba56ae0
6909054
be0649a
e4258fc
ba45995
cccbd74
a545b78
393fa70
8cf8248
aafe0d2
e065364
6d9947b
44e000a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,13 @@ import android.content.Context | |
| import android.content.pm.PackageInfo | ||
| import android.os.Build | ||
| import com.aheaditec.talsec_security.security.api.ExternalIdResult | ||
| import com.aheaditec.talsec_security.security.api.MalwareScanScope | ||
| import com.aheaditec.talsec_security.security.api.ReasonMode | ||
| import com.aheaditec.talsec_security.security.api.ScopeType | ||
| import com.aheaditec.talsec_security.security.api.SuspiciousAppDetectionConfig | ||
| import com.aheaditec.talsec_security.security.api.SuspiciousAppInfo | ||
| import io.flutter.plugin.common.MethodChannel | ||
| import org.json.JSONObject | ||
| import com.aheaditec.freerasp.generated.PackageInfo as FlutterPackageInfo | ||
| import com.aheaditec.freerasp.generated.SuspiciousAppInfo as FlutterSuspiciousAppInfo | ||
|
|
||
|
|
@@ -33,7 +38,7 @@ internal inline fun runResultCatching(result: MethodChannel.Result, block: () -> | |
| * this [SuspiciousAppInfo]. | ||
| */ | ||
| internal fun SuspiciousAppInfo.toPigeon(context: Context): FlutterSuspiciousAppInfo { | ||
| return FlutterSuspiciousAppInfo(this.packageInfo.toPigeon(context), this.reason) | ||
| return FlutterSuspiciousAppInfo(this.packageInfo.toPigeon(context), this.reasons.toList()) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -83,3 +88,42 @@ internal fun ExternalIdResult.resolve(result: MethodChannel.Result) { | |
| is ExternalIdResult.Error -> result.error("external-id-failure", this.errorMsg, null) | ||
| } | ||
| } | ||
|
|
||
| internal fun JSONObject.toMalwareScanScope(): MalwareScanScope { | ||
| val scopeTypeStr = optString("scanScope", "SIDELOADED_ONLY") | ||
| val scanScope = runCatching { ScopeType.valueOf(scopeTypeStr) }.getOrDefault(ScopeType.SIDELOADED_ONLY) | ||
| val trustedInstallSources = optJSONArray("trustedInstallSources")?.let { arr -> | ||
| (0 until arr.length()).map { arr.getString(it) } | ||
| } | ||
| return MalwareScanScope(scanScope, trustedInstallSources) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SIDELOADED_ONLY is hardcoded as a string in optString, then again as an enum in getOrDefault, then again as a fallback object — three places, two languages, no compile-time guarantee they stay in sync. The Dart side makes scanScope required (so it can't be missing on the wire) but the Kotlin side still defends against it being absent with a string default. We need only one source of truth. Suggested approach: just throw exception here if scope is missing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored |
||
| } | ||
|
|
||
| internal fun JSONObject.toSuspiciousAppDetectionConfig(): SuspiciousAppDetectionConfig { | ||
| val packageNames = optJSONArray("packageNames")?.let { arr -> | ||
| (0 until arr.length()).map { arr.getString(it) }.toSet() | ||
| } | ||
| val hashes = optJSONArray("hashes")?.let { arr -> | ||
| (0 until arr.length()).map { arr.getString(it) }.toSet() | ||
| } | ||
| val requestedPermissions = optJSONArray("requestedPermissions")?.let { outer -> | ||
| (0 until outer.length()).map { i -> | ||
| val inner = outer.getJSONArray(i) | ||
| (0 until inner.length()).map { j -> inner.getString(j) }.toSet() | ||
| }.toSet() | ||
| } | ||
| val grantedPermissions = optJSONArray("grantedPermissions")?.let { outer -> | ||
| (0 until outer.length()).map { i -> | ||
| val inner = outer.getJSONArray(i) | ||
| (0 until inner.length()).map { j -> inner.getString(j) }.toSet() | ||
| }.toSet() | ||
| } | ||
| val malwareScanScope = optJSONObject("malwareScanScope")?.toMalwareScanScope() | ||
| ?: MalwareScanScope(ScopeType.SIDELOADED_ONLY, emptyList()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not fallback to default args - it creates non-debuggable issues. Just throw exception
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The single-level and nested patterns each repeat a lot. The file already has a perfectly good generic helper for this — Utils.kt defines extractArray() and processArray() with reified types. Can you try to reuse those ? Also, mapTo(mutableSetOf()) avoids building a List and then copying to a Set.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored |
||
| val reasonModeStr = optString("reasonMode") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. org.json.JSONObject#optString(name) returns "" (empty string) for a missing key, not null. The next line correctly uses isNullOrEmpty() so it works, but the variable name reasonModeStr plus the isNullOrEmpty() check imply the value can be null — which it can't from this API. Slightly misleading and inconsistent with the optString("scanScope", "SIDELOADED_ONLY") pattern used 30 lines above, which uses the two-arg overload to provide a default.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored |
||
| val reasonMode = if (reasonModeStr.isNullOrEmpty()) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we set default in Dart to ReasonMode.HIGHEST_CONFIDENCE, make the argument required in kotlin and throw exception if missing?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored |
||
| ReasonMode.HIGHEST_CONFIDENCE | ||
| } else { | ||
| runCatching { ReasonMode.valueOf(reasonModeStr) }.getOrDefault(ReasonMode.HIGHEST_CONFIDENCE) | ||
| } | ||
| return SuspiciousAppDetectionConfig(packageNames, hashes, requestedPermissions, grantedPermissions, malwareScanScope, reasonMode) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,8 @@ class AndroidConfig { | |
| required this.packageName, | ||
| required this.signingCertHashes, | ||
| this.supportedStores = const [], | ||
| this.malwareConfig, | ||
| @Deprecated('Use suspiciousAppDetectionConfig instead') this.malwareConfig, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping both APIs is going to be huge pain. What if I specify both in config? which is going to be used? Let's remove the old API altogether (on all platforms) We are doing new major here so we can do this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed completely |
||
| this.suspiciousAppDetectionConfig, | ||
| }) { | ||
| ConfigVerifier.verifyAndroid(this); | ||
| } | ||
|
|
@@ -34,5 +35,9 @@ class AndroidConfig { | |
| final List<String> supportedStores; | ||
|
|
||
| /// Malware configuration for Android. | ||
| @Deprecated('Use suspiciousAppDetectionConfig instead') | ||
| final MalwareConfig? malwareConfig; | ||
|
|
||
| /// Suspicious app detection configuration for Android. | ||
| final SuspiciousAppDetectionConfig? suspiciousAppDetectionConfig; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you generate this using pigeon? Or are those manual edits?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Manual edit 😅 |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| export 'android_config.dart'; | ||
| export 'ios_config.dart'; | ||
| export 'malware_config.dart'; | ||
| export 'suspicious_app_detection_config.dart'; | ||
| export 'talsec_config.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to raise major because of this (on all platforms)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumped to 8.0.0