Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Remove username password integration test #5544

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

Open
RyAuld wants to merge 5 commits into main
base: main
Choose a base branch
Loading
from remove-username-password-integration-tests

Conversation

@RyAuld
Copy link
Contributor

@RyAuld RyAuld commented Oct 23, 2025

Summary
Removes username/password integration tests that use the deprecated Resource Owner Password Credentials (ROPC) flow.

Rationale

  • Deprecated API: The username/password authentication flow tested in this file uses ROPC, which is deprecated and not recommended for production use
  • Security Concerns: Deliberate test failures followed by successes may trigger Azure AD's spray attack detection, potentially marking test users as risky and generating security alerts
  • Build Reliability: These tests have been causing intermittent build failures due to authentication timing issues and account lockout policies
  • Impact
  • Improves build stability by removing flaky tests
  • Eliminates potential security alert noise from test authentication patterns
  • No loss of critical functionality testing since ROPC is deprecated
  • Files Changed

❌ Removed: UsernamePasswordIntegrationTests.NetFwk.cs

@RyAuld RyAuld requested a review from a team as a code owner October 23, 2025 18:23
}
else
{
authResult = await (((IConfidentialClientApplication)clientApp) as IByUsernameAndPassword)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the IByUsernameAndPassword.AcquireTokenByUsernamePassword is not deprecated. Not sure if we should remove the tests that use this? CC @bgavrilMS

This is for confidential clients it looks like.

Copy link
Member

Choose a reason for hiding this comment

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

See

AcquireTokenByUsernameAndPasswordConfidentialParameterBuilder AcquireTokenByUsernamePassword(IEnumerable<string> scopes, string username, string password);

Copy link
Contributor

@Avery-Dunn Avery-Dunn Oct 23, 2025

Choose a reason for hiding this comment

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

When it was originally deprecated there were some comments about CCA, seemed like Scott wanted it deprecated too for consistency but ultimately it was left as-is: #5355 (comment)

But the CCA version is going to be deprecated at some point too, right? Or is that expected to stick around for a while?

(not sure about the other MSALs, but over in MSAL Java we only had ROPC in the public client so this wasn't an issue)

Copy link
Member

@bgavrilMS bgavrilMS Oct 24, 2025

Choose a reason for hiding this comment

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

Agreed. I marked the tests that I think can be deleted. I'm ok to delete the incorrect password tests and some PCA tests, but not the CCA tests.

RyAuld reacted with thumbs up emoji
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bgavrilMS @trwalke Addressed PR feedback by restoring the file and selectively removing tests as requested. Kept core ROPC tests (ROPC_AAD_Async, ROPC_AAD_CCA_Async) while removing deprecated/duplicate tests (Arlington, MSA, telemetry, and incorrect password tests). Also cleaned up orphaned helper methods and unused constants. Ready for review.

Copy link
Member

@trwalke trwalke left a comment

Choose a reason for hiding this comment

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

verify if CCA scenario is needed.

@RyAuld RyAuld force-pushed the remove-username-password-integration-tests branch from 68fefb0 to d9ce14a Compare October 24, 2025 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bgavrilMS bgavrilMS bgavrilMS left review comments

@trwalke trwalke trwalke requested changes

@Avery-Dunn Avery-Dunn Avery-Dunn approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /