Create endpoint for listing spaces a user has a role in #545 #586

xcoulon · 2018-08-08T09:55:18Z

The service has been tested under the following conditions:

a user has a role on a space
a user has a role in the parent org, and a default mapping provides
. a role on the child space
a user belongs to team which has a role on a space
a user belongs to a team which has a role on an org, and a default
. role mapping provides a role on the space

Fixes #545

fabric8cd · 2018-08-08T09:59:20Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-1

fabric8cd · 2018-08-08T16:32:21Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-2

sbose78 · 2018-08-09T06:39:10Z

authorization/role/repository/role_blackbox_test.go

+			require.Len(t, roles, 1)
+			assert.Equal(t, space.Resource().ResourceType.Name, roles[0].ResourceType)
+			assert.Equal(t, authorization.SpaceAdminRole, roles[0].RoleName)
+			assert.ElementsMatch(t, roles[0].Scopes, []string{


Interesting, wasn't aware of this.

sbose78 · 2018-08-09T06:40:16Z

gormtestsupport/db_test_suite.go

@@ -85,7 +87,9 @@ func (s *DBTestSuite) PopulateDBTestSuite(ctx context.Context) {

 // TearDownSuite implements suite.TearDownAllSuite
 func (s *DBTestSuite) TearDownSuite() {
-	s.cleanSuite()
+	if s.Configuration.IsCleanTestDataEnabled() {


Could you add a comment why this is being done?

well, I thought that the comment on the configuration.ConfigurationData.IsCleanTestDataEnabled() was clear enough?

Sure, but a line of comment explaining why would we have to sometimes disable the test data cleanup, would be helpful.

ok, let me add this, then ;)

sbose78 · 2018-08-09T06:41:20Z

migration/sql-files/035-unique_constraint_default_role_mapping.sql

@@ -0,0 +1,2 @@
+-- We must not allow multiple creations of role mapping for a given type of resource with the same 'from' and 'to' roles.
+alter table default_role_mapping add constraint unique_default_role_mapping UNIQUE (resource_type_id, from_role_id, to_role_id);


Thankfully, we don't have an exposed API for this, else you would have had to consider migration.

right, that's something we would have needed to handle, too. Same would need to apply for "custom" role mappings, but I have not started work on those ones yet. Probably in another PR

fabric8cd · 2018-08-09T07:24:37Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-3

codecov · 2018-08-09T07:33:33Z

Codecov Report

Merging #586 into master will increase coverage by 0.21%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #586      +/-   ##
==========================================
+ Coverage    72.6%   72.81%   +0.21%     
==========================================
  Files          84       84              
  Lines        8331     8480     +149     
==========================================
+ Hits         6049     6175     +126     
- Misses       1817     1835      +18     
- Partials      465      470       +5

Impacted Files	Coverage Δ
authorization/authorization.go	`88.04% <ø> (ø)`	⬆️
...horization/role/service/role_management_service.go	`90.9% <0%> (-1.69%)`	⬇️
migration/migration.go	`58.59% <100%> (+0.26%)`	⬆️
configuration/configuration.go	`82.15% <100%> (+0.06%)`	⬆️
...horization/role/repository/default_role_mapping.go	`75% <60%> (-1%)`	⬇️
authorization/role/repository/role.go	`81.09% <83.48%> (+1.42%)`	⬆️
controller/user.go	`89.83% <93.75%> (+4.64%)`	⬆️
...orization/invitation/service/invitation_service.go	`71.71% <0%> (-0.29%)`	⬇️
notification/service/notification.go	`97.59% <0%> (+2.18%)`	⬆️

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 76c48fe...dc22faf. Read the comment docs.

fabric8cd · 2018-08-09T08:22:46Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-4

fabric8cd · 2018-08-09T09:58:07Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-5

fabric8cd · 2018-08-09T10:44:11Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-6

fabric8cd · 2018-08-10T07:31:32Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-8

adding a new endpoint in the `/user` base: `/spaces` to list spaces in which the current user (identitfied by the request token) has a role. includes changes in the `UserService` and `RoleRepository` to retrieve resources of a given type for which a user as a direct role, or a role via a team, or a role via a default role mapping while having a direct role or via a team on another resource. For example, a user has `admin` role on an org and is given the `contributor` on the space via a default role mapping. Custom role mappings are not supported yet. also: - refactor `identityIDFromWrapper` as standalone function - fix the missing call to the gorm callback when adding a member to a team, by calling the repository instead of performing a direct SQL insert. Fixes #545 Signed-off-by: Xavier Coulon <[email protected]>

fabric8cd · 2018-08-10T10:08:31Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-9

Signed-off-by: Xavier Coulon <[email protected]>

fabric8cd · 2018-08-10T14:30:56Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-10

xcoulon · 2018-08-10T15:16:15Z

[test]

Signed-off-by: Xavier Coulon <[email protected]>

fabric8cd · 2018-08-16T09:54:18Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-11

dipak-pawar

Thank you @xcoulon. I have a few minor comments to address. Also, I haven't seen anything related to account in design/account.go. How about renaming it to design/user.go?

dipak-pawar · 2018-08-16T12:09:42Z

design/account.go

+	a.Attribute("scopes", a.ArrayOf(d.String), "The scopes associated with the role of the user in the corresponding resource")
+	a.Required("role", "scopes")
+})
+
 // updateidentityDataAttributes represents an identified user object attributes used for updating a user.
 var updateUserDataAttributes = a.Type("UpdateIdentityDataAttributes", func() {


variable updateUserDataAttributes has different name in documentation. Can you please change it?

sure, let me fix that

dipak-pawar · 2018-08-16T12:22:02Z

controller/user_blackbox_test.go

+
+		t.Run("role on no space", func(t *testing.T) {
+			// given
+			g := s.NewTestGraph()


I think you can use s.Graph directly here, because we are creating it beforeEach test. https://github.com/fabric8-services/fabric8-auth/blob/master/gormtestsupport/db_test_suite.go#L72

I'd rather create a new TestGraph for each subtest because in the latest commits of this PR, I need to pass the current *testing.T as an argument, because in case of assertion failure, the test panics (that's a side-effect of using sub tests)

It's one of the reason I don't like subtests :) It looks nicer in test reports but doesn't provide good isolation between separate tests.

@alexeykazakov I do not agree with you on that: sub tests also allow for running a set of tests at once, which can be very convenient.

I’m not saying that nicer reports is the only advantage of subtests. I just pointed that there are some disadvantages too. Like lack of proper test isolation.

yes, I agree with you for the lack of test isolation, and this is annoying. I'll see if there's something we can do about it. For now, a good way to work-around this limitation is by making sure that test data do not conflict with each other...

dipak-pawar · 2018-08-16T12:22:19Z

controller/user_blackbox_test.go

+
+		t.Run("role on 1 space", func(t *testing.T) {
+			// given
+			g := s.NewTestGraph()


same as above

dipak-pawar · 2018-08-16T12:36:53Z

design/account.go

+			// requirement and value will be handled by the controller method,
+			// in order to be able to return a proper JSON-API response to the client in case
+			// of a bad request
+			a.Param("type", d.String, "the type of resource to list")


How about adding sample values here? Currently, we are only supporting space. This way it will helpful to understand for consumer about what are supported types and they can use the same. Something like Enum("spaces")

ok, let me see that

actually, we could do that, but then Goa will immediately return an error while processing the request with an invalid or empty type parameter, without calling the controller, which prevents us from returning a JSON-API response. So we need to be permissive in the design and handle the type value in the controller.

(and now I remember that I tried that but reverted the change as I could not test the case of invalid/empty type parameter as I needed to)

dipak-pawar · 2018-08-16T12:40:02Z

controller/user.go

+		resourceType = authorization.ResourceTypeSpace
+	}
+	if resourceType == "" {
+		return jsonapi.JSONErrorResponse(ctx, errors.NewBadParameterError("type", *ctx.Type))


how about giving supported parameters in error to help cosumer of this api?

ok, let me change the error to include the list of supported types (ie, spaces only for now)

Signed-off-by: Xavier Coulon <[email protected]>

fabric8cd · 2018-08-16T15:11:39Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-12

xcoulon · 2018-08-16T15:24:12Z

@dipak-pawar thanks for your feedback and let me address your comments

Signed-off-by: Xavier Coulon <[email protected]>

fabric8cd · 2018-08-16T15:49:36Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-13

xcoulon · 2018-08-16T15:57:55Z

@dipak-pawar I addressed your comments in fffc4ab

Signed-off-by: Xavier Coulon <[email protected]>

fabric8cd · 2018-08-17T06:50:24Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-14

Signed-off-by: Xavier Coulon <[email protected]>

fabric8cd · 2018-08-17T13:18:53Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-15

fabric8cd · 2018-08-20T14:24:39Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-16

sbose78

Some minor changes requested - let's please avoid refactoring not directly related to the PR ( especially around API design declarations :) ) since it becomes hard to trace down later on.

sbose78 · 2018-08-20T15:01:13Z

authorization/role/repository/role.go

+        	AND ir.deleted_at IS NULL
+		GROUP BY 
+		  	role.role_id, role.name, res.resource_id
+		/* list the roles on resources of the given type when the user has a role inherited by an ancestor resource via default role mapping */


*inherited from an ancestor?

oh, my bad. Thanks for spotting this grammar mistake!

sbose78 · 2018-08-21T04:20:37Z

design/user.go

+			a.Required("type")
+		})
+		a.Description("List resources of a given type with a role for the current user")
+		// a.UseTrait("conditional")


remove this please.

sure, that was a left-over since support for conditional requests will be implemented in #606 - just opened, forgot about it before so thanks for reminding me about it! ;)

sbose78 · 2018-08-21T04:24:53Z

authorization/role/repository/role_blackbox_test.go

+			assert.Len(t, roles, 0)
+		})
+
+		t.Run("individual is contributor in the parent organization with default role mapping", func(t *testing.T) {


Nice tests, could you list these scenarios as a checklist in the PR description/commit message ?

xcoulon · 2018-08-21T07:56:11Z

@sbose78 thanks for the comments/review, I'll take you comments into account. I got caught in the design refactoring, but this should have probably been better in a separate PR. I'll try to keep the next ones more focused.

The repository is tested under the following conditions: - a user has a role on a space - a user has a role in the parent org, and a default mapping provides . a role on the child space - a user belongs to team which has a role on a space - a user belongs to a team which has a role on an org, and a default . role mapping provides a role on the space Fixes #545 Signed-off-by: Xavier Coulon <[email protected]>

fabric8cd · 2018-08-21T08:04:11Z

@xcoulon snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-586-17

xcoulon · 2018-08-21T09:35:28Z

thanks for the reviews and approvals, @alexeykazakov, @dipak-pawar and @sbose78!

xcoulon added the work-in-progress label Aug 8, 2018

sbose78 reviewed Aug 9, 2018

View reviewed changes

refactor endpoint to be more generic and add tests

f5a8cfb

Signed-off-by: Xavier Coulon <[email protected]>

xcoulon changed the title ~~WIP - Create endpoint for listing spaces a user has a role in #545~~ Create endpoint for listing spaces a user has a role in #545 Aug 10, 2018

alien-ike removed the work-in-progress label Aug 10, 2018

xcoulon requested a review from sbryzak August 10, 2018 15:16

xcoulon requested a review from dipak-pawar August 13, 2018 09:32

Limit all_resources view to resource of given type (or parents)

d1d291f

Signed-off-by: Xavier Coulon <[email protected]>

dipak-pawar suggested changes Aug 16, 2018

View reviewed changes

refactor test with custom roles instead of 'admin', etc.

34d0489

Signed-off-by: Xavier Coulon <[email protected]>

Addressing Dipak's comments

fffc4ab

Signed-off-by: Xavier Coulon <[email protected]>

split 'account.go' in 'user.go' and 'userinfo.go' in design

00b3b0f

Signed-off-by: Xavier Coulon <[email protected]>

dipak-pawar approved these changes Aug 17, 2018

View reviewed changes

split design files to match existing controllers + required 'type' param

051b991

Signed-off-by: Xavier Coulon <[email protected]>

Merge branch 'master' into Issue545_list_spaces

1a54217

sbose78 suggested changes Aug 21, 2018

View reviewed changes

sbose78 reviewed Aug 21, 2018

View reviewed changes

xcoulon added 2 commits August 21, 2018 09:56

Merge branch 'master' into Issue545_list_spaces

ae40738

sbose78 approved these changes Aug 21, 2018

View reviewed changes

sbose78 merged commit 15e78f7 into fabric8-services:master Aug 21, 2018

sbose78 mentioned this pull request Aug 29, 2018

Remove role names from API #629

Closed

xcoulon deleted the Issue545_list_spaces branch August 29, 2018 13:55

		@@ -0,0 +1,2 @@
		-- We must not allow multiple creations of role mapping for a given type of resource with the same 'from' and 'to' roles.
		alter table default_role_mapping add constraint unique_default_role_mapping UNIQUE (resource_type_id, from_role_id, to_role_id);

Create endpoint for listing spaces a user has a role in #545 #586

Create endpoint for listing spaces a user has a role in #545 #586

Conversation

xcoulon commented Aug 8, 2018 • edited by sbose78 Loading

fabric8cd commented Aug 8, 2018

fabric8cd commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabric8cd commented Aug 9, 2018

codecov bot commented Aug 9, 2018 • edited Loading

Codecov Report

fabric8cd commented Aug 9, 2018

fabric8cd commented Aug 9, 2018

fabric8cd commented Aug 9, 2018

fabric8cd commented Aug 10, 2018

fabric8cd commented Aug 10, 2018

fabric8cd commented Aug 10, 2018

xcoulon commented Aug 10, 2018

fabric8cd commented Aug 16, 2018

dipak-pawar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabric8cd commented Aug 16, 2018

xcoulon commented Aug 16, 2018

fabric8cd commented Aug 16, 2018

xcoulon commented Aug 16, 2018

fabric8cd commented Aug 17, 2018

fabric8cd commented Aug 17, 2018

fabric8cd commented Aug 20, 2018

sbose78 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcoulon commented Aug 21, 2018

fabric8cd commented Aug 21, 2018

xcoulon commented Aug 21, 2018

xcoulon commented Aug 8, 2018 •

edited by sbose78

Loading

codecov bot commented Aug 9, 2018 •

edited

Loading