carmenlau commented 5 years ago

Description

For updating tenant config by skycli, we had plan that there will be a convertor in controller to convert current tenant config to a more user friendly version. Since the tenant config will keep updating during development, it is different to keep the convertor up to date. Had a short discussion with @louischan-oursky, would suggest to remove the convertor and update the server tenant to be more user friendly and update it directly.

So we should update the spec

Update the spec to remove the convertor. We should only have 1 version of tenant config file which is the tenant config itself.
For updating email template, tenant config should only accept url. So templates should be deployed as static assets, and update the link in tenant config. The drawback will be the templates files will become public but it should be fine?

Related issues:

skygear-server:
1. update tenant config to be more user friendly.
2. update tenant config with better default
controller
1. Endpoint to update tenant config
  1. Masked auth db url and app name should not be editable
  2. Support validation with json schema (maybe put it in skygear-server, so easier to keep track the version)
skycli
1. Update and view tenant config api

Scenario

Put list of Scenario how developers will use this API

Portal Design

Remove this section if the feature have no Portal

API Design

Remove this section if the feature have no API

Open Questions

Put a list of open questions here before a complete design / specification is decided

Related Issues

Server Issues
Client Issues
Guides Issues

chpapa commented 5 years ago

@carmenlau I'm not quite sure what it is about... is it something I should pay attention to? I thought tenant config is an internal format stored at controller. Skycli should have nothing to do with it as skycli always call the appropriate API which update the tenant config. I'm a bit suprised they skycli and tenant config are in the same abstraction level instead of communicate between another layer of API / services.

louischan-oursky commented 5 years ago

I thought tenant config is an internal format stored at controller. Skycli should have nothing to do with it as skycli always call the appropriate API which update the tenant config.

Some configuration (e.g. which login ID are recognized) are stored in tenant config. Currently we have no ways for the developer to update that.

I'm a bit suprised they skycli and tenant config are in the same abstraction level instead of communicate between another layer of API / services.

The extra level of abstraction here seems unnecessary. Actually this feature is in the spec now

https://github.com/SkygearIO/features/blob/master/features/270-next-skycli/commands.md#skycli-app-view-tenant-config https://github.com/SkygearIO/features/blob/master/features/270-next-skycli/commands.md#skycli-app-update-tenant-config

What we are proposing is actually reducing yet another level of abstraction

Our plan is to revamp the structure of the tenant config so that it is user facing without any conversion. Then we can just show and allow the developer to update it directly. Of course this means the structure of the tenant config become part of the public API.

chpapa commented 5 years ago

@louischan-oursky Thanks for clarifying.

Not sure if I misunderstood, let me clarify with the terminology first:

user tenant config - part of the public API protocol, which is a config file format.
server tenant config - the actual tenant config stored at server.

And we are now looking at merging 1 and 2.

However, there are some fields not editable? For example, APP_NAME and some DB config, they should not be editable, or even should be kept secrets from the user especially in a cluster environment

BTW, on the issue's original description:

The drawback will be the templates files will become public but it should be fine?

My gut feeling is that is not fine; Some system might be a private deployment hence even the welcome email template is private. Not to mention there could be some config in future that need to be a file and private; The tenant config need some way to either access private files, or embed binary in it.

louischan-oursky commented 5 years ago

However, there are some fields not editable? For example, APP_NAME and some DB config, they should not be editable, or even should be kept secrets from the user especially in a cluster environment

Correct. Some configuration are not editable. We may discuss whether we should include non-editable configuration in the user tenant config.

My gut feeling is that is not fine; Some system might be a private deployment hence even the welcome email template is private. Not to mention there could be some config in future that need to be a file and private; The tenant config need some way to either access private files, or embed binary in it.

For the purpose of HTML email template, some clients do not support external CSS so maybe we can just support inline HTML email template. Then we do not need to handle private static asset.

From my past experience on using skygear v1 email template, the pain point is that I have my templates stored in version control system (e.g. git), what I want is that when I run skycli app deploy, I can place those templates in a directory and skycli will update the templates for me.

