-
Notifications
You must be signed in to change notification settings - Fork 1
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
[IDP-1182] Add endpoint for updating groups_external
#355
Changes from all commits
2f02ddc
d3c0cec
21615b9
f6cd14b
1e32a84
257004a
838d822
449bae7
efe4e7c
0b76181
8e3b043
a55b671
31ec3af
de76f3a
85032eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,24 @@ types: | |
"code": 0, | ||
"status": 400 | ||
} | ||
GroupsExternalUpdate: | ||
type: object | ||
properties: | ||
email_address: string | ||
app_prefix: | ||
type: string | ||
description: > | ||
The app-specific prefix, without the trailing hyphen. | ||
groups: | ||
type: string[] | ||
description: > | ||
The desired list of groups, including the app-prefix. | ||
example: | | ||
{ | ||
"email_address": "[email protected]", | ||
"app_prefix": "myapp", | ||
"groups": ["myapp-users", "myapp-managers"] | ||
} | ||
UserResponse: | ||
description: > | ||
Information on user record. Password is not included if a password | ||
|
@@ -418,6 +436,25 @@ types: | |
description: A server-side error occurred. | ||
body: | ||
type: Error | ||
/external-groups: | ||
put: | ||
description: > | ||
Update a user's list of external groups for a given app-prefix to be | ||
exactly the given list, leaving external groups with other prefixes | ||
unchanged. | ||
body: | ||
type: GroupsExternalUpdate | ||
responses: | ||
204: | ||
description: > | ||
The user's external groups for that app-prefix were updated. | ||
404: | ||
description: > | ||
No such user found. | ||
422: | ||
description: The given data does not satisfy some validation rule. | ||
body: | ||
type: Error | ||
/{employee_id}: | ||
get: | ||
description: Get information about a specific user. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1003,6 +1003,80 @@ public function isPromptForMfa(): bool | |
return false; | ||
} | ||
|
||
public function updateExternalGroups($appPrefix, $appExternalGroups): bool | ||
{ | ||
foreach ($appExternalGroups as $appExternalGroup) { | ||
if (! str_starts_with($appExternalGroup, $appPrefix . '-')) { | ||
$this->addErrors([ | ||
'groups_external' => sprintf( | ||
'The given group %s does not start with the given prefix (%s)', | ||
json_encode($appExternalGroup), | ||
json_encode($appPrefix) | ||
), | ||
]); | ||
return false; | ||
} | ||
} | ||
Comment on lines
+1008
to
+1019
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the incoming group names omitted the prefix or the separate prefix property was omitted, this validation wouldn't be necessary. Doing so would remove one possible failure scenario. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True. I considered that option, but I'd like the group names (as typed into the Google Sheet) to exactly match the group names that REAP will receive via SAML, to help make it more intuitive. If I had them simply type in everything after the prefix (on the Google Sheet), then they would type in something like "europe" and have to know/remember that it will come to REAP via SAML as "reap-europe". It seemed better to keep that prefix as visible as possible, so that they don't set up rules for a "europe" group and then be confused when it doesn't work. |
||
$this->removeInMemoryExternalGroupsFor($appPrefix); | ||
$this->addInMemoryExternalGroups($appExternalGroups); | ||
|
||
$this->scenario = self::SCENARIO_UPDATE_USER; | ||
$saved = $this->save(true, ['groups_external']); | ||
if ($saved) { | ||
return true; | ||
} else { | ||
Yii::warning(sprintf( | ||
'Failed to update external groups for %s: %s', | ||
$this->email, | ||
join(', ', $this->getFirstErrors()) | ||
)); | ||
return false; | ||
} | ||
} | ||
|
||
/** | ||
* Remove all entries from this User object's list of external groups that | ||
* begin with the given prefix. | ||
* | ||
* NOTE: | ||
* This only updates the property in memory. It leaves the calling code to | ||
* call `save()` on this User when desired. | ||
* | ||
* @param $appPrefix | ||
* @return void | ||
*/ | ||
private function removeInMemoryExternalGroupsFor($appPrefix) | ||
{ | ||
$currentExternalGroups = explode(',', $this->groups_external); | ||
$newExternalGroups = []; | ||
foreach ($currentExternalGroups as $externalGroup) { | ||
if (! str_starts_with($externalGroup, $appPrefix . '-')) { | ||
$newExternalGroups[] = $externalGroup; | ||
} | ||
} | ||
$this->groups_external = join(',', $newExternalGroups); | ||
} | ||
|
||
/** | ||
* Add the given groups to this User objects' list of external groups. | ||
* | ||
* NOTE: | ||
* This only updates the property in memory. It leaves the calling code to | ||
* call `save()` on this User when desired. | ||
* | ||
* @param $newExternalGroups | ||
* @return void | ||
*/ | ||
private function addInMemoryExternalGroups($newExternalGroups) | ||
{ | ||
$newCommaSeparatedExternalGroups = sprintf( | ||
'%s,%s', | ||
$this->groups_external, | ||
join(',', $newExternalGroups) | ||
); | ||
$this->groups_external = trim($newCommaSeparatedExternalGroups, ','); | ||
} | ||
|
||
/** | ||
* Update the date field that corresponds to the current nag state | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
<?php | ||
|
||
namespace Sil\SilIdBroker\Behat\Context; | ||
|
||
use Behat\Gherkin\Node\TableNode; | ||
use common\models\User; | ||
use Webmozart\Assert\Assert; | ||
|
||
class GroupsExternalUpdatesContext extends GroupsExternalContext | ||
{ | ||
/** | ||
* @When I update that user's list of :appPrefix external groups to the following: | ||
*/ | ||
public function iUpdateThatUsersListOfExternalGroupsToTheFollowing($appPrefix, TableNode $table) | ||
{ | ||
$externalGroups = []; | ||
foreach ($table as $row) { | ||
$externalGroups[] = $row['externalGroup']; | ||
} | ||
|
||
$this->cleanRequestBody(); | ||
$this->setRequestBody('email_address', $this->getUserEmailAddress()); | ||
$this->setRequestBody('app_prefix', $appPrefix); | ||
$this->setRequestBody('groups', $externalGroups); | ||
|
||
$this->iRequestTheResourceBe('/user/external-groups', 'updated'); | ||
} | ||
|
||
/** | ||
* @Then that user's list of external groups should be :commaSeparatedExternalGroups | ||
*/ | ||
public function thatUsersListOfExternalGroupsShouldBe($commaSeparatedExternalGroups) | ||
{ | ||
$user = User::findByEmail($this->getUserEmailAddress()); | ||
Assert::same($user->groups_external, $commaSeparatedExternalGroups); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
Feature: Updating a User's list of external groups | ||
|
||
Background: | ||
Given the requester is authorized | ||
|
||
Scenario: Add an external group to a user's list for a particular app | ||
Given a user exists | ||
And that user's list of external groups is "wiki-users" | ||
When I update that user's list of "wiki" external groups to the following: | ||
| externalGroup | | ||
| wiki-users | | ||
| wiki-managers | | ||
Then the response status code should be 204 | ||
And that user's list of external groups should be "wiki-users,wiki-managers" | ||
|
||
Scenario: Remove an external group from a user's list for a particular app | ||
Given a user exists | ||
And that user's list of external groups is "wiki-users,wiki-managers" | ||
When I update that user's list of "wiki" external groups to the following: | ||
| externalGroup | | ||
| wiki-managers | | ||
Then the response status code should be 204 | ||
And that user's list of external groups should be "wiki-managers" | ||
|
||
Scenario: Leave a user's external groups for a different app unchanged | ||
Given a user exists | ||
And that user's list of external groups is "wiki-users,map-europe" | ||
When I update that user's list of "map" external groups to the following: | ||
| externalGroup | | ||
| map-america | | ||
Then the response status code should be 204 | ||
And that user's list of external groups should be "wiki-users,map-america" | ||
|
||
Scenario: Try to add an external group that does not match the given app-prefix | ||
Given a user exists | ||
And that user's list of external groups is "wiki-users" | ||
When I update that user's list of "wiki" external groups to the following: | ||
| externalGroup | | ||
| map-america | | ||
Then the response status code should be 422 | ||
And the response body should contain "prefix" | ||
And that user's list of external groups should be "wiki-users" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the prefix included as a separate property as well as included in the group names themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new sync process (yet to be written) will have a single, set prefix (per instance), so that a given copy of the new sync process can only affect external groups with that prefix.
The current need is for syncing from a Google Sheet controlled by some of the REAP admins, to enable them to add "reap-____" groups to users signing into REAP (because the new platform, Preservica, requires the use of SAML-provided groups for controlling authorization). This will let them add specific groups to specific users in specific IDPs, to control what those users are allowed to do in REAP. The
app_prefix
will be controlled by us, who set up and run the instance of the new sync process. Thegroups
are controlled by the REAP admins (via the Google Sheet), so this is just another way to ensure the data they provide meets the constraints we have established for this new part of our system.I hope that made sense. Feel free to ask more questions and/or review the parent YouTrack issue (IDP-1153) for more details.