Repository navigation
feat(cognito): implement user MFA preferences - #2896
Conversation
|
🎉 Thanks for your first pull request to Floci! Your CI checks need a maintainer to approve them before they run. That is GitHub's standard gate on first-time contributors, not a problem with your PR — so if the checks look like they are doing nothing, that is why. Once a maintainer approves, CI and the compatibility suite start automatically. Nothing is needed from you in the meantime. While you wait, a couple of things that make review faster:
Come join us in Slack — it is the fastest way to reach maintainers if you get stuck, or want feedback on an approach before investing more time in it. |
|
| Filename | Overview |
|---|---|
| src/main/java/io/github/hectorvent/floci/services/cognito/CognitoService.java | Implements both MFA-preference update paths and fixes rejected-update mutation ordering, but the self-service path still trusts unverified JWT claims. |
| src/main/java/io/github/hectorvent/floci/services/cognito/CognitoJsonHandler.java | Adds JSON dispatch and request parsing for both MFA-preference operations, passing the self-service token directly into the service. |
| src/main/java/io/github/hectorvent/floci/services/cognito/model/CognitoUser.java | Adds persisted email MFA settings to the Cognito user model. |
| src/main/java/io/github/hectorvent/floci/services/cognito/model/EmailMfaSettings.java | Defines the reflection-registered persisted model for enabled and preferred email MFA state. |
| src/test/java/io/github/hectorvent/floci/services/cognito/CognitoServiceTest.java | Covers administrative updates, invalid-update atomicity, and clearing preferred MFA when email MFA is disabled. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Request[Admin or self-service MFA request] --> Route[Cognito JSON handler]
Route --> Mode{Operation type}
Mode -->|Admin| Lookup[Look up requested pool and user]
Mode -->|Self-service| Claims[Extract pool and username from token]
Claims --> Revocation[Check revocation and global sign-out]
Revocation --> Lookup
Lookup --> Validate[Compute and validate MFA preference]
Validate --> Persist[Persist EmailMfaSettings]
Persist --> Response[Return success]
Reviews (3): Last reviewed commit: "fix(cognito): register email MFA setting..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| String username = extractUsernameFromToken(accessToken); | ||
| String poolId = extractPoolIdFromToken(accessToken); | ||
| String jti = extractJtiFromToken(accessToken); | ||
|
|
||
| if (username == null || poolId == null || jti == null) { |
There was a problem hiding this comment.
When a caller supplies a forged, expired, or wrong-type JWT containing the required claims, setUserMFAPreference trusts those decoded claims and updates the selected user's MFA preference without verifying the signature, expiry, or token_use. This allows unauthorized changes to another user's MFA settings. How this was verified: The request token flows directly through claim-only extraction and revocation checks to userStore.put, with no authenticity, expiry, or token-type validation.
Context Used: AGENTS.md (source)
| if (enabled != null) { | ||
| settings.setEnabled(enabled); | ||
| } | ||
|
|
||
| if (preferredMfa != null) { | ||
| settings.setPreferredMfa(preferredMfa); | ||
| } | ||
|
|
||
| if (settings.isPreferredMfa() && !settings.isEnabled()) { | ||
| throw new AwsException( | ||
| "InvalidParameterException", | ||
| "Preferred MFA setting cannot be enabled when the MFA method is disabled.", | ||
| 400 | ||
| ); |
There was a problem hiding this comment.
When a request sets PreferredMfa=true while email MFA is disabled, this method mutates the live settings object before throwing InvalidParameterException. The rejected preference therefore remains in memory and can be retained by a later partial update, so a failed request still changes the user's effective MFA state.
Context Used: AGENTS.md (source)
pgermosen
left a comment
pgermosen
left a comment
There was a problem hiding this comment.
@AbarnaaSree thanks for taking this on, and welcome, this is your first PR here. A few things to work through before this is ready.
The conflict and the size of the diff both trace back to the same thing: your description says SetUserPoolMfaConfig/GetUserPoolMfaConfig are handled separately in "Part 1," but the diff and commit list here include a full setUserPoolMfaConfig implementation anyway. That's PR #2831, a different contributor's still-open, unmerged PR. It looks like this branch was built on top of #2831 rather than main, so it's carrying those commits as its own, which is both why it conflicts and why the diff is bigger and more confusing than your own description promises. Worth rebasing onto main (once #2831 lands, or cleanly against it in the meantime) so this PR's diff only contains the user-preference work it's actually about.
Once that's sorted, there's a real bug in updateEmailMfaPreference worth fixing: it mutates the live EmailMfaSettings object before the validate-and-throw check runs. I traced this through the actual storage layer rather than taking it on faith, InMemoryStorage.get() returns the literal object reference from the backing map, not a copy, so when a request combines something like Enabled=false with a previously-set PreferredMfa=true and gets rejected, the mutation already happened in place before the exception fires. A later GetUser sees the partially-applied state from a request that was supposed to fail atomically, not the state from before it. Validating before mutating, or mutating a copy and only writing back after validation passes, would fix it.
On the JWT point greptile raised: the code claim is accurate, setUserMFAPreference decodes the token's identity claims with no signature verification, but that's not something this PR introduces. Every existing self-service access-token endpoint in this file, getUser, updateUserAttributes, deleteUserAttributes, does the identical unverified decode already. So this is extending an existing pattern rather than opening a new hole, worth being aware of rather than something to fix in this PR alone. One thing worth knowing, though: AWS's own docs for this specific operation say Cognito doesn't evaluate IAM policies for it at all, authorization is purely the access token's validity, which makes a forgeable token a more direct bypass here than for operations IAM could also gate.
Last thing, not confirmed against a live measurement either way, just an internal inconsistency worth checking: your own setUserPoolMfaConfig method explicitly clears the factor configuration when MFA is turned off, but updateEmailMfaPreference doesn't do the equivalent. Disabling email MFA while it was previously the preferred method throws instead of succeeding, since PreferredMfa isn't cleared along with it. Might be worth applying the same clearing logic there.
hectorvent
added
waiting-author
2e8ef99 to
e17fb3d
Compare
|
Thanks for the review! I’ve rebased the work onto |
pgermosen
left a comment
pgermosen
left a comment
There was a problem hiding this comment.
Good progress, both things from last round are genuinely fixed. updateEmailMfaPreference now validates before it mutates anything, and there's a regression test proving the prior state survives a rejected update. Disabling email MFA now clears PreferredMfa instead of throwing too, traced both branches and that's correct, including the case where a request tries to enable PreferredMfa and disable the method in the same call, which still correctly throws.
The thing actually blocking the build right now is PersistedModelReflectionTest failing on EmailMfaSettings, it's missing @RegisterForReflection. It's a field on the persisted CognitoUser now, so it's reachable from storage, and every sibling model class in this service carries that annotation, CognitoUser itself does. Adding it should clear the build. While you're in there, @JsonIgnoreProperties(ignoreUnknown = true) would match the sibling classes too, though that one isn't what's failing.
One thing carried over from last round, not new: setUserMFAPreference still authorizes off an access token with no signature check, same as every other self-service endpoint in this file already does, so not something to fix here, just flagging again for completeness.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
The reflection fix is pushed in |
pgermosen
left a comment
pgermosen
left a comment
There was a problem hiding this comment.
That's the fix, confirmed. Both annotations added, matching the sibling classes, and CI is green now. Nothing else outstanding.
pgermosen
removed
the
waiting-author
|
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
3 participants
Summary
Implements the per-user MFA preference portion of #2433.
Changes
Added
AdminSetUserMFAPreferenceAdded
SetUserMFAPreferenceAdded persisted
EmailMfaSettingstoCognitoUserEnabledPreferredMfaAdded validation to prevent
PreferredMfafrom being enabled when email MFA is disabledAdded access-token validation for
SetUserMFAPreferenceUpdated the existing
SetUserPoolMfaConfigregression test to expect404 ResourceNotFoundExceptionfor a non-existent user poolTesting
git diff --cached --checkpassesScope
This PR focuses on the per-user MFA preference portion of #2433.
SetUserPoolMfaConfig/GetUserPoolMfaConfigare being handled separately in Part 1.The
EMAIL_OTPauthentication challenge remains out of scope as described in #2433.