refactor: apply ConfigurationProperties
This commit is contained in:
simple321vip
@@ -9,3 +9,141 @@
|
||||
file tools or Python instead.
|
||||
2. Use English in code. Avoid Chinese, Japanese, or any other non-ASCII
|
||||
natural language inside source files.
|
||||
|
||||
## Boot rules
|
||||
|
||||
The very first time I ran `java -cp ... cn.violin.iam.ViolinIamApplication`,
|
||||
Spring failed with "Parameter 0 of constructor in
|
||||
`cn.violin.core.config.WebMvcConfig` required a bean of type
|
||||
`cn.violin.core.interceptor.AuthenticationInterceptor` that could not be
|
||||
found". The unit-test suite was green but had never actually booted the
|
||||
context. Concrete gotchas to fix:
|
||||
|
||||
- **`@SpringBootApplication` MUST set `scanBasePackages = {"cn.violin"}`**
|
||||
explicitly. The default only scans the app's own sub-package
|
||||
(`cn.violin.iam`), which excludes `cn.violin.core.interceptor.AuthenticationInterceptor`,
|
||||
`CurrentUserHandlerMethodArgumentResolver`, `TraceIdFilter`, and other
|
||||
framework beans. Spring Boot auto-config registers the
|
||||
`@AutoConfiguration` classes themselves, but their `@Bean` dependencies
|
||||
that are not themselves exposed via another `@Bean` still need component-scan
|
||||
coverage. Compare with `violin-auth`, which has always used
|
||||
`scanBasePackages = {"cn.violin"}`; it never tripped over this.
|
||||
|
||||
- **`@MapperScan("cn.violin.*.mapper")` — never use bare `cn.violin`.**
|
||||
The broad form hoovers up every interface in the project, including
|
||||
framework-internal ones like `JwksClient`, and MyBatis then complains
|
||||
at runtime:
|
||||
`Invalid bound statement (not found): cn.violin.core.security.JwksClient.loadKeys`.
|
||||
Narrowing the pattern prevents MyBatis from treating non-mapper
|
||||
interfaces as proxies.
|
||||
|
||||
- **`@Service` impls whose interface is in the same package should be
|
||||
marked `@Primary` when `scanBasePackages` is widened.** Once
|
||||
`cn.violin.*` is scanned, Spring ends up holding two `BeanDefinition`s
|
||||
of the same bean type (the impl + some derived candidate), and
|
||||
`@Autowired` of the interface fails with `NoUniqueBeanDefinitionException:
|
||||
expected single matching bean but found 2: OAuthServiceImpl,OAuthService`.
|
||||
Adding `@Primary` to the impl collapses that to a single candidate.
|
||||
|
||||
- **Spring 6 forbids `@PostConstruct` methods with parameters.**
|
||||
`SnowflakeId.init(SnowflakeProperties props)` looked innocuous but
|
||||
`IllegalStateException: Lifecycle annotation requires a no-arg method`
|
||||
hit on `@SpringBootTest`. Use `InitializingBean` + `@Autowired` for
|
||||
the setter (properties injection remains valid via
|
||||
`wireProperties`). Same pattern applies to any other singleton that
|
||||
needs configuration at boot.
|
||||
|
||||
- **`@ConfigurationProperties` on Java `record` types needs
|
||||
`@ConstructorBinding`** when the canonical constructor does not match
|
||||
Spring's expected discovery. Without it, Spring fails with
|
||||
`No default constructor found`.
|
||||
|
||||
- **Adding `@SpringBootTest` early catches all four of these.** Place a
|
||||
smoke test (`ApplicationContextLoadsTest`) in
|
||||
`src/test/java/.../boot/` and run `mvn test`. If the unit tests are
|
||||
green but the boot test fails, the bug is precisely the kind that
|
||||
silently passes when scanning only `@Component`-annotated classes
|
||||
but blows up at full startup.
|
||||
|
||||
## Design notes (do not skip)
|
||||
|
||||
## Design notes (do not skip)
|
||||
|
||||
### JWT revocation propagation — what we tried, what we picked, and why
|
||||
|
||||
The naive question "should we drop `customerId` from the JWT claim so
|
||||
downstream must call IAM every request?" was the wrong frame. Below is
|
||||
the actual decision tree we walked.
|
||||
|
||||
#### Problem
|
||||
A user's `t_user.customer_id` is migrated from tenant A to tenant B. The
|
||||
JWT issued at login still carries `customerId=A`. Without propagation
|
||||
the user keeps operating under A's RBAC scope (and SAR cache) for the
|
||||
remainder of the token TTL.
|
||||
|
||||
#### Industry patterns surveyed
|
||||
|
||||
1. **Short TTL + RFC 7662 introspection** (Google / AWS / GitHub /
|
||||
Slack). Token TTL = 5-15 min; each request consults an introspection
|
||||
endpoint (with local cache) to check the token is still live.
|
||||
Revocation propagates in <1 s via shared cache / pub-sub.
|
||||
|
||||
2. **`revokedCustomerBindings` cache** (medium-scale).
|
||||
`Cache<userId, oldCustomerId>` with TTL covering the longest possible
|
||||
token lifetime. `UserService.changeCustomer(...)` writes the DB row
|
||||
then publishes `(userId, oldCustomerId)` into the cache. The cache
|
||||
TTL ensures stale entries eventually fall out without a cron.
|
||||
|
||||
3. **Active binding flag** (per-user cache of currently-valid
|
||||
customerId). cache miss → fall back to DB lookup. cache hit but
|
||||
cache.customerId != JWT claim → treat as revoke signal.
|
||||
|
||||
4. **Kafka / event-driven revocation** (large clusters, >10 services).
|
||||
IAM emits `user.tenant.changed` to a topic; subscribers invalidate
|
||||
local cache on receipt. Strictly over-engineered for a 1-issuer /
|
||||
5-subscriber fleet.
|
||||
|
||||
#### What we picked and why
|
||||
|
||||
We picked **option 2** (`revokedCustomerBindings` cache) and combined
|
||||
it with **token TTL reduction** from 24 h to **1 h**. Net result:
|
||||
|
||||
- Worst-case staleness window = `min(token TTL, cache TTL)` = 1 h.
|
||||
- No per-request IAM call (the user's framing-question was "is the
|
||||
extra IAM call worth it?" — answer: no, because it moves IAM onto
|
||||
the hot path and turns it into a single point of failure).
|
||||
- Single-instance IAM means cache state is authoritative without
|
||||
sticky sessions or shared cache.
|
||||
|
||||
#### What we did NOT do (and the reasons)
|
||||
|
||||
- **Drop `customerId` from JWT claim.** Would force every permission
|
||||
check to call `POST /api/v1/internal/check-permission`, adding
|
||||
2-5 ms latency per request and making IAM the hot path. Token
|
||||
claim is now treated as **informational only** — never trusted
|
||||
for authorization. All decisions go through IAM SAR.
|
||||
- **Re-sign JWT on every customer change.** Requires Kafka or
|
||||
webhook infra to broadcast — engineering cost far exceeds risk
|
||||
reduction given the 1 h TTL already covers the window.
|
||||
- **Use Authentik's session revocation directly.** Adds an upstream
|
||||
dependency IAM cannot fail without.
|
||||
|
||||
#### Implementation hooks already in place
|
||||
|
||||
| File | Role |
|
||||
| --- | --- |
|
||||
| `cn.violin.common.context.RequestContext` | exposes `setCustomerIdHint(...)` and `getVerifiedCustomerId()` — verified provenance is now mandatory for SAR / PermissionAspect / RbacController / ResourcePermissionAspect |
|
||||
| `SubjectAccessReviewRequest.customerVerified` | explicit boolean; SAR denies on `false` |
|
||||
| `PermissionAspect` | reads `getVerifiedCustomerId()` first, throws `UnauthorizedException` if null |
|
||||
| `ResourcePermissionAspect` | same |
|
||||
| `RbacController.access-check` | same |
|
||||
| `InternalController.check-permission` | accepts only verified (set from HMAC + ServiceAllowlist mapping) |
|
||||
|
||||
#### Followup still pending (backlog, not on the critical path)
|
||||
|
||||
Add `CustomerBindingRevocationCache` and have `UserService.changeCustomer(...)`
|
||||
push `(sub, oldCustomerId)` into it. Have `PermissionAspect` consult the cache
|
||||
as a secondary check (after verified gate). Reduce default `expirationMs`
|
||||
in `ViolinJwtProperties` from `86_400_000` (24 h) to `3_600_000` (1 h).
|
||||
These three together close the revocation window to the bound of TTL.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user