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.

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

Merged
merged 15 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions api.raml
Original file line number Diff line number Diff line change
Expand Up @@ -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": "john_smith@example.com",
"app_prefix": "myapp",
"groups": ["myapp-users", "myapp-managers"]
Comment on lines +62 to +63
Copy link
Contributor

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?

Copy link
Contributor Author

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. The groups 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.

}
UserResponse:
description: >
Information on user record. Password is not included if a password
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 4 additions & 0 deletions application/behat.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ default:
paths:
- "features/groups-external.feature"
contexts: [ Sil\SilIdBroker\Behat\Context\GroupsExternalContext ]
groups_external_updates_features:
paths:
- "features/groups-external-updates.feature"
contexts: [ Sil\SilIdBroker\Behat\Context\GroupsExternalUpdatesContext ]
hibp_unit_tests_features:
paths:
- "features/hibp-unit-tests.feature"
Expand Down
74 changes: 74 additions & 0 deletions application/common/models/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
*/
Expand Down
5 changes: 5 additions & 0 deletions application/features/bootstrap/GroupsExternalContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ private function setThatUsersPassword(string $password)
));
}

protected function getUserEmailAddress(): string
{
return $this->userEmailAddress;
}

/**
* @Given that user's list of groups is :commaSeparatedGroups
*/
Expand Down
37 changes: 37 additions & 0 deletions application/features/bootstrap/GroupsExternalUpdatesContext.php
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);
}
}
42 changes: 42 additions & 0 deletions application/features/groups-external-updates.feature
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"
2 changes: 0 additions & 2 deletions application/features/groups-external.feature
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ Feature: Incorporating custom (external) groups in a User's `members` list
| two |
| {idpName} |

# Scenario: Update a user's `groups_external` list, given a group prefix and list of groups

# # Scenarios that belong in the new "groups_external" sync:
# Scenario: Send 1 notification email if sync finds group(s) for a user not in this IDP
# Scenario: Add entries in the synced Google Sheet to the `groups_external` field
Expand Down
1 change: 1 addition & 0 deletions application/frontend/config/main.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
'GET user' => 'user/index',
'GET user/<employeeId:\w+>' => 'user/view',
'POST user' => 'user/create',
'PUT user/external-groups' => 'user/update-external-groups',
'PUT user/<employeeId:\w+>' => 'user/update',
'PUT user/<employeeId:\w+>/password' => 'user/update-password',
'PUT user/<employeeId:\w+>/password/assess' => 'user/assess-password',
Expand Down
21 changes: 21 additions & 0 deletions application/frontend/controllers/UserController.php
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,27 @@ public function actionUpdate(string $employeeId)
return $user;
}

public function actionUpdateExternalGroups()
{
$emailAddress = Yii::$app->request->getBodyParam('email_address');
$appPrefix = Yii::$app->request->getBodyParam('app_prefix');
$externalGroups = Yii::$app->request->getBodyParam('groups');

$user = User::findByEmail($emailAddress);
if ($user === null) {
Yii::$app->response->statusCode = 404;
return;
}

if ($user->updateExternalGroups($appPrefix, $externalGroups)) {
Yii::$app->response->statusCode = 204;
return;
}

$errors = join(', ', $user->getFirstErrors());
throw new UnprocessableEntityHttpException($errors);
}

public function actionUpdatePassword(string $employeeId)
{
$user = User::findOne(['employee_id' => $employeeId]);
Expand Down