Audit of classes in firebase app distribution to minimize user access #3196

manny-jimenez · 2021-12-01T15:59:07Z

No description provided.

google-oss-bot · 2021-12-01T16:06:19Z

Coverage Report

Affected SDKs

No changes between base commit (dd1c336) and head commit (78339c4f).

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (78339c4f) is created by Prow via merging commits: dd1c336 34702b3.

google-oss-bot · 2021-12-01T16:16:00Z

Binary Size Report

Affected SDKs

firebase-app-distribution

Type Base (dd1c336) Head (78339c4f) Diff

aar 117 kB 117 kB +19 B (+0.0%)

apk (release) 1.55 MB 1.55 MB +20 B (+0.0%)

Test Logs

Notes

Head commit (78339c4f) is created by Prow via merging commits: dd1c336 34702b3.

manny-jimenez · 2021-12-05T23:15:06Z

/retest

rachaprince · 2021-12-06T15:59:12Z

...ution/src/main/java/com/google/firebase/app/distribution/AppDistributionReleaseInternal.java

@@ -27,7 +27,7 @@
 *     href="https://github.com/google/auto/tree/master/value">https://github.com/google/auto/tree/master/value</a>
 */
 @AutoValue
-public abstract class AppDistributionReleaseInternal {
+abstract class AppDistributionReleaseInternal {

  @NonNull
  public static Builder builder() {


I think we should make the property methods in this class package private. External classes shouldn't need to access this data object

...ase-app-distribution/src/main/java/com/google/firebase/app/distribution/UpdateAabClient.java

...ase-app-distribution/src/main/java/com/google/firebase/app/distribution/UpdateApkClient.java

...pp-distribution/src/main/java/com/google/firebase/app/distribution/SignInResultActivity.java

rachaprince · 2021-12-10T16:09:36Z

...tribution/src/main/java/com/google/firebase/app/distribution/ReleaseIdentificationUtils.java

@@ -29,7 +29,7 @@
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;

-public final class ReleaseIdentificationUtils {
+final class ReleaseIdentificationUtils {
  private static final String TAG = "ReleaseIdentification";
  private static final int BYTES_IN_LONG = 8;



I think the methods in this class should also be package-private

Good call. I thought since the class is package-private it was fine but its better to be consistent.

rachaprince · 2021-12-10T16:10:20Z

...pp-distribution/src/main/java/com/google/firebase/app/distribution/SignInResultActivity.java

@@ -25,6 +25,8 @@
 public class SignInResultActivity extends AppCompatActivity {
  private static final String TAG = "SignInResultActivity:";

+  public SignInResultActivity() {}


Do we need this? Seems like the class is public already, so the default constructor will be public too

This was a left over from trying to make it package-private will remove

I'm pretty sure even if the class is marked as public, the constructor won't be public unless it's also marked as such.

good to know! thanks!

...ase-app-distribution/src/main/java/com/google/firebase/app/distribution/InstallActivity.java

lfkellogg

LGTM!

rachaprince

Nice work!

manny-jimenez · 2021-12-10T17:00:38Z

/test smoke-tests

rachaprince · 2021-12-10T19:33:46Z

Looking at the files generated by api.txt, I have a few questions about the visibility of some classes and methods:

@mannyjimenez0810 do you know if we could reduce the visibility of any of the following?:

FirebaseAppDistributionLifeCycleNotifier
FirebaseAppDistributionFileProvider
UpdateProgress.Builder

(It might be worth doing a search for "public" in the app distribution package to see if there are any others we missed)

manny-jimenez · 2021-12-10T21:33:28Z

@rachaprince Great Question.

FirebaseAppDistributionLifeCycleNotifier
This was an oversight on my part. Will fix in a future PR.

FirebaseAppDistributionFileProvider
I tried making this also package private but I believe due to it being in the manifest I was unable to.
See here

firebase-android-sdk/firebase-app-distribution/src/main/AndroidManifest.xml

Line 49 in 8bc2ab4

    
           android:name="com.google.firebase.app.distribution.FirebaseAppDistributionFileProvider"

I get this exception java.lang.RuntimeException: Unable to get provider com.google.firebase.app.distribution.FirebaseAppDistributionFileProvider: java.lang.IllegalAccessException: java.lang.Class<com.google.firebase.app.distribution.FirebaseAppDistributionFileProvider> is not accessible from java.lang.Class<android.app.AppComponentFactory>

UpdateProgress.Builder
Another good catch. I saw we return UpdateProgress so I didn't look at the methods as carefully as I should have. Will also fix this in an upcoming PR.

Thanks for catching these. Will do a search for public in the package to see if theres other pieces I missed.

Audit of classes in firebase app distribution to minimize user access

5ef6ebc

google-cla bot added the cla: yes Override cla label Dec 1, 2021

google-oss-bot added the size/M label Dec 1, 2021

Manny Jimenez added 5 commits December 1, 2021 11:53

Making file provider public

4aa4419

Fixing check changed

4bf7134

lint changes

715c6de

Changing activities to be public again

a0530f5

Fixing errors

d22e66c

google-oss-bot added size/S and removed size/M labels Dec 1, 2021

Manny Jimenez added 4 commits December 1, 2021 13:56

Merging changes

97377a0

Making classes public again

fa7af04

Changing more public methods to package private

5396478

Adding public constructors

b4c7753

google-oss-bot added size/M and removed size/S labels Dec 5, 2021

Manny Jimenez added 2 commits December 5, 2021 16:16

Changing classes back to public

165795a

Merge remote-tracking branch 'origin/master' into fad_audit

d8e877c

firebase deleted a comment from google-oss-bot Dec 5, 2021

Manny Jimenez added 3 commits December 5, 2021 18:39

File Provider must be public

d3f93ae

Lint fix

b5b57b8

Making registrar back to public

34e5b05

manny-jimenez requested review from rachaprince and pranavrajgopal December 6, 2021 00:11

rachaprince reviewed Dec 6, 2021

View reviewed changes

Manny Jimenez and others added 2 commits December 8, 2021 10:31

Responding to feedback part1

d30d7c2

Merge branch 'master' into fad_audit

3633bc8

responding to feedback about release internal

56a8220

manny-jimenez requested a review from lfkellogg December 10, 2021 16:02

lfkellogg reviewed Dec 10, 2021

View reviewed changes

...pp-distribution/src/main/java/com/google/firebase/app/distribution/SignInResultActivity.java Outdated Show resolved Hide resolved

rachaprince reviewed Dec 10, 2021

View reviewed changes

lfkellogg reviewed Dec 10, 2021

View reviewed changes

...ase-app-distribution/src/main/java/com/google/firebase/app/distribution/InstallActivity.java Outdated Show resolved Hide resolved

lfkellogg mentioned this pull request Dec 10, 2021

Fix API text files for App Distribution #3232

Closed

Responding to feedback

34702b3

lfkellogg approved these changes Dec 10, 2021

View reviewed changes

rachaprince approved these changes Dec 10, 2021

View reviewed changes

manny-jimenez merged commit 8bc2ab4 into master Dec 10, 2021

manny-jimenez deleted the fad_audit branch December 10, 2021 17:47

firebase locked and limited conversation to collaborators Jan 10, 2022

Audit of classes in firebase app distribution to minimize user access #3196

Audit of classes in firebase app distribution to minimize user access #3196

Uh oh!

Conversation

manny-jimenez commented Dec 1, 2021

Uh oh!

google-oss-bot commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

Test Logs

Notes

Uh oh!

google-oss-bot commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-app-distribution

Test Logs

Notes

Uh oh!

manny-jimenez commented Dec 5, 2021

Uh oh!

rachaprince Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rachaprince Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

manny-jimenez Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

rachaprince Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

manny-jimenez Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

lfkellogg Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

rachaprince Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lfkellogg left a comment

Choose a reason for hiding this comment

Uh oh!

rachaprince left a comment

Choose a reason for hiding this comment

Uh oh!

manny-jimenez commented Dec 10, 2021

Uh oh!

rachaprince commented Dec 10, 2021

Uh oh!

manny-jimenez commented Dec 10, 2021

Uh oh!

Uh oh!

google-oss-bot commented Dec 1, 2021 •

edited

Loading

google-oss-bot commented Dec 1, 2021 •

edited

Loading

`firebase-app-distribution`