From 9244ad6ea6c13df755b5d34896de290c51c28e0b Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Sun, 11 Feb 2024 14:19:54 +0100 Subject: [PATCH 01/14] Revert "Revert "Rewrite Filterable trait used for Squad filters" (#663)" This reverts commit 7d1013099e242e6b0613d5c9b96009ba13cdb3e4. --- .gitignore | 5 + src/Exceptions/InvalidFilterException.php | 28 ++++ src/Models/Filterable.php | 187 ++++++++-------------- src/Models/QueryGroupBuilder.php | 125 +++++++++++++++ tests/Squads/AllianceRuleTest.php | 40 +++++ tests/Squads/NoRulesTest.php | 12 +- 6 files changed, 271 insertions(+), 126 deletions(-) create mode 100644 src/Exceptions/InvalidFilterException.php create mode 100644 src/Models/QueryGroupBuilder.php diff --git a/.gitignore b/.gitignore index 8b5b2bb7e..c90a0e72e 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,8 @@ .env.php .env .idea/ + +# testing +vendor/ +composer.lock +.phpunit.cache/test-results diff --git a/src/Exceptions/InvalidFilterException.php b/src/Exceptions/InvalidFilterException.php new file mode 100644 index 000000000..79f3e6fe5 --- /dev/null +++ b/src/Exceptions/InvalidFilterException.php @@ -0,0 +1,28 @@ +getFilters(), 'and') && ! property_exists($this->getFilters(), 'or')) return true; - // init a new object based on parameter - $class = get_class($member); + $query = new QueryGroupBuilder($member->newQuery(), true); - return (new $class)::where($member->getKeyName(), $member->id) - ->where(function ($query) { - - // verb will determine what kind of method we have to use (simple andWhere or orWhere) - $verb = property_exists($this->getFilters(), 'and') ? 'whereHas' : 'orWhereHas'; - - // rules will determine all objects and ruleset in the current object root - $rules = property_exists($this->getFilters(), 'and') ? $this->getFilters()->and : $this->getFilters()->or; - - // sort rules by path - $sorted_rules = $this->sortFiltersByRelations($rules); - - // TODO: find a way to handle this using recursive loop and determine common patterns - $sorted_rules->each(function ($rules_group, $path) use ($query, $verb) { - - if (is_int($path)) { - - $parent_verb = $verb == 'whereHas' ? 'where' : 'orWhere'; - - $query->$parent_verb(function ($q2) use ($rules_group, $parent_verb) { - - // all pairs will be group in distinct collection due to previous group by - // as a result, we have to iterate over each members - $rules_group->each(function ($rules) use ($parent_verb, $q2) { - - // determine the match kind for the current pair - // sort all rules from this pair in order to ensure relationship consistency - $group_verb = property_exists($rules, 'and') ? 'whereHas' : 'orWhereHas'; - $rules_group = $this->sortFiltersByRelations(property_exists($rules, 'and') ? - $rules->and : $rules->or); - - $rules_group->each(function ($rules, $path) use ($parent_verb, $group_verb, $q2) { - - // prepare query from current pair group - $q2->$parent_verb(function ($q3) use ($rules, $path, $group_verb) { - $q3->$group_verb($path, function ($q4) use ($rules, $group_verb) { - - // prevent dummy query by encapsulating rules outside relations - $q4->where(function ($q5) use ($rules, $group_verb) { - $this->applyRules($q5, $group_verb, $rules); - }); - }); - }); - }); - }); - }); - - } else { - - $rules = $rules_group; - - // using group by, we've pair all relationships by their top level relation - // $query->whereHas('characters(.*)', function ($sub_query) { ... } - $query->$verb($path, function ($q2) use ($rules, $verb) { - - // override the complete rule group with a global where. - // doing it so will prevent SQL query like - // (users.id = tokens.user_id OR character_id = ? OR character_id = ?) - // when multiple rules are applied on same path. - $q2->where(function ($q3) use ($rules, $verb) { - $this->applyRules($q3, $verb, $rules); - }); - }); - } - }); - })->exists(); - } - - /** - * @param array $rules - * @return \Illuminate\Support\Collection - */ - private function sortFiltersByRelations(array $rules) - { - return collect($rules)->sortBy('path')->groupBy(function ($rule) { - if (! property_exists($rule, 'path')) - return false; - - $relation_members = explode('.', $rule->path); + // make sure we only allow results of the entity we are checking count + $query->where(function (Builder $inner_query) use ($member) { + $inner_query->where($member->getKeyName(), $member->getKey()); + }); - return $relation_members[0]; + // wrap this in an inner query to ensure it is '(correct_entity_check) AND (rule1 AND/OR rule2)' + $query->where(function ($inner_query) { + $this->applyGroup($inner_query, $this->getFilters()); }); + + return $query->getUnderlyingQuery()->exists(); } /** - * @param \Illuminate\Database\Eloquent\Builder $query - * @param string $group_verb - * @param array|\Illuminate\Support\Collection $rules - * @return \Illuminate\Database\Eloquent\Builder + * Applies a filter group to $query. + * + * @param Builder $query the query to add the filter group to + * @param stdClass $group the filter group configuration + * + * @throws InvalidFilterException */ - private function applyRules(Builder $query, string $group_verb, $rules) + private function applyGroup(Builder $query, stdClass $group): void { - $query_operator = $group_verb == 'whereHas' ? 'where' : 'orWhere'; - - if (is_array($rules)) - $rules = collect($rules); + $query_group = new QueryGroupBuilder($query, property_exists($group, 'and')); - $rules->sortBy('path')->groupBy('path')->each(function ($relation_rules, $relation) use ($query, $query_operator) { - if (strpos($relation, '.') !== false) { - $relation = substr($relation, strpos($relation, '.') + 1); + $rules = $query_group->isAndGroup() ? $group->and : $group->or; - $query->whereHas($relation, function ($q2) use ($query_operator, $relation_rules) { - $q2->where(function ($q3) use ($relation_rules, $query_operator) { - foreach ($relation_rules as $index => $rule) { - if ($rule->operator == 'contains') { - $json_operator = $query_operator == 'where' ? 'whereJsonContains' : 'orWhereJsonContains'; - $q3->$json_operator($rule->field, $rule->criteria); - } else { - $q3->$query_operator($rule->field, $rule->operator, $rule->criteria); - } - } - }); - }); + foreach ($rules as $rule){ + // check if this is a nested group or not + if(property_exists($rule, 'path')){ + $this->applyRule($query_group, $rule); } else { - $query->where(function ($q2) use ($relation_rules, $query_operator) { - foreach ($relation_rules as $index => $rule) { - if ($rule->operator == 'contains') { - $json_operator = $query_operator == 'where' ? 'whereJsonContains' : 'orWhereJsonContains'; - $q2->$json_operator($rule->field, $rule->criteria); - } else { - $q2->$query_operator($rule->field, $rule->operator, $rule->criteria); - } - } + // this is a nested group + $query_group->where(function ($group_query) use ($rule) { + $this->applyGroup($group_query, $rule); }); } - }); + } + } - return $query; + /** + * Applies a rule to a query group. + * + * @param QueryGroupBuilder $query the query to add the rule to + * @param stdClass $rule the rule configuration + * + * @throws InvalidFilterException + */ + private function applyRule(QueryGroupBuilder $query, stdClass $rule): void { + // 'is' operator + if($rule->operator === '=' || $rule->operator === '<' || $rule->operator === '>'){ + // normal comparison operations need to relation to exist + $query->whereHas($rule->path, function (Builder $inner_query) use ($rule) { + $inner_query->where($rule->field, $rule->operator, $rule->criteria); + }); + } elseif ($rule->operator === '<>' || $rule->operator === '!=') { + // not equal is special cased since a missing relation is the same as not equal + $query->whereDoesntHave($rule->path, function (Builder $inner_query) use ($rule) { + $inner_query->where($rule->field, $rule->criteria); + }); + } elseif($rule->operator === 'contains'){ + // contains is maybe a misleading name, since it actually checks if json contains a value + $query->whereHas($rule->path, function (Builder $inner_query) use ($rule) { + $inner_query->whereJsonContains($rule->field, $rule->criteria); + }); + } else { + throw new InvalidFilterException(sprintf('Unknown rule operator: \'%s\'', $rule->operator)); + } } } diff --git a/src/Models/QueryGroupBuilder.php b/src/Models/QueryGroupBuilder.php new file mode 100644 index 000000000..e74a8b5e7 --- /dev/null +++ b/src/Models/QueryGroupBuilder.php @@ -0,0 +1,125 @@ +query = $query; + $this->is_and_group = $is_and_group; + } + + /** + * @return bool Returns true when the where clauses are linked by AND + */ + public function isAndGroup(): bool { + return $this->is_and_group; + } + + /** + * @return Builder The underlying query builder used for this group + */ + public function getUnderlyingQuery(): Builder + { + return $this->query; + } + + /** + * Either adds a 'where' or 'orWhere' to the query, depending on if it is an AND linked group or not. + * + * @param Closure $callback a callback to add constraints + * @return $this + * + * @see Builder::where + * @see Builder::orWhere + */ + public function where(Closure $callback): QueryGroupBuilder { + if($this->is_and_group){ + $this->query->where($callback); + } else { + $this->query->orWhere($callback); + } + + return $this; + } + + /** + * Either adds a 'whereHas' or 'orWhereHas' to the query, depending on if it is an AND linked group or not. + * + * @param string $relation the relation to check for existence + * @param Closure $callback a callback to add more constraints + * @return $this + * + * @see Builder::whereHas + * @see Builder::orWhereHas + */ + public function whereHas(string $relation, Closure $callback): QueryGroupBuilder { + if($this->is_and_group){ + $this->query->whereHas($relation, $callback); + } else { + $this->query->orWhereHas($relation, $callback); + } + + return $this; + } + + /** + * Either adds a 'whereDoesntHave' or 'orWhereDoesntHave' to the query, depending on if it is an AND linked group or not. + * + * @param string $relation the relation to check for absence + * @param Closure $callback a callback to add more constraints + * @return $this + * + * @see Builder::whereDoesntHave + * @see Builder::orWhereDoesntHave + */ + public function whereDoesntHave(string $relation, Closure $callback): QueryGroupBuilder { + if($this->is_and_group){ + $this->query->whereDoesntHave($relation, $callback); + } else { + $this->query->orWhereDoesntHave($relation, $callback); + } + + return $this; + } +} diff --git a/tests/Squads/AllianceRuleTest.php b/tests/Squads/AllianceRuleTest.php index cd60cacb0..5a93e477c 100644 --- a/tests/Squads/AllianceRuleTest.php +++ b/tests/Squads/AllianceRuleTest.php @@ -155,4 +155,44 @@ public function testUserHasCharacterInAlliance() $this->assertFalse($squad->isEligible($user)); } } + + /** + * This test checks whether a character from a corp outside an alliance is eligible for a squad with a alliance is not filter. + * In SeAT 4, this was not working properly + */ + public function testCharacterHasNoAllianceWithAllianceIsNotFilter(){ + $squad = new Squad([ + 'name' => 'Testing Squad', + 'description' => 'Some description', + 'type' => 'auto', + 'filters' => json_encode([ + 'and' => [ + [ + 'name' => 'alliance', + 'path' => 'characters.affiliation', + 'field' => 'alliance_id', + 'operator' => '<>', + 'criteria' => 99000000, + 'text' => 'Random Alliance', + ], + ], + ]), + ]); + + $user = User::first(); + + $user->characters->each(function ($character){ + $character->affiliation->update([ + 'alliance_id' => 99000000, + ]); + }); + $this->assertFalse($squad->isEligible($user)); + + $user->characters->each(function ($character){ + $character->affiliation->update([ + 'alliance_id' => null, + ]); + }); + $this->assertTrue($squad->isEligible($user)); + } } diff --git a/tests/Squads/NoRulesTest.php b/tests/Squads/NoRulesTest.php index ce73cd3d1..ce29211aa 100644 --- a/tests/Squads/NoRulesTest.php +++ b/tests/Squads/NoRulesTest.php @@ -27,6 +27,7 @@ use Orchestra\Testbench\TestCase; use Seat\Eveapi\Models\Character\CharacterInfo; use Seat\Eveapi\Models\RefreshToken; +use Seat\Web\Exceptions\InvalidFilterException; use Seat\Web\Models\Squads\Squad; use Seat\Web\Models\User; use Seat\Web\WebServiceProvider; @@ -72,10 +73,10 @@ protected function setUp(): void Event::fake(); - CharacterInfo::factory(50) + CharacterInfo::factory(5) ->create(); - User::factory(10) + User::factory(1) ->create() ->each(function ($user) { CharacterInfo::whereDoesntHave('refresh_token')->get() @@ -99,11 +100,10 @@ public function testUserWithNoRules() ]); // pickup users - $users = User::all(); + $user = User::first(); // ensure no users are eligible - foreach ($users as $user) { - $this->assertTrue($squad->isEligible($user)); - } + $this->assertTrue($squad->isEligible($user), 'Squad without filter is open to anyone'); + } } From 6304b2d39b10bb842e90885055e97197e9e30d52 Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Mon, 15 Jan 2024 19:51:55 +0100 Subject: [PATCH 02/14] add vendor to gitignore From ab710eef87e9b35573312b2d742b65d556e92954 Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Mon, 15 Jan 2024 19:52:42 +0100 Subject: [PATCH 03/14] add unit test for seat 5 CVE --- tests/Squads/SameCharacterAcrossRulesTest.php | 142 ++++++++++++++++++ 1 file changed, 142 insertions(+) create mode 100644 tests/Squads/SameCharacterAcrossRulesTest.php diff --git a/tests/Squads/SameCharacterAcrossRulesTest.php b/tests/Squads/SameCharacterAcrossRulesTest.php new file mode 100644 index 000000000..96cc886b6 --- /dev/null +++ b/tests/Squads/SameCharacterAcrossRulesTest.php @@ -0,0 +1,142 @@ +set('database.default', 'testbench'); + $app['config']->set('database.connections.testbench', [ + 'driver' => 'sqlite', + 'database' => ':memory:', + 'prefix' => '', + ]); + $app['config']->set('database.redis.client', 'mock'); + } + + /** + * @param \Illuminate\Foundation\Application $app + * @return array|string[] + */ + protected function getPackageProviders($app) + { + return [ + RedisMockServiceProvider::class, + WebServiceProvider::class, + ]; + } + + protected function setUp(): void + { + parent::setUp(); + + $this->loadMigrationsFrom(realpath(__DIR__ . '/../database/migrations')); + + Event::fake(); + } + + public function testGroupUsesSameCharacterForAllRules() + { + $CORPORATION_ID = 98681714; + $ROLE = 'Director'; + + // STEP: spawn test squad + $squad = new Squad([ + 'name' => 'Testing Squad', + 'description' => 'Some description', + 'type' => 'auto', + 'filters' => json_encode([ + 'and' => [ + [ + 'name'=>'corporation', + 'path'=>'characters.affiliation', + 'field'=>'corporation_id', + 'criteria'=>strval($CORPORATION_ID), + 'operator'=>'=', + 'text'=>'Backbone Trading Inc' + ], + [ + 'name'=>'role', + 'path'=>'characters.corporation_roles', + 'field'=>'role', + 'criteria'=>$ROLE, + 'operator'=>'=', + 'text'=>$ROLE + ], + ] + ]), + ]); + + // STEP: Create user with characters + // get a user with two or more character + $user = User::factory()->create(); + // get two characters + $character_1 = CharacterInfo::factory()->create(); + $character_2 = CharacterInfo::factory()->create(); + // attach character to user + RefreshToken::factory()->create([ + 'character_id' => $character_1->character_id, + 'user_id' => $user->id, + ]); + RefreshToken::factory()->create([ + 'character_id' => $character_2->character_id, + 'user_id' => $user->id, + ]); + + // attach affiliation to first character + CharacterAffiliation::factory()->create([ + 'corporation_id' => $CORPORATION_ID, + 'character_id' => $character_1->character_id + ]); + + // attach role to second character + CharacterRole::factory()->create([ + 'role' => $ROLE, + 'character_id' => $character_2->character_id + ]); + + $this->assertFalse($squad->isEligible($user)); + } +} From bf8ea0d86c979ca3b489338db756f2bba4987d9f Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Mon, 15 Jan 2024 19:56:59 +0100 Subject: [PATCH 04/14] add unit test for group CVE --- tests/Squads/SameCharacterAcrossRulesTest.php | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/tests/Squads/SameCharacterAcrossRulesTest.php b/tests/Squads/SameCharacterAcrossRulesTest.php index 96cc886b6..de5371c40 100644 --- a/tests/Squads/SameCharacterAcrossRulesTest.php +++ b/tests/Squads/SameCharacterAcrossRulesTest.php @@ -139,4 +139,70 @@ public function testGroupUsesSameCharacterForAllRules() $this->assertFalse($squad->isEligible($user)); } + + public function testSquadUsesSameCharacterAcrossGroups() { + $CORPORATION_ID = 98681714; + $ROLE = 'Director'; + + // STEP: spawn test squad + $squad = new Squad([ + 'name' => 'Testing Squad', + 'description' => 'Some description', + 'type' => 'auto', + 'filters' => json_encode([ + 'and' => [ + [ + 'name'=>'corporation', + 'path'=>'characters.affiliation', + 'field'=>'corporation_id', + 'criteria'=>strval($CORPORATION_ID), + 'operator'=>'=', + 'text'=>'Backbone Trading Inc' + ], + [ + 'and'=>[ + [ + 'name'=>'role', + 'path'=>'characters.corporation_roles', + 'field'=>'role', + 'criteria'=>$ROLE, + 'operator'=>'=', + 'text'=>$ROLE + ], + ], + ], + ] + ]), + ]); + + // STEP: Create user with characters + // get a user with two or more character + $user = User::factory()->create(); + // get two characters + $character_1 = CharacterInfo::factory()->create(); + $character_2 = CharacterInfo::factory()->create(); + // attach character to user + RefreshToken::factory()->create([ + 'character_id' => $character_1->character_id, + 'user_id' => $user->id, + ]); + RefreshToken::factory()->create([ + 'character_id' => $character_2->character_id, + 'user_id' => $user->id, + ]); + + // attach affiliation to first character + CharacterAffiliation::factory()->create([ + 'corporation_id' => $CORPORATION_ID, + 'character_id' => $character_1->character_id + ]); + + // attach role to second character + CharacterRole::factory()->create([ + 'role' => $ROLE, + 'character_id' => $character_2->character_id + ]); + + $this->assertFalse($squad->isEligible($user)); + } } From a606360e260e6cf6699efecd1e3b091a816056e8 Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Mon, 15 Jan 2024 20:07:08 +0100 Subject: [PATCH 05/14] add documentation explaining the test cases --- tests/Squads/SameCharacterAcrossRulesTest.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/Squads/SameCharacterAcrossRulesTest.php b/tests/Squads/SameCharacterAcrossRulesTest.php index de5371c40..d75091fa6 100644 --- a/tests/Squads/SameCharacterAcrossRulesTest.php +++ b/tests/Squads/SameCharacterAcrossRulesTest.php @@ -77,6 +77,15 @@ protected function setUp(): void Event::fake(); } + /** + * The goal of this test is to prevent a CVE that was discovered in SeAT 5. If a group has two rules and is linked + * by AND, a user with two characters shall be checked. One character fulfills the first rule, the second fulfills + * the second rule. As the UI in SeAT asks the user to specify the rules for one character and then SeAT checks + * the user character by character, this user should not be eligible. He has no character that fulfills both + * rules. This test ensures that a user in this situation doesn't have access. + * + * @return void + */ public function testGroupUsesSameCharacterForAllRules() { $CORPORATION_ID = 98681714; @@ -140,6 +149,14 @@ public function testGroupUsesSameCharacterForAllRules() $this->assertFalse($squad->isEligible($user)); } + /** + * This test build upon Seat\Tests\Web\Squads\SameCharacterAcrossRulesTest::testGroupUsesSameCharacterForAllRules + * and additionally check that this also happens across rule groups. For a filter with one rule and another rule + * wrapped inside a group, all linked by AND, the user needs to have a character that fulfills both rules. It is not + * sufficient to have a user with two characters, where each character only fulfills one of the rules. + * + * @return void + */ public function testSquadUsesSameCharacterAcrossGroups() { $CORPORATION_ID = 98681714; $ROLE = 'Director'; From e0bdc62bea66b1dfc12ddfa61179ddf65ed41904 Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Mon, 15 Jan 2024 20:17:54 +0100 Subject: [PATCH 06/14] add test for sso scope rules --- tests/Squads/SsoScopeRuleTest.php | 154 ++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+) create mode 100644 tests/Squads/SsoScopeRuleTest.php diff --git a/tests/Squads/SsoScopeRuleTest.php b/tests/Squads/SsoScopeRuleTest.php new file mode 100644 index 000000000..25564ed3e --- /dev/null +++ b/tests/Squads/SsoScopeRuleTest.php @@ -0,0 +1,154 @@ +set('database.default', 'testbench'); + $app['config']->set('database.connections.testbench', [ + 'driver' => 'sqlite', + 'database' => ':memory:', + 'prefix' => '', + ]); + $app['config']->set('database.redis.client', 'mock'); + } + + /** + * @param \Illuminate\Foundation\Application $app + * @return array|string[] + */ + protected function getPackageProviders($app) + { + return [ + RedisMockServiceProvider::class, + WebServiceProvider::class, + ]; + } + + protected function setUp(): void + { + parent::setUp(); + + $this->loadMigrationsFrom(realpath(__DIR__ . '/../database/migrations')); + + Event::fake(); + + CharacterInfo::factory(50) + ->create(); + + User::factory(10) + ->create() + ->each(function ($user) { + CharacterInfo::whereDoesntHave('refresh_token')->get() + ->random(rand(1, 5))->each(function ($character) use ($user) { + RefreshToken::factory()->create([ + 'character_id' => $character->character_id, + 'user_id' => $user->id, + ]); + }); + }); + } + + public function testUserDoesntHaveScope() + { + // spawn test squad + $squad = new Squad([ + 'name' => 'Testing Squad', + 'description' => 'Some description', + 'type' => 'auto', + 'filters' => json_encode([ + 'and' => [ + [ + 'name' => 'scopes', + 'path' => 'characters.refresh_token', + 'field' => 'scopes', + 'criteria' => 'publicData', + 'operator' => 'contains', + 'text' => 'publicData' + ] + ] + ]) + ]); + + $user = User::first(); + + // remove all scopes + $user->refresh_tokens->each(function ($token) { + $token->scopes = []; + $token->save(); + }); + + $this->assertFalse($squad->isEligible($user)); + } + + public function testUserHasScope() + { + // spawn test squad + $squad = new Squad([ + 'name' => 'Testing Squad', + 'description' => 'Some description', + 'type' => 'auto', + 'filters' => json_encode([ + 'and' => [ + [ + 'name' => 'scopes', + 'path' => 'characters.refresh_token', + 'field' => 'scopes', + 'criteria' => 'publicData', + 'operator' => 'contains', + 'text' => 'publicData' + ] + ] + ]) + ]); + + $user = User::first(); + + // remove all scopes + $user->refresh_tokens->each(function ($token) { + $token->scopes = ['publicData']; + $token->save(); + }); + + $this->assertTrue($squad->isEligible($user)); + } +} From ed40d0b836c583f088ed355194c7407f9a3599b6 Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Mon, 15 Jan 2024 21:58:30 +0100 Subject: [PATCH 07/14] extract unit test from reverted commit From 0d9564da82841753fdd1f8a5c87b7be8f8047dc5 Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Sun, 11 Feb 2024 15:52:03 +0100 Subject: [PATCH 08/14] refactor: isEligible -> isUserEligible, deferred migration --- src/Models/Squads/Squad.php | 36 +++++++++++++++++-- src/Observers/AbstractSquadObserver.php | 12 ++++--- ...06_220254_upgrade_squads_maj4_min4_hf2.php | 15 ++------ src/resources/views/squads/show.blade.php | 2 +- .../AllianceCorporationPairFiltersTest.php | 16 ++++----- tests/Squads/AllianceRuleTest.php | 10 +++--- tests/Squads/CharacterRuleTest.php | 6 ++-- tests/Squads/CorporationRuleTest.php | 6 ++-- tests/Squads/ItemRuleTest.php | 6 ++-- tests/Squads/NoRulesTest.php | 2 +- tests/Squads/RoleRuleTest.php | 6 ++-- tests/Squads/SameCharacterAcrossRulesTest.php | 4 +-- .../SkillAffiliationRulesGroupsTest.php | 8 ++--- tests/Squads/SkillLevelRuleTest.php | 6 ++-- tests/Squads/SkillRuleTest.php | 6 ++-- .../Squads/SkillSkillLevelPairFiltersTest.php | 16 ++++----- tests/Squads/SsoScopeRuleTest.php | 4 +-- tests/Squads/TitleRuleTest.php | 6 ++-- 18 files changed, 95 insertions(+), 72 deletions(-) diff --git a/src/Models/Squads/Squad.php b/src/Models/Squads/Squad.php index 4d4400ef5..e255aac5e 100644 --- a/src/Models/Squads/Squad.php +++ b/src/Models/Squads/Squad.php @@ -27,6 +27,7 @@ use Illuminate\Support\Str; use Intervention\Image\Facades\Image; use LasseRafn\InitialAvatarGenerator\InitialAvatar; +use Seat\Web\Exceptions\InvalidFilterException; use Seat\Web\Http\Scopes\SquadScope; use Seat\Web\Models\Acl\Role; use Seat\Web\Models\Filterable; @@ -78,7 +79,7 @@ protected static function boot() static::addGlobalScope(new SquadScope()); - self::updated(function ($model) { + self::updated(function (Squad $model) { // apply updates only if filters or type has been altered if (! array_key_exists('filters', $model->getChanges()) && @@ -87,7 +88,7 @@ protected static function boot() // kick members which are non longer eligible according to new filters $model->members->each(function ($user) use ($model) { - if (! $model->isEligible($user)) + if (! $model->isUserEligible($user)) $model->members()->detach($user->id); }); @@ -99,13 +100,42 @@ protected static function boot() })->get(); $users->each(function ($user) use ($model) { - if ($model->isEligible($user)) + if ($model->isUserEligible($user)) $model->members()->save($user); }); } }); } + /** + * @throws InvalidFilterException + */ + public function isUserEligible(User $user): bool { + return $this->isEligible($user); + } + + /** + * Checks all users for eligibility. This function is used in migrations after major changes and bugs in the squad + * eligibility code to ensure that we are in a valid state. + * @return void + * @throws InvalidFilterException + */ + public static function recheckAllUsers(): void { + Squad::where('type', 'auto')->get()->each(function (Squad $squad) { + User::chunk(100, function ($users) use ($squad) { + $users->each(function (User $user) use ($squad) { + $is_member = $squad->members()->where('id', $user->id)->exists(); + + if ($is_member && ! $squad->isUserEligible($user)) + $squad->members()->detach($user->id); + + if (! $is_member && $squad->isUserEligible($user)) + $squad->members()->attach($user->id); + }); + }); + }); + } + /** * @return bool */ diff --git a/src/Observers/AbstractSquadObserver.php b/src/Observers/AbstractSquadObserver.php index 8d20243dd..4c54e881f 100644 --- a/src/Observers/AbstractSquadObserver.php +++ b/src/Observers/AbstractSquadObserver.php @@ -23,6 +23,7 @@ namespace Seat\Web\Observers; use Illuminate\Database\Eloquent\Model; +use Seat\Web\Exceptions\InvalidFilterException; use Seat\Web\Models\Squads\Squad; use Seat\Web\Models\User; @@ -44,7 +45,8 @@ abstract protected function findRelatedUser(Model $fired_model): ?User; /** * Update squads to which the user owning model firing the event is member. * - * @param \Illuminate\Database\Eloquent\Model $fired_model The model which fired the catch event + * @param \Illuminate\Database\Eloquent\Model $fired_model The model which fired the catch event + * @throws InvalidFilterException */ protected function updateUserSquads(Model $fired_model) { @@ -61,14 +63,14 @@ protected function updateUserSquads(Model $fired_model) })->get(); // remove the user from squads to which he's non longer eligible. - $member_squads->each(function ($squad) use ($user) { - if (! $squad->isEligible($user)) + $member_squads->each(function (Squad $squad) use ($user) { + if (! $squad->isUserEligible($user)) $squad->members()->detach($user->id); }); // add the user to squads from which he's not already a member. - $other_squads->each(function ($squad) use ($user) { - if ($squad->isEligible($user)) + $other_squads->each(function (Squad $squad) use ($user) { + if ($squad->isUserEligible($user)) $squad->members()->save($user); }); } diff --git a/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php b/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php index 08e3e2ac3..ac28c6405 100644 --- a/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php +++ b/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php @@ -21,6 +21,7 @@ */ use Illuminate\Database\Migrations\Migration; +use Seat\Services\Facades\DeferredMigration; use Seat\Web\Models\Squads\Squad; use Seat\Web\Models\User; @@ -36,18 +37,8 @@ class UpgradeSquadsMaj4Min4Hf2 extends Migration */ public function up() { - Squad::where('type', 'auto')->get()->each(function ($squad) { - User::chunk(100, function ($users) use ($squad) { - $users->each(function ($user) use ($squad) { - $is_member = $squad->members()->where('id', $user->id)->exists(); - - if ($is_member && ! $squad->isEligible($user)) - $squad->members()->detach($user->id); - - if (! $is_member && $squad->isEligible($user)) - $squad->members()->attach($user->id); - }); - }); + DeferredMigration::schedule(function (){ + Squad::recheckAllUsers(); }); } diff --git a/src/resources/views/squads/show.blade.php b/src/resources/views/squads/show.blade.php index d9c478da9..4e54de54f 100644 --- a/src/resources/views/squads/show.blade.php +++ b/src/resources/views/squads/show.blade.php @@ -158,7 +158,7 @@ @endif - @if(! $squad->is_candidate && $squad->type == 'manual' && $squad->isEligible(auth()->user())) + @if(! $squad->is_candidate && $squad->type == 'manual' && $squad->isUserEligible(auth()->user())) @if($squad->moderators->isEmpty())
{!! csrf_field() !!} diff --git a/tests/Squads/AllianceCorporationPairFiltersTest.php b/tests/Squads/AllianceCorporationPairFiltersTest.php index 591e30a20..2fee23bd2 100644 --- a/tests/Squads/AllianceCorporationPairFiltersTest.php +++ b/tests/Squads/AllianceCorporationPairFiltersTest.php @@ -126,7 +126,7 @@ public function testUserDoesNotHaveCharacterInBothAllianceAndCorporation() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -171,8 +171,8 @@ public function testUserHasCharacterInBothAllianceAndCorporation() // ensure no users are eligible foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } @@ -210,7 +210,7 @@ public function testUserDoesNotHaveCharacterInEitherAllianceOrCorporation() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -256,8 +256,8 @@ public function testUserHasCharacterInEitherAllianceOrCorporation() // ensure no users are eligible foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } $reference_character->affiliation->update([ @@ -270,8 +270,8 @@ public function testUserHasCharacterInEitherAllianceOrCorporation() // ensure no users are eligible foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } } diff --git a/tests/Squads/AllianceRuleTest.php b/tests/Squads/AllianceRuleTest.php index 5a93e477c..fa7813043 100644 --- a/tests/Squads/AllianceRuleTest.php +++ b/tests/Squads/AllianceRuleTest.php @@ -118,7 +118,7 @@ public function testUserHasNoCharacterInAlliance() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -151,8 +151,8 @@ public function testUserHasCharacterInAlliance() foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } @@ -186,13 +186,13 @@ public function testCharacterHasNoAllianceWithAllianceIsNotFilter(){ 'alliance_id' => 99000000, ]); }); - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); $user->characters->each(function ($character){ $character->affiliation->update([ 'alliance_id' => null, ]); }); - $this->assertTrue($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)); } } diff --git a/tests/Squads/CharacterRuleTest.php b/tests/Squads/CharacterRuleTest.php index 8388cc6b3..11bdcc8cc 100644 --- a/tests/Squads/CharacterRuleTest.php +++ b/tests/Squads/CharacterRuleTest.php @@ -114,7 +114,7 @@ public function testUserHasNoCharacter() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -146,8 +146,8 @@ public function testUserHasCharacter() foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } } diff --git a/tests/Squads/CorporationRuleTest.php b/tests/Squads/CorporationRuleTest.php index cf2307b8a..987af5d12 100644 --- a/tests/Squads/CorporationRuleTest.php +++ b/tests/Squads/CorporationRuleTest.php @@ -118,7 +118,7 @@ public function testUserHasNoCharacterInCorporation() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -151,8 +151,8 @@ public function testUserHasCharacterInCorporation() foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } } diff --git a/tests/Squads/ItemRuleTest.php b/tests/Squads/ItemRuleTest.php index 53a43447f..1b7578ce5 100644 --- a/tests/Squads/ItemRuleTest.php +++ b/tests/Squads/ItemRuleTest.php @@ -118,7 +118,7 @@ public function testUserHasNoCharacterWithItem() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -151,8 +151,8 @@ public function testUserHasCharacterWithItem() foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } } diff --git a/tests/Squads/NoRulesTest.php b/tests/Squads/NoRulesTest.php index ce29211aa..bbe484a60 100644 --- a/tests/Squads/NoRulesTest.php +++ b/tests/Squads/NoRulesTest.php @@ -103,7 +103,7 @@ public function testUserWithNoRules() $user = User::first(); // ensure no users are eligible - $this->assertTrue($squad->isEligible($user), 'Squad without filter is open to anyone'); + $this->assertTrue($squad->isUserEligible($user), 'Squad without filter is open to anyone'); } } diff --git a/tests/Squads/RoleRuleTest.php b/tests/Squads/RoleRuleTest.php index 8a47b2885..c7e8905f4 100644 --- a/tests/Squads/RoleRuleTest.php +++ b/tests/Squads/RoleRuleTest.php @@ -122,7 +122,7 @@ public function testUserHasNoCharacterWithRole() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -161,8 +161,8 @@ public function testUserHasCharacterWithRole() // ensure no users are eligible foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } } diff --git a/tests/Squads/SameCharacterAcrossRulesTest.php b/tests/Squads/SameCharacterAcrossRulesTest.php index d75091fa6..beadbc4a3 100644 --- a/tests/Squads/SameCharacterAcrossRulesTest.php +++ b/tests/Squads/SameCharacterAcrossRulesTest.php @@ -146,7 +146,7 @@ public function testGroupUsesSameCharacterForAllRules() 'character_id' => $character_2->character_id ]); - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } /** @@ -220,6 +220,6 @@ public function testSquadUsesSameCharacterAcrossGroups() { 'character_id' => $character_2->character_id ]); - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } diff --git a/tests/Squads/SkillAffiliationRulesGroupsTest.php b/tests/Squads/SkillAffiliationRulesGroupsTest.php index 1e7348212..26b3b87ce 100644 --- a/tests/Squads/SkillAffiliationRulesGroupsTest.php +++ b/tests/Squads/SkillAffiliationRulesGroupsTest.php @@ -152,7 +152,7 @@ public function testUserDoNotMeetConditions() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -218,20 +218,20 @@ public function testUserMeetConditions() 'corporation_id' => 98541700, ]); - $this->assertFalse($squad->isEligible($reference_user)); + $this->assertFalse($squad->isUserEligible($reference_user)); $reference_character->affiliation->update([ 'alliance_id' => 99000000, 'corporation_id' => 98541699, ]); - $this->assertFalse($squad->isEligible($reference_user)); + $this->assertFalse($squad->isUserEligible($reference_user)); $reference_character->affiliation->update([ 'alliance_id' => 99000000, 'corporation_id' => 98541700, ]); - $this->assertTrue($squad->isEligible($reference_user)); + $this->assertTrue($squad->isUserEligible($reference_user)); } } diff --git a/tests/Squads/SkillLevelRuleTest.php b/tests/Squads/SkillLevelRuleTest.php index 7bebbdc2b..36b7711b1 100644 --- a/tests/Squads/SkillLevelRuleTest.php +++ b/tests/Squads/SkillLevelRuleTest.php @@ -118,7 +118,7 @@ public function testUserHasNoCharacterWithShillLevel() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -154,8 +154,8 @@ public function testUserHasCharacterWithSkillLevel() // ensure no users are eligible foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } } diff --git a/tests/Squads/SkillRuleTest.php b/tests/Squads/SkillRuleTest.php index 74da04c84..73a8951a2 100644 --- a/tests/Squads/SkillRuleTest.php +++ b/tests/Squads/SkillRuleTest.php @@ -118,7 +118,7 @@ public function testUserHasNoCharacterWithSkill() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -154,8 +154,8 @@ public function testUserHasCharacterWithSkill() // ensure no users are eligible foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } } diff --git a/tests/Squads/SkillSkillLevelPairFiltersTest.php b/tests/Squads/SkillSkillLevelPairFiltersTest.php index 18765e8ec..c98bd53f4 100644 --- a/tests/Squads/SkillSkillLevelPairFiltersTest.php +++ b/tests/Squads/SkillSkillLevelPairFiltersTest.php @@ -126,7 +126,7 @@ public function testUserHasNoCharacterWithSkillAndSkillLevel() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -171,8 +171,8 @@ public function testUserHasCharacterWithSkillAndSkillLevel() // ensure no users are eligible foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } @@ -210,7 +210,7 @@ public function testUserHasNoCharacterWithSkillOrSkillLevel() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -256,8 +256,8 @@ public function testUserHasCharacterWithSkillOrSkillLevel() // ensure no users are eligible foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } $reference_character->skills->first()->update([ @@ -271,8 +271,8 @@ public function testUserHasCharacterWithSkillOrSkillLevel() // ensure no users are eligible foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } } diff --git a/tests/Squads/SsoScopeRuleTest.php b/tests/Squads/SsoScopeRuleTest.php index 25564ed3e..d4cea0d4f 100644 --- a/tests/Squads/SsoScopeRuleTest.php +++ b/tests/Squads/SsoScopeRuleTest.php @@ -117,7 +117,7 @@ public function testUserDoesntHaveScope() $token->save(); }); - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } public function testUserHasScope() @@ -149,6 +149,6 @@ public function testUserHasScope() $token->save(); }); - $this->assertTrue($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)); } } diff --git a/tests/Squads/TitleRuleTest.php b/tests/Squads/TitleRuleTest.php index 0a47ef919..4cc518a59 100644 --- a/tests/Squads/TitleRuleTest.php +++ b/tests/Squads/TitleRuleTest.php @@ -138,7 +138,7 @@ public function testUserHasNoCharacterWithTitle() // ensure no users are eligible foreach ($users as $user) { - $this->assertFalse($squad->isEligible($user)); + $this->assertFalse($squad->isUserEligible($user)); } } @@ -179,8 +179,8 @@ public function testUserHasCharacterWithTitle() // ensure no users are eligible foreach ($users as $user) { $user->id == $reference_user->id ? - $this->assertTrue($squad->isEligible($user)) : - $this->assertFalse($squad->isEligible($user)); + $this->assertTrue($squad->isUserEligible($user)) : + $this->assertFalse($squad->isUserEligible($user)); } } } From 3616a395e217c1fea3f1cc1a0b4a335477ee15a1 Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Sun, 11 Feb 2024 17:52:44 +0100 Subject: [PATCH 09/14] refactor: use character for squad eligibility --- src/Models/QueryGroupBuilder.php | 24 +++- src/Models/Squads/Squad.php | 7 +- ...2024_02_11_155433_update_squad_filters.php | 105 ++++++++++++++++++ src/resources/views/squads/edit.blade.php | 16 +-- 4 files changed, 139 insertions(+), 13 deletions(-) create mode 100644 src/database/migrations/2024_02_11_155433_update_squad_filters.php diff --git a/src/Models/QueryGroupBuilder.php b/src/Models/QueryGroupBuilder.php index e74a8b5e7..6d702fcae 100644 --- a/src/Models/QueryGroupBuilder.php +++ b/src/Models/QueryGroupBuilder.php @@ -95,9 +95,17 @@ public function where(Closure $callback): QueryGroupBuilder { */ public function whereHas(string $relation, Closure $callback): QueryGroupBuilder { if($this->is_and_group){ - $this->query->whereHas($relation, $callback); + if($relation !== '') { + $this->query->whereHas($relation, $callback); + } else { + $this->query->where($callback); + } } else { - $this->query->orWhereHas($relation, $callback); + if($relation !== '') { + $this->query->orWhereHas($relation, $callback); + } else { + $this->query->orWhere($callback); + } } return $this; @@ -115,9 +123,17 @@ public function whereHas(string $relation, Closure $callback): QueryGroupBuilder */ public function whereDoesntHave(string $relation, Closure $callback): QueryGroupBuilder { if($this->is_and_group){ - $this->query->whereDoesntHave($relation, $callback); + if($relation !== '') { + $this->query->whereDoesntHave($relation, $callback); + } else { + $this->query->whereNot($callback); + } } else { - $this->query->orWhereDoesntHave($relation, $callback); + if($relation !== '') { + $this->query->orWhereDoesntHave($relation, $callback); + } else { + $this->query->orWhereNot($callback); + } } return $this; diff --git a/src/Models/Squads/Squad.php b/src/Models/Squads/Squad.php index e255aac5e..8af396029 100644 --- a/src/Models/Squads/Squad.php +++ b/src/Models/Squads/Squad.php @@ -111,7 +111,12 @@ protected static function boot() * @throws InvalidFilterException */ public function isUserEligible(User $user): bool { - return $this->isEligible($user); + // Check all user characters. If any character is eligible, the user is eligible too + foreach ($user->characters as $character){ + if($this->isEligible($character)) return true; + } + // no eligible character found, so the user is not eligible + return false; } /** diff --git a/src/database/migrations/2024_02_11_155433_update_squad_filters.php b/src/database/migrations/2024_02_11_155433_update_squad_filters.php new file mode 100644 index 000000000..5b2fd1794 --- /dev/null +++ b/src/database/migrations/2024_02_11_155433_update_squad_filters.php @@ -0,0 +1,105 @@ +select('id','filters')->chunkById(50, function ($squads){ + foreach ($squads as $squad) { + $this->updateSquad($squad, function ($path) { + // refresh tokens are a special case, since they don't go over character. + // However, they also work over character when a plural s is removed + if($path === 'refresh_tokens') return 'refresh_token'; + + $parts = explode('.',$path); + + logger()->error(json_encode($parts).json_encode(array_slice($parts,1))); + + if($parts[0] !== 'characters' ) { + throw new Exception('Cannot migrate squad filter: filter path doesn\'t go over character'); + } + + + return implode('.',array_slice($parts,1)); + }); + } + }); + + // since the squad filter change with this migration, we have to recompute the eligibility of everyone + DeferredMigration::schedule(function (){ + Squad::recheckAllUsers(); + }); + } + + private function updateSquad($squad, $callback) { + $filter = json_decode($squad->filters); + $this->updateFilter($filter, $callback); + DB::table('squads') + ->where('id',$squad->id) + ->update([ + 'filters' => json_encode($filter) + ]); + } + + private function updateFilter($filter, $callback) { + if(property_exists($filter,'and')){ + foreach ($filter->and as $rule) { + $this->updateFilter($rule, $callback); + } + } else if (property_exists($filter,'or')){ + foreach ($filter->or as $rule) { + $this->updateFilter($rule, $callback); + } + } else if (property_exists($filter,'path')){ + $filter->path = $callback($filter->path); + } + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { +// DB::table('squads')->select('id','filters')->chunkById(50, function ($squads){ +// foreach ($squads as $squad) { +// $this->updateSquad($squad, function ($path) { +// // we deliberately ignore the refresh_tokens special case since keeping it over character fixes a security issue +// return sprintf('characters.%s',$path); +// }); +// } +// }); + } +}; diff --git a/src/resources/views/squads/edit.blade.php b/src/resources/views/squads/edit.blade.php index a81a4a498..05c6fbec4 100644 --- a/src/resources/views/squads/edit.blade.php +++ b/src/resources/views/squads/edit.blade.php @@ -85,14 +85,14 @@ @include('web::components.filters.modals.filters.filters', [ 'filters' => [ (object) ['name' => 'scopes', 'src' => route('seatcore::fastlookup.scopes'), 'path' => 'refresh_tokens', 'field' => 'scopes', 'label' => 'Scopes'], - (object) ['name' => 'character', 'src' => route('seatcore::fastlookup.characters'), 'path' => 'characters', 'field' => 'character_infos.character_id', 'label' => 'Character'], - (object) ['name' => 'title', 'src' => route('seatcore::fastlookup.titles'), 'path' => 'characters.titles', 'field' => 'id', 'label' => 'Title'], - (object) ['name' => 'corporation', 'src' => route('seatcore::fastlookup.corporations'), 'path' => 'characters.affiliation', 'field' => 'corporation_id', 'label' => 'Corporation'], - (object) ['name' => 'alliance', 'src' => route('seatcore::fastlookup.alliances'), 'path' => 'characters.affiliation', 'field' => 'alliance_id', 'label' => 'Alliance'], - (object) ['name' => 'skill', 'src' => route('seatcore::fastlookup.skills'), 'path' => 'characters.skills', 'field' => 'skill_id', 'label' => 'Skill'], - (object) ['name' => 'skill_level', 'src' => [['id' => 1, 'text' => 'Level 1'], ['id' => 2, 'text' => 'Level 2'], ['id' => 3, 'text' => 'Level 3'], ['id' => 4, 'text' => 'Level 4'], ['id' => 5, 'text' => 'Level 5']], 'path' => 'characters.skills', 'field' => 'trained_skill_level', 'label' => 'Skill Level'], - (object) ['name' => 'type', 'src' => route('seatcore::fastlookup.items'), 'path' => 'characters.assets', 'field' => 'type_id', 'label' => 'Item'], - (object) ['name' => 'role', 'src' => route('seatcore::fastlookup.roles'), 'path' => 'characters.corporation_roles', 'field' => 'role', 'label' => 'Role'], + (object) ['name' => 'character', 'src' => route('seatcore::fastlookup.characters'), 'path' => '', 'field' => 'character_id', 'label' => 'Character'], + (object) ['name' => 'title', 'src' => route('seatcore::fastlookup.titles'), 'path' => 'titles', 'field' => 'id', 'label' => 'Title'], + (object) ['name' => 'corporation', 'src' => route('seatcore::fastlookup.corporations'), 'path' => 'affiliation', 'field' => 'corporation_id', 'label' => 'Corporation'], + (object) ['name' => 'alliance', 'src' => route('seatcore::fastlookup.alliances'), 'path' => 'affiliation', 'field' => 'alliance_id', 'label' => 'Alliance'], + (object) ['name' => 'skill', 'src' => route('seatcore::fastlookup.skills'), 'path' => 'skills', 'field' => 'skill_id', 'label' => 'Skill'], + (object) ['name' => 'skill_level', 'src' => [['id' => 1, 'text' => 'Level 1'], ['id' => 2, 'text' => 'Level 2'], ['id' => 3, 'text' => 'Level 3'], ['id' => 4, 'text' => 'Level 4'], ['id' => 5, 'text' => 'Level 5']], 'path' => 'skills', 'field' => 'trained_skill_level', 'label' => 'Skill Level'], + (object) ['name' => 'type', 'src' => route('seatcore::fastlookup.items'), 'path' => 'assets', 'field' => 'type_id', 'label' => 'Item'], + (object) ['name' => 'role', 'src' => route('seatcore::fastlookup.roles'), 'path' => 'corporation_roles', 'field' => 'role', 'label' => 'Role'], ], ]) @endsection From aae186f38ca4539a86dc2a054756c80267c1ff6d Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Sun, 11 Feb 2024 17:57:29 +0100 Subject: [PATCH 10/14] update test cases --- .../AllianceCorporationPairFiltersTest.php | 16 ++++++++-------- tests/Squads/AllianceRuleTest.php | 6 +++--- tests/Squads/CharacterRuleTest.php | 4 ++-- tests/Squads/CorporationRuleTest.php | 4 ++-- tests/Squads/ItemRuleTest.php | 4 ++-- tests/Squads/RoleRuleTest.php | 4 ++-- tests/Squads/SameCharacterAcrossRulesTest.php | 8 ++++---- tests/Squads/SkillAffiliationRulesGroupsTest.php | 16 ++++++++-------- tests/Squads/SkillLevelRuleTest.php | 4 ++-- tests/Squads/SkillRuleTest.php | 4 ++-- tests/Squads/SkillSkillLevelPairFiltersTest.php | 16 ++++++++-------- tests/Squads/SsoScopeRuleTest.php | 4 ++-- tests/Squads/TitleRuleTest.php | 4 ++-- 13 files changed, 47 insertions(+), 47 deletions(-) diff --git a/tests/Squads/AllianceCorporationPairFiltersTest.php b/tests/Squads/AllianceCorporationPairFiltersTest.php index 2fee23bd2..84db5b446 100644 --- a/tests/Squads/AllianceCorporationPairFiltersTest.php +++ b/tests/Squads/AllianceCorporationPairFiltersTest.php @@ -103,7 +103,7 @@ public function testUserDoesNotHaveCharacterInBothAllianceAndCorporation() 'and' => [ [ 'name' => 'alliance', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'alliance_id', 'operator' => '=', 'criteria' => 99000050, @@ -111,7 +111,7 @@ public function testUserDoesNotHaveCharacterInBothAllianceAndCorporation() ], [ 'name' => 'corporation', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'corporation_id', 'operator' => '=', 'criteria' => 98541700, @@ -141,7 +141,7 @@ public function testUserHasCharacterInBothAllianceAndCorporation() 'and' => [ [ 'name' => 'alliance', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'alliance_id', 'operator' => '=', 'criteria' => 99000050, @@ -149,7 +149,7 @@ public function testUserHasCharacterInBothAllianceAndCorporation() ], [ 'name' => 'corporation', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'corporation_id', 'operator' => '=', 'criteria' => 98541700, @@ -187,7 +187,7 @@ public function testUserDoesNotHaveCharacterInEitherAllianceOrCorporation() 'or' => [ [ 'name' => 'alliance', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'alliance_id', 'operator' => '=', 'criteria' => 99000050, @@ -195,7 +195,7 @@ public function testUserDoesNotHaveCharacterInEitherAllianceOrCorporation() ], [ 'name' => 'corporation', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'corporation_id', 'operator' => '=', 'criteria' => 98541700, @@ -225,7 +225,7 @@ public function testUserHasCharacterInEitherAllianceOrCorporation() 'or' => [ [ 'name' => 'alliance', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'alliance_id', 'operator' => '=', 'criteria' => 99000050, @@ -233,7 +233,7 @@ public function testUserHasCharacterInEitherAllianceOrCorporation() ], [ 'name' => 'corporation', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'corporation_id', 'operator' => '=', 'criteria' => 98541700, diff --git a/tests/Squads/AllianceRuleTest.php b/tests/Squads/AllianceRuleTest.php index fa7813043..89aa1eb7a 100644 --- a/tests/Squads/AllianceRuleTest.php +++ b/tests/Squads/AllianceRuleTest.php @@ -103,7 +103,7 @@ public function testUserHasNoCharacterInAlliance() 'and' => [ [ 'name' => 'alliance', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'alliance_id', 'operator' => '=', 'criteria' => 99000000, @@ -133,7 +133,7 @@ public function testUserHasCharacterInAlliance() 'and' => [ [ 'name' => 'alliance', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'alliance_id', 'operator' => '=', 'criteria' => 99000000, @@ -169,7 +169,7 @@ public function testCharacterHasNoAllianceWithAllianceIsNotFilter(){ 'and' => [ [ 'name' => 'alliance', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'alliance_id', 'operator' => '<>', 'criteria' => 99000000, diff --git a/tests/Squads/CharacterRuleTest.php b/tests/Squads/CharacterRuleTest.php index 11bdcc8cc..65965e5de 100644 --- a/tests/Squads/CharacterRuleTest.php +++ b/tests/Squads/CharacterRuleTest.php @@ -99,7 +99,7 @@ public function testUserHasNoCharacter() 'and' => [ [ 'name' => 'character', - 'path' => 'characters', + 'path' => '', 'field' => 'character_infos.character_id', 'operator' => '=', 'criteria' => 90174490, @@ -132,7 +132,7 @@ public function testUserHasCharacter() 'and' => [ [ 'name' => 'character', - 'path' => 'characters', + 'path' => '', 'field' => 'character_infos.character_id', 'operator' => '=', 'criteria' => $reference_user->characters->first()->character_id, diff --git a/tests/Squads/CorporationRuleTest.php b/tests/Squads/CorporationRuleTest.php index 987af5d12..30ac75977 100644 --- a/tests/Squads/CorporationRuleTest.php +++ b/tests/Squads/CorporationRuleTest.php @@ -103,7 +103,7 @@ public function testUserHasNoCharacterInCorporation() 'and' => [ [ 'name' => 'corporation', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'corporation_id', 'operator' => '=', 'criteria' => 98541700, @@ -133,7 +133,7 @@ public function testUserHasCharacterInCorporation() 'and' => [ [ 'name' => 'corporation', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'corporation_id', 'operator' => '=', 'criteria' => 98541700, diff --git a/tests/Squads/ItemRuleTest.php b/tests/Squads/ItemRuleTest.php index 1b7578ce5..c1d3f4432 100644 --- a/tests/Squads/ItemRuleTest.php +++ b/tests/Squads/ItemRuleTest.php @@ -103,7 +103,7 @@ public function testUserHasNoCharacterWithItem() 'and' => [ [ 'name' => 'type', - 'path' => 'characters.assets', + 'path' => 'assets', 'field' => 'type_id', 'operator' => '=', 'criteria' => 2160, @@ -133,7 +133,7 @@ public function testUserHasCharacterWithItem() 'and' => [ [ 'name' => 'type', - 'path' => 'characters.assets', + 'path' => 'assets', 'field' => 'type_id', 'operator' => '=', 'criteria' => 2160, diff --git a/tests/Squads/RoleRuleTest.php b/tests/Squads/RoleRuleTest.php index c7e8905f4..0b5d0a5d6 100644 --- a/tests/Squads/RoleRuleTest.php +++ b/tests/Squads/RoleRuleTest.php @@ -107,7 +107,7 @@ public function testUserHasNoCharacterWithRole() 'and' => [ [ 'name' => 'role', - 'path' => 'characters.corporation_roles', + 'path' => 'corporation_roles', 'field' => 'role', 'operator' => '=', 'criteria' => 'role_1000', @@ -137,7 +137,7 @@ public function testUserHasCharacterWithRole() 'and' => [ [ 'name' => 'role', - 'path' => 'characters.corporation_roles', + 'path' => 'corporation_roles', 'field' => 'role', 'operator' => '=', 'criteria' => 'role_1000', diff --git a/tests/Squads/SameCharacterAcrossRulesTest.php b/tests/Squads/SameCharacterAcrossRulesTest.php index beadbc4a3..8eaed60e7 100644 --- a/tests/Squads/SameCharacterAcrossRulesTest.php +++ b/tests/Squads/SameCharacterAcrossRulesTest.php @@ -100,7 +100,7 @@ public function testGroupUsesSameCharacterForAllRules() 'and' => [ [ 'name'=>'corporation', - 'path'=>'characters.affiliation', + 'path'=>'affiliation', 'field'=>'corporation_id', 'criteria'=>strval($CORPORATION_ID), 'operator'=>'=', @@ -108,7 +108,7 @@ public function testGroupUsesSameCharacterForAllRules() ], [ 'name'=>'role', - 'path'=>'characters.corporation_roles', + 'path'=>'corporation_roles', 'field'=>'role', 'criteria'=>$ROLE, 'operator'=>'=', @@ -170,7 +170,7 @@ public function testSquadUsesSameCharacterAcrossGroups() { 'and' => [ [ 'name'=>'corporation', - 'path'=>'characters.affiliation', + 'path'=>'affiliation', 'field'=>'corporation_id', 'criteria'=>strval($CORPORATION_ID), 'operator'=>'=', @@ -180,7 +180,7 @@ public function testSquadUsesSameCharacterAcrossGroups() { 'and'=>[ [ 'name'=>'role', - 'path'=>'characters.corporation_roles', + 'path'=>'corporation_roles', 'field'=>'role', 'criteria'=>$ROLE, 'operator'=>'=', diff --git a/tests/Squads/SkillAffiliationRulesGroupsTest.php b/tests/Squads/SkillAffiliationRulesGroupsTest.php index 26b3b87ce..f74de13d4 100644 --- a/tests/Squads/SkillAffiliationRulesGroupsTest.php +++ b/tests/Squads/SkillAffiliationRulesGroupsTest.php @@ -107,7 +107,7 @@ public function testUserDoNotMeetConditions() 'and' => [ [ 'name' => 'skill', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'skill_id', 'operator' => '=', 'criteria' => 3350, @@ -115,7 +115,7 @@ public function testUserDoNotMeetConditions() ], [ 'name' => 'skill level', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'trained_skill_level', 'operator' => '>', 'criteria' => 5, @@ -127,7 +127,7 @@ public function testUserDoNotMeetConditions() 'and' => [ [ 'name' => 'corporation', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'corporation_id', 'operator' => '=', 'criteria' => 98541700, @@ -135,7 +135,7 @@ public function testUserDoNotMeetConditions() ], [ 'name' => 'alliance', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'alliance_id', 'operator' => '=', 'criteria' => 99000000, @@ -169,7 +169,7 @@ public function testUserMeetConditions() 'and' => [ [ 'name' => 'skill', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'skill_id', 'operator' => '=', 'criteria' => 3350, @@ -177,7 +177,7 @@ public function testUserMeetConditions() ], [ 'name' => 'skill level', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'trained_skill_level', 'operator' => '>', 'criteria' => 5, @@ -189,7 +189,7 @@ public function testUserMeetConditions() 'and' => [ [ 'name' => 'corporation', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'corporation_id', 'operator' => '=', 'criteria' => 98541700, @@ -197,7 +197,7 @@ public function testUserMeetConditions() ], [ 'name' => 'alliance', - 'path' => 'characters.affiliation', + 'path' => 'affiliation', 'field' => 'alliance_id', 'operator' => '=', 'criteria' => 99000000, diff --git a/tests/Squads/SkillLevelRuleTest.php b/tests/Squads/SkillLevelRuleTest.php index 36b7711b1..a33afdaad 100644 --- a/tests/Squads/SkillLevelRuleTest.php +++ b/tests/Squads/SkillLevelRuleTest.php @@ -103,7 +103,7 @@ public function testUserHasNoCharacterWithShillLevel() 'and' => [ [ 'name' => 'skill level', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'trained_skill_level', 'operator' => '>', 'criteria' => 5, @@ -133,7 +133,7 @@ public function testUserHasCharacterWithSkillLevel() 'and' => [ [ 'name' => 'skill level', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'trained_skill_level', 'operator' => '>', 'criteria' => 4, diff --git a/tests/Squads/SkillRuleTest.php b/tests/Squads/SkillRuleTest.php index 73a8951a2..2b4a33464 100644 --- a/tests/Squads/SkillRuleTest.php +++ b/tests/Squads/SkillRuleTest.php @@ -103,7 +103,7 @@ public function testUserHasNoCharacterWithSkill() 'and' => [ [ 'name' => 'skill', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'skill_id', 'operator' => '=', 'criteria' => 3350, @@ -133,7 +133,7 @@ public function testUserHasCharacterWithSkill() 'and' => [ [ 'name' => 'skill', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'skill_id', 'operator' => '=', 'criteria' => 3350, diff --git a/tests/Squads/SkillSkillLevelPairFiltersTest.php b/tests/Squads/SkillSkillLevelPairFiltersTest.php index c98bd53f4..d4a9d6872 100644 --- a/tests/Squads/SkillSkillLevelPairFiltersTest.php +++ b/tests/Squads/SkillSkillLevelPairFiltersTest.php @@ -103,7 +103,7 @@ public function testUserHasNoCharacterWithSkillAndSkillLevel() 'and' => [ [ 'name' => 'skill', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'skill_id', 'operator' => '=', 'criteria' => 3350, @@ -111,7 +111,7 @@ public function testUserHasNoCharacterWithSkillAndSkillLevel() ], [ 'name' => 'skill level', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'trained_skill_level', 'operator' => '>', 'criteria' => 4, @@ -141,7 +141,7 @@ public function testUserHasCharacterWithSkillAndSkillLevel() 'and' => [ [ 'name' => 'skill', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'skill_id', 'operator' => '=', 'criteria' => 3350, @@ -149,7 +149,7 @@ public function testUserHasCharacterWithSkillAndSkillLevel() ], [ 'name' => 'skill level', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'trained_skill_level', 'operator' => '>', 'criteria' => 4, @@ -187,7 +187,7 @@ public function testUserHasNoCharacterWithSkillOrSkillLevel() 'or' => [ [ 'name' => 'skill', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'skill_id', 'operator' => '=', 'criteria' => 3350, @@ -195,7 +195,7 @@ public function testUserHasNoCharacterWithSkillOrSkillLevel() ], [ 'name' => 'skill level', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'trained_skill_level', 'operator' => '>', 'criteria' => 4, @@ -225,7 +225,7 @@ public function testUserHasCharacterWithSkillOrSkillLevel() 'or' => [ [ 'name' => 'skill', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'skill_id', 'operator' => '=', 'criteria' => 3350, @@ -233,7 +233,7 @@ public function testUserHasCharacterWithSkillOrSkillLevel() ], [ 'name' => 'skill level', - 'path' => 'characters.skills', + 'path' => 'skills', 'field' => 'trained_skill_level', 'operator' => '>', 'criteria' => 4, diff --git a/tests/Squads/SsoScopeRuleTest.php b/tests/Squads/SsoScopeRuleTest.php index d4cea0d4f..6fda01ca7 100644 --- a/tests/Squads/SsoScopeRuleTest.php +++ b/tests/Squads/SsoScopeRuleTest.php @@ -99,7 +99,7 @@ public function testUserDoesntHaveScope() 'and' => [ [ 'name' => 'scopes', - 'path' => 'characters.refresh_token', + 'path' => 'refresh_token', 'field' => 'scopes', 'criteria' => 'publicData', 'operator' => 'contains', @@ -131,7 +131,7 @@ public function testUserHasScope() 'and' => [ [ 'name' => 'scopes', - 'path' => 'characters.refresh_token', + 'path' => 'refresh_token', 'field' => 'scopes', 'criteria' => 'publicData', 'operator' => 'contains', diff --git a/tests/Squads/TitleRuleTest.php b/tests/Squads/TitleRuleTest.php index 4cc518a59..5eeb8c244 100644 --- a/tests/Squads/TitleRuleTest.php +++ b/tests/Squads/TitleRuleTest.php @@ -123,7 +123,7 @@ public function testUserHasNoCharacterWithTitle() 'and' => [ [ 'name' => 'title', - 'path' => 'characters.titles', + 'path' => 'titles', 'field' => 'name', 'operator' => '=', 'criteria' => 'id', @@ -163,7 +163,7 @@ public function testUserHasCharacterWithTitle() 'and' => [ [ 'name' => 'title', - 'path' => 'characters.titles', + 'path' => 'titles', 'field' => 'id', 'operator' => '=', 'criteria' => $reference_title->id, From def1ff6ba7967174aa9341ee6f812f49c17722ff Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Sun, 11 Feb 2024 18:08:22 +0100 Subject: [PATCH 11/14] fix migration --- ...2024_02_11_155433_update_squad_filters.php | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/database/migrations/2024_02_11_155433_update_squad_filters.php b/src/database/migrations/2024_02_11_155433_update_squad_filters.php index 5b2fd1794..7af8c807a 100644 --- a/src/database/migrations/2024_02_11_155433_update_squad_filters.php +++ b/src/database/migrations/2024_02_11_155433_update_squad_filters.php @@ -44,13 +44,10 @@ public function up() $parts = explode('.',$path); - logger()->error(json_encode($parts).json_encode(array_slice($parts,1))); - if($parts[0] !== 'characters' ) { throw new Exception('Cannot migrate squad filter: filter path doesn\'t go over character'); } - return implode('.',array_slice($parts,1)); }); } @@ -93,13 +90,15 @@ private function updateFilter($filter, $callback) { */ public function down() { -// DB::table('squads')->select('id','filters')->chunkById(50, function ($squads){ -// foreach ($squads as $squad) { -// $this->updateSquad($squad, function ($path) { -// // we deliberately ignore the refresh_tokens special case since keeping it over character fixes a security issue -// return sprintf('characters.%s',$path); -// }); -// } -// }); + DB::table('squads')->select('id','filters')->chunkById(50, function ($squads){ + foreach ($squads as $squad) { + $this->updateSquad($squad, function ($path) { + if($path === '') return 'characters'; + + // we deliberately ignore the refresh_tokens special case since keeping it over character fixes a security issue + return sprintf('characters.%s',$path); + }); + } + }); } }; From 6d361bc58580db1520af81fef1928b78411bcd9a Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Thu, 15 Feb 2024 09:25:33 +0100 Subject: [PATCH 12/14] bump dependency --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 2b53888a5..b7cbff3ac 100644 --- a/composer.json +++ b/composer.json @@ -32,7 +32,7 @@ "erusev/parsedown": "^1.7", "eveseat/eseye": "^3.0", "eveseat/eveapi": "^5.0", - "eveseat/services": "^5.0", + "eveseat/services": "^5.0.3", "guzzlehttp/guzzle": "^7.0", "intervention/image": "^2.0", "laravel/framework": "^10.0", From 01ff432c2fd2cf130781f533edc88856f0d9d4cf Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Tue, 5 Mar 2024 19:06:46 +0100 Subject: [PATCH 13/14] fix membership recomputation --- src/Models/Squads/Squad.php | 60 +++++++++---------- ...06_220254_upgrade_squads_maj4_min4_hf2.php | 2 +- ...2024_02_11_155433_update_squad_filters.php | 2 +- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/src/Models/Squads/Squad.php b/src/Models/Squads/Squad.php index 8af396029..7e25fd868 100644 --- a/src/Models/Squads/Squad.php +++ b/src/Models/Squads/Squad.php @@ -86,24 +86,7 @@ protected static function boot() ! array_key_exists('type', $model->getChanges())) return; - // kick members which are non longer eligible according to new filters - $model->members->each(function ($user) use ($model) { - if (! $model->isUserEligible($user)) - $model->members()->detach($user->id); - }); - - // invite members which are eligible according to new filters (only for auto squads) - if ($model->type == 'auto') { - $users = User::standard() - ->whereDoesntHave('squads', function (Builder $query) use ($model) { - $query->where('id', $model->id); - })->get(); - - $users->each(function ($user) use ($model) { - if ($model->isUserEligible($user)) - $model->members()->save($user); - }); - } + $model->recomputeSquadMemberships(); }); } @@ -120,24 +103,41 @@ public function isUserEligible(User $user): bool { } /** - * Checks all users for eligibility. This function is used in migrations after major changes and bugs in the squad - * eligibility code to ensure that we are in a valid state. + * Checks if the members of this squad are still eligible and if new members have to be added. + * This function is typically called after changes in the squad configuration or in a deferred migration if the behaviour of the squad eligibility logic changes * @return void * @throws InvalidFilterException */ - public static function recheckAllUsers(): void { - Squad::where('type', 'auto')->get()->each(function (Squad $squad) { - User::chunk(100, function ($users) use ($squad) { - $users->each(function (User $user) use ($squad) { - $is_member = $squad->members()->where('id', $user->id)->exists(); + private function recomputeSquadMemberships(): void { + // kick members which are non longer eligible according to new filters + $this->members->each(function ($user) { + if (! $this->isUserEligible($user)) + $this->members()->detach($user->id); + }); - if ($is_member && ! $squad->isUserEligible($user)) - $squad->members()->detach($user->id); + // invite members which are eligible according to new filters (only for auto squads) + if ($this->type == 'auto') { + $users = User::standard() + ->whereDoesntHave('squads', function (Builder $query) { + $query->where('id', $this->id); + })->get(); - if (! $is_member && $squad->isUserEligible($user)) - $squad->members()->attach($user->id); - }); + $users->each(function ($user) { + if ($this->isUserEligible($user)) + $this->members()->save($user); }); + } + } + + /** + * Checks all users for eligibility. This function is used in migrations after major changes and bugs in the squad + * eligibility code to ensure that we are in a valid state. + * @return void + * @throws InvalidFilterException + */ + public static function recomputeAllSquadMemberships(): void { + Squad::where('type', 'auto')->get()->each(function (Squad $squad) { + $squad->recomputeSquadMemberships(); }); } diff --git a/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php b/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php index ac28c6405..78e78a7ff 100644 --- a/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php +++ b/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php @@ -38,7 +38,7 @@ class UpgradeSquadsMaj4Min4Hf2 extends Migration public function up() { DeferredMigration::schedule(function (){ - Squad::recheckAllUsers(); + Squad::recomputeAllSquadMemberships(); }); } diff --git a/src/database/migrations/2024_02_11_155433_update_squad_filters.php b/src/database/migrations/2024_02_11_155433_update_squad_filters.php index 7af8c807a..9a9c87313 100644 --- a/src/database/migrations/2024_02_11_155433_update_squad_filters.php +++ b/src/database/migrations/2024_02_11_155433_update_squad_filters.php @@ -55,7 +55,7 @@ public function up() // since the squad filter change with this migration, we have to recompute the eligibility of everyone DeferredMigration::schedule(function (){ - Squad::recheckAllUsers(); + Squad::recomputeAllSquadMemberships(); }); } From d1b788a022ce5b4bc4c183fbd0378ca04df602b4 Mon Sep 17 00:00:00 2001 From: recursive_tree Date: Thu, 14 Mar 2024 13:45:32 +0100 Subject: [PATCH 14/14] styleci --- src/Models/Squads/Squad.php | 7 ++++- src/Observers/AbstractSquadObserver.php | 3 +- ...06_220254_upgrade_squads_maj4_min4_hf2.php | 3 +- ...2024_02_11_155433_update_squad_filters.php | 28 +++++++++---------- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/Models/Squads/Squad.php b/src/Models/Squads/Squad.php index 7e25fd868..93bde1efc 100644 --- a/src/Models/Squads/Squad.php +++ b/src/Models/Squads/Squad.php @@ -98,14 +98,17 @@ public function isUserEligible(User $user): bool { foreach ($user->characters as $character){ if($this->isEligible($character)) return true; } + // no eligible character found, so the user is not eligible return false; } /** * Checks if the members of this squad are still eligible and if new members have to be added. - * This function is typically called after changes in the squad configuration or in a deferred migration if the behaviour of the squad eligibility logic changes + * This function is typically called after changes in the squad configuration or in a deferred migration if the behaviour of the squad eligibility logic changes. + * * @return void + * * @throws InvalidFilterException */ private function recomputeSquadMemberships(): void { @@ -132,7 +135,9 @@ private function recomputeSquadMemberships(): void { /** * Checks all users for eligibility. This function is used in migrations after major changes and bugs in the squad * eligibility code to ensure that we are in a valid state. + * * @return void + * * @throws InvalidFilterException */ public static function recomputeAllSquadMemberships(): void { diff --git a/src/Observers/AbstractSquadObserver.php b/src/Observers/AbstractSquadObserver.php index 4c54e881f..b0189f9aa 100644 --- a/src/Observers/AbstractSquadObserver.php +++ b/src/Observers/AbstractSquadObserver.php @@ -45,7 +45,8 @@ abstract protected function findRelatedUser(Model $fired_model): ?User; /** * Update squads to which the user owning model firing the event is member. * - * @param \Illuminate\Database\Eloquent\Model $fired_model The model which fired the catch event + * @param \Illuminate\Database\Eloquent\Model $fired_model The model which fired the catch event + * * @throws InvalidFilterException */ protected function updateUserSquads(Model $fired_model) diff --git a/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php b/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php index 78e78a7ff..9eac8d81e 100644 --- a/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php +++ b/src/database/migrations/2020_12_06_220254_upgrade_squads_maj4_min4_hf2.php @@ -23,7 +23,6 @@ use Illuminate\Database\Migrations\Migration; use Seat\Services\Facades\DeferredMigration; use Seat\Web\Models\Squads\Squad; -use Seat\Web\Models\User; /** * Class UpgradeSquadsMaj4Min4Hf2. @@ -37,7 +36,7 @@ class UpgradeSquadsMaj4Min4Hf2 extends Migration */ public function up() { - DeferredMigration::schedule(function (){ + DeferredMigration::schedule(function () { Squad::recomputeAllSquadMemberships(); }); } diff --git a/src/database/migrations/2024_02_11_155433_update_squad_filters.php b/src/database/migrations/2024_02_11_155433_update_squad_filters.php index 9a9c87313..a20d15318 100644 --- a/src/database/migrations/2024_02_11_155433_update_squad_filters.php +++ b/src/database/migrations/2024_02_11_155433_update_squad_filters.php @@ -25,9 +25,7 @@ use Seat\Services\Facades\DeferredMigration; use Seat\Web\Models\Squads\Squad; - -return new class extends Migration -{ +return new class extends Migration { /** * Run the migrations. * @@ -35,26 +33,26 @@ */ public function up() { - DB::table('squads')->select('id','filters')->chunkById(50, function ($squads){ + DB::table('squads')->select('id', 'filters')->chunkById(50, function ($squads) { foreach ($squads as $squad) { $this->updateSquad($squad, function ($path) { // refresh tokens are a special case, since they don't go over character. // However, they also work over character when a plural s is removed if($path === 'refresh_tokens') return 'refresh_token'; - $parts = explode('.',$path); + $parts = explode('.', $path); - if($parts[0] !== 'characters' ) { + if($parts[0] !== 'characters') { throw new Exception('Cannot migrate squad filter: filter path doesn\'t go over character'); } - return implode('.',array_slice($parts,1)); + return implode('.', array_slice($parts, 1)); }); } }); // since the squad filter change with this migration, we have to recompute the eligibility of everyone - DeferredMigration::schedule(function (){ + DeferredMigration::schedule(function () { Squad::recomputeAllSquadMemberships(); }); } @@ -63,22 +61,22 @@ private function updateSquad($squad, $callback) { $filter = json_decode($squad->filters); $this->updateFilter($filter, $callback); DB::table('squads') - ->where('id',$squad->id) + ->where('id', $squad->id) ->update([ - 'filters' => json_encode($filter) + 'filters' => json_encode($filter), ]); } private function updateFilter($filter, $callback) { - if(property_exists($filter,'and')){ + if(property_exists($filter, 'and')){ foreach ($filter->and as $rule) { $this->updateFilter($rule, $callback); } - } else if (property_exists($filter,'or')){ + } elseif (property_exists($filter, 'or')){ foreach ($filter->or as $rule) { $this->updateFilter($rule, $callback); } - } else if (property_exists($filter,'path')){ + } elseif (property_exists($filter, 'path')){ $filter->path = $callback($filter->path); } } @@ -90,13 +88,13 @@ private function updateFilter($filter, $callback) { */ public function down() { - DB::table('squads')->select('id','filters')->chunkById(50, function ($squads){ + DB::table('squads')->select('id', 'filters')->chunkById(50, function ($squads) { foreach ($squads as $squad) { $this->updateSquad($squad, function ($path) { if($path === '') return 'characters'; // we deliberately ignore the refresh_tokens special case since keeping it over character fixes a security issue - return sprintf('characters.%s',$path); + return sprintf('characters.%s', $path); }); } });