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-1176] Add groups_external field to User #352

Merged
merged 22 commits into from
Aug 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
07d8a98
Add first `groups_external` test and implement first two steps
forevermatt Aug 13, 2024
42b3213
Implement (failing) test step for setting `groups_external` value
forevermatt Aug 13, 2024
6c6c14a
Use migration to add `user.groups_external` field to database
forevermatt Aug 13, 2024
41702f2
Regenerate the base models (excluding `groups_external` changes)
forevermatt Aug 13, 2024
6aa3d7a
Regenerate UserBase model to include `groups_external` field
forevermatt Aug 13, 2024
bd0b0b1
Set `user.groups_external`'s default value to be an empty string
forevermatt Aug 13, 2024
db30f68
Switch to fluent syntax (not DB-specific) for defining the new field
forevermatt Aug 13, 2024
3f47bb3
Regenerate UserBase model now that `groups_external` has a default value
forevermatt Aug 13, 2024
ffb2674
Get new test successfully signing in as the test user
forevermatt Aug 13, 2024
bb5f000
Add phpMyAdmin container for testdb
forevermatt Aug 14, 2024
7dbad6e
Improve groups_external test, implement next (failing) step
forevermatt Aug 14, 2024
11d9cf4
Merge branch 'develop' into feature/idp-1176-add-groups-external-field
forevermatt Aug 14, 2024
54fd8ba
Make groups_external test failure message more informative
forevermatt Aug 14, 2024
9fcaa42
Include groups_external values in a User's `member` list during login
forevermatt Aug 14, 2024
0d43d78
Ensure empty `groups` and empty `groups_external` fields work properly
forevermatt Aug 14, 2024
6543ed4
Merge branch 'develop' into feature/idp-1176-add-groups-external-field
forevermatt Aug 19, 2024
690289e
Customize the label for the new `groups_external` field
forevermatt Aug 19, 2024
4f6e211
Stop running CI/CD tests as soon as one of them fails
forevermatt Aug 19, 2024
ca0c7a2
Fix groups_external test to detect an empty string in the `members` list
forevermatt Aug 19, 2024
931f4db
Include the array contents in the failed-test output
forevermatt Aug 19, 2024
b608e9f
Remove now-unused new test step
forevermatt Aug 19, 2024
6ce06ac
Avoid adding empty string to `members` when `groups_external` is empty
forevermatt Aug 19, 2024
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
4 changes: 4 additions & 0 deletions application/behat.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ default:
paths:
- "features/email.feature"
contexts: [ Sil\SilIdBroker\Behat\Context\EmailContext ]
groups_external_features:
paths:
- "features/groups-external.feature"
contexts: [ Sil\SilIdBroker\Behat\Context\GroupsExternalContext ]
hibp_unit_tests_features:
paths:
- "features/hibp-unit-tests.feature"
Expand Down
11 changes: 11 additions & 0 deletions application/common/models/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,17 @@ public function fields(): array
'member' => function (self $model) {
if (!empty($model->groups)) {
$member = explode(',', $model->groups);
} else {
$member = [];
}
Comment on lines 571 to 576
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the else added? Is there some sort of "strict" mode that would flag an error when assigning to a new array key on a yet-undefined variable (e.g. line 581 or 585). If so, then maybe this is better:

Suggested change
'member' => function (self $model) {
if (!empty($model->groups)) {
$member = explode(',', $model->groups);
} else {
$member = [];
}
'member' => function (self $model) {
$member = [];
if (!empty($model->groups)) {
$member = explode(',', $model->groups);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a variable without defining it (e.g. assigning something to an array on an uninitialized array, in PHP) seems like a code smell. I simply added the else to ensure that the array would exist and be in the desired state, regardless of which path through the logic was executed.

Initializing the array first, as you suggested, is another valid option, but since it would be replaced with a different array within the if-block, I decided to only set it to an empty array if needed, rather than every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for discussing it, though 👍


$externalGroups = explode(',', $model->groups_external);
foreach ($externalGroups as $externalGroup) {
if (!empty($externalGroup)) {
$member[] = $externalGroup;
}
}

$member[] = \Yii::$app->params['idpName'];
return $member;
},
Expand Down Expand Up @@ -969,6 +979,7 @@ public function attributeLabels()
$labels['last_synced_utc'] = Yii::t('app', 'Last Synced (UTC)');
$labels['created_utc'] = Yii::t('app', 'Created (UTC)');
$labels['deactivated_utc'] = Yii::t('app', 'Deactivated (UTC)');
$labels['groups_external'] = Yii::t('app', 'Groups (External)');

return $labels;
}
Expand Down
4 changes: 3 additions & 1 deletion application/common/models/UserBase.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
* @property string|null $manager_email
* @property string $hide
* @property string|null $groups
* @property string $groups_external
* @property string|null $personal_email
* @property string|null $expires_on
* @property string $nag_for_mfa_after
Expand Down Expand Up @@ -60,7 +61,7 @@ public function rules()
[['active', 'locked', 'require_mfa', 'hide'], 'string'],
[['last_changed_utc', 'last_synced_utc', 'review_profile_after', 'last_login_utc', 'expires_on', 'nag_for_mfa_after', 'nag_for_method_after', 'created_utc', 'deactivated_utc'], 'safe'],
[['uuid'], 'string', 'max' => 64],
[['employee_id', 'first_name', 'last_name', 'display_name', 'username', 'email', 'manager_email', 'groups', 'personal_email'], 'string', 'max' => 255],
[['employee_id', 'first_name', 'last_name', 'display_name', 'username', 'email', 'manager_email', 'groups', 'groups_external', 'personal_email'], 'string', 'max' => 255],
[['employee_id'], 'unique'],
[['username'], 'unique'],
[['email'], 'unique'],
Expand Down Expand Up @@ -93,6 +94,7 @@ public function attributeLabels()
'manager_email' => Yii::t('app', 'Manager Email'),
'hide' => Yii::t('app', 'Hide'),
'groups' => Yii::t('app', 'Groups'),
'groups_external' => Yii::t('app', 'Groups External'),
'personal_email' => Yii::t('app', 'Personal Email'),
'expires_on' => Yii::t('app', 'Expires On'),
'nag_for_mfa_after' => Yii::t('app', 'Nag For Mfa After'),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

use yii\db\Migration;

/**
* Class m240813_155757_add_groups_external_field_to_user
*/
class m240813_155757_add_groups_external_field_to_user extends Migration
{
public function safeUp()
{
$this->addColumn(
'{{user}}',
'groups_external',
$this->string()->notNull()->defaultValue('')->after('groups')

Choose a reason for hiding this comment

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

The Yii syntax is ugly.

Copy link
Contributor

Choose a reason for hiding this comment

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

PHP syntax is ugly. 😛

);
}

public function safeDown()
{
$this->dropColumn('{{user}}', 'groups_external');
}
}
2 changes: 1 addition & 1 deletion application/features/bootstrap/FeatureContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,7 @@ public function theResponseShouldContainAMemberArrayWithOnlyTheseElements($key,
if ($want == '{idpName}') {
$want = \Yii::$app->params['idpName'];
}
Assert::true(in_array($want, $property), '"' . $want . '" not in array');
Assert::true(in_array($want, $property), '"' . $want . '" not in array: ' . json_encode($property));
$n++;
}

Expand Down
114 changes: 114 additions & 0 deletions application/features/bootstrap/GroupsExternalContext.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
<?php

namespace Sil\SilIdBroker\Behat\Context;

use Behat\Gherkin\Node\TableNode;
use common\models\User;
use FeatureContext;
use Webmozart\Assert\Assert;

class GroupsExternalContext extends FeatureContext
{
private User $user;
private string $userEmailAddress = '[email protected]';
private string $userPassword = 'dummy-password-#1';

/**
* @Given a user exists
*/
public function aUserExists()
{
$this->deleteThatTestUser();
$this->createTestUser();
$this->setThatUsersPassword($this->userPassword);
}

private function deleteThatTestUser()
{
$user = User::findByEmail($this->userEmailAddress);
if ($user !== null) {
$didDeleteUser = $user->delete();
Assert::notFalse($didDeleteUser, sprintf(
'Failed to delete existing test user: %s',
join("\n", $user->getFirstErrors())
));
}
}

private function createTestUser()
{
$user = new User([
'email' => $this->userEmailAddress,
'employee_id' => '11111',
'first_name' => 'John',
'last_name' => 'Smith',
'username' => 'john_smith',
]);
$user->scenario = User::SCENARIO_NEW_USER;

$createdNewUser = $user->save();
Assert::true($createdNewUser, sprintf(
'Failed to create test user: %s',
join("\n", $user->getFirstErrors())
));
$user->refresh();

$this->user = $user;
}

private function setThatUsersPassword(string $password)
{
$this->user->scenario = User::SCENARIO_UPDATE_PASSWORD;
$this->user->password = $password;

Assert::true($this->user->save(), sprintf(
"Failed to set the test user's password: %s",
join("\n", $this->user->getFirstErrors())
));
}

/**
* @Given that user's list of groups is :commaSeparatedGroups
*/
public function thatUsersListOfGroupsIs($commaSeparatedGroups)
{
$this->user->groups = $commaSeparatedGroups;
$this->user->scenario = User::SCENARIO_UPDATE_USER;

$savedChanges = $this->user->save();
Assert::true($savedChanges, sprintf(
'Failed to set list of `groups` on test user: %s',
join("\n", $this->user->getFirstErrors())
));
}

/**
* @Given that user's list of external groups is :commaSeparatedExternalGroups
*/
public function thatUsersListOfExternalGroupsIs($commaSeparatedExternalGroups)
{
$this->user->groups_external = $commaSeparatedExternalGroups;
$this->user->scenario = User::SCENARIO_UPDATE_USER;

$savedChanges = $this->user->save();
Assert::true($savedChanges, sprintf(
'Failed to set list of `groups_external` on test user: %s',
join("\n", $this->user->getFirstErrors())
));
}

/**
* @When I sign in as that user
*/
public function iSignInAsThatUser()
{
$dataForTableNode = [
['property', 'value'],
['username', $this->user->username],
['password', $this->userPassword],
];
$this->iProvideTheFollowingValidData(new TableNode($dataForTableNode));
$this->iRequestTheResourceBe('/authentication', 'created');
$this->theResponseStatusCodeShouldBe(200);
}
}
49 changes: 49 additions & 0 deletions application/features/groups-external.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
Feature: Incorporating custom (external) groups in a User's `members` list

Background:
Given the requester is authorized

# Scenarios that belong here in ID Broker:

Scenario: Include external groups in a User's `members` list
Given a user exists
And that user's list of groups is "one,two"
And that user's list of external groups is "app-three,app-four"
When I sign in as that user
Then the response should contain a member array with only these elements:
| element |
| one |
| two |
| app-three |
| app-four |
| {idpName} |

Scenario: Gracefully handle an empty list of groups in a User's `members` list
Given a user exists
And that user's list of groups is ""
And that user's list of external groups is "app-three,app-four"
When I sign in as that user
Then the response should contain a member array with only these elements:
| element |
| app-three |
| app-four |
| {idpName} |

Scenario: Gracefully handle an empty list of external groups in a User's `members` list
Given a user exists
And that user's list of groups is "one,two"
And that user's list of external groups is ""
When I sign in as that user
Then the response should contain a member array with only these elements:
| element |
| one |
| 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
# Scenario: Remove entries not in the synced Google Sheet from the `groups_external` field
# Scenario: Only use entries from the synced Google Sheet that specify this IDP
2 changes: 1 addition & 1 deletion application/run-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ apachectl start
rc=$?; if [[ $rc != 0 ]]; then exit $rc; fi

# Run the feature tests
./vendor/bin/behat --strict
./vendor/bin/behat --strict --stop-on-failure

# If they failed, exit.
rc=$?; if [[ $rc != 0 ]]; then exit $rc; fi
13 changes: 13 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,19 @@ services:
PMA_USER: user
PMA_PASSWORD: pass

phpmyadmintest:
image: phpmyadmin:5
ports:
- "51142:80"
depends_on:
- testdb
env_file:
- ./local.env
environment:
PMA_HOST: testdb
PMA_USER: appfortests
PMA_PASSWORD: appfortests

raml2html:
image: mattjtodd/raml2html
platform: linux/amd64
Expand Down