Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FieldRule #2362

Merged
merged 6 commits into from
Jan 15, 2024
Merged

Fix FieldRule #2362

merged 6 commits into from
Jan 15, 2024

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Dec 8, 2023

Closes #2344

This fixes three issues with FieldRule:

  • username can also be a list,
  • realm.name is the field name, using an intermediate realm object leads to a parsing exception,
  • metadata is not an object: values are of the form metadata.key = value.

Sources:

I only tested in Kibana, as the Python client only exposes rules and does not go deeper.

Regarding the backports, realm.name and metadata are unusable today, but username is. Should this be separated in two pull requests, so that the realm.name and metadata get backported to 8.12, 8.11 and 7.17?

* `username` can also be a list,
* `realm.name` is the field name, using an intermediate `realm` object does not work.
* `metadata` is not a key: keys are of the form `metadata.key = value`
Copy link
Contributor

@Anaethelion Anaethelion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments, unsure about the non_exhaustive tag but this seems the way to go. I'll defer to @swallez for confirmation.

specification/security/_types/RoleMappingRule.ts Outdated Show resolved Hide resolved
Co-authored-by: Laurent Saint-Félix <[email protected]>
@pquentin pquentin requested a review from Anaethelion December 13, 2023 13:23
Copy link
Contributor

@Anaethelion Anaethelion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Co-authored-by: Laurent Saint-Félix <[email protected]>
Copy link
Member

@JoshMock JoshMock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this @pquentin. 🙏

Copy link
Member

@swallez swallez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the ES code, a field rule is a single key/value pair where the value can be a single scalar value or an array of scalar values. So in theory that should just be type FieldRule = SingleKeyDictionary<String, ScalarValue|ScalarValue[]>.

However, the approach used in this PR consisting in just adding @non_exhaustive is the one that limits the breaking changes in strongly typed clients. It will be a bit less easy to use and may allow users to provide arbitrary values and not only scalar values, but that's the tradeoff to limit breaking changes.

We should however add a code comment (and not jsdoc comment that ends up in API docs) explaining this decision. Something like:

// This should have been defined as SingleKeyDictionary<String, ScalarValue|ScalarValue[]>
// However, this was initially defined as a container with a limited number of variants,
// and was later made non_exhaustive to limit breaking changes.

Finally, we should remove realm.name that was added in this PR. It's very specific and covered by the @non_exhaustive.

@pquentin pquentin requested a review from swallez January 3, 2024 16:35
@pquentin
Copy link
Member Author

pquentin commented Jan 3, 2024

Thanks for the review! Please take another look.

Copy link
Contributor

github-actions bot commented Jan 3, 2024

Following you can find the validation results for the APIs you have changed.

