Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feature/aris/issue_dinsic_618 #4018

Merged
merged 1 commit into from
Oct 5, 2021
Merged

feature/aris/issue_dinsic_618 #4018

merged 1 commit into from
Oct 5, 2021

Conversation

ariskotsomitopoulos
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos commented Sep 15, 2021 •
edited
Loading

Apps targeting Android 12 and higher are required to specify an explicit value for android:exported when the corresponding
component has an intent filter defined.

android:exported=“true”

Activities

.features.MainActivity 
[intent-filter] —> Required to launch the app

.features.login.LoginActivity
[intent-filter] -> Redirection URL after SSO login in external browser

.features.login2.LoginActivity2
[intent-filter] -> Redirection URL after SSO login in external browser

.features.link.LinkHandlerActivity
[intent-filter] -> Deep Link Converter/Transformer

.features.permalink.PermalinkHandlerActivity
[intent-filter] -> Deep Links Handling

.features.share.IncomingShareActivity
[intent-filter] -> Share functionality

Services

.features.call.telecom.VectorConnectionService
[intent-filter] -> Call connection service.  [maybe false?]

Receivers

androidx.media.session.MediaButtonReceiver
[intent-filter] -> Translate hardware media playback buttons  [maybe false?]

android:exported=“false”

All the other activities, services, receivers, providers

@ariskotsomitopoulos ariskotsomitopoulos force-pushed the feature/aris/issue_dinsic_618 branch from 05fda21 to 8702ff5 Compare September 15, 2021 11:01
ariskotsomitopoulos
ariskotsomitopoulos commented Sep 15, 2021
View reviewed changes
vector/src/main/AndroidManifest.xml Outdated
<receiver android:name="androidx.media.session.MediaButtonReceiver">
<receiver
android:name="androidx.media.session.MediaButtonReceiver"
android:exported="true" >
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this will also work with exported="false" ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you test it please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with wireless earbuds if click on call events work with exported="false" and they seem to work (onMediaButtonEvent function is triggered). So I will update it with false. If there is an other usage other than that, that you can think of let me know

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

ariskotsomitopoulos
ariskotsomitopoulos commented Sep 15, 2021
View reviewed changes
vector/src/main/AndroidManifest.xml Outdated
@@ -338,7 +412,8 @@

<service
android:name=".features.call.telecom.VectorConnectionService"
android:permission="android.permission.BIND_TELECOM_CONNECTION_SERVICE">
android:permission="android.permission.BIND_TELECOM_CONNECTION_SERVICE"
android:exported="true">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this will also work with exported="false" ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you test it please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think VectorConnectionService is not even working so there will be no problem to set exported=false. I also test incoming and outgoing calls and they work properly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks!

@github-actions
Copy link

github-actions bot commented Sep 15, 2021 •
edited
Loading

Unit Test Results

  42 files  ±0    42 suites  ±0   44s ⏱️ ±0s
  83 tests ±0    83 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
220 runs  ±0  220 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit f4e50a3. ± Comparison against base commit f4e50a3.

♻️ This comment has been updated with latest results.

bmarty
bmarty reviewed Sep 20, 2021
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update.
Can you add in the Manifest itself one line of comment to explain why exported="true" is mandatory? You can use the related comment form the PR itself.
Thanks

@ariskotsomitopoulos
Copy link
Contributor Author

Thanks for the update.
Can you add in the Manifest itself one line of comment to explain why exported="true" is mandatory? You can use the related comment form the PR itself.
Thanks

done

@bmarty
Copy link
Member

bmarty commented Sep 24, 2021

Thanks for the update.
Can you add in the Manifest itself one line of comment to explain why exported="true" is mandatory? You can use the related comment form the PR itself.
Thanks

done

Sorry there have been a misunderstanding, your last commit was not what I was expecting. I wanted you to explicitly comment all Android Manifest element with android:exported set to "true" to justify why it is mandatory for the app to work properly.

We will take care of Android 12 support when we will be targeting those devices. In the mean time, I guess there is no pb to install on Android 12, since compatibility mode will occur on the device.

@ariskotsomitopoulos
Copy link
Contributor Author

Thanks for the update.
Can you add in the Manifest itself one line of comment to explain why exported="true" is mandatory? You can use the related comment form the PR itself.
Thanks

done

Sorry there have been a misunderstanding, your last commit was not what I was expecting. I wanted you to explicitly comment all Android Manifest element with android:exported set to "true" to justify why it is mandatory for the app to work properly.

We will take care of Android 12 support when we will be targeting those devices. In the mean time, I guess there is no pb to install on Android 12, since compatibility mode will occur on the device.

Thanks, now its more clear, I will update accordingly !

@bmarty bmarty added this to the Sprint - Element 1.3.2 milestone Oct 1, 2021
bmarty
bmarty reviewed Oct 4, 2021
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. See my remarks. Also can you squash all the commits please?

vector/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
vector/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
vector/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
vector/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
vector/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
@ariskotsomitopoulos ariskotsomitopoulos force-pushed the feature/aris/issue_dinsic_618 branch from 9f3211b to 15275eb Compare October 5, 2021 10:22
bmarty
bmarty reviewed Oct 5, 2021
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. One final comment.
Also can you add a file for the changelog please (filename 4018.misc would be fine)? As we are doing this for the forks, it will help to track this work.

@ariskotsomitopoulos ariskotsomitopoulos force-pushed the feature/aris/issue_dinsic_618 branch from 15275eb to 647badb Compare October 5, 2021 10:51
@ariskotsomitopoulos
Minimize the use of exported="true" in android Manifest
f21d89e 
 - Add comments on Add exported="true" attributes
 - Disable manifest exporting for:
      - (service) VectorConnectionService
      - (receiver) MediaButtonReceiver
@ariskotsomitopoulos ariskotsomitopoulos force-pushed the feature/aris/issue_dinsic_618 branch from 647badb to f21d89e Compare October 5, 2021 10:57
bmarty
bmarty approved these changes Oct 5, 2021
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

@bmarty bmarty merged commit f4e50a3 into develop Oct 5, 2021
@bmarty bmarty deleted the feature/aris/issue_dinsic_618 branch October 5, 2021 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmarty bmarty bmarty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Sprint - Element 1.3.2
Development

Successfully merging this pull request may close these issues.
2 participants
@ariskotsomitopoulos @bmarty