Skip to content

Use single FRT [MSAL] #2550

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

Merged
merged 17 commits into from
Apr 23, 2025
Merged

Use single FRT [MSAL] #2550

merged 17 commits into from
Apr 23, 2025

Conversation

juan-arias
Copy link
Member

Proposed changes

Add support to the new FRT in test apps.

Type of change

  • Feature work
  • Bug fix
  • Documentation
  • Engineering change
  • Test
  • Logging/Telemetry

Risk

  • High – Errors could cause MAJOR regression of many scenarios. (Example: new large features or high level infrastructure changes)
  • Medium – Errors could cause regression of 1 or more scenarios. (Example: somewhat complex bug fixes, small new features)
  • Small – No issues are expected. (Example: Very small bug fixes, string changes, or configuration settings changes)

Additional information

@juan-arias juan-arias requested review from a team as code owners March 12, 2025 18:19
@ameyapat
Copy link
Contributor

Cloned PR for jarias/use-single-frt


AI description (iteration 1)

PR Classification

New feature

PR Summary

This pull request introduces functionality to check and update the status of Family Refresh Tokens (FRT) in the cache.

  • MSIDAccountCredentialsCacheTests.m: Added multiple test cases to verify the behavior of the checkFRTEnabled method under various conditions.
  • MSIDAccountCredentialCache.m: Implemented the checkFRTEnabled method to determine the FRT status and update settings based on feature flags.
  • Added a new file MSIDFamilyRefreshToken.m to handle the creation and management of family refresh tokens.

@ameyapat
Copy link
Contributor

"Added 1 Inline Code Review comment(s) at the following location(s):

  • /msal-from-github/MSAL/IdentityCore/IdentityCore/src/MSIDRequestContext.h at Lines 33-35

AI-generated content may be incorrect

"

@ameyapat
Copy link
Contributor

"AI code review (iteration 1)
There is a minor spelling mistake in the comment. The word 'disablle' should be corrected to 'disable'.
Here is the suggested code:

/**
 Temporal property to disable Family Refresh Token. This will be removed in future, added to allow 1P apps to disable this feature themselves.
 Enabled by default, also configured to be enabled/disabled remotely by Microsoft.
 */

This comment refers to line 33 to 35 in the new file.
AI-generated content may be incorrect

Veena11
Veena11 previously approved these changes Mar 13, 2025
@ameyapat
Copy link
Contributor

Cloned PR for jarias/use-single-frt


AI description (iteration 1)

PR Classification

New feature implementation.

PR Summary

This pull request introduces the functionality to check and update the Family Refresh Token (FRT) settings based on feature flags.

  • MSIDAccountCredentialsCacheTests.m: Added multiple tests to verify the behavior of the new FRT feature under various conditions.
  • MSIDAccountCredentialCache.m: Implemented methods to check and update FRT settings based on feature flags and keychain items.
  • MSIDFamilyRefreshToken.m: Added a new class to represent the Family Refresh Token, including initialization, equality checks, and cache handling.

@ameyapat
Copy link
Contributor

"Added 2 Inline Code Review comment(s) at the following location(s):

  • /msal-from-github/MSAL/IdentityCore/IdentityCore/src/oauth2/token/MSIDFamilyRefreshToken.h at Line 33
  • /msal-from-github/MSAL/IdentityCore/IdentityCore/src/MSIDRequestContext.h at Lines 33-35

AI-generated content may be incorrect

"

@ameyapat
Copy link
Contributor

"AI code review (iteration 1)
It might be better to add a null check for the refreshToken parameter in the initializer method to ensure that the object is properly initialized and to avoid potential runtime errors.

Here is the suggested code:

This comment refers to line 33 in the new file.
AI-generated content may be incorrect

@ameyapat
Copy link
Contributor

"AI code review (iteration 1)
The comment mentions that the property will be removed in the future. It might be better to add a deprecation warning or annotation to inform developers that this property is temporary and will be removed.
Here is the suggested code:

/**
 * Temporal property to disable Family Refresh Token. This will be removed in future, added to allow 1P apps to disable this feature themselves.
 * Enabled by default, also configured to be enabled/disabled remotely by Microsoft.
 * @deprecated This property will be removed in future versions.
 */
@property (nonatomic, readwrite) BOOL disableFRT;

This comment refers to line 33 to 35 in the new file.
AI-generated content may be incorrect

@ameyapat
Copy link
Contributor

Cloned PR for jarias/use-single-frt


AI description (iteration 1)

PR Classification

New feature

PR Summary

This pull request introduces a new feature to check and update Family Refresh Token (FRT) settings based on feature flags.

  • MSIDAccountCredentialsCacheTests.m: Added multiple tests to verify the behavior of the new FRT feature.
  • MSIDAccountCredentialCache.m: Implemented methods to check and update FRT settings based on feature flags.
  • MSIDFamilyRefreshToken.m: Added a new class to handle Family Refresh Tokens, including initialization, equality checks, and cache handling.

@@ -29,10 +29,17 @@ class MSALNativeAuthRequestContext: MSIDRequestContext {
private let _correlationId: UUID
private let _telemetryRequestId: String
private var _serverCorrelationId: UUID? // TODO: Setting the server correlation id here is wrong. Needs refactoring.
var disableFRT: Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need it to be public property?

Copy link
Member Author

Choose a reason for hiding this comment

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

Property 'disableFRT' must be as accessible as its enclosing type because it matches a requirement in protocol 'MSIDRequestContext'

This can change if there is an equivalent to MSALGlobalConfig in CommonCore (the other PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is no longer needed, removed.

@@ -469,6 +470,20 @@ - (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(N
}
break;
}
case MSIDFamilyRefreshTokenType:
{
MSIDRefreshToken *refreshToken = (MSIDRefreshToken *) token;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we do "isKindOfClass" before casting?

Copy link
Member Author

Choose a reason for hiding this comment

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

The switch is reading credentialType from each individual class; but it was casting to the base class instead of the right class. Updated

@@ -365,6 +366,20 @@ - (NSView *)outlineView:(NSOutlineView *)outlineView viewForTableColumn:(NSTable
MSIDBaseToken *token = (MSIDBaseToken *)item;
switch (token.credentialType)
{
case MSIDFamilyRefreshTokenType:
{
MSIDRefreshToken *refreshToken = (MSIDRefreshToken *) token;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here: isKindOfClass?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above

# Conflicts:
#	CHANGELOG.md
#	MSAL/IdentityCore
… use method from MSIDAccountCredentialCache instead.
- MSAL/IdentityCore
@juan-arias juan-arias merged commit 7ef79b5 into dev Apr 23, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants