requireAuthentication() takes Activity as a parameter and stores it in a field inside the SecureCredentialsManager class which leads to context leaks

[KrasimirGatevMentorMate]

Version of the library used: 2.4.0

Inside the SecureCredentialsManager class has a private var activity: Activity? = null field which is set inside the requireAuthentication() method. This will lead to leaks when using the SecureCredentialsManager as a singleton with application context as the Activity context will leak.

[lbalmaceda]

@KrasimirGatevMentorMate Yes. The activity is kept around when the pre-authentication is required before retrieving the credentials from the storage. An activity context is needed to start the system activity that prompts authentication. A good addition could be resolving this use case using the Activity Results API. What do you suggest?

[KrasimirGatevMentorMate]

You are correct - actually startActivityForResult is now depricated and registerForActivityResult is the new recommended way to listen for activity results in Android. You will still need an activity or fragment context from which to call registerForActivityResult though. You can implement it in such a way as not to store a refererence to the activity/fragment context. Alternatively if you still need to store it, it should be cleaned up manually and/or used as a weak reference, so the context object can be garbage collected.

[lbalmaceda]

I will explore if it's feasible to use the Activity Results API in the current implementation. That is, without introducing breaking changes nor alternate method signatures that would make use "handle 2 different flows" at the same time. Otherwise, it would be a feature request for a future Credentials Manager implementation (a rework from scratch).

About the cleanup strategy, the reason we don't do that automatically is that once you initialize this, you could be calling getCredentials multiple times. So, we can't say, for example, that after obtaining the credentials the first time, the activity will be set to null. We could leverage that to the developers by adding a new method "clear context" that does that but still, I think it's error-prone as many developers would miss making that call even if we document it's recommended for them to do.

I'm curious about the WeakReference. We could wrap and store the context in one of them, but if that's null at usage time, we would need the developer to re-call the requireAuthentication method to set it back. And that would need to be signaled via an exception in the provided callback.

[lbalmaceda]

I made a quick attempt to try this out, and I think it can be added without introducing new signatures. The requirement would be to pass an AppCompatActivity instead of a regular Activity when invoking the requireAuthentication method. If that's the case, then we don't store the activity anywhere, and we make use of the results API to handle that with contracts.

I have not tested this yet, nor added unit tests. If you can please clone and manually test the PR to verify if there were improvements. I'm not sure how you were measuring the leaks before. This is the PR #519

[KrasimirGatevMentorMate]

@lbalmaceda Unfortunately I will not be able to test the change, as I have already implemented the activity result functionality outside the Auth0 library, so I will not be using the improved method, but I am happy to have contributed to the improvement of the Auth0 Android sdk.