opensearch-project / OpenSearch-Dashboards

📊 Open source visualization dashboards for OpenSearch.
https://opensearch.org/docs/latest/dashboards/index/
Apache License 2.0
1.65k stars 866 forks source link

[RFC] User level settings #7821

Open ruanyl opened 3 weeks ago

ruanyl commented 3 weeks ago

Introduction

Currently, OpenSearch-Dashboards provide global settings that apply to all users. The purpose of this RFC is to introduce a new feature that allows to customize advance settings at user level. This feature ensures that when a user changes their settings, it will not affect other users.

Goals

UX changes

  1. There will be a new page "User settings" added to allow user to update their own settings from a dedicated place.
  2. The current global advance settings page will remain, changes on this page will affect only the global settings.

Design details

To support user-level settings we'll need to modify the current ui settings schema by introducing a new field scopes to indicate whether a setting is global or user or both.

Registering a setting with scope

The scopes is an array, it can contain {type: 'global'} or {type: 'user'} or both. If no scope provided, it defaults to global.

For example:

{
    dateFormat: {
      name: i18n.translate('core.ui_settings.params.dateFormatTitle', {
        defaultMessage: 'Date format',
      }),
      value: 'MMM D, YYYY @ HH:mm:ss.SSS',
      description: '...',
      schema: schema.string(),
+     scopes: [{type: 'global'}, {type: 'user'}]
    }
}

Reading a setting with scope

When calling uiSettings.get(key), it returns the value based on the following rules:

  1. If the setting was registered with scope global or empty, the global setting will be returned.
  2. If the setting was registered with scope user, the user setting will be returned
  3. If the setting was registered with both user and global, the user setting will be returned if it's set, otherwise, return the global setting
  4. If calling uiSettings.get(key, defaultValue, 'user') with scope specified, it will return the setting of the specified scope. If the value doesn't not exist, then return the defaultValue. This requires to update the current uiSettings service's interface

Writing a setting with scope

When calling uiSettings.set(key, value), it follows the following rules:

  1. If the setting was registered with scope global or empty, it will write the value to global setting saved object.
  2. If the setting was registered with scope user, it will write the value to user setting saved object.
  3. If the setting was registered with both user and global, it will by default update the user setting saved object.
  4. If need calling uiSettings.set(key, value, 'global') with scope specified, it will write the value to the corresponding saved object. This requires to update the current uiSettings service's interface

How to store user setting?

The global setting saved object should remain unchanged: it is a saved object which type is config and id is the current OSD version.

For user setting, the saved object type will also be config but the id will be the current login username. Thus, user setting requires OSD to enable authentication to provide the username

Access control

When saved object permission control is enabled(ACL), the permission will be attached to the user setting saved object with permissions which ensures users won't be able to read others setting. However, ACL is not mandatory.

{
  type: 'config',
  id: '<username>',
  permissions: {
    read: {users: ['<username>']},
    write: {user: ['<username>']}
  }
}

Summary

The implementation of a new scopes field in the settings schema ensures a seamless integration with the existing uiSettings client while maintaining the compatibility with existing codebase and it should be non-breaking changes.

Open questions

  1. The current global settings saved object will always have the id the same as current OSD version, whenever OSD upgrade, a new saved object will be created with the current version as its id. Should user setting follow the same behavior by add OSD version when constructing the id?
ashwin-pc commented 3 weeks ago

Thanks for the nice explanation of how this feature works with its interfaces. I love a lot of things about this. Especially how clean the change is w.r.t how saved objects are created. A few questions:

  1. I noticed that you introduced workspace settings for 2.14. How does this play with that? Are we still keeping workspace settings?
  2. For the modified interface for uiSettings, we are just adding an optional property right?
  3. Can you add a mock of how you are adding user settings to the UI if you have it?
  4. For your open question, yeah I think keeping the version in the id might be useful. Not sure if it's useful, but it's safer and doesn't really change your design :)
  5. How does this work if authentication is not enabled? Also would love to learn more about how authentication will work in OSD since this seems to be different from how tenants in the security plugin work

Not related to this RFC but where can I find more details about permission based saved objects (ACL)

AMoo-Miki commented 3 weeks ago

I always thought if we do implement user-settings, it would simply inherit the value from the level above it. For a setting X, we would pick from user's saved object, and if missing we pick from the global saved object, then the value from config file, and finally schema defaults. In tenant-based systems, the tenant's saved object would be between user and global.

