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

[BUG] Add a test to ensure that using PATCH API to update Roles Mappings works on a static role #3276

Closed
cwperks opened this issue Aug 31, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@cwperks
Copy link
Member

cwperks commented Aug 31, 2023

With the recent refactor of the security REST APIs introduced in #3123 a regression was made that changed the behavior of how the PATCH API to update roles mappings worked.

Before the PR was merged the PATCH API was able to perform operations on static roles.

> curl -XPATCH https://admin:admin@localhost:9200/_plugins/_security/api/rolesmapping/all_access --insecure -H "Content-Type: application/json" --data '
[
        {
        "op": "remove",
        "path": "/users",
        "users": ["jwt_test"]
        }
]
'
{"status":"OK","message":"'all_access' updated."}

After the PR was merged the PATCH API started failing with the following error:

> curl -XPATCH https://admin:admin@localhost:9200/_plugins/_security/api/rolesmapping/all_access --insecure -H "Content-Type: application/json" --data '
[
        {
        "op": "remove",
        "path": "/users",
        "users": ["jwt_test"]
        }
]
'
{"status":"FORBIDDEN","message":"Resource 'all_access' is static."}

The reason this was not caught is because there currently is no unit test to assert the behavior of the PATCH API to update roles mappings on a static role. There should be a unit test to assert the intended behavior.

@cwperks cwperks added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels Aug 31, 2023
@peternied
Copy link
Member

As described this is a functional test scenario, why do you think this should be a unit test?

@peternied peternied removed the untriaged Require the attention of the repository maintainers and may need to be prioritized label Aug 31, 2023
@cwperks cwperks changed the title [BUG] Add a unit test to ensure that using PATCH API to update Roles Mappings works on a static role [BUG] Add a test to ensure that using PATCH API to update Roles Mappings works on a static role Aug 31, 2023
@willyborankin
Copy link
Collaborator

@peternied We have such tests in RolesMappingApiTest but they cover only hidden roles verification not reserved and statics both for PUT and PATCH as result it is not clear from tests how it should work.

@stephen-crawford
Copy link
Contributor

[Triage] This PR: #3278, resolved this issue so going to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants