Skip to content

Commit e4e3f36

Browse files
committed
refactor(mfa): address second round of PR review feedback
- Remove redundant setDefaultAccessDeniedHandler call in setupMfa(); the builder already initializes the default handler - Replace mutable HashMap with immutable Map.of() for factor-to-URI map - Tighten unprotected URI from /user/mfa/** wildcard to /user/mfa/status to avoid silently exposing future MFA endpoints - Remove redundant toUpperCase() calls in MfaAPI.getMfaStatus() since requiredFactors is already uppercased during stream mapping - Remove unused HashMap and AccessDeniedHandlerImpl imports
1 parent 27c4ffd commit e4e3f36

2 files changed

Lines changed: 10 additions & 14 deletions

File tree

src/main/java/com/digitalsanctuary/spring/user/api/MfaAPI.java

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -58,18 +58,15 @@ public ResponseEntity<JSONResponse> getMfaStatus(Authentication authentication)
5858
Collection<? extends GrantedAuthority> authorities = authentication.getAuthorities();
5959

6060
for (String factor : requiredFactors) {
61-
String normalized = factor.toUpperCase();
62-
String authorityString = MfaConfiguration.mapFactorToAuthority(normalized);
61+
String authorityString = MfaConfiguration.mapFactorToAuthority(factor);
6362
if (authorityString != null && hasAuthority(authorities, authorityString)) {
64-
satisfiedFactors.add(normalized);
63+
satisfiedFactors.add(factor);
6564
} else {
66-
missingFactors.add(normalized);
65+
missingFactors.add(factor);
6766
}
6867
}
6968
} else {
70-
for (String factor : requiredFactors) {
71-
missingFactors.add(factor.toUpperCase());
72-
}
69+
missingFactors.addAll(requiredFactors);
7370
}
7471

7572
MfaStatusResponse status = MfaStatusResponse.builder()

src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import java.util.ArrayList;
44
import java.util.Arrays;
55
import java.util.Collections;
6-
import java.util.HashMap;
6+
77
import java.util.List;
88
import java.util.Map;
99
import java.util.Set;
@@ -29,7 +29,7 @@
2929
import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder;
3030
import org.springframework.security.crypto.password.PasswordEncoder;
3131
import org.springframework.security.web.SecurityFilterChain;
32-
import org.springframework.security.web.access.AccessDeniedHandlerImpl;
32+
3333
import org.springframework.security.web.access.DelegatingMissingAuthorityAccessDeniedHandler;
3434
import org.springframework.security.web.authentication.LoginUrlAuthenticationEntryPoint;
3535
import org.springframework.security.web.session.HttpSessionEventPublisher;
@@ -263,9 +263,9 @@ private void setupMfa(HttpSecurity http) throws Exception {
263263
DelegatingMissingAuthorityAccessDeniedHandler.Builder handlerBuilder =
264264
DelegatingMissingAuthorityAccessDeniedHandler.builder();
265265

266-
Map<String, String> factorToUri = new HashMap<>();
267-
factorToUri.put("PASSWORD", mfaConfigProperties.getPasswordEntryPointUri());
268-
factorToUri.put("WEBAUTHN", mfaConfigProperties.getWebauthnEntryPointUri());
266+
Map<String, String> factorToUri = Map.of(
267+
"PASSWORD", mfaConfigProperties.getPasswordEntryPointUri(),
268+
"WEBAUTHN", mfaConfigProperties.getWebauthnEntryPointUri());
269269

270270
for (String factor : mfaConfigProperties.getFactors()) {
271271
String authority = MfaConfiguration.mapFactorToAuthority(factor);
@@ -276,7 +276,6 @@ private void setupMfa(HttpSecurity http) throws Exception {
276276
}
277277

278278
DelegatingMissingAuthorityAccessDeniedHandler handler = handlerBuilder.build();
279-
handler.setDefaultAccessDeniedHandler(new AccessDeniedHandlerImpl());
280279
http.exceptionHandling(handling -> handling.accessDeniedHandler(handler));
281280
log.info("MFA configured with access denied handler for factors: {}", mfaConfigProperties.getFactors());
282281
}
@@ -322,7 +321,7 @@ private List<String> getUnprotectedURIsList() {
322321
unprotectedURIs.add("/dev/**");
323322
}
324323
if (mfaConfigProperties.isEnabled()) {
325-
unprotectedURIs.add("/user/mfa/**");
324+
unprotectedURIs.add("/user/mfa/status");
326325
}
327326
unprotectedURIs.removeAll(Collections.emptyList());
328327
return unprotectedURIs;

0 commit comments

Comments
 (0)