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

Automate test cases for MWPJ scenarios. #1817

Merged
merged 30 commits into from
May 26, 2023
Merged

Automate test cases for MWPJ scenarios. #1817

merged 30 commits into from
May 26, 2023

Conversation

p3dr0rv
Copy link
Contributor

@p3dr0rv p3dr0rv commented May 2, 2023 •
edited

@p3dr0rv p3dr0rv changed the title Multiple WPJ test cases Add test cases for MWPJ scenarios. May 2, 2023
@p3dr0rv p3dr0rv changed the title Add test cases for MWPJ scenarios. Automate test cases for MWPJ scenarios. May 2, 2023
@p3dr0rv p3dr0rv mentioned this pull request May 2, 2023
Merged
@p3dr0rv p3dr0rv marked this pull request as ready for review May 2, 2023 23:08
@p3dr0rv p3dr0rv requested a review from a team as a code owner May 2, 2023 23:08
@rpdome
Copy link
Member

rpdome commented May 4, 2023 •
edited

Reviewed the test plans

Firstly,
-2519809: get wpj entries should be invoked after 7/8 to verify the result.

  • 2519835: I suspect there might be a lab API for disabling device. (there's a ui for that in swagger).
  • 2521768 sounds like a bug. I actually expect the token endpoint PKeyAuth to kick in. (MicrosoftStsOAuth2Strategy.performPKeyAuthRequest()). Could you please look into this?

I also edited some of the tests to make them more clear.

Secondly, Could you please add the following tests

  1. Join with legacy WPJ (tenant A), then join with mwpj (tenant b). legacy should return tenant A and mwpj should return both.
  2. Join with mwpj, then get account in legacy wpj
  3. join with mwpj, acquire PRT, join with mwpj with another account from the same tenant. One entry should be returned, SSO shall not break (PRT is still usable without extra prompts).
  4. Test MWPJ's join/getDeviceState/installCert/leave - against old broker (that doesn't have your feature shipped. Maybe pick a build from early last year).

In a separate PR?

fadidurah
fadidurah reviewed May 9, 2023
View reviewed changes
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.

We should probably improve the readability of these test cases.

@p3dr0rv
Copy link
Contributor Author

p3dr0rv commented May 15, 2023 •
edited

@rpdome

  • 2519809: Done
  • 2519835: The existing lab API has a delete device operation, disable a device is a different operation,
  • 252176: PkeyAuth is triggered on broker msal versions higher that 9.0
Step Operation Claim Account Controller result Notes
1 AcquireTokenInteractive   AccountA-TenantA BrokerLocalController AT and RT  
2 AcquireTokenInteractive deviceId AccountB-TenantA BrokerLocalController Device is regitered to tenant A and accountB has a PRT account A does not have a PRT
3 AcquireTokenSilently   AccountA-TenantA BrokerLocalController return cached AT because account A does not have PRT and it is a silent call, we retun the BrokerMSALController even when the tenant is registered
4 AcquireTokenSilently deviceId AccountA-TenantA BrokerLocalController AADSTS50187: Failed to perform device authentication. PkeyAuth is not triggered unless broker_msal version is 9.0 or higher
5 AcquireTokenInteractive deviceId AccountA-TenantA BrokerLocalController we get a PRT for account A, then use PRT to get  AT with deviceid claim  
  • Added the test cases you mentioned,
    • 1: 2563653
    • 2: 2563664
    • 3: extend 2521960, (please validate this is correct)
    • 4: 2563668

@rpdome
Copy link
Member

rpdome commented May 16, 2023 •
edited

Test cases:

2563653 and 2563664 - LGTM
2563668 - this is an upgrade scenario. We're not testing the compatibility between 2 modes here - that should already be covered by other tests. So... you would need to install legacy broker app, perform the tasks, then update the app.

Extending 2521960 is okay - but I would still prefer having a separate test case for that (because if it breaks, it's clearer what exactly breaks. If you have a test case that tries to cover too many flows... you'll have to spend more time debugging).

2521768 - let's create 2 separate cases.

  1. above 9.0, return device id in step 8. (test that silent flow pkeyauth works with the new mwpj change)
  2. any protocol version. skip step 8. get device id via 9. (tests that PRT upgrade works with mwpj change).

@p3dr0rv
Copy link
Contributor Author

p3dr0rv commented May 24, 2023 •
edited

  • 256366: change to a update scenario
  • 2521960: break in two test cases (2521960 Device registration entry migration (different upn - same tenant) , 2579654 After entry migration PRT is still usable without extra prompts.)
  • 2521768 : break in two test cases (2521768 pkey auth silent disable, 2578879 pkey auth silent enable)

https://identitydivision.visualstudio.com/Engineering/_build/results?buildId=1104688&view=ms.vss-test-web.build-test-results-tab

rpdome
rpdome approved these changes May 25, 2023
View reviewed changes
Copy link
Member

@rpdome rpdome left a comment

Choose a reason for hiding this comment

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

add some comments - some nit, some not so nits. please address them.

otherwise lgtm.

...ava/com/microsoft/identity/client/msal/automationapp/testpass/broker/mwpj/TestCase2563668.kt Outdated Show resolved Hide resolved
...ava/com/microsoft/identity/client/msal/automationapp/testpass/broker/mwpj/TestCase2521768.kt Show resolved Hide resolved
...ava/com/microsoft/identity/client/msal/automationapp/testpass/broker/mwpj/TestCase2521946.kt Outdated Show resolved Hide resolved
...ava/com/microsoft/identity/client/msal/automationapp/testpass/broker/mwpj/TestCase2521960.kt Outdated Show resolved Hide resolved
rpdome

This comment was marked as duplicate.

Sign in to view

rpdome

This comment was marked as duplicate.

Sign in to view

rpdome

This comment was marked as duplicate.

Sign in to view

p3dr0rv added a commit to AzureAD/microsoft-authentication-library-common-for-android that referenced this pull request May 25, 2023
@p3dr0rv
Modify Broker Host to support e2e tests that require MWPJ enable (#2034)
0020ae3 
Related PR
AzureAD/microsoft-authentication-library-for-android#1817
@p3dr0rv p3dr0rv mentioned this pull request May 25, 2023
Merged
@p3dr0rv
Copy link
Contributor Author

p3dr0rv commented May 26, 2023

add some comments - some nit, some not so nits. please address them.

otherwise lgtm.

Update test cases and code accordingly

p3dr0rv added a commit to AzureAD/microsoft-authentication-library-common-for-android that referenced this pull request May 26, 2023
@p3dr0rv
add function to get the device id using the legacy BrokerHost App (#2051
54fd772 
)

Used here
AzureAD/microsoft-authentication-library-for-android#1817
@p3dr0rv p3dr0rv merged commit f247862 into dev May 26, 2023
5 checks passed
@p3dr0rv p3dr0rv deleted the pedroro/mwpj-ui-test branch May 26, 2023 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpdome rpdome rpdome approved these changes

@fadidurah fadidurah fadidurah approved these changes

Assignees
No one assigned
Labels
test-automation testapps
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@p3dr0rv @rpdome @fadidurah