-
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-1176] Add groups_external
field to User
#352
Merged
forevermatt
merged 22 commits into
develop
from
feature/idp-1176-add-groups-external-field
Aug 20, 2024
Merged
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 42b3213
Implement (failing) test step for setting `groups_external` value
forevermatt 6c6c14a
Use migration to add `user.groups_external` field to database
forevermatt 41702f2
Regenerate the base models (excluding `groups_external` changes)
forevermatt 6aa3d7a
Regenerate UserBase model to include `groups_external` field
forevermatt bd0b0b1
Set `user.groups_external`'s default value to be an empty string
forevermatt db30f68
Switch to fluent syntax (not DB-specific) for defining the new field
forevermatt 3f47bb3
Regenerate UserBase model now that `groups_external` has a default value
forevermatt ffb2674
Get new test successfully signing in as the test user
forevermatt bb5f000
Add phpMyAdmin container for testdb
forevermatt 7dbad6e
Improve groups_external test, implement next (failing) step
forevermatt 11d9cf4
Merge branch 'develop' into feature/idp-1176-add-groups-external-field
forevermatt 54fd8ba
Make groups_external test failure message more informative
forevermatt 9fcaa42
Include groups_external values in a User's `member` list during login
forevermatt 0d43d78
Ensure empty `groups` and empty `groups_external` fields work properly
forevermatt 6543ed4
Merge branch 'develop' into feature/idp-1176-add-groups-external-field
forevermatt 690289e
Customize the label for the new `groups_external` field
forevermatt 4f6e211
Stop running CI/CD tests as soon as one of them fails
forevermatt ca0c7a2
Fix groups_external test to detect an empty string in the `members` list
forevermatt 931f4db
Include the array contents in the failed-test output
forevermatt b608e9f
Remove now-unused new test step
forevermatt 6ce06ac
Avoid adding empty string to `members` when `groups_external` is empty
forevermatt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
23 changes: 23 additions & 0 deletions
23
application/console/migrations/m240813_155757_add_groups_external_field_to_user.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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') | ||
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. The Yii syntax is ugly. 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. PHP syntax is ugly. 😛 |
||
); | ||
} | ||
|
||
public function safeDown() | ||
{ | ||
$this->dropColumn('{{user}}', 'groups_external'); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
114 changes: 114 additions & 0 deletions
114
application/features/bootstrap/GroupsExternalContext.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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: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.
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.
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.
Thanks for discussing it, though 👍