I feel that this definition of scopes and the two settings interfaces would complicate the user experience as well as the maintainability of the code.

I was thinking that Admin users will get this interface when the access Dashboards Settings where they get to set their user settings, as well as define tenant and global defaults.

Admin User Settings

When viewing either of the defaults tabs, they get the ability to lock a setting to prevent any change to it:

Admin Settings

Users without Admin access will get to set set the user-level settings:

User Settings

The interface shows the user the source of any defaults and provides them the ability to update tenant settings to match theirs (for simplicity).

OSD installations that don't have tenants will simply not show that portion of the interface.

Internally, we would have different saved objects for each, like it is today and the original RFC envisioned. However, the client would internally maintain the values for all of the levels which will be new as the client kinda flattens the hierarchy as it reads them in. By having all of the values in the client, we facilitate access to the "default" values which are those before user settings overrides them.

The locking mechanism provide full control over locking down settings and would eliminate the need for overrides from the config file which isn't widely available.

This alternative proposal doesn't accommodate defining scopes for a setting because i couldn't think of any reason for it. However, if there is a compelling reason, my proposal is compatible with it too.

ruanyl commented 3 weeks ago

@ashwin-pc Thank you for your questions!

  1. I noticed that you introduced workspace settings for 2.14. How does this play with that? Are we still keeping workspace settings?

That's a really good point, to be honest, I've be thinking of including that in this RFC. But currently, workspace level setting is implemented in workspace plugin. I didn't include that into this RFC because it's workspace specific implementation at the moment, I'm afraid of including it will make the scope of this RFC too larger for the audience. I believe the same design could apply to workspace level setting as well, but maybe it could discussed in a separate issue.

  1. For the modified interface for uiSettings, we are just adding an optional property right?

Yes, the scope will be an optional property, and it's optional parameters as well when calling uiSettings.set and uiSettings.get

  1. Can you add a mock of how you are adding user settings to the UI if you have it?

Here is a screenshot, user default workspace will the the first user level setting to be added.

image
  1. For your open question, yeah I think keeping the version in the id might be useful. Not sure if it's useful, but it's safer and doesn't really change your design :)

Thanks for the suggestion, that sounds also reasonable to me :)

  1. How does this work if authentication is not enabled? Also would love to learn more about how authentication will work in OSD since this seems to be different from how tenants in the security plugin work

When authentication is disabled, meaning we won't be able to get user info(user id), then user setting won't exist, there won't be a user setting page. For authentication, what we're expecting auth info been injected into http request, so that we can identify the current user and retrieve user id from it.

Not related to this RFC but where can I find more details about permission based saved objects (ACL)

The current implementation of ACL is inside workspace plugin, you can find overview design from here https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4633, but the PR is a bit outdated, I'm planning to update it soon.

In a nut shell, it will attach permission field to saved object that requires permission check, for example:

{
  id: 'my-workspace',
  type: 'workspace',
  permissions: {
    read: {
      users: [<user_name>],
      groups: [<backend_role>],
    },
    write: {
      users: [<user_name>],
      groups: [<backend_role>],
    }
  }
}

And it will validate the current user's permission to the saved object when call saved object API, such as get/update/delete

ruanyl commented 3 weeks ago

@AMoo-Miki Thank you for the amazing feedbacks!

it would simply inherit the value from the level above it

That's exactly what I originally thought, the main reason of introducing scopes is there will settings that are user only, for example, setting of user's default workspace, there won't be a global default workspace setting as default workspace must be user specific, since not every user has permission to all workspaces. So not all settings are global settings.

I can imagine when we're splitting the current advance settings into different portions: application settings, user settings and workspace settings, there will be more such cases. @kgcreative may be can comment more on this.

I feel that this definition of scopes and the two settings interfaces would complicate the user experience as well as the maintainability of the code.

There will be a separate page for user to update user settings, as far as I see by the time, the UX mockup looks pretty intuitive to me. I posted a screenshot in my previous comment.

For maintainability, the scopes is optional, it's only needed when we want to move/add a setting to user settings, and the required change is minimum as well. Would you mind to provide a bit more details about what you're concerning?

When viewing either of the defaults tabs, they get the ability to lock a setting to prevent any change to it:

Btw, thank for providing the mockup, I love it! For "get the ability to lock a setting", I don't have a strong opinion about this, perhaps this requires more inputs from UX.

In tenant-based systems, the tenant's saved object would be between user and global.

The interface shows the user the source of any defaults and provides them the ability to update tenant settings to match theirs (for simplicity).

