FusionAuth / fusionauth-typescript-client

A TypeScript client for FusionAuth
https://fusionauth.io
Apache License 2.0
63 stars 27 forks source link

EventType includes unused types #88

Open jthn opened 11 months ago

jthn commented 11 months ago

FusionAuth API version 1.48.1, running from docker container in development mode. Typescript client version 1.48.0.

In the EventType enum the following members are defined in the type but are not returned from the API when provided

    AuditLogCreate = "audit-log.create",
    EventLogCreate = "event-log.create",
    KickstartSuccess = "kickstart.success",

This may be ultimately a bug in the API, as I can see those event types are listed in the docs as being available since 1.30.0, and I would expect those events to be in the tenant response if they were defined when the tenant was last updated.

Additionally, with EventConfiguration defined as:

export interface EventConfiguration {
    events?: Record<EventType, EventConfigurationData>;
}

This means that you must define all of the members of the enum when building an object of type Tenant, and the response from the API does not satisfy the type as declared.

Additionally, when working with the API, it seems that I don't need to provide all of the events when making requests to the API. When I send a PUT to the tenant endpoint with an incomplete list of events, the request is accepted just fine and then resulting GETs for that tenant return only the events I provided in the PUT.

I think it might make sense to modify EventConfiguration to something like:

export interface EventConfiguration {
    events?: { [key in EventType]?: EventConfigurationData }
}

Which would allow you to define only the events you want to send through to the API. Would this be a sensible change? Is it possible I have something misconfigured on my end?

I have a workaround on my end with type assertions, but I would love it if I could remove that and only define the events that I want to enable.

mooreds commented 11 months ago

I would expect those events to be in the tenant response if they were defined when the tenant was last updated.

These events are instance wide, not set at the tenant. Is that perhaps the issue?

I would need to think more deeply about your other suggestions, but appreciate the feedback.

jthn commented 10 months ago

These events are instance wide, not set at the tenant. Is that perhaps the issue?

That makes sense, especially as you cannot configure them in the context of a tenant directly. I wouldn't really expect them to be in the response of the Tenant, because you can't select them from that context, only from Settings > Webhooks. However the Tenant API Docs do define those as part of the request and response bodies.

The API endpoint will accept the following payload for a PATCH:

{
  "tenant": {
    "eventConfiguration": {
      "events": {
        "audit-log.create": {
          "enabled": true,
            "transactionType": "None"
        }
      }
    }
  }
}

But then the resulting response doesn't include them.

I do think this is well outside the scope of the client, but it seems like the API shouldn't allow these as a part of the eventConfiguration payload since they aren't relevant to the Tenant.

mooreds commented 10 months ago

I agree. We should error if you try to patch a tenant with these events.

Would you mind filing an issue here: https://github.com/FusionAuth/fusionauth-issues/issues/ ?

jthn commented 10 months ago

No problem. Created https://github.com/FusionAuth/fusionauth-issues/issues/2546.

On the topic of the types in this client, I know that it has the potential to set off other issues, but it seems like having optional types is more consistent with the behavior of the API.

In my particular use case, I am tracking the state of the response, and when the request has more events defined than the response, it always shows up as having a difference with the remote state. I can go into more details about my implementation if you feel that it is relevant.

My current workaround is to just do a type assertion on the event configuration, but I generally try to avoid that. There are some other workarounds that I could employ that keep the type intact, but I don't want to wire in custom logic to remove things from my state comparison function.

Happy to answer further questions if you have them, and I do understand that this is a low impact change with the potential for other issues, and as such do understand if it isn't a top of mind thing right now.