Skip to content

Commit 4e91023

Browse files
rvowlesrvowles
committed
fix: group management fixes
There is no indication from the groups API that a user is a superuser in a group and is therefore not deletable. Further the standard group API returns unnecessary member information.
1 parent eaff1a8 commit 4e91023

35 files changed

Lines changed: 402 additions & 400 deletions

File tree

‎adks/e2e-sdk/app/steps/group.ts‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,22 +52,40 @@ When('I assign the superuser to the group', async function() {
5252
expect(response.data.members.find( p => p.id.id === userId)).to.not.be.undefined;
5353
});
5454

55-
Then('I can delete the supergroup from the group', async function() {
55+
Then('I cannot delete the superuser from the group', async function() {
5656
const world = this as SdkWorld;
5757
const userResponse = await world.superuser.personApi.getPerson("self");
5858
const userId = (userResponse.data as Person).id.id;
59-
const response = await world.superuser.groupApi.deletePersonFromGroup(world.group.id, userId, true);
60-
expect(response.status).to.eq(200);
61-
expect(response.data.members.find( p => p.id.id === userId)).to.be.undefined;
59+
try {
60+
await world.superuser.groupApi.deletePersonFromGroup(world.group.id, userId, false);
61+
expect(true, `call succeeded to delete supergroup from portfolio group but should not have`).to.be.false;
62+
} catch (e) {
63+
expect(e.response.status).to.eq(404);
64+
}
6265
});
6366

64-
Then(/^the (superuser|user) is marked in the group as a (superuser|user)$/, async function(userSource: string, userType: string) {
67+
Then(/^the (superuser|user) (is|is not) in the group as a (superuser|user)$/, async function(userSource: string, isRule: string, userType: string) {
6568
const world = this as SdkWorld;
6669
const user = (userSource === 'superuser') ? world.superuser : world.user;
6770
expect(user.me).to.not.be.undefined;
6871
expect(world.group).to.not.be.undefined;
69-
const suserValue = userType === 'superuser';
70-
expect(world.group.sMembers.find(p => p.superuser === suserValue && p.person.id === user.personId))
72+
73+
const foundSuperuser = world.group.superMembers.find(p => p === user.personId);
74+
const found = world.group.simpleMembers.find(p => p.id === user.personId);
75+
76+
if (userType === 'superuser') {
77+
if (isRule === 'is') {
78+
expect(foundSuperuser, `${userSource} not superuser in group ${world.group.name}`).to.not.be.undefined;
79+
} else { // is not
80+
expect(foundSuperuser, `${userSource} not superuser in in group ${world.group.name}`).to.be.undefined;
81+
}
82+
} else {
83+
if (isRule === 'is') {
84+
expect(found, `${userSource} in group ${world.group.name} and is not`).to.not.be.undefined;
85+
} else { // is not
86+
expect(found, `${userSource} in group ${world.group.name} and should not be`).to.be.undefined;
87+
}
88+
}
7189
});
7290

7391
Then('I assign the new user to the new group', async function() {

‎adks/e2e-sdk/features/permission-check.feature‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,13 @@ Feature: Certain permission combinations should work as expected
88
And I create a new user
99
And I assign the new user to the new group
1010
And I get the portfolio admin group
11-
And the superuser is marked in the group as a superuser
12-
And the user is marked in the group as a user
11+
And the superuser is in the group as a superuser
12+
And the superuser is in the group as a user
13+
And the user is in the group as a user
14+
And the user is not in the group as a superuser
1315

1416
@delete-superuser-from-group
1517
Scenario: I add the superuser to the portfolio admin group and then remove them
1618
Given I create a new portfolio
1719
And I get the portfolio admin group
18-
When I assign the superuser to the group
19-
Then I can delete the supergroup from the group
20+
Then I cannot delete the superuser from the group

‎admin-frontend/open_admin_app/lib/routes/manage_group_route.dart‎

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import 'package:bloc_provider/bloc_provider.dart';
2+
import 'package:collection/collection.dart';
23
import 'package:flutter/material.dart';
34
import 'package:mrapi/api.dart';
45
import 'package:open_admin_app/common/ga_id.dart';
@@ -145,22 +146,22 @@ class ManageGroupRouteState extends State<ManageGroupRoute> {
145146
label: Text(
146147
AppLocalizations.of(context)!.columnName),
147148
onSort: (columnIndex, ascending) {
148-
onSortColumn(group.sMembers,
149+
onSortColumn(group.simpleMembers,
149150
columnIndex, ascending);
150151
}),
151152
DataColumn(
152153
label: Text(
153154
AppLocalizations.of(context)!.columnEmail),
154155
onSort: (columnIndex, ascending) {
155-
onSortColumn(group.sMembers,
156+
onSortColumn(group.simpleMembers,
156157
columnIndex, ascending);
157158
},
158159
),
159160
DataColumn(
160161
label: Text(AppLocalizations.of(context)!
161162
.columnMemberType),
162163
onSort: (columnIndex, ascending) {
163-
onSortColumn(group.sMembers,
164+
onSortColumn(group.simpleMembers,
164165
columnIndex, ascending);
165166
},
166167
),
@@ -173,22 +174,22 @@ class ManageGroupRouteState extends State<ManageGroupRoute> {
173174
onSort: (i, a) => {}),
174175
],
175176
rows: [
176-
for (GroupPerson member in group.sMembers)
177+
for (AnemicPerson member in group.simpleMembers)
177178
DataRow(cells: [
178179
DataCell(
179-
Text(member.person.name ?? ''),
180+
Text(member.name),
180181
),
181182
DataCell(Text(
182-
member.person.type == PersonType.person
183-
? member.person.email!
183+
member.type == PersonType.person
184+
? member.email!
184185
: "")),
185186
DataCell(Text(
186-
member.person.type == PersonType.person
187+
member.type == PersonType.person
187188
? AppLocalizations.of(context)!
188189
.memberTypeUser
189190
: AppLocalizations.of(context)!
190191
.memberTypeServiceAccount)),
191-
DataCell(!member.superuser
192+
DataCell((group.admin != true) || group.superMembers.firstWhereOrNull((s) => s == member.id) == null
192193
? Tooltip(
193194
message: AppLocalizations.of(context)!
194195
.removeFromGroup,
@@ -197,13 +198,13 @@ class ManageGroupRouteState extends State<ManageGroupRoute> {
197198
onPressed: () async {
198199
try {
199200
await bloc.removeFromGroup(
200-
group, member.person.id);
201+
group, member.id);
201202
if (context.mounted) {
202203
bloc.mrClient.addSnackbar(
203204
Text(AppLocalizations.of(
204205
context)!
205206
.memberRemovedFromGroup(
206-
member.person.name,
207+
member.name,
207208
group
208209
.name)));
209210
}
@@ -230,39 +231,39 @@ class ManageGroupRouteState extends State<ManageGroupRoute> {
230231
);
231232
}
232233

233-
void onSortColumn(List<GroupPerson> people, int columnIndex, bool ascending) {
234+
void onSortColumn(List<AnemicPerson> people, int columnIndex, bool ascending) {
234235
setState(() {
235236
if (columnIndex == 0) {
236237
if (ascending) {
237238
people.sort((a, b) {
238-
return a.person.name.toLowerCase().compareTo(b.person.name!.toLowerCase());
239+
return a.name.toLowerCase().compareTo(b.name.toLowerCase());
239240
});
240241
} else {
241242
people.sort((a, b) {
242-
return b.person.name.toLowerCase().compareTo(a.person.name.toLowerCase());
243+
return b.name.toLowerCase().compareTo(a.name.toLowerCase());
243244
});
244245
}
245246
}
246247
if (columnIndex == 1) {
247248
if (ascending) {
248249
people.sort((a, b) =>
249-
a.person.email!.toLowerCase().compareTo(b.person.email!.toLowerCase()));
250+
a.email!.toLowerCase().compareTo(b.email!.toLowerCase()));
250251
} else {
251252
people.sort((a, b) =>
252-
b.person.email!.toLowerCase().compareTo(a.person.email!.toLowerCase()));
253+
b.email!.toLowerCase().compareTo(a.email!.toLowerCase()));
253254
}
254255
}
255256
if (columnIndex == 2) {
256257
if (ascending) {
257-
people.sort((a, b) => a.person.type
258+
people.sort((a, b) => a.type
258259
.toString()
259260
.toLowerCase()
260-
.compareTo(b.person.type.toString().toLowerCase()));
261+
.compareTo(b.type.toString().toLowerCase()));
261262
} else {
262-
people.sort((a, b) => b.person.type
263+
people.sort((a, b) => b.type
263264
.toString()
264265
.toLowerCase()
265-
.compareTo(a.person.type.toString().toLowerCase()));
266+
.compareTo(a.type.toString().toLowerCase()));
266267
}
267268
}
268269
if (sortColumnIndex == columnIndex) {

‎admin-frontend/open_admin_app/lib/widgets/apps/manage_app_bloc.dart‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -275,15 +275,11 @@ class ManageAppBloc implements Bloc, ManagementRepositoryAwareBloc {
275275
}
276276

277277
Future<Group?> updateGroupWithEnvironmentRoles(String? gid, Group group) async {
278-
// make sure members are null or they all get removed
279-
group.members = [];
280-
281278
try {
282279
final updatedGroup = await _groupServiceApi
283-
.updateGroupOnPortfolio(portfolio!.id, group,
280+
.updateGroupOnPortfolio(portfolio!.id, UpdateGroup(id: group.id, version: group.version,
281+
applicationRoles: group.applicationRoles, environmentRoles: group.environmentRoles),
284282
includeGroupRoles: true,
285-
includeMembers: false,
286-
updateMembers: false,
287283
applicationId: applicationId,
288284
updateApplicationGroupRoles: true,
289285
updateEnvironmentGroupRoles: true);

‎admin-frontend/open_admin_app/lib/widgets/group/group_bloc.dart‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,8 @@ class GroupBloc implements Bloc {
107107
groupToUpdate.name = name;
108108

109109
final newGroup = await _groupServiceApi.updateGroupOnPortfolio(
110-
mrClient.currentPortfolio!.id, groupToUpdate,
111-
includeMembersV2: true, updateMembers: false);
110+
mrClient.currentPortfolio!.id, UpdateGroup(id: groupToUpdate.id, version: groupToUpdate.version, name: name),
111+
includeMembersV2: true);
112112

113113
// tell the portfolio groups to update as the name has changed.
114114
await getGroups(focusGroup: groupToUpdate);
@@ -121,7 +121,7 @@ class GroupBloc implements Bloc {
121121
await mrClient.dialogError(e, s,
122122
messageTitle: 'Failed to update group',
123123
messageBody:
124-
'Failed to update group because of a duplicate or other conflict.');
124+
'Failed to update group because of a duplicate or other update conflict.');
125125
return false;
126126
}
127127
}

‎backend/mr-api/mr-api.yaml‎

Lines changed: 38 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -468,12 +468,6 @@ paths:
468468
required: false
469469
schema:
470470
type: boolean
471-
- name: updateMembers
472-
description: "update members, deleting those that are not passed"
473-
in: query
474-
schema:
475-
type: boolean
476-
required: false
477471
- name: updateEnvironmentGroupRoles
478472
description: "update environment group roles, deleting any not passed"
479473
in: query
@@ -498,7 +492,7 @@ paths:
498492
content:
499493
application/json:
500494
schema:
501-
$ref: "#/components/schemas/Group"
495+
$ref: "#/components/schemas/UpdateGroup"
502496
responses:
503497
200:
504498
description: "Resulting group"
@@ -2173,65 +2167,6 @@ paths:
21732167
description: "forbidden"
21742168
404:
21752169
description: "not found"
2176-
put:
2177-
deprecated: true
2178-
security:
2179-
- bearerAuth: [ ]
2180-
tags:
2181-
- GroupService
2182-
description: "Update a group"
2183-
operationId: updateGroup
2184-
parameters:
2185-
- name: updateMembers
2186-
description: "update members, deleting those that are not passed"
2187-
in: query
2188-
schema:
2189-
type: boolean
2190-
required: false
2191-
- name: updateEnvironmentGroupRoles
2192-
description: "update environment group roles, deleting any not passed"
2193-
in: query
2194-
schema:
2195-
type: boolean
2196-
required: false
2197-
- name: updateApplicationGroupRoles
2198-
description: "update application group roles, deleting any not passed"
2199-
in: query
2200-
schema:
2201-
type: boolean
2202-
required: false
2203-
- name: applicationId
2204-
description: "if updating the application group roles, and the application id is passed, then the changes are limited to that application"
2205-
in: query
2206-
schema:
2207-
type: string
2208-
format: uuid
2209-
required: false
2210-
requestBody:
2211-
required: true
2212-
content:
2213-
application/json:
2214-
schema:
2215-
$ref: "#/components/schemas/Group"
2216-
responses:
2217-
200:
2218-
description: "Resulting group"
2219-
content:
2220-
application/json:
2221-
schema:
2222-
$ref: "#/components/schemas/Group"
2223-
400:
2224-
description: "Bad Request"
2225-
401:
2226-
description: "no permission"
2227-
403:
2228-
description: "forbidden"
2229-
404:
2230-
description: "not found"
2231-
409:
2232-
description: "duplicate user or duplicate group"
2233-
422:
2234-
description: "version conflict"
22352170
delete:
22362171
security:
22372172
- bearerAuth: [ ]
@@ -3996,6 +3931,34 @@ components:
39963931
default: []
39973932
items:
39983933
$ref: "#/components/schemas/ApplicationGroupRole"
3934+
UpdateGroup:
3935+
type: object
3936+
required:
3937+
- id
3938+
properties:
3939+
id:
3940+
type: string
3941+
format: uuid
3942+
nullable: false
3943+
name:
3944+
type: string
3945+
maxLength: 255
3946+
nullable: true
3947+
version:
3948+
type: integer
3949+
format: int64
3950+
nullable: false
3951+
applicationRoles:
3952+
nullable: true
3953+
type: array
3954+
default: []
3955+
items:
3956+
$ref: "#/components/schemas/ApplicationGroupRole"
3957+
environmentRoles:
3958+
type: array
3959+
default: []
3960+
items:
3961+
$ref: "#/components/schemas/EnvironmentGroupRole"
39993962
Group:
40003963
allOf:
40013964
- $ref: "#/components/schemas/Audit"
@@ -4028,11 +3991,18 @@ components:
40283991
type: string
40293992
minLength: 1
40303993
maxLength: 255
4031-
sMembers:
3994+
superMembers:
3995+
description: "This indicates which ids in the simpleMembers are superusers. It is a read only field and is ignored if it is passed back."
3996+
type: array
3997+
default: []
3998+
items:
3999+
type: string
4000+
format: uuid
4001+
simpleMembers:
40324002
type: array
40334003
default: []
40344004
items:
4035-
$ref: "#/components/schemas/GroupPerson"
4005+
$ref: "#/components/schemas/AnemicPerson"
40364006
members:
40374007
type: array
40384008
default: []

0 commit comments

Comments
 (0)