API Status Request Response
security.activate_user_profile Missing test Missing test
security.authenticate 🟢 15/15 15/15
security.bulk_update_api_keys 🟠 Missing type Missing type
security.change_password 🟢 6/6 6/6
security.clear_api_key_cache 🟢 9/9 9/9
security.clear_cached_privileges 🟢 3/3 3/3
security.clear_cached_realms 🟢 1/1 1/1
security.clear_cached_roles 🟢 2/2 2/2
security.clear_cached_service_tokens 🟢 4/4 4/4
security.create_api_key 🟢 25/25 16/16
security.create_cross_cluster_api_key 🟠 Missing type Missing type
security.create_service_token 🟢 2/2 2/2
security.delete_privileges 🟢 6/6 6/6
security.delete_role_mapping 🟢 9/9 9/9
security.delete_role 🟢 8/8 8/8
security.delete_service_token Missing test Missing test
security.delete_user 🟢 9/9 9/9
security.disable_user_profile Missing test Missing test
security.disable_user 🟢 3/3 3/3
security.enable_user_profile Missing test Missing test
security.enable_user 🟢 4/4 4/4
security.enroll_kibana Missing test Missing test
security.enroll_node Missing test Missing test
security.get_api_key 🟢 14/14 14/14
security.get_builtin_privileges 🟢 2/2 2/2
security.get_privileges 🟢 12/12 12/12
security.get_role_mapping 🔴 18/18 10/18
security.get_role 🟢 20/20 20/20
security.get_service_accounts Missing test Missing test
security.get_service_credentials 🟢 1/1 1/1
security.get_settings 🟠 Missing type Missing type
security.get_token 🟢 21/21 20/20
security.get_user_privileges 🟢 7/7 7/7
security.get_user_profile Missing test Missing test
security.get_user 🟢 24/24 24/24
security.grant_api_key Missing test Missing test
security.has_privileges_user_profile Missing test Missing test
security.has_privileges 🟢 9/9 9/9
security.invalidate_api_key 🟢 10/10 10/10
security.invalidate_token 🟢 11/11 11/11
security.oidc_authenticate 🟠 Missing type Missing type
security.oidc_logout 🟠 Missing type Missing type
security.oidc_prepare_authentication 🟠 Missing type Missing type
security.put_privileges 🟢 10/10 10/10
security.put_role_mapping 🔴 2/11 11/11
security.put_role 🟢 26/26 25/25
security.put_user 🟢 39/39 38/38
security.query_api_keys 🟢 7/7 7/7
security.saml_authenticate Missing test Missing test
security.saml_complete_logout Missing test Missing test
security.saml_invalidate Missing test Missing test
security.saml_logout Missing test Missing test
security.saml_prepare_authentication Missing test Missing test
security.saml_service_provider_metadata Missing test Missing test
security.suggest_user_profiles Missing test Missing test
security.update_api_key Missing test Missing test
security.update_cross_cluster_api_key 🟠 Missing type Missing type
security.update_settings 🟠 Missing type Missing type
security.update_user_profile_data Missing test Missing test

You can validate these APIs yourself by using the make validate target.

1 similar comment
Copy link
Contributor

github-actions bot commented Jan 3, 2024

Following you can find the validation results for the APIs you have changed.

API Status Request Response
security.activate_user_profile Missing test Missing test
security.authenticate 🟢 15/15 15/15
security.bulk_update_api_keys 🟠 Missing type Missing type
security.change_password 🟢 6/6 6/6
security.clear_api_key_cache 🟢 9/9 9/9
security.clear_cached_privileges 🟢 3/3 3/3
security.clear_cached_realms 🟢 1/1 1/1
security.clear_cached_roles 🟢 2/2 2/2
security.clear_cached_service_tokens 🟢 4/4 4/4
security.create_api_key 🟢 25/25 16/16
security.create_cross_cluster_api_key 🟠 Missing type Missing type
security.create_service_token 🟢 2/2 2/2
security.delete_privileges 🟢 6/6 6/6
security.delete_role_mapping 🟢 9/9 9/9
security.delete_role 🟢 8/8 8/8
security.delete_service_token Missing test Missing test
security.delete_user 🟢 9/9 9/9
security.disable_user_profile Missing test Missing test
security.disable_user 🟢 3/3 3/3
security.enable_user_profile Missing test Missing test
security.enable_user 🟢 4/4 4/4
security.enroll_kibana Missing test Missing test
security.enroll_node Missing test Missing test
security.get_api_key 🟢 14/14 14/14
security.get_builtin_privileges 🟢 2/2 2/2
security.get_privileges 🟢 12/12 12/12
security.get_role_mapping 🔴 18/18 10/18
security.get_role 🟢 20/20 20/20
security.get_service_accounts Missing test Missing test
security.get_service_credentials 🟢 1/1 1/1
security.get_settings 🟠 Missing type Missing type
security.get_token 🟢 21/21 20/20
security.get_user_privileges 🟢 7/7 7/7
security.get_user_profile Missing test Missing test
security.get_user 🟢 24/24 24/24
security.grant_api_key Missing test Missing test
security.has_privileges_user_profile Missing test Missing test
security.has_privileges 🟢 9/9 9/9
security.invalidate_api_key 🟢 10/10 10/10
security.invalidate_token 🟢 11/11 11/11
security.oidc_authenticate 🟠 Missing type Missing type
security.oidc_logout 🟠 Missing type Missing type
security.oidc_prepare_authentication 🟠 Missing type Missing type
security.put_privileges 🟢 10/10 10/10
security.put_role_mapping 🔴 2/11 11/11
security.put_role 🟢 26/26 25/25
security.put_user 🟢 39/39 38/38
security.query_api_keys 🟢 7/7 7/7
security.saml_authenticate Missing test Missing test
security.saml_complete_logout Missing test Missing test
security.saml_invalidate Missing test Missing test
security.saml_logout Missing test Missing test
security.saml_prepare_authentication Missing test Missing test
security.saml_service_provider_metadata Missing test Missing test
security.suggest_user_profiles Missing test Missing test
security.update_api_key Missing test Missing test
security.update_cross_cluster_api_key 🟠 Missing type Missing type
security.update_settings 🟠 Missing type Missing type
security.update_user_profile_data Missing test Missing test

