-
Notifications
You must be signed in to change notification settings - Fork 126
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
Add native auth to MSAL test app #2001
Conversation
…tautomation' into sammy/add-native-auth-to-msaltestautomation
@@ -0,0 +1,132 @@ | |||
package com.microsoft.identity.client.testapp.nativeauth |
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.
Let's add License to all these new kotlin files
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.
Done, and for all other .kt files
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.
Need licenses in all new .kt
files
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.
Might be able to test the proguard rules in the dist variant of the test app now, since we incorporate native auth into it.
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.
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.
@Yuki-YuXin this is because test app is pulling an outdated MSAL version (4.x), one that doesn't have native auth included yet. I'll update this in the PR.
…tautomation' into sammy/add-native-auth-to-msaltestautomation # Conflicts: # testapps/testapp/build.gradle
android:icon="@drawable/ic_baseline_lock_reset_24" | ||
android:title=" SSPR" /> | ||
|
||
</menu> |
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.
nit: new line ending; here and everywhere
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.
Addressed
@@ -83,6 +87,9 @@ android { | |||
proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.pro' | |||
} | |||
} | |||
buildFeatures { |
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.
What does this do? and why is it needed?
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.
What does this do
https://developer.android.com/topic/libraries/view-binding
why is it needed?
It's a much improved process of managing view (interactions, etc.) that has been rolled out by Android for quite some time. New and improved way of doing things.
import com.microsoft.identity.client.testapp.nativeauth.EmailSignInSignUpFragment | ||
import com.microsoft.identity.client.testapp.nativeauth.PasswordResetFragment | ||
|
||
class NativeAuthFragment : Fragment() { |
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.
javadoc on all classes
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.
Addressed
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.
I still don't see javadoc on this particular class though
commit() | ||
} | ||
} | ||
} |
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.
new line ending; here and everywhere
// THE SOFTWARE. | ||
package com.microsoft.identity.client.testapp.nativeauth | ||
|
||
interface Constants { |
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.
javadoc everywhere
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.
Addressed
import com.microsoft.identity.nativeauth.INativeAuthPublicClientApplication | ||
|
||
/** | ||
* Utility for managing MSAL SDK instance INativeAuthPublicClientApplication |
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.
* Utility for managing MSAL SDK instance INativeAuthPublicClientApplication | |
* Utility for managing MSAL SDK instance INativeAuthPublicClientApplication. |
Also probably run code formatter
public static final String STATE = "state"; | ||
public static final String CODE_LENGTH = "code_length"; | ||
public static final String SENT_TO = "sent_to"; | ||
public static final String CHANNEL = "channel"; |
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.
Please add some short comments/javadoc describing these and mentioning that these are related to Native Auth (given that these are placed in shared class)
For MSAL team:
In order to let the native auth team execute (manual) release tests, an application is needed that supports the various use flows that are part of the release tests.
Changes:
For native auth team:
Changes compared to previous versions of this application:
country
andcity
, but taken dynamically from the UI (through new key and value text fields).CodeFragment
displays API'schannel
,sentTo
andcodeLength
fields, which can now be used for validation for correct behaviour.Note:
This application is not in a state where it can be used as a CI-generated artefact by the native auth team. Developers using this application will need to change the config JSON file and update it to one of several tenant configurations, recompile and run it. Support for native auth in the Labs environment is needed to make this application usable as a CI-generated artefact. Once Labs support is added, the native auth screens will need config selection UI, similar to the Fragment dedicated to testing
acquireToken()
.