FEATURE: Stronger data validation + defaults on `PUT /api/settings`

[Amphaal]

Actually, there is only PUT verb enabled on this path, which will result in replacing the whole setting file: some settings in body are explicit requirements (as the API tells us if we do not set them), but some more that are not required by the API are actually required for the UI to keep functionning ! For example, if you call PUT /api/settings and missing some parameters configuring an SMTP server, the Settings's SMTP tab goes completely blank, same for bounces.

Can we have defaults values where required if expected properties are missing in the body ? If not, telling us with HTTP 500 that some properties are missing and need to be set, with somehow documented possibles values in Swagger ?

[nayanthulkar28]

Hello, Would like to work on this. Can you provide an request response example to understand it better?

[Amphaal]

I am using Ansible for this usecase for sending HTTP requests. Here is what I am actually doing :

- name: Update settings
  ansible.builtin.uri:
    url: "{{ listmonk_api_url_part }}/settings"
    url_username: "{{ listmonk_admin_username }}"
    url_password: "{{ listmonk_admin_password }}"
    validate_certs: false
    method: PUT
    headers:
      Content-Type: "application/json"
    body_format: json
    body:
      # app
      app.lang: fr
      app.root_url: https://{{ listmonk_domain }}
      app.site_name: "{{ listmonk_site_name }}"
      app.logo_url: "{{ listmonk_logo_url }}"
      app.favicon_url: "{{ listmonk_favicon_url }}"
      app.from_email: "{{ listmonk_from_email }}"
      app.enable_public_subscription_page: true
      app.enable_public_archive: true
      app.send_optin_confirmation: true
      app.check_updates: true
      # performance
      app.batch_size: 1000
      app.concurrency: 10
      app.max_send_errors: 1000
      app.message_rate: 10
      # upload
      upload.provider: filesystem
      upload.filesystem.upload_path: uploads
      upload.filesystem.upload_uri: /uploads
      upload.extensions: ['.*', '.png']
      # privacy
      privacy.unsubscribe_header: true
      privacy.allow_blocklist: true
      privacy.allow_preferences: true
      privacy.allow_export: true
      privacy.allow_wipe: true
      #
      # bounce.enabled: false
      # bounce.webhooks_enabled: true
      # bounce.actions.soft.count: 1
      # bounce.actions.soft.action: "none"
      # bounce.actions.hard.count: 1
      # bounce.actions.hard.action: "none"
      # bounce.actions.complaint.count: 1
      # bounce.actions.complaint.action: "none"
      # bounce.ses_enabled: false
      # bounce.sendgrid_enabled: false
      # bounce.sendgrid_key: ""
      # bounce.postmark.enabled: false
      # bounce.postmark.username: ""
      # bounce.postmark.password: ""
      # bounce.mailboxes: []
      #
      smtp:
        - uuid: ""
          enabled: true
          port: 587
          host: docker-mailserver.docker-mailserver
          hello_hostname: "{{ docker_mailserver_host }}"
          auth_protocol: login
          tls_type: STARTTLS
          tls_skip_verify: true
          max_conns: 10
          max_msg_retries: 2
          idle_timeout: 15s
          wait_timeout: 5s
          username: "{{ donotreply_username }}"
          password: "{{ donotreply_password }}"
          email_headers: []

Please notice that I MUST redeclare defaults like app.* and such, which are already completely fine by me. If not, the UI breaks. This example still breaks Settings -> Bounces UI, havent found what to set for it to keep functionning.

[nayanthulkar28]

Does this happens with only SMPT and bounce use case? Or it can happen with any field

[Amphaal]

Any field, really. Like with my example above, if i run this script minus the smtp.* declaration, SMTP UI goes fully blank.

[nayanthulkar28]

Got it.

[nayanthulkar28]

I assume that while updating settings, frontend is providing all the attributes(regardless its new updated values only). So this type of bug won't happen through UI. The problem arises when we try to update this manually and misses out some field which are required by UI. For ex. in bounce case if we miss out bounce.action the UI crashes. An easy fix would be making all attribute as required/mandatory and return error if not provided.

[Amphaal]

Seems legit for a PUT verb. What would be nice for most usescases regarding updating settings, is to enable POST verb, and allow atomic properties replacement through it. With this, we could just let actual settings as it and only update those that we wish to change.

[nayanthulkar28]

Yes, we could actually make this change to PUT method only, as this API will use to updating setting. Thought of this as my second approach but will require lots of code change. I'm fine with both the approaches.

[nayanthulkar28]

@Amphaal can you confirm this ? Will start with development

[Amphaal]

Just do what makes the more sense to you, as long at it makes the usage safer and documented :)

[github-actions[bot]]

This issue has been marked 'stale' after 90 days of inactivity. If there is no further activity, it will be closed in 7 days.