chpapa commented 5 years ago

So the problem was just terminology? We have two layer of abstraction:

server tenant config - the actual config format for Skygear Cluster to consume
user tenant config - part of the public API protocol.

From my past experience on using skygear v1 email template, the pain point is that I have my templates stored in version control system (e.g. git), what I want is that when I run skycli app deploy, I can place those templates in a directory and skycli will update the templates for me.

So maybe that should be solved by skycli able to process user tenant config and upload the templates to the server (controller?), and the templates will be stored as part of the server tenant config?

louischan-oursky commented 5 years ago

So the problem was just terminology? We have two layer of abstraction

Down to the implementation, it is a bit complicated. We have 3 representations of the same configuration.

The first representation is JSON. JSON is the storage representation and the transport representation. We store the tenant config as JSON and deserialize it back to Go values for manipulation.

The second representation is environment variable. We support loading tenant configuration from environment variable when the components (gateway and auth) has dev mode turned on. This feature is mainly for the skygear developers (us).

The third representation is YAML. This is the representation when the developer use skycli app [view|edit]-tenant-config. The reason why we do not use JSON is because JSON is not too friendly to edit directly. The YAML is converted to JSON when it is submitted to the API so we must only use a subset of YAML that is compatible with JSON.

To implement skycli app [view|edit]-tenant-config, we need to hide some parts of the tenant config. The main point here is to hide, not to have yet another configuration format.

The goal of this issue is to update the current tenant format so that it is both user-facing and hide-friendly.

So maybe that should be solved by skycli able to process user tenant config and upload the templates to the server (controller?), and the templates will be stored as part of the server tenant config?

It could be a shorthand command that help the developer to

skycli app view-tenant-config
Load the templates and assign them into right positions in the config
skycli app edit-tenant-config -f <the-edited-config>

This is an example of the current tenant config in its JSON representation

{
  "DATABASE_URL": "postgres://postgres:@localhost:5432/postgres?sslmode=disable",
  "API_KEY": "api_key",
  "MASTER_KEY": "master_key",
  "APP_NAME": "config",
  "URL_PREFIX": "",
  "CORS_HOST": "*",
  "AUTH": {
    "LOGIN_IDS_KEY_WHITELIST": [],
    "CUSTOM_TOKEN_SECRET": ""
  },
  "TOKEN_STORE": {
    "SECRET": "master_key",
    "EXPIRY": 0
  },
  "USER_AUDIT": {
    "ENABLED": false,
    "TRAIL_HANDLER_URL": "",
    "PW_MIN_LENGTH": 0,
    "PW_UPPERCASE_REQUIRED": false,
    "PW_LOWERCASE_REQUIRED": false,
    "PW_DIGIT_REQUIRED": false,
    "PW_SYMBOL_REQUIRED": false,
    "PW_MIN_GUESSABLE_LEVEL": 0,
    "PW_EXCLUDED_KEYWORDS": null,
    "PW_EXCLUDED_FIELDS": null,
    "PW_HISTORY_SIZE": 0,
    "PW_HISTORY_DAYS": 0,
    "PW_EXPIRY_DAYS": 0
  },
  "SMTP": {
    "HOST": "",
    "PORT": 25,
    "MODE": "normal",
    "LOGIN": "",
    "PASSWORD": ""
  },
  "TWILIO": {
    "ACCOUNT_SID": "",
    "AUTH_TOKEN": "",
    "FROM": ""
  },
  "NEXMO": {
    "API_KEY": "",
    "API_SECRET": "",
    "FROM": ""
  },
  "FORGOT_PASSWORD": {
    "APP_NAME": "config",
    "URL_PREFIX": "",
    "SECURE_MATCH": false,
    "SENDER_NAME": "",
    "SENDER": "no-reply@skygeario.com",
    "SUBJECT": "Reset password instruction",
    "REPLY_TO_NAME": "",
    "REPLY_TO": "",
    "RESET_URL_LIFE_TIME": 43200,
    "SUCCESS_REDIRECT": "",
    "ERROR_REDIRECT": "",
    "EMAIL_TEXT_URL": "",
    "EMAIL_HTML_URL": "",
    "RESET_HTML_URL": "",
    "RESET_SUCCESS_HTML_URL": "",
    "RESET_ERROR_HTML_URL": ""
  },
  "WELCOME_EMAIL": {
    "ENABLED": false,
    "URL_PREFIX": "",
    "SENDER_NAME": "",
    "SENDER": "no-reply@skygeario.com",
    "SUBJECT": "Welcome!",
    "REPLY_TO_NAME": "",
    "REPLY_TO": "",
    "TEXT_URL": "",
    "HTML_URL": ""
  },
  "SSO_SETTING": {
    "URL_PREFIX": "",
    "JS_SDK_CDN_URL": "https://code.skygear.io/js/skygear/latest/skygear.min.js",
    "STATE_JWT_SECRET": "",
    "AUTO_LINK_PROVIDER_KEYS": null,
    "ALLOWED_CALLBACK_URLS": null
  },
  "SSO_PROVIDERS": null,
  "SSO_CONFIGS": [],
  "USER_VERIFY": {
    "URL_PREFIX": "",
    "AUTO_UPDATE": false,
    "AUTO_SEND_SIGNUP": false,
    "AUTO_SEND_UPDATE": false,
    "REQUIRED": false,
    "CRITERIA": "",
    "ERROR_REDIRECT": "",
    "ERROR_HTML_URL": "",
    "KEYS": null,
    "KEY_CONFIGS": null
  },
  "HOOKS": null
}

