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.

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

Merged
merged 16 commits into from
Jan 24, 2024

Conversation

SammyO
Copy link
Contributor

@SammyO SammyO commented Jan 10, 2024

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:

  • The existing MSAL test app is extended with a Native Auth section, which is accessed from the side menu.
  • Updated CODEOWNERS to reflect native auth ownership of the newly added classes.

For native auth team:

Changes compared to previous versions of this application:

  • Sign up attributes are no longer hardcoded to country and city, but taken dynamically from the UI (through new key and value text fields).
  • AttributesRequired` state will navigate to the new, generic AttributesFragment. Logs need to be checked for details about which attributes are required, and thus which keys need to be supplied.
  • CodeFragment displays API's channel, sentTo and codeLength 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().

@SammyO SammyO marked this pull request as ready for review January 11, 2024 16:24
@SammyO SammyO requested a review from a team as a code owner January 11, 2024 16:24
@SammyO SammyO added the No-Changelog This change does not update the changelog. label Jan 11, 2024
…tautomation' into sammy/add-native-auth-to-msaltestautomation
@@ -0,0 +1,132 @@
package com.microsoft.identity.client.testapp.nativeauth
Copy link
Contributor

@fadidurah fadidurah Jan 11, 2024

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

Copy link
Contributor Author

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

Copy link
Contributor

@fadidurah fadidurah left a 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

Copy link
Contributor

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.

Copy link
Contributor

@Yuki-YuXin Yuki-YuXin Jan 12, 2024

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

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.

@SammyO SammyO requested a review from fadidurah January 12, 2024 16:22
android:icon="@drawable/ic_baseline_lock_reset_24"
android:title=" SSPR" />

</menu>
Copy link
Contributor

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

Copy link
Contributor Author

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

@SammyO SammyO Jan 18, 2024

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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

javadoc on all classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

Copy link
Contributor

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()
}
}
}
Copy link
Contributor

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

javadoc everywhere

Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Utility for managing MSAL SDK instance INativeAuthPublicClientApplication
* Utility for managing MSAL SDK instance INativeAuthPublicClientApplication.

Also probably run code formatter

Comment on lines +117 to +120
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";
Copy link
Contributor

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)

@SammyO SammyO merged commit 1f146d0 into dev Jan 24, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No-Changelog This change does not update the changelog. testapps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants