Use single FRT #1470
Use single FRT #1470
Conversation
…action is required
# Conflicts: # IdentityCore/src/MSIDConstants.h # IdentityCore/src/requests/MSIDInteractiveAuthorizationCodeRequest.m
…re flight sent by broker
IdentityCore/src/MSIDConstants.h
Outdated
| MSIDIsFRTEnabledStatusNotEnabled = 0, | ||
|
|
||
| // FRT is enabled | ||
| MSIDIsFRTEnabledStatusActive, |
There was a problem hiding this comment.
Question: Is there a difference between enabled and active? If not, can we use the same (enable vs disable) or active/not active
There was a problem hiding this comment.
No difference, updated to enable/disable
|
|
||
| // Check if FRT is enabled by feature flight | ||
| MSIDFlightManager *flightManager = [MSIDFlightManager sharedInstance]; | ||
| BOOL flagEnableFRT = [flightManager boolForKey:@"enable_client_sfrt_by_tenant_id"]; // TODO: Replace this by the constant from the other branch, and remove the hardcoded YES |
There was a problem hiding this comment.
nit: do you still need TODO here?
There was a problem hiding this comment.
Yes, I will replace this when the other PR (#1489) is merged.
| @@ -132,6 +134,77 @@ - (void)acquireTokenWithCodeResult:(MSIDAuthorizationCodeResult *) __unused auth | |||
| #endif | |||
| } | |||
|
|
|||
| - (void)updateCustomHeadersForFRTSupportIfNeeded | |||
| { | |||
| #if !EXCLUDE_FROM_MSALCPP && !AD_BROKER | |||
There was a problem hiding this comment.
For my learning: Any reason this is not included in OneAuth/MSAL?
There was a problem hiding this comment.
OneAuth already do this on their side, but we didn't.
|
|
||
| // Check if FRT is enabled by feature flight | ||
| MSIDFlightManager *flightManager = [MSIDFlightManager sharedInstance]; | ||
| BOOL flagEnableFRT = [flightManager boolForKey:@"enable_client_sfrt_by_tenant_id"];// || YES; // TODO: Replace this by the constant from the other branch, and remove the hardcoded YES |
There was a problem hiding this comment.
Is this TODO still need?
There was a problem hiding this comment.
With the other PR merged, I have replaced this string with the right constant.
IdentityCore/src/MSIDBasicContext.h
Outdated
| @@ -32,6 +32,7 @@ NS_ASSUME_NONNULL_BEGIN | |||
| @property (nonatomic, nullable) NSString *logComponent; | |||
| @property (nonatomic, nullable) NSString *telemetryRequestId; | |||
| @property (nonatomic, nullable) NSDictionary *appRequestMetadata; | |||
| @property (nonatomic, readwrite) BOOL disableFRT; | |||
There was a problem hiding this comment.
nit: readwrite is by default I think, can be removed
|
|
||
| @interface MSIDFamilyRefreshToken : MSIDRefreshToken | ||
| { | ||
|
|
There was a problem hiding this comment.
nit: if not property, please consider removing the {} block
|
|
||
| - (instancetype)initWithRefreshToken:(MSIDRefreshToken *)refreshToken | ||
| { | ||
| if (refreshToken && [refreshToken isKindOfClass:[MSIDRefreshToken class]]) |
There was a problem hiding this comment.
- Can we do the validation before passing into the init method? And also log the type of the token if type is not expected
- Please consider to remove duplicate code. e.g. if this is a subclass, maybe the credentialType is only needed?
| @@ -149,6 +151,11 @@ - (void)showWebComponentWithCompletion:(MSIDWebviewAuthCompletionHandler)complet | |||
|
|
|||
| } | |||
|
|
|||
| - (void)updateCustomHeadersForFRTSupportIfNeeded | |||
| { | |||
There was a problem hiding this comment.
nit saw this pattern in code base:
NSAssert(NO, @"Abstract method.");
return;
# Conflicts: # changelog.txt
… states: - On - Off - Ignore, keep as is
Proposed changes
Update the use of family refresh tokens, and instead use a single family refresh token.
Type of change
Risk
Additional information