As you can see the keys are all in uppercase. SSO related settings are splitted into SSO_SETTING SSO_PROVIDERS and SSO_CONFIGS which are very confusing. For example, why are there SSO_SETTING and SSO_CONFIGS?

chpapa commented 5 years ago

@louischan-oursky understood. My only comment is this:

To implement skycli app [view|edit]-tenant-config, we need to hide some parts of the tenant config. The main point here is to hide, not to have yet another configuration format.

The goal of this issue is to update the current tenant format so that it is both user-facing and hide-friendly.

Implementation and architecturally, I still think it is better if we think of it as two layer of abstraction between the "user facing tenant config" and "server tenant config"

There will be information that we need to hide from the user, but also, for example, some information CANNOT be overridden by users.

So the problem of the wording above is it sounds like they are same thing, just "same data but hide some fields".

I'm perfectly fine with us to optimize the config format for usability, and merge the two configs' format to a certain extend to enhance usability and maybe even improve some code, but just want to remind it is very dangerous to architecturally and use naming make developers in future think like these two configs are identical; While in fact they are different and have access control/security implication.

Hope it make sense and I didn't misunderstood anything :)

louischan-oursky commented 5 years ago

Understood. I am going to write a proposal to separate user config from tenant config. So tenant config is not editable (or even readable) by user.

louischan-oursky commented 4 years ago

Had a discussion with team and drew the following conclusions:

Edit tenant config never result in modifying the database directly.
Some parts of the config are assumed to be immutable. If they are mutated, the behavior of the Auth gear will become undefined.
- Only new login ID key can be added. Removing an existing login ID key or changing the type of an existing login ID key are undefined.
- The client ID of OAuth provider must not be changed. Some OAuth provider, e.g. Facebook, claims that user ID is unique per OAuth application, not globally.
- Custom Token Provider must return the same sub for the same user entity.
For some parts of the config, changes do not affect existing data.
- Changing user verification criteria does not affect verified status of existing users.
- Changing password policy does not affect authentication of existing users. Though we can have a new feature to warn the user if the existing password violates the policy.

We should definitely put the above conclusions into the spec when we have time.

SkygearIO / features

Support update tenant config by skycli #324

Description

Scenario

Portal Design

API Design

Open Questions

Related Issues