Skip to content

LookoutUI: configure displayname#4795

Open
d2burkhalter wants to merge 3 commits intoarmadaproject:masterfrom
d2burkhalter:fix/configure-displayname
Open

LookoutUI: configure displayname#4795
d2burkhalter wants to merge 3 commits intoarmadaproject:masterfrom
d2burkhalter:fix/configure-displayname

Conversation

@d2burkhalter
Copy link
Copy Markdown
Collaborator

What type of PR is this?

This PR allows operators to configure which oidc claim is used to display a username

What this PR does / why we need it

Currently the username is set to the sub claim which is not always human readable for example when doing local development using authentication with Keycloak
Default:
Screenshot 2026-03-24 at 4 11 45 PM
With uiConfig.oidc.displayNameClaim=name:
Screenshot 2026-03-24 at 4 10 52 PM

Special notes for your reviewer

Signed-off-by: David.Burkhalter <d2.burkhalter@gmail.com>
…playname

Signed-off-by: David.Burkhalter <d2.burkhalter@gmail.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 24, 2026

Greptile Summary

This PR adds a configurable displayNameClaim field to the OIDC configuration, letting operators specify which OIDC claim (e.g. name, preferred_username, email) is shown as the username in Lookout UI instead of always defaulting to sub. The change is consistent across the Go config struct (OidcConfig.DisplayNameClaim *string) and the TypeScript interface (displayNameClaim?: string), and a worked example for local Keycloak development is included in _local/lookout/config-auth.yaml.

Key points:

  • The Go side correctly uses omitempty so the field is absent from the JSON payload when not configured, and the TS side marks the field optional — the two sides are symmetric.
  • The named OidcConfig struct is a welcome refactor over the previous anonymous struct, making future extensions cleaner.
  • The hook correctly guards the claim value with typeof displayName === "string" before using it, and emits a console.warn (with a useful message) when the configured claim is absent or wrong-typed — addressing the silent-fallback concern from a prior review.
  • getConfig() is called at module level rather than inside the hook; this works fine for a static SPA config but couples the hook to module-initialization order and can complicate unit tests that swap configs between cases (see inline comment).

Confidence Score: 4/5

  • PR is safe to merge; the one inline suggestion is non-blocking and only affects testability.
  • The implementation is correct end-to-end — Go struct, TypeScript interface, and hook logic are all consistent. Previous review concerns (silent fallback, type width) have been fully addressed with a console.warn and a plain string type. The only remaining note is the module-level getConfig() call, which is a style/testability concern rather than a runtime bug.
  • No files require special attention; hooks.ts has a minor testability note about the module-level getConfig() call.

Important Files Changed

Filename Overview
internal/lookoutui/src/oidcAuth/hooks.ts Adds displayNameClaim resolution logic with proper string-type guard and console.warn fallback; config is read at module level rather than inside the hook, which is safe for a static SPA config but worth noting.
internal/lookout/configuration/types.go Extracts anonymous Oidc struct into a named OidcConfig type and adds DisplayNameClaim *string with appropriate omitempty tagging and doc comment.
internal/lookoutui/src/config/types.ts Adds optional displayNameClaim?: string to OidcConfig interface, matching the Go side's *string (omitempty) — correct and consistent.
_local/lookout/config-auth.yaml Adds displayNameClaim: "name" example for local Keycloak development; comment is clear and accurate.

Sequence Diagram

sequenceDiagram
    participant Server as Go Server
    participant Window as window.__LOOKOUT_UI_CONFIG__
    participant Config as getConfig()
    participant Hook as useUsername hook
    participant OIDC as OIDC UserManager
    participant UI as React UI

    Server->>Window: Serve UIConfig (with oidc.displayNameClaim)
    Note over Config: Module initialization (once)
    Window->>Config: getConfig() reads config
    Config-->>Hook: config (module-level constant)

    Hook->>OIDC: userManager.getUser()
    OIDC-->>Hook: user (with profile claims)

    alt displayNameClaim configured
        Hook->>Hook: profile[displayNameClaim]
        alt claim is a string
            Hook->>UI: setUsername(displayName)
        else claim missing or not a string
            Hook->>Hook: console.warn(misconfiguration)
            Hook->>UI: setUsername(user.profile.sub)
        end
    else no displayNameClaim
        Hook->>UI: setUsername(user.profile.sub)
    end
Loading

Reviews (2): Last reviewed commit: "fix: make displayNameClaim type more gen..." | Re-trigger Greptile

Signed-off-by: David.Burkhalter <d2.burkhalter@gmail.com>
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.

1 participant