feat: Issue 98 controller functionality#114
feat: Issue 98 controller functionality#114springframeworkguru wants to merge 6 commits intoGreenButtonAlliance:mainfrom
Conversation
…pdated: Application Information, Customer Information, Customer Account, Electric Power Quality, Interval Block, Meter Reading, Usage Point, Reading Type, Usage Summary.
dfcoffin
left a comment
There was a problem hiding this comment.
Code Review — PR #114
Thanks for this contribution! The service layer expansion, mapper fixes, and repository cleanup are solid work. There are three items that need to be addressed before merge — all are small fixes that will prevent runtime failures during API testing.
Blocking Issues (please fix before merge)
1. SQL Injection — see inline comment on OAuth2ClientManagementController.java:174
2. Opaque Token Fallback — see inline comment on AuthorizationServerConfig.java:154
3. Batch Export — see inline comment on ApplicationInformationServiceImpl.java:124
Tech Debt — Documented for Future Work
The following items are noted for future cleanup. They are acceptable for the current testing-focused scope of this PR:
Auth Server in DataCustodian (Architecture)
The auth server code in openespi-datacustodian/authserver/ is understood as a temporary arrangement to enable integrated API testing. This should be separated before any production deployment. Consider creating an ADR to document the separation plan and timeline.
Security Items for Hardening (before production)
/connect/registerendpoint (ClientRegistrationController) has no@PreAuthorize— open client registration{noop}password storage inClientRegistrationController— plaintext secrets{bcrypt}secretis not a valid bcrypt hash — will fail password validationSystem.out.printlninAuthorizationServerConfig.initializeDefaultClients()— should use logger
OIDC Gating
OIDC is now only enabled when JWT is configured. On main, OIDC was always enabled. Since registered clients include OidcScopes.OPENID, this may break OIDC flows in the default (opaque token) path. If this is intentional for the testing setup, please document the rationale.
Export Services
- 7 new per-resource export services are functionally redundant with existing
UsageExportService/CustomerExportService— consider consolidating - 3 of 7 new export services have no tests (
CustomerAccountExportService,ElectricPowerQualitySummaryExportService,IntervalBlockExportService) ApplicationInformationExportServiceTestuses JUnit assertions instead of project-standard AssertJ
Mock Admin Endpoints
OAuthAdminController.listTokens() and listAuthorizations() return hardcoded mock data, not actual system state. These should be wired to real data before production use.
Inconsistent @Repository Cleanup
@Repository was removed from IntervalBlockRepository but not from other Spring Data JPA interfaces. Consider applying consistently.
What Looks Good
- MapStruct mapper updates — Correctly ignoring IdentifiedObject base class fields not in the XSD
IntervalBlockRepositoryquery refactor — Derived method name is semantically equivalent to the explicit@QueryApplicationInformationServiceinterface expansion — Well-defined, follows existing patternsApplicationInformationServiceImplCRUD methods — Proper@Transactionalannotations, resolved TODO forfindByDataCustodianClientId- JAXB annotation placement — Correctly on DTOs only, no entity/embeddable violations
- Namespace handling — Correct ESPI and Atom namespaces throughout
Reviewed with Claude Code
|
|
||
| // Add sorting | ||
| String direction = "desc".equalsIgnoreCase(sortDirection) ? "DESC" : "ASC"; | ||
| sql.append(" ORDER BY c.").append(sortBy).append(" ").append(direction); |
There was a problem hiding this comment.
Blocking: SQL Injection
The sortBy request parameter is concatenated directly into SQL without validation:
sql.append(" ORDER BY c.").append(sortBy).append(" ").append(direction);An attacker with ADMIN role could inject arbitrary SQL through this parameter.
Suggested fix — whitelist allowed column names:
private static final Set<String> ALLOWED_SORT_COLUMNS = Set.of(
"client_id", "client_name", "client_id_issued_at");
// In listClients():
if (!ALLOWED_SORT_COLUMNS.contains(sortBy)) {
sortBy = "client_id";
}| if (isJwtResourceServerConfigured()) { | ||
| resourceServer.jwt(Customizer.withDefaults()); | ||
| } else { | ||
| resourceServer.opaqueToken(Customizer.withDefaults()); |
There was a problem hiding this comment.
Blocking: Opaque Token Fallback Will Fail at Runtime
When JWT properties are not set, this falls back to opaqueToken(Customizer.withDefaults()), but no OpaqueTokenIntrospector bean is defined. The introspectionUri, clientId, and clientSecret fields are injected via @Value but never wired into any bean. This will cause a startup failure.
Suggested fix — wire the introspector:
resourceServer.opaqueToken(opaque -> opaque
.introspectionUri(introspectionUri)
.introspectionClientCredentials(clientId, clientSecret));| List<ApplicationInformationDto> dtos = entities.stream() | ||
| .map(applicationInformationMapper::toDto) | ||
| .toList(); | ||
| applicationInformationExportService.exportDto(dtos, outputStream); |
There was a problem hiding this comment.
Blocking: Batch Export Will Throw JAXBException at Runtime
This passes a raw List<ApplicationInformationDto> to exportDto(). JAXB Marshaller.marshal() cannot serialize a plain java.util.List — it requires an @XmlRootElement-annotated wrapper.
Suggested fix — either wrap in an AtomFeedDto:
AtomFeedDto feed = new AtomFeedDto();
// populate feed with entries from dtos
applicationInformationExportService.exportDto(feed, outputStream);Or export entities individually:
for (ApplicationInformationEntity entity : entities) {
export(entity, outputStream);
}
Follow-up: Flyway Migration ReviewAdditional review of the 13 new Flyway migration scripts in No Table Name Conflicts with MainThe PR's tables ( Blocking IssueMySQL V3_0_0 INSERT Will Fail — Column Mismatch
These columns exist in the PostgreSQL Fix: Either add the missing columns to Additional Flyway ConcernsFlyway Won't Discover the New Files (MySQL/PostgreSQL)The PR adds migration files under flyway:
locations: classpath:db/migration/mysqlThis path ( Fix: Update the Flyway locations to include the correct paths: flyway:
locations:
- classpath:db/migration
- classpath:db/vendor/mysqlMySQL/PostgreSQL Schema InconsistencyThe Reviewed with Claude Code |
This pull request introduces several enhancements and refactors to the authorization server configuration, mapping layers, service interfaces, and export services. The main goals are to improve JWT/Opaque token support, expand mapping and service capabilities, and add dedicated export services for domain resources.
Closes #98
Security and Authorization Improvements:
AuthorizationServerConfignow conditionally enables OpenID Connect and JWT support based on the presence of JWT configuration properties. If JWT properties are set, the server uses JWT for resource server authentication; otherwise, it falls back to opaque tokens. This makes the server more flexible and configurable. [1] [2] [3] [4]Service Layer Enhancements:
ApplicationInformationServiceinterface and its implementation have been expanded to support CRUD operations, DTO conversion, and XML export/import forApplicationInformationEntity. The implementation now uses a new export service for marshalling. [1] [2] [3] [4] [5] [6]ApplicationInformationandCustomerAccountresources, providing JAXB-based XML marshalling with correct namespace handling. [1] [2]Mapping Layer Updates:
ElectricPowerQualitySummaryMapperandLineItemMapper) to ignore additional fields not defined in the domain XSDs, ensuring cleaner DTO-to-entity conversion. [1] [2] [3]Repository Refactoring:
@Repositoryannotations from Spring Data JPA interfaces and simplified a custom query inIntervalBlockRepositoryto use Spring Data method naming conventions. [1] [2] [3]Documentation and Code Quality:
Details of the most important changes:
1. Security and Authorization Configuration
AuthorizationServerConfig, based on the presence of JWT configuration properties (jwtIssuerUri,jwtJwkSetUri). [1] [2] [3] [4]2. Service Layer Enhancements
ApplicationInformationServiceand its implementation to support CRUD operations, DTO conversion, and XML export/import forApplicationInformationEntity. [1] [2] [3] [4] [5] [6]ApplicationInformationandCustomerAccountresources to handle JAXB-based XML marshalling with correct namespaces. [1] [2]3. Mapping Layer Updates
4. Repository Refactoring
@Repositoryannotations and simplified queries in JPA repositories. [1] [2] [3]5. Documentation Improvements