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.

Sign up for GitHub

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

Jump to bottom

feat!: ADR-014: Permissions improvements #886

Merged
merged 22 commits into from
Jun 8, 2022
Merged

feat!: ADR-014: Permissions improvements #886

merged 22 commits into from
Jun 8, 2022

Conversation

RiccardoM
Copy link
Contributor

@RiccardoM RiccardoM commented May 31, 2022 •
edited
Loading

Description

This PR improves the permissions systems as described on ADR-014.

Closes: #855

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@RiccardoM
feat!: ADR-014: Improve the permission system
1e6cc68 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM
Merge branch 'master' of github.com:desmos-labs/desmos into riccardo/…
8cf3879 
…adr-014-implementation

� Conflicts:
�	x/subspaces/legacy/v2/store_test.go
�	x/subspaces/simulation/genesis.go
�	x/subspaces/simulation/operations_groups.go
�	x/subspaces/simulation/operations_permissions.go
�	x/subspaces/types/permissions.go
�	x/subspaces/types/permissions_test.go
@RiccardoM
fixed tests and lints
0ee11e2 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM
Merge branch 'master' of github.com:desmos-labs/desmos into riccardo/…
1e3180e 
…adr-014-implementation
@github-actions github-actions bot added x/CLI x/relationships x/subspaces Issue on the x/subspaces module labels May 31, 2022
@RiccardoM
fixed strings
95168cd 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@codecov
Copy link

codecov bot commented May 31, 2022 •
edited
Loading

Codecov Report

Merging #886 (b7a545b) into master (40f3e5c) will increase coverage by 0.17%.
The diff coverage is 81.31%.

@@            Coverage Diff             @@
##           master     #886      +/-   ##
==========================================
+ Coverage   82.73%   82.90%   +0.17%     
==========================================
  Files         133      132       -1     
  Lines       11548    11693     +145     
