Skip to content

MW-1449 - Superset embedded dashboard support#23

Merged
dszafranek merged 2 commits into
masterfrom
MW-1449
May 26, 2026
Merged

MW-1449 - Superset embedded dashboard support#23
dszafranek merged 2 commits into
masterfrom
MW-1449

Conversation

@dszafranek

Copy link
Copy Markdown
Contributor

Replaces the legacy superset-patchup OAuth flow with guest token authentication compatible with Superset 6.0 and the new SolDeveloreporting stack. WIP until the reporting stack integration is tested and merged.

Summary

  • Add GET /api/reports/superset/guest-token endpoint that proxies guest token requests to Superset
  • Add embeddedUuid field to DashboardReport entity for storing Superset embedded dashboard UUIDs
  • Add SupersetService for Superset API communication (login, CSRF, guest token)

@dszafranek dszafranek force-pushed the MW-1449 branch 2 times, most recently from 0e4f566 to 61d4779 Compare April 1, 2026 16:13
@sonarqubecloud

sonarqubecloud Bot commented Apr 1, 2026

Copy link
Copy Markdown

@dszafranek dszafranek changed the title WIP: MW-1449 - Superset embedded dashboard support MW-1449 - Superset embedded dashboard support Apr 3, 2026

@mgrochalskisoldevelo mgrochalskisoldevelo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two points:

  • CHANGELOG entry is missing.
  • The migration timestamp doesn't match the existing convention. Existing migrations follow the yyyyMMddHHmmssSSS pattern (14 characters), but the new one is 20260401000000000__... (17 characters). The easiest fix is to regenerate it using the Gradle task, which produces the correct format automatically:
    • docker compose run --service-ports report
    • gradle generateMigration -PmigrationName=name_of_migration
  • Please avoid hardcoded English error messages - I'd recommend throwing with a generic message key instead of including the raw csrfBody in the exception.

Also what are your thoughts on these?

  • Do we actually need defaults for superset.admin.user and superset.admin.password? If not, I'd drop them from the config and require them via environment variables only.
  • A couple of thoughts on SupersetGuestTokenController.getGuestToken:
    • It might be worth looking up the dashboard_reports row by embedded_uuid and returning 404 if it's missing, rather than passing the request through unconditionally.
    • Optionally, we could check the user's role and permissions against which dashboards they're allowed to see. This may be follow-up scope rather than something for this PR - just flagging.

…t by REPORTS_VIEW and embeddedUuid

- Rename migration to a canonical now()-based timestamp via the generateMigration Gradle task
- Throw a localized ServerException (with new SupersetMessageKeys) instead of including raw Superset response bodies in IllegalStateException messages
- Drop the insecure admin/changeme @value defaults; SupersetService now short-circuits with a "not configured" ServerException when superset.url/admin user/password are unset, so the service still boots for adopters who do not use Superset
- Add existsByEmbeddedUuid lookup and return 404 when no dashboard report matches the requested UUID
- Gate the guest-token endpoint behind permissionService.canViewReports()
- Add CHANGELOG entry
@sonarqubecloud

Copy link
Copy Markdown

@dszafranek

Copy link
Copy Markdown
Contributor Author

Two points:

  • CHANGELOG entry is missing.

  • The migration timestamp doesn't match the existing convention. Existing migrations follow the yyyyMMddHHmmssSSS pattern (14 characters), but the new one is 20260401000000000__... (17 characters). The easiest fix is to regenerate it using the Gradle task, which produces the correct format automatically:

    • docker compose run --service-ports report
    • gradle generateMigration -PmigrationName=name_of_migration
  • Please avoid hardcoded English error messages - I'd recommend throwing with a generic message key instead of including the raw csrfBody in the exception.

Also what are your thoughts on these?

  • Do we actually need defaults for superset.admin.user and superset.admin.password? If not, I'd drop them from the config and require them via environment variables only.

  • A couple of thoughts on SupersetGuestTokenController.getGuestToken:

    • It might be worth looking up the dashboard_reports row by embedded_uuid and returning 404 if it's missing, rather than passing the request through unconditionally.
    • Optionally, we could check the user's role and permissions against which dashboards they're allowed to see. This may be follow-up scope rather than something for this PR - just flagging.

Thanks for the review. Changes made:

  • I added the changelog
  • I modified the migration file name, it's generated with the gradle task now. Note that it is still 17 characters not 14, all other migrations in this repo follow the same convention. Please let me know if I'm missing something here
  • Error messages are localized now

From your "optional" suggestions:

  • Good point on the defaults, it's better to skip it. I removed default superset login/password. I left it as an empty string so that the service still builds for implementations that do not want to use superset. I also added a check in SupersetService.getGuestToken to make sure to throw an exception when credentials are empty
  • I modified getGuestToken method to throw NotFoundMessageException if no dashboard matches
  • I added simple permission check by using "permissionService.canViewReports()". I think if we need anything more complex it should be handled by a separate ticket like you suggested.

@mgrochalskisoldevelo Could you please review again?

@mgrochalskisoldevelo mgrochalskisoldevelo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@dszafranek dszafranek merged commit 1dd630c into master May 26, 2026
4 checks passed
@dszafranek dszafranek deleted the MW-1449 branch May 26, 2026 07:26
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.

2 participants