Skip to content

Commit 47483e0

Browse files
committed
fix: Address PR review feedback for RegistrationGuard SPI
- Default deny reason to "Registration denied." when null/blank - Add INFO logging on guard denial in UserAPI, DSOAuth2UserService, DSOidcUserService - Add startup log in RegistrationGuardConfiguration when default guard is registered - Remove unnecessary @ExtendWith(MockitoExtension.class) from DefaultRegistrationGuardTest and RegistrationDecisionTest - Replace inline Mockito.mock() with @mock field in UserAPIRegistrationGuardTest - Add tests for null/blank deny reason defaulting - Update REGISTRATION-GUARD.md troubleshooting to reference actual log messages, document error code 6
1 parent 59b65bd commit 47483e0

9 files changed

Lines changed: 40 additions & 14 deletions

File tree

REGISTRATION-GUARD.md

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,12 @@ When a guard denies a registration, the behavior depends on the registration pat
154154
| **OAuth2** | `OAuth2AuthenticationException` with error code `"registration_denied"` — handled by Spring Security's OAuth2 failure handler |
155155
| **OIDC** | `OAuth2AuthenticationException` with error code `"registration_denied"` — handled by Spring Security's OAuth2 failure handler |
156156

157+
The JSON error code `6` identifies a registration guard denial specifically, distinguishing it from other registration errors (e.g., code `1` for validation failures, code `2` for duplicate accounts). Client-side code can check this code to display targeted messaging.
158+
157159
For OAuth2/OIDC denials, customize the user experience by configuring Spring Security's OAuth2 login failure handler to inspect the error code and display an appropriate message.
158160

161+
All denied registrations are logged at INFO level with the email, source, and denial reason.
162+
159163
## Key Constraints
160164

161165
- **Single-bean SPI** — Only one `RegistrationGuard` bean may be active at a time. This is not a chain or filter pattern; define exactly one guard.
@@ -168,12 +172,8 @@ For OAuth2/OIDC denials, customize the user experience by configuring Spring Sec
168172
**Guard Not Activating**
169173
- Ensure your guard class is annotated with `@Component` (or otherwise registered as a Spring bean)
170174
- Verify the class is within a package that is component-scanned by your application
171-
- Check that `DefaultRegistrationGuard` is being replaced — enable debug logging:
172-
```yaml
173-
logging:
174-
level:
175-
com.digitalsanctuary.spring.user.registration: DEBUG
176-
```
175+
- At startup, the library logs `"No custom RegistrationGuard bean found — using DefaultRegistrationGuard (permit-all)"` at INFO level. If you see this message, your custom guard bean is not being detected.
176+
- You can also check the active guard via `/actuator/beans` (if enabled) or your IDE's Spring tooling.
177177