OSD installations that don't have tenants will simply not show that portion of the interface.

My understanding is a tenant has its own OpenSearch-Dashboards index, their saved object are stored isolated. Do you mean the tenant's advance setting is currently inheriting the Global tenant? Please correct me if I'm wrong :)

Internally, we would have different saved objects for each, like it is today and the original RFC envisioned. I think we're aligned on how to store the data ;-)

However, the client would internally maintain the values for all of the levels which will be new as the client kinda flattens the hierarchy as it reads them in. By having all of the values in the client, we facilitate access to the "default" values which are those before user settings overrides them.

For the implementation details, there could various, we're thinking of two possible approaches:

  1. Via refactor the current ui setting client to support levels of data and maintain the override logic in ui setting client
  2. Via saved object client wrapper, setting could be read/write to the saved object based on it's scope. This way requires minimum changes in ui settings client.

But I think we can discuss the implementation separately.

This alternative proposal doesn't accommodate defining scopes for a setting because i couldn't think of any reason for it. However, if there is a compelling reason, my proposal is compatible with it too.

If not all settings are global settings, like I mentioned above, it doesn't sound this proposal will fit(hope I fully get your points). Also, If we gonna support more levels of settings, for example, apart from global setting and user setting, if we added workspace level settings, could your approach flexible enough to support such case?

ashwin-pc commented 3 weeks ago

When authentication is disabled, meaning we won't be able to get user info(user id), then user setting won't exist, there won't be a user setting page. For authentication, what we're expecting auth info been injected into http request, so that we can identify the current user and retrieve user id from it.

That makes sense.

The current implementation of ACL is inside workspace plugin, you can find overview design from here https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4633, but the PR is a bit outdated, I'm planning to update it soon.

Has it been implemented yet? If so, is there a doc on how I can try it out? :)

ruanyl commented 3 weeks ago

Has it been implemented yet? If so, is there a doc on how I can try it out? :)

Yes, workspace is a great example, you can try it on future playground, the workspace you can see is the workspace that you have permission. Right now, you can already see a few workspaces that because the workspace permission is set to read: {users: ['*']}. You can see the permission setting page for workspace when you open a workspace and go to workspace setting page. But perhaps you cannot update it, because you don't have write permission.

Hailong-am commented 3 weeks ago

4. For your open question, yeah I think keeping the version in the id might be useful. Not sure if it's useful, but it's safer and doesn't really change your design :)

keep the version that will need to have upgrade logic when OSD version bumps for user level settings also, to make the change more simper we can use fixed document id that is username

kavilla commented 3 weeks ago

config is versioned on start up within the id so i think it would be consistent. makes sense. it actually makes it a lot easier to understand what is happening and which default values are being pulled from.

virajsanghvi commented 1 week ago

Questions:

I noticed that you introduced workspace settings for 2.14. How does this play with that? Are we still keeping workspace settings?

That's a really good point, to be honest, I've be thinking of including that in this RFC. But currently, workspace level setting is implemented in workspace plugin. I didn't include that into this RFC because it's workspace specific implementation at the moment, I'm afraid of including it will make the scope of this RFC too larger for the audience. I believe the same design could apply to workspace level setting as well, but maybe it could discussed in a separate issue.

Do plugins define their settings independently of global settings instead of participating/contributing to the global settings? Is this the behavior we want? If we want user specific workspace settings, it seems undesirable to reimplement the same behavior again.

Hailong-am commented 1 week ago
  • How does this play with browser settings? Does it replace them? Otherwise, it feels like that should be a "scope" as well and we should drop preferBrowserSettings?

No, it doesn't replace browser settings. They are stored in different places, user setting stores in .kibana index, however browser settings should be in browser local storage. Their have use cases for both, user settings support cross browser/devices but not for multi tenancy. However, preferBrowserSettings do supports multi tenancy.

  • In terms of get() behavior, technically the precedence is browser value (if it still exists) -> user value -> global value -> argument defaultValue's value -> schema default value, right?

Yes.

  • Why are scopes an object? What types of extensions are we leaving the door open to?

It's a enum or a group of enums

export enum UiSettingScope {
  GLOBAL = 'global',
  USER = 'user',
}
  • Do we need to provide a mechanism to get the defined value at different levels? For themes/browser versions, it is necessary to understand the difference between the default value and the browser specified value. Not sure if that's the case here for user (don't think it is), but asking as we may want to think through the method we add to support it.

Yes, we have add scope query parameter in get api that supports get values from specified scope.