Skip to content

feat: Issue 98 controller functionality#114

Open
springframeworkguru wants to merge 6 commits intoGreenButtonAlliance:mainfrom
springframeworkguru:98-controller-functionality
Open

feat: Issue 98 controller functionality#114
springframeworkguru wants to merge 6 commits intoGreenButtonAlliance:mainfrom
springframeworkguru:98-controller-functionality

Conversation

@springframeworkguru
Copy link
Contributor

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:

  • The AuthorizationServerConfig now 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:

  • The ApplicationInformationService interface and its implementation have been expanded to support CRUD operations, DTO conversion, and XML export/import for ApplicationInformationEntity. The implementation now uses a new export service for marshalling. [1] [2] [3] [4] [5] [6]
  • Added new export services for ApplicationInformation and CustomerAccount resources, providing JAXB-based XML marshalling with correct namespace handling. [1] [2]

Mapping Layer Updates:

  • Expanded MapStruct mappers (ElectricPowerQualitySummaryMapper and LineItemMapper) to ignore additional fields not defined in the domain XSDs, ensuring cleaner DTO-to-entity conversion. [1] [2] [3]

Repository Refactoring:

  • Removed unnecessary @Repository annotations from Spring Data JPA interfaces and simplified a custom query in IntervalBlockRepository to use Spring Data method naming conventions. [1] [2] [3]

Documentation and Code Quality:

  • Improved JavaDoc formatting in several service classes for consistency and clarity. [1] [2]

Details of the most important changes:

1. Security and Authorization Configuration

  • Conditional enabling of OpenID Connect and JWT/Opaque token support in AuthorizationServerConfig, based on the presence of JWT configuration properties (jwtIssuerUri, jwtJwkSetUri). [1] [2] [3] [4]

2. Service Layer Enhancements

  • Expanded ApplicationInformationService and its implementation to support CRUD operations, DTO conversion, and XML export/import for ApplicationInformationEntity. [1] [2] [3] [4] [5] [6]
  • Added new export services for ApplicationInformation and CustomerAccount resources to handle JAXB-based XML marshalling with correct namespaces. [1] [2]

3. Mapping Layer Updates

  • Updated MapStruct mappers to ignore additional fields not defined in the XSDs, improving DTO/entity conversion accuracy. [1] [2] [3]

4. Repository Refactoring

  • Removed unnecessary @Repository annotations and simplified queries in JPA repositories. [1] [2] [3]

5. Documentation Improvements

  • Enhanced JavaDoc formatting for better readability and consistency. [1] [2]

…pdated: Application Information, Customer Information, Customer Account, Electric Power Quality, Interval Block, Meter Reading, Usage Point, Reading Type, Usage Summary.
@springframeworkguru springframeworkguru changed the title Feet Issue 98 controller functionality Feat Issue 98 controller functionality Mar 2, 2026
@springframeworkguru springframeworkguru changed the title Feat Issue 98 controller functionality feat Issue 98 controller functionality Mar 2, 2026
@springframeworkguru springframeworkguru changed the title feat Issue 98 controller functionality feat: Issue 98 controller functionality Mar 2, 2026
Copy link
Contributor

@dfcoffin dfcoffin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/register endpoint (ClientRegistrationController) has no @PreAuthorize — open client registration
  • {noop} password storage in ClientRegistrationController — plaintext secrets
  • {bcrypt}secret is not a valid bcrypt hash — will fail password validation
  • System.out.println in AuthorizationServerConfig.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)
  • ApplicationInformationExportServiceTest uses 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
  • IntervalBlockRepository query refactor — Derived method name is semantically equivalent to the explicit @Query
  • ApplicationInformationService interface expansion — Well-defined, follows existing patterns
  • ApplicationInformationServiceImpl CRUD methods — Proper @Transactional annotations, resolved TODO for findByDataCustodianClientId
  • 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

@dfcoffin
Copy link
Contributor

dfcoffin commented Mar 2, 2026

Follow-up: Flyway Migration Review

Additional review of the 13 new Flyway migration scripts in openespi-datacustodian/src/main/resources/db/vendor/.

No Table Name Conflicts with Main

The PR's tables (oauth2_registered_client, oauth2_authorization, oauth2_authorization_consent, espi_application_info, oauth2_audit_log, oauth2_token_usage, oauth2_consent_details, oauth2_client_metrics, datacustodian_integration_mapping, etc.) are entirely separate from main's ESPI domain tables. No ALTER TABLE statements cross-reference main's tables. This is clean.


Blocking Issue

MySQL V3_0_0 INSERT Will Fail — Column Mismatch

openespi-datacustodian/src/main/resources/db/vendor/mysql/V3_0_0__add_default_data_and_test_clients.sql inserts into espi_application_info referencing 6 columns that do not exist in the MySQL V1_0_0 schema:

  • client_description
  • contact_name
  • contact_email
  • scope
  • grant_types
  • response_types

These columns exist in the PostgreSQL V1_0_0 but were never added to the MySQL version. The MySQL migration will fail at runtime with column-not-found errors.

Fix: Either add the missing columns to V1_0_0 or V2_0_0 for MySQL, or remove them from the V3_0_0 INSERT statement. The MySQL and PostgreSQL espi_application_info schemas should be harmonized.


Additional Flyway Concerns

Flyway Won't Discover the New Files (MySQL/PostgreSQL)

The PR adds migration files under db/vendor/mysql/ and db/vendor/postgresql/, but the datacustodian Flyway config in application.yml and application-dev-mysql.yml points to:

flyway:
  locations: classpath:db/migration/mysql

This path (db/migration/mysql) does not exist in either module. The new migration files will never be executed for MySQL/PostgreSQL profiles. Only the H2 test profile (classpath:db/vendor/h2) would pick up the PR's V4_0_0__create_oauth2_schema.sql.

Fix: Update the Flyway locations to include the correct paths:

flyway:
  locations:
    - classpath:db/migration
    - classpath:db/vendor/mysql

MySQL/PostgreSQL Schema Inconsistency

The espi_application_info table has substantially different column definitions between MySQL V1_0_0 and PostgreSQL V1_0_0. These should be harmonized so both databases have the same schema.


Reviewed with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Complete Get Controller Functionality

2 participants