==========================================
+ Hits         9554     9694     +140     
+ Misses       1634     1628       -6     
- Partials      360      371      +11
Impacted Files Coverage Δ
x/posts/keeper/external_hooks.go 60.71% <ø> (+3.02%) ⬆️
x/relationships/keeper/hooks.go 66.66% <0.00%> (ø)
x/reports/keeper/external_hooks.go 66.66% <0.00%> (+3.25%) ⬆️
x/subspaces/types/genesis.go 100.00% <ø> (ø)
x/posts/keeper/msg_server.go 85.23% <62.50%> (ø)
x/subspaces/keeper/msg_server.go 88.46% <66.66%> (ø)
x/subspaces/types/msgs.go 98.29% <66.66%> (-0.71%) ⬇️
x/subspaces/legacy/v3/store.go 70.87% <70.00%> (-3.04%) ⬇️
x/subspaces/types/permissions.go 77.77% <77.96%> (+4.24%) ⬆️
x/subspaces/legacy/v2/permissions.go 93.75% <91.66%> (-6.25%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40f3e5c...b7a545b. Read the comment docs.

RiccardoM and others added 11 commits May 31, 2022 14:08
@RiccardoM
added the migration code
17c534f 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM
fixed linting
acafc4e 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM
updated swagger data
84a399d 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM
Merge branch 'master' of github.com:desmos-labs/desmos into riccardo/…
f76e76d 
…adr-014-permissions-improvements

� Conflicts:
�	client/docs/swagger-ui/swagger.yaml
�	proto/desmos/subspaces/v2/genesis.proto
�	proto/desmos/subspaces/v2/models.proto
�	proto/desmos/subspaces/v2/msgs.proto
�	proto/desmos/subspaces/v2/query.proto
�	x/posts/keeper/msg_server.go
�	x/posts/keeper/msg_server_test.go
�	x/posts/keeper/subspaces_hooks.go
�	x/posts/simulation/operations_attachments.go
�	x/posts/simulation/operations_polls.go
�	x/posts/simulation/operations_posts.go
�	x/relationships/keeper/hooks.go
�	x/subspaces/client/cli/cli_test.go
�	x/subspaces/client/cli/tx.go
�	x/subspaces/keeper/alias_functions.go
�	x/subspaces/keeper/genesis.go
�	x/subspaces/keeper/genesis_test.go
�	x/subspaces/keeper/grpc_query.go
�	x/subspaces/keeper/grpc_query_test.go
�	x/subspaces/keeper/hooks.go
�	x/subspaces/keeper/invariants_test.go
�	x/subspaces/keeper/msg_server.go
�	x/subspaces/keeper/msg_server_test.go
�	x/subspaces/keeper/permissions.go
�	x/subspaces/keeper/permissions_test.go
�	x/subspaces/keeper/subspaces.go
�	x/subspaces/keeper/subspaces_test.go
�	x/subspaces/legacy/v2/keys.go
�	x/subspaces/legacy/v2/models.go
�	x/subspaces/legacy/v2/permissions.go
�	x/subspaces/legacy/v2/store.go
�	x/subspaces/legacy/v2/store_test.go
�	x/subspaces/legacy/v3/store.go
�	x/subspaces/legacy/v3/store_test.go
�	x/subspaces/module.go
�	x/subspaces/simulation/decoder_test.go
�	x/subspaces/simulation/genesis.go
�	x/subspaces/simulation/operations_groups.go
�	x/subspaces/simulation/operations_permissions.go
�	x/subspaces/simulation/operations_subspaces.go
�	x/subspaces/types/genesis.go
�	x/subspaces/types/genesis.pb.go
�	x/subspaces/types/genesis_test.go
�	x/subspaces/types/hooks.go
�	x/subspaces/types/keys.go
�	x/subspaces/types/models.go
�	x/subspaces/types/models.pb.go
�	x/subspaces/types/msgs.go
�	x/subspaces/types/msgs.pb.go
�	x/subspaces/types/permissions.go
�	x/subspaces/types/permissions_test.go
�	x/subspaces/types/query.pb.go
�	x/subspaces/types/query.pb.gw.go
@RiccardoM
fixing tests
a845edf 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM
Updated Swagger definition
bc896bb
@RiccardoM
fixed tests
7a91f82 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM
fixed proto linting
0f30060 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM
Merge branch 'master' of github.com:desmos-labs/desmos into riccardo/…
df41ee2 
…adr-014-permissions-improvements
@RiccardoM
Merge branch 'master' of github.com:desmos-labs/desmos into riccardo/…
d52127c 
…adr-014-permissions-improvements

� Conflicts:
�	x/subspaces/types/genesis_test.go
�	x/subspaces/types/permissions.go
�	x/subspaces/types/permissions_test.go
@RiccardoM
merged master
028734c 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM RiccardoM marked this pull request as ready for review June 7, 2022 10:02
@RiccardoM RiccardoM requested review from dadamu and leobragaz and removed request for dadamu June 7, 2022 10:02
@RiccardoM
added changeset entry
7dec592 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM RiccardoM changed the title feat: ADR-014: Permissions improvements feat!: ADR-014: Permissions improvements Jun 7, 2022
@RiccardoM RiccardoM added the automerge Automatically merge PR once all prerequisites pass label Jun 7, 2022
dadamu
dadamu reviewed Jun 8, 2022
View reviewed changes
x/subspaces/types/permissions.go Outdated

// newPermission returns a new Permission containing the given value
func newPermission(permissionName string) Permission {
return strings.ToUpper(strings.ReplaceAll(permissionName, " ", "_"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better to merge all space first to avoid the typo case like registering write content then getting WRITE__CONTENT permission.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now improved the function to also handle such cases. Thanks for spotting this! 🙏

@RiccardoM
fixed suggestions
2ed26f0 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM RiccardoM requested a review from dadamu June 8, 2022 06:53
@RiccardoM
added missing tests
907394c 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM
fixed suggestion
1ca0864 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM RiccardoM requested a review from dadamu June 8, 2022 08:41
@RiccardoM
updated CLI tx examples
b7a545b 
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM RiccardoM requested a review from dadamu June 8, 2022 10:05
@dadamu
Copy link
Contributor

dadamu commented Jun 8, 2022 •
edited
Loading

Can we have a query to show the all registered permissions? I think it would be useful to check existing permissions.

@RiccardoM
Copy link
Contributor Author

Can we have a query to show the all registered permissions? I think it would be useful to check existing permissions.

Unfortunately, as permissions are registered when the binary is executed, no. They are not stored inside the current context so that's not possible. We will however have all the details inside our docs

dadamu
dadamu approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@dadamu dadamu left a comment

Choose a reason for hiding this comment

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

Ready to go from my side~

@mergify mergify bot merged commit a97e44b into master Jun 8, 2022
@mergify mergify bot deleted the riccardo/adr-014-permissions-improvements branch June 8, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dadamu dadamu dadamu approved these changes

@leobragaz leobragaz leobragaz approved these changes

Assignees
No one assigned
Labels
automerge Automatically merge PR once all prerequisites pass x/CLI x/relationships x/subspaces Issue on the x/subspaces module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve the management of permissions
3 participants
@RiccardoM @dadamu @leobragaz