feat: Add FoD Session MFA Code Request Command - #1080
Conversation
| @Option(names = {"-t", "--tenant"}, required = true) | ||
| @MaskValue(sensitivity = LogSensitivityLevel.low, description = "FOD TENANT") | ||
| private String tenant; | ||
| @Option(names = {"--delivery-mode", "-m"}, required = true) |
There was a problem hiding this comment.
Let's keep this option, but make it optional. If not specified, we try both.
| public class FoDSessionRequestMfaCodeCommand extends AbstractOutputCommand implements IJsonNodeSupplier, IActionCommandResultSupplier { | ||
| @Getter @Mixin private FoDOutputHelperMixins.RequestMfaCode outputHelper; | ||
| @Mixin private FoDSessionLoginOptions.FoDUrlConfigOptions urlConfigOptions; | ||
| @Mixin private UserCredentialOptions userCredentials; |
There was a problem hiding this comment.
The login command already uses an ArgGroup that declares user, password, and tenant options, right? Why not just re-use that here?
|
|
||
| @Override | ||
| public JsonNode getJsonNode() { | ||
| FoDMfaHelper.requestMfaCode( |
There was a problem hiding this comment.
Please use a similar approach as FoDOAuthHelper, i.e., use IFoUserCredentials to pass tenant/user/password as a single method parameter.
| .asEmpty(); | ||
|
|
||
| //security hardening | ||
| java.util.Arrays.fill(password, ' '); // Clear original char array |
There was a problem hiding this comment.
- Please use proper import
- This method doesn't own the
char[], so clearing it here is a side-effect that callers may not expect (maybe caller wants to do something else with the password). As per best practices, method side effects should be avoided. It would be better to have the caller (that declares/owns thechar[]) decide whether thechar[]can be cleared or is still needed for something else. - Note that we don't explicitly clear password character arrays anywhere else, so not sure how to best make this consistent, i.e., either remove this line, or update all other commands to explicitly clear password character arrays.
| public static final String CMD_NAME = "upload-file"; | ||
| } | ||
|
|
||
| @Command(aliases = "mfa") |
There was a problem hiding this comment.
Although it's nice to have a short alias, I'm not 100% sure whether I like this alias ('mfa' by itself has a meaning, but doesn't describe what this command does, and also maybe in the future we need to add other MFA-related commands).
Related, can we think of a shorter full command name? I think the option on the login command is now --code, so maybe we can name the command request-code? Any other word for 'request'? Maybe ask Shajaan for suggestions?
Maybe we should reconsider this altogether, thinking first about whether we should improve the MFA handling on the login command. Some potential approaches:
- Restructure MFA options on
logincommand:--mfa=<code>and--totp=<code>, then have separatemfacommand to request MFA code (thus command name matching thelogin--mfaoption name) - Integrate MFA request into
logincommand, i.e.,--mfa[=<code>], if no code given, we check whether there's a cached code; if not (or cached code results in denied exception due to being expired), we send the 'request MFA' request and then prompt the user to enter the MFA code
In other words, maybe we should take a step back to decide on the most user-friendly approach (while maintaining backward compatibility, possibly marking existing functionality as deprecated).
2 participants
Adds a new
fcli fod session request-mfa-codecommand to request Multi-Factor Authentication (MFA) codes from Fortify on Demand (FoD) via Email or SMS delivery methods.Triggers FoD to send a multi-factor authentication code to the specified delivery method for the given tenant and user. Use the received code with
fcli fod session login --code <code>to complete authentication.