You can validate these APIs yourself by using the make validate target.

Copy link
Member

@swallez swallez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pquentin pquentin merged commit 383d22a into main Jan 15, 2024
8 checks passed
@pquentin pquentin deleted the fix-role-mapping-rule branch January 15, 2024 10:47
github-actions bot pushed a commit that referenced this pull request Jan 15, 2024
* Fix FieldRule

* `username` can also be a list,
* `realm.name` is the field name, using an intermediate `realm` object does not work.
* `metadata` is not a key: keys are of the form `metadata.key = value`

* Apply suggestions from code review

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Run `make contrib`

* Update RoleMappingRule.ts

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Address review comments

---------

Co-authored-by: Laurent Saint-Félix <[email protected]>
(cherry picked from commit 383d22a)
github-actions bot pushed a commit that referenced this pull request Jan 15, 2024
* Fix FieldRule

* `username` can also be a list,
* `realm.name` is the field name, using an intermediate `realm` object does not work.
* `metadata` is not a key: keys are of the form `metadata.key = value`

* Apply suggestions from code review

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Run `make contrib`

* Update RoleMappingRule.ts

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Address review comments

---------

Co-authored-by: Laurent Saint-Félix <[email protected]>
(cherry picked from commit 383d22a)
github-actions bot pushed a commit that referenced this pull request Jan 15, 2024
* Fix FieldRule

* `username` can also be a list,
* `realm.name` is the field name, using an intermediate `realm` object does not work.
* `metadata` is not a key: keys are of the form `metadata.key = value`

* Apply suggestions from code review

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Run `make contrib`

* Update RoleMappingRule.ts

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Address review comments

---------

Co-authored-by: Laurent Saint-Félix <[email protected]>
(cherry picked from commit 383d22a)
pquentin added a commit that referenced this pull request Jan 15, 2024
* Fix FieldRule

* `username` can also be a list,
* `realm.name` is the field name, using an intermediate `realm` object does not work.
* `metadata` is not a key: keys are of the form `metadata.key = value`

* Apply suggestions from code review

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Run `make contrib`

* Update RoleMappingRule.ts

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Address review comments

---------

Co-authored-by: Laurent Saint-Félix <[email protected]>
(cherry picked from commit 383d22a)

Co-authored-by: Quentin Pradet <[email protected]>
pquentin added a commit that referenced this pull request Jan 15, 2024
* Fix FieldRule

* `username` can also be a list,
* `realm.name` is the field name, using an intermediate `realm` object does not work.
* `metadata` is not a key: keys are of the form `metadata.key = value`

* Apply suggestions from code review

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Run `make contrib`

* Update RoleMappingRule.ts

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Address review comments

---------

Co-authored-by: Laurent Saint-Félix <[email protected]>
(cherry picked from commit 383d22a)

Co-authored-by: Quentin Pradet <[email protected]>
pquentin added a commit that referenced this pull request Jan 15, 2024
* Fix FieldRule

* `username` can also be a list,
* `realm.name` is the field name, using an intermediate `realm` object does not work.
* `metadata` is not a key: keys are of the form `metadata.key = value`

* Apply suggestions from code review

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Run `make contrib`

* Update RoleMappingRule.ts

Co-authored-by: Laurent Saint-Félix <[email protected]>

* Address review comments

---------

Co-authored-by: Laurent Saint-Félix <[email protected]>
(cherry picked from commit 383d22a)

Co-authored-by: Quentin Pradet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SecurityRealm type in SecurityPutRoleMappingRequest results in a parsing error.
4 participants