feat: fcli fod session login: Add support for TOTP and multi-factor authentication (MFA) codes in the FoD session login command - #1073
Conversation
…s in the FoD session login command
| FoDTokenCreateResponse createTokenResponse = FoDOAuthHelper.createToken(urlConfig, loginOptions.getUserCredentials(), loginOptions.getAuthOptions().getScopes()); | ||
| sessionDescriptor = new FoDSessionDescriptor(urlConfig, createTokenResponse); | ||
| } else if (loginOptions.hasUserCredentials()) { | ||
| var credBuilder = BasicFoDUserCredentials.builder() |
There was a problem hiding this comment.
Why did you move this code for building credential objects into this class? The original code was much more compact with better separation of concerns. With the new MFA support, I'd expect a single line like this to create the token:
FoDTokenCreateResponse createTokenResponse = FoDOAuthHelper.createToken(urlConfig, loginOptions.getUserCredentials(), loginOptions.getMfaCode(), loginOptions.getAuthOptions().getScopes());
FoDOAuthHelper should be null-safe for MFA code, i.e., it should handle both full object is null, or non-null object with securityCode being null. For loginOptions::getMfaCode, probably best to return null object if securityCode option value is null.
| FoDTokenCreateResponse createTokenResponse = FoDOAuthHelper.createToken(urlConfig, credBuilder.build(), | ||
| authCodeBuilder.build(), loginOptions.getAuthOptions().getScopes()); | ||
| sessionDescriptor = new FoDSessionDescriptor(urlConfig, createTokenResponse); | ||
| } catch (UnexpectedHttpResponseException e) { |
There was a problem hiding this comment.
- Current exception handling block is too long, taking away attention from actual logic above. Closely related, it doesn't make sense to construct three local strings with static contents; better to move these to actual constants, which would automatically make exception handling code much shorter
- Why do we have dedicated exception handling when authenticating with user credentials, but not when authenticating with client credentials? Shouldn't we throw a similar
FcliSimpleExceptionwith appropriate guidance (like client credentials incorrect or expired)?
There was a problem hiding this comment.
I have had class-level constants created for the messages and have used them.
Note: Across the entire codebase, every other command throws FcliSimpleException with the message string inline at the throw site, either as a string literal directly or via String.format(...). No other command pre-defines multi-line message constants like MFA_GUIDANCE, ERROR_WITH_CODE, and ERROR_WITHOUT_CODE.
There was a problem hiding this comment.
Added dedicated exception handling for client credentials as well.
| /** | ||
| * Basic immutable FoD user credentials with builder pattern. | ||
| */ | ||
| public static final class BasicFoDUserCredentials implements IFoDUserCredentials { |
There was a problem hiding this comment.
I think (but please double-check) that BasicFoDUserCredentials and the new BasicFoDUserAuthCode are currently only referenced within FoDSessionLoginOptions, so from that perspective, it makes more sense to keep these as inner classes instead of separate top-level types. However, the HTTP MCP server for example defines its own HttpMcpFoDUserCredentials class with similar implementation as BasicFoDUserCredentials. As such, it may makes sense to keep the new top-level BasicFoD* classes, and change the MCP HTTP server (and any other code that creates IFoDUserCredentials) to utilize these top-level BasicFoD* classes instead of providing their own implementations of IFoDUserCredentials.
Can you please check:
- Which classes currently implement
IFoDUserCredentialsand similar interfaces (like SSC variant)? - Whether these can be easily replaced with a single, top-level
Basic*implementation
To summarize:
- If existing implementations of
IFoDUserCredentialscan be easily merged, keep the top-levelBasic*classes and refactor all code to use this single implementation - If there are significant differences between implementations and
Basic*classes are only instantiated inFoDSessionLoginOptions, move thoseBasic*classes back toFoDSessionLoginOptionsas inner classes
There was a problem hiding this comment.
The Basic* classes are kept as top-level classes. please review the code now.
| public static final FoDTokenCreateResponse createToken(IUrlConfig urlConfig, IFoDUserCredentials uc, String... scopes) { | ||
| Map<String,Object> formData = generateTokenRequest(uc, scopes); | ||
| Map<String,Object> formData = generateTokenRequest(uc, IFoDUserAuthCode.NONE, scopes); | ||
| try ( var unirest = UnirestHelper.createUnirestInstance() ) { |
There was a problem hiding this comment.
- Indentation seems wrong (looks like this was already wrong in original code, but please fix)
…hnicalException, update java.instructions.md also
2 participants
Summary:
Adds support for TOTP and Multi-Factor Authentication (MFA) security codes during FoD session authentication. Users can now provide a security code as part of the FoD session login command when their FoD tenant requires additional authentication.
New CLI Options:
--code (-c): Optional parameter to provide a TOTP or MFA security code. Can be used interactively or as a command-line argument.--totp: Optional flag indicating that the provided code is a TOTP generated by an authenticator application. If omitted, the code is treated as a standard MFA code (for example, a code received via email).Tested the following scenarios:
Benefits: