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

Permissions cleanup #110

Merged
merged 7 commits into from Jan 24, 2023
Merged

Permissions cleanup #110

merged 7 commits into from Jan 24, 2023

Conversation

pdeziel
Copy link
Contributor

@pdeziel pdeziel commented Jan 23, 2023

Scope of changes

This is an attempt to cleanup the permissions, moving them to a package in Quarterdeck and adding ones required by Tenant.

Fixes SC-12875

Type of change

  • new feature
  • bug fix
  • documentation
  • testing
  • technical debt
  • other (describe)

Acceptance criteria

This is mostly a refactoring story but the permissions should be reviewed carefully to make sure we are on the same page.

Author checklist

  • I have manually tested the change and/or added automation in the form of unit tests or integration tests
  • I have updated the dependencies list
  • I have recompiled and included new protocol buffers to reflect changes I made
  • I have added new test fixtures as needed to support added tests
  • Check this box if a reviewer can merge this pull request after approval (leave it unchecked if you want to do it yourself)
  • I have moved the associated Shortcut story to "Ready for Review"

Reviewer(s) checklist

  • I have reviewed the new permissions that have been added and they make sense
  • The role-permission assignments are still correct

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #12875: Update tenant permissions.

pdeziel
pdeziel commented Jan 23, 2023
View reviewed changes
pkg/tenant/projects_test.go
@@ -111,64 +111,6 @@ func (suite *tenantTestSuite) TestTenantProjectList() {
}
}

func (suite *tenantTestSuite) TestTenantProjectCreate() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got moved in the merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok - I trust that this is ok; thank you for making a note of it!

pdeziel
pdeziel commented Jan 23, 2023
View reviewed changes
pkg/quarterdeck/db/migrations/0002_default_data.sql Outdated
(2, 'organizations:delete', 'Can delete organizations and their data', false, true, datetime('now'), datetime('now')),
(3, 'organizations:list', 'Can view the list of organizations', false, true, datetime('now'), datetime('now')),
(4, 'organizations:edit', 'Can make changes to organizations', false, true, datetime('now'), datetime('now')),
(5, 'organizations:detail', 'Can view details of an organization', false, true, datetime('now'), datetime('now')),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an area of uncertainty for me, should we create a new role which has the top level organization permissions?

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 the "Owner" role is well suited to the organization privilege -- so I would just use that role for these.

bbengfort
bbengfort reviewed Jan 24, 2023
View reviewed changes
Copy link
Contributor

@bbengfort bbengfort left a comment

Choose a reason for hiding this comment

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

Wow - this was more work than I expected; we should probably bump this story a point (or two). I'm going to add the test I mentioned and there are a couple of cleanup items we should take care of before merging.

pkg/quarterdeck/permissions/permissions.go Outdated
Comment on lines 7 to 11
CreateOrganizations = "organizations:create"
DeleteOrganizations = "organizations:delete"
ListOrganizations = "organizations:list"
EditOrganizations = "organizations:edit"
DetailOrganizations = "organizations:detail"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need both list and detail permissions, could we simplify it into a read permission?

Copy link
Contributor

Choose a reason for hiding this comment

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

Users will just list the organizations they belong to.

pkg/quarterdeck/db/migrations/0002_default_data.sql Outdated
(2, 'organizations:delete', 'Can delete organizations and their data', false, true, datetime('now'), datetime('now')),
(3, 'organizations:list', 'Can view the list of organizations', false, true, datetime('now'), datetime('now')),
(4, 'organizations:edit', 'Can make changes to organizations', false, true, datetime('now'), datetime('now')),
(5, 'organizations:detail', 'Can view details of an organization', false, true, datetime('now'), datetime('now')),
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 the "Owner" role is well suited to the organization privilege -- so I would just use that role for these.

pkg/quarterdeck/db/migrations/0002_default_data.sql Outdated Show resolved Hide resolved
pkg/quarterdeck/db/models/apikeys_test.go Outdated Show resolved Hide resolved
pkg/quarterdeck/db/models/users_test.go Outdated
@@ -197,5 +197,5 @@ func (m *modelTestSuite) TestUserPermissions() {
// Fetch the permissions for the user
permissions, err := user.Permissions(context.Background(), false)
require.NoError(err, "could not fetch permissions for user")
require.Len(permissions, 15)
require.Len(permissions, 17)
Copy link
Contributor

Choose a reason for hiding this comment

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

Because the user is an owner so has all of the permissions? Could we add a comment to that effect to warn the next time we change the permissions?

pkg/quarterdeck/permissions/permissions.go Outdated
// permissions in the quarterdeck database.
const (
// Organizations management
CreateOrganizations = "organizations:create"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need organizations:create do we? Anyone can create an organization even without be authenticated (e.g. when you first register).

pkg/quarterdeck/permissions/permissions.go
CreateTopics = "topics:create"
EditTopics = "topics:edit"
DestroyTopics = "topics:destroy"
ReadTopics = "topics:read"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding all of these permission strings!

pkg/tenant/projects_test.go
@@ -111,64 +111,6 @@ func (suite *tenantTestSuite) TestTenantProjectList() {
}
}

func (suite *tenantTestSuite) TestTenantProjectCreate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok - I trust that this is ok; thank you for making a note of it!

bbengfort
bbengfort approved these changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@bbengfort bbengfort left a comment

Choose a reason for hiding this comment

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

I updated the fixtures again to make sure the tests pass without changing the apikey_test.go file.

@bbengfort bbengfort merged commit c59e064 into main Jan 24, 2023
@bbengfort bbengfort deleted the sc-12875 branch January 24, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbengfort bbengfort bbengfort approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pdeziel @bbengfort