178178
**Multiple Guards Defined**
179179
- Only one `RegistrationGuard` bean is allowed. If multiple beans are defined, Spring will throw a `NoUniqueBeanDefinitionException` at startup.

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@ public ResponseEntity<JSONResponse> registerUserAccount(@Valid @RequestBody User
112112
RegistrationDecision decision = registrationGuard.evaluate(
113113
new RegistrationContext(userDto.getEmail(), RegistrationSource.FORM, null));
114114
if (!decision.allowed()) {
115+
log.info("Registration denied for email: {} source: FORM reason: {}", userDto.getEmail(), decision.reason());
115116
return buildErrorResponse(decision.reason(), 6, HttpStatus.FORBIDDEN);
116117
}
117118

@@ -409,6 +410,7 @@ public ResponseEntity<JSONResponse> registerPasswordlessAccount(@Valid @RequestB
409410
RegistrationDecision decision = registrationGuard.evaluate(
410411
new RegistrationContext(dto.getEmail(), RegistrationSource.PASSWORDLESS, null));
411412
if (!decision.allowed()) {
413+
log.info("Registration denied for email: {} source: PASSWORDLESS reason: {}", dto.getEmail(), decision.reason());
412414
return buildErrorResponse(decision.reason(), 6, HttpStatus.FORBIDDEN);
413415
}
414416

src/main/java/com/digitalsanctuary/spring/user/registration/RegistrationDecision.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ public static RegistrationDecision allow() {
2525
* @return a denying decision with the given reason
2626
*/
2727
public static RegistrationDecision deny(String reason) {
28+
if (reason == null || reason.trim().isEmpty()) {
29+
reason = "Registration denied.";
30+
}
2831
return new RegistrationDecision(false, reason);
2932
}
3033
}

src/main/java/com/digitalsanctuary/spring/user/registration/RegistrationGuardConfiguration.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,18 +4,22 @@
44
import org.springframework.context.annotation.Bean;
55
import org.springframework.context.annotation.Configuration;
66

7+
import lombok.extern.slf4j.Slf4j;
8+
79
/**
810
* Auto-configuration for the {@link RegistrationGuard} SPI.
911
*
1012
* <p>Registers a {@link DefaultRegistrationGuard} (permit-all) when no custom
1113
* {@link RegistrationGuard} bean is defined by the consuming application.</p>
1214
*/
15+
@Slf4j
1316
@Configuration
1417
public class RegistrationGuardConfiguration {
1518

1619
@Bean
1720
@ConditionalOnMissingBean(RegistrationGuard.class)
1821
public RegistrationGuard registrationGuard() {
22+
log.info("No custom RegistrationGuard bean found — using DefaultRegistrationGuard (permit-all)");
1923
return new DefaultRegistrationGuard();
2024
}
2125
}

src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,8 @@ public User handleOAuthLoginSuccess(String registrationId, OAuth2User oAuth2User
107107
RegistrationDecision decision = registrationGuard.evaluate(
108108
new RegistrationContext(user.getEmail(), RegistrationSource.OAUTH2, registrationId));
109109
if (!decision.allowed()) {
110+
log.info("Registration denied for email: {} source: OAUTH2 provider: {} reason: {}",
111+
user.getEmail(), registrationId, decision.reason());
110112
throw new OAuth2AuthenticationException(
111113
new OAuth2Error("registration_denied"), decision.reason());
112114
}

src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,8 @@ public User handleOidcLoginSuccess(String registrationId, OidcUser oidcUser) {
9797
RegistrationDecision decision = registrationGuard.evaluate(
9898
new RegistrationContext(user.getEmail(), RegistrationSource.OIDC, registrationId));
9999
if (!decision.allowed()) {
100+
log.info("Registration denied for email: {} source: OIDC provider: {} reason: {}",
101+
user.getEmail(), registrationId, decision.reason());
100102
throw new OAuth2AuthenticationException(
101103
new OAuth2Error("registration_denied"), decision.reason());
102104
}

src/test/java/com/digitalsanctuary/spring/user/api/UserAPIRegistrationGuardTest.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,9 @@ class UserAPIRegistrationGuardTest {
7070
@Mock
7171
private RegistrationGuard registrationGuard;
7272

73+
@Mock
74+
private WebAuthnCredentialManagementService webAuthnService;
75+
7376
@InjectMocks
7477
private UserAPI userAPI;
7578

@@ -155,7 +158,6 @@ void shouldRejectPasswordlessRegistrationWhenGuardDenies() throws Exception {
155158
dto.setFirstName("Test");
156159
dto.setLastName("User");
157160

158-
WebAuthnCredentialManagementService webAuthnService = org.mockito.Mockito.mock(WebAuthnCredentialManagementService.class);
159161
when(webAuthnCredentialManagementServiceProvider.getIfAvailable()).thenReturn(webAuthnService);
160162
when(registrationGuard.evaluate(any(RegistrationContext.class)))
161163
.thenReturn(RegistrationDecision.deny("Beta access required"));
@@ -183,7 +185,6 @@ void shouldAllowPasswordlessRegistrationWhenGuardAllows() throws Exception {
183185
.disabled()
184186
.build();
185187

186-
WebAuthnCredentialManagementService webAuthnService = org.mockito.Mockito.mock(WebAuthnCredentialManagementService.class);
187188
when(webAuthnCredentialManagementServiceProvider.getIfAvailable()).thenReturn(webAuthnService);
188189
when(registrationGuard.evaluate(any(RegistrationContext.class)))
189190
.thenReturn(RegistrationDecision.allow());

src/test/java/com/digitalsanctuary/spring/user/registration/DefaultRegistrationGuardTest.java

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,7 @@
44

55
import org.junit.jupiter.api.DisplayName;
66
import org.junit.jupiter.api.Test;
7-
import org.junit.jupiter.api.extension.ExtendWith;
8-
import org.mockito.junit.jupiter.MockitoExtension;
97

10-
@ExtendWith(MockitoExtension.class)
118
@DisplayName("DefaultRegistrationGuard Tests")
129
class DefaultRegistrationGuardTest {
1310

src/test/java/com/digitalsanctuary/spring/user/registration/RegistrationDecisionTest.java

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,7 @@
44

55
import org.junit.jupiter.api.DisplayName;
66
import org.junit.jupiter.api.Test;
7-
import org.junit.jupiter.api.extension.ExtendWith;
8-
import org.mockito.junit.jupiter.MockitoExtension;
97

10-
@ExtendWith(MockitoExtension.class)
118
@DisplayName("RegistrationDecision Tests")
129
class RegistrationDecisionTest {
1310

@@ -28,4 +25,22 @@ void shouldCreateDenyDecision() {
2825
assertThat(decision.allowed()).isFalse();
2926
assertThat(decision.reason()).isEqualTo("Invite code required");
3027
}
28+
29+
@Test
30+
@DisplayName("Should default reason when deny is called with null")
31+
void shouldDefaultReasonWhenNull() {
32+
RegistrationDecision decision = RegistrationDecision.deny(null);
33+
34+
assertThat(decision.allowed()).isFalse();
35+
assertThat(decision.reason()).isEqualTo("Registration denied.");
36+
}
37+
38+
@Test
39+
@DisplayName("Should default reason when deny is called with blank string")
40+
void shouldDefaultReasonWhenBlank() {
41+
RegistrationDecision decision = RegistrationDecision.deny(" ");
42+
43+
assertThat(decision.allowed()).isFalse();
44+
assertThat(decision.reason()).isEqualTo("Registration denied.");
45+
}
3146
}

0 commit comments

Comments
 (0)