From aac4640ff49dd619ecf852e5dac15da419cbc25c Mon Sep 17 00:00:00 2001 From: "Emma C. Hughes" <84008144+emmachughes@users.noreply.github.com> Date: Wed, 18 Dec 2024 14:35:00 +0100 Subject: [PATCH 1/3] add contextual permissions --- sourcecode/hub/app/Enums/ContentRole.php | 23 +++ sourcecode/hub/app/Enums/ContentUserRole.php | 12 -- sourcecode/hub/app/Events/ContentSaving.php | 14 ++ .../hub/app/Events/LtiPlatformDeleting.php | 15 ++ .../Controllers/Admin/ContextController.php | 34 +++ .../Admin/LtiPlatformController.php | 32 +++ .../Http/Controllers/ContentController.php | 31 ++- .../Requests/AddContextToContentRequest.php | 22 ++ .../AddContextToLtiPlatformRequest.php | 24 +++ .../app/Http/Requests/Api/ContentRequest.php | 8 +- .../app/Http/Requests/StoreContextRequest.php | 22 ++ .../app/Listeners/AddContextsToContent.php | 45 ++++ .../hub/app/Listeners/ContentListener.php | 2 + .../hub/app/Listeners/LtiPlatformListener.php | 15 ++ sourcecode/hub/app/Models/Content.php | 28 ++- sourcecode/hub/app/Models/ContentUser.php | 4 +- sourcecode/hub/app/Models/Context.php | 26 +++ sourcecode/hub/app/Models/LtiPlatform.php | 36 ++++ .../hub/app/Models/LtiPlatformContext.php | 15 ++ sourcecode/hub/app/Policies/ContentPolicy.php | 106 ++++++++-- .../hub/database/factories/ContentFactory.php | 10 +- .../hub/database/factories/ContextFactory.php | 28 +++ .../database/factories/LtiPlatformFactory.php | 7 + .../2024_12_12_080000_add_content_context.php | 44 ++++ sourcecode/hub/lang/en/messages.php | 16 +- sourcecode/hub/lang/nb/messages.php | 1 - .../views/admin/contexts/index.blade.php | 37 ++++ .../hub/resources/views/admin/index.blade.php | 6 + .../admin/lti-platforms/contexts.blade.php | 54 +++++ .../views/admin/lti-platforms/edit.blade.php | 2 +- .../views/admin/lti-platforms/index.blade.php | 2 + .../views/components/form/dropdown.blade.php | 19 +- .../views/components/form/field.blade.php | 7 + .../resources/views/content/roles.blade.php | 57 ++++- sourcecode/hub/routes/web.php | 31 +++ sourcecode/hub/tests/Browser/AdminTest.php | 33 +++ .../Browser/Components/LtiPlatformCard.php | 1 + sourcecode/hub/tests/Browser/ContentTest.php | 194 +++++++++++++++++- sourcecode/hub/tests/Feature/ContentTest.php | 49 +++++ .../hub/tests/Unit/Enum/ContentRoleTest.php | 34 +++ 40 files changed, 1074 insertions(+), 72 deletions(-) create mode 100644 sourcecode/hub/app/Enums/ContentRole.php delete mode 100644 sourcecode/hub/app/Enums/ContentUserRole.php create mode 100644 sourcecode/hub/app/Events/ContentSaving.php create mode 100644 sourcecode/hub/app/Events/LtiPlatformDeleting.php create mode 100644 sourcecode/hub/app/Http/Controllers/Admin/ContextController.php create mode 100644 sourcecode/hub/app/Http/Requests/AddContextToContentRequest.php create mode 100644 sourcecode/hub/app/Http/Requests/AddContextToLtiPlatformRequest.php create mode 100644 sourcecode/hub/app/Http/Requests/StoreContextRequest.php create mode 100644 sourcecode/hub/app/Listeners/AddContextsToContent.php create mode 100644 sourcecode/hub/app/Listeners/LtiPlatformListener.php create mode 100644 sourcecode/hub/app/Models/Context.php create mode 100644 sourcecode/hub/app/Models/LtiPlatformContext.php create mode 100644 sourcecode/hub/database/factories/ContextFactory.php create mode 100644 sourcecode/hub/database/migrations/2024_12_12_080000_add_content_context.php create mode 100644 sourcecode/hub/resources/views/admin/contexts/index.blade.php create mode 100644 sourcecode/hub/resources/views/admin/lti-platforms/contexts.blade.php create mode 100644 sourcecode/hub/tests/Unit/Enum/ContentRoleTest.php diff --git a/sourcecode/hub/app/Enums/ContentRole.php b/sourcecode/hub/app/Enums/ContentRole.php new file mode 100644 index 000000000..95d3601d3 --- /dev/null +++ b/sourcecode/hub/app/Enums/ContentRole.php @@ -0,0 +1,23 @@ + [ContentRole::Owner, ContentRole::Editor, ContentRole::Reader], + ContentRole::Editor => [ContentRole::Editor, ContentRole::Reader], + ContentRole::Reader => [ContentRole::Reader], + }, true); + } +} diff --git a/sourcecode/hub/app/Enums/ContentUserRole.php b/sourcecode/hub/app/Enums/ContentUserRole.php deleted file mode 100644 index fd4838c85..000000000 --- a/sourcecode/hub/app/Enums/ContentUserRole.php +++ /dev/null @@ -1,12 +0,0 @@ -view('admin.contexts.index', [ + 'contexts' => $contexts, + ]); + } + + public function add(StoreContextRequest $request): RedirectResponse + { + $context = new Context(); + $context->fill($request->validated()); + $context->save(); + + return redirect()->route('admin.contexts.index') + ->with('alert', trans('messages.context-added')); + } +} diff --git a/sourcecode/hub/app/Http/Controllers/Admin/LtiPlatformController.php b/sourcecode/hub/app/Http/Controllers/Admin/LtiPlatformController.php index f9714980a..fcc2de360 100644 --- a/sourcecode/hub/app/Http/Controllers/Admin/LtiPlatformController.php +++ b/sourcecode/hub/app/Http/Controllers/Admin/LtiPlatformController.php @@ -4,14 +4,18 @@ namespace App\Http\Controllers\Admin; +use App\Enums\ContentRole; +use App\Http\Requests\AddContextToLtiPlatformRequest; use App\Http\Requests\StoreLtiPlatformRequest; use App\Http\Requests\UpdateLtiPlatformRequest; +use App\Models\Context; use App\Models\LtiPlatform; use Illuminate\Contracts\View\View; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Symfony\Component\HttpFoundation\Response; +use function redirect; use function route; use function to_route; @@ -71,4 +75,32 @@ public function destroy(LtiPlatform $platform, Request $request): Response return to_route('admin.lti-platforms.index'); } + + public function contexts(LtiPlatform $platform): View + { + // @phpstan-ignore larastan.noUnnecessaryCollectionCall + $availableContexts = Context::all() + ->diff($platform->contexts) + ->mapWithKeys(fn (Context $context) => [$context->id => $context->name]); + + return view('admin.lti-platforms.contexts', [ + 'available_contexts' => $availableContexts, + 'platform' => $platform, + ]); + } + + public function addContext( + LtiPlatform $platform, + AddContextToLtiPlatformRequest $request, + ): RedirectResponse { + $context = Context::where('id', $request->validated('context')) + ->firstOrFail(); + + $platform->contexts()->attach($context, [ + 'role' => ContentRole::from($request->validated('role')), + ]); + + return redirect()->back() + ->with('alert', trans('messages.context-added-to-lti-platform')); + } } diff --git a/sourcecode/hub/app/Http/Controllers/ContentController.php b/sourcecode/hub/app/Http/Controllers/ContentController.php index 96e1056fb..755be6215 100644 --- a/sourcecode/hub/app/Http/Controllers/ContentController.php +++ b/sourcecode/hub/app/Http/Controllers/ContentController.php @@ -4,9 +4,10 @@ namespace App\Http\Controllers; -use App\Enums\ContentUserRole; +use App\Enums\ContentRole; use App\Enums\ContentViewSource; use App\Enums\LtiToolEditMode; +use App\Http\Requests\AddContextToContentRequest; use App\Http\Requests\ContentStatisticsRequest; use App\Http\Requests\ContentStatusRequest; use App\Http\Requests\DeepLinkingReturnRequest; @@ -15,6 +16,7 @@ use App\Lti\LtiLaunchBuilder; use App\Models\Content; use App\Models\ContentVersion; +use App\Models\Context; use App\Models\LtiPlatform; use App\Models\LtiTool; use App\Models\LtiToolExtra; @@ -145,11 +147,36 @@ public function history(Content $content): View public function roles(Content $content): View { + // @phpstan-ignore larastan.noUnnecessaryCollectionCall + $availableContexts = Context::all() + ->diff($content->contexts) + ->mapWithKeys(fn (Context $context) => [$context->id => $context->name]); + return view('content.roles', [ 'content' => $content, + 'available_contexts' => $availableContexts, ]); } + public function addContext(Content $content, AddContextToContentRequest $request): RedirectResponse + { + $context = Context::where('id', $request->validated('context')) + ->firstOrFail(); + + $content->contexts()->attach($context); + + return redirect()->back() + ->with('alert', trans('messages.context-added-to-content')); + } + + public function removeContext(Content $content, Context $context): RedirectResponse + { + $content->contexts()->detach($context->id); + + return redirect()->back() + ->with('alert', trans('messages.context-removed-from-content')); + } + public function create(): View { $tools = LtiTool::all(); @@ -287,7 +314,7 @@ public function ltiStore( $content = new Content(); $content->saveQuietly(); $content->users()->save($this->getUser(), [ - 'role' => ContentUserRole::Owner, + 'role' => ContentRole::Owner, ]); $version = $content->createVersionFromLinkItem($item, $tool, $this->getUser()); diff --git a/sourcecode/hub/app/Http/Requests/AddContextToContentRequest.php b/sourcecode/hub/app/Http/Requests/AddContextToContentRequest.php new file mode 100644 index 000000000..a1fc2b7bb --- /dev/null +++ b/sourcecode/hub/app/Http/Requests/AddContextToContentRequest.php @@ -0,0 +1,22 @@ + + */ + public function rules(): array + { + return [ + 'context' => ['required', 'string', Rule::exists(Context::class, 'id')], + ]; + } +} diff --git a/sourcecode/hub/app/Http/Requests/AddContextToLtiPlatformRequest.php b/sourcecode/hub/app/Http/Requests/AddContextToLtiPlatformRequest.php new file mode 100644 index 000000000..bc25563fe --- /dev/null +++ b/sourcecode/hub/app/Http/Requests/AddContextToLtiPlatformRequest.php @@ -0,0 +1,24 @@ + + */ + public function rules(): array + { + return [ + 'context' => ['required', 'string', Rule::exists(Context::class, 'id')], + 'role' => ['required', Rule::enum(ContentRole::class)], + ]; + } +} diff --git a/sourcecode/hub/app/Http/Requests/Api/ContentRequest.php b/sourcecode/hub/app/Http/Requests/Api/ContentRequest.php index 1b3eea2d9..088f40a06 100644 --- a/sourcecode/hub/app/Http/Requests/Api/ContentRequest.php +++ b/sourcecode/hub/app/Http/Requests/Api/ContentRequest.php @@ -4,7 +4,7 @@ namespace App\Http\Requests\Api; -use App\Enums\ContentUserRole; +use App\Enums\ContentRole; use App\Models\User; use Illuminate\Contracts\Auth\Access\Gate; use Illuminate\Foundation\Http\FormRequest; @@ -39,7 +39,7 @@ public function rules(Gate $gate): array 'roles.*.role' => [ Rule::prohibitedIf(fn () => $gate->denies('admin')), - Rule::enum(ContentUserRole::class), + Rule::enum(ContentRole::class), 'required_with:roles.*.user', ], @@ -48,7 +48,7 @@ public function rules(Gate $gate): array } /** - * @return array + * @return array */ public function getRoles(): array { @@ -56,7 +56,7 @@ public function getRoles(): array return array_map(fn (array $role) => [ 'user' => User::where('id', $role['user'])->firstOrFail(), - 'role' => ContentUserRole::from($role['role']), + 'role' => ContentRole::from($role['role']), ], $roles); } diff --git a/sourcecode/hub/app/Http/Requests/StoreContextRequest.php b/sourcecode/hub/app/Http/Requests/StoreContextRequest.php new file mode 100644 index 000000000..402056bd9 --- /dev/null +++ b/sourcecode/hub/app/Http/Requests/StoreContextRequest.php @@ -0,0 +1,22 @@ + + */ + public function rules(): array + { + return [ + 'name' => ['required', 'string', 'regex:/^\w+$/', Rule::unique(Context::class, 'name')], + ]; + } +} diff --git a/sourcecode/hub/app/Listeners/AddContextsToContent.php b/sourcecode/hub/app/Listeners/AddContextsToContent.php new file mode 100644 index 000000000..b8c8e3300 --- /dev/null +++ b/sourcecode/hub/app/Listeners/AddContextsToContent.php @@ -0,0 +1,45 @@ +getLaunchingLtiPlatform(); + + if (!$platform) { + return; + } + + foreach ($platform->contexts as $context) { + $event->content->contexts()->attach($context); + } + } + + private function getLaunchingLtiPlatform(): LtiPlatform|null + { + if (!$this->request->hasPreviousSession()) { + return null; + } + + $key = $this->request->session()->get('lti.oauth_consumer_key'); + + if ($key === null) { + return null; + } + + return LtiPlatform::where('key', $key)->first(); + } +} diff --git a/sourcecode/hub/app/Listeners/ContentListener.php b/sourcecode/hub/app/Listeners/ContentListener.php index dfcd99da1..fbd638b55 100644 --- a/sourcecode/hub/app/Listeners/ContentListener.php +++ b/sourcecode/hub/app/Listeners/ContentListener.php @@ -17,6 +17,8 @@ public function handleForceDeleting(ContentForceDeleting $event): void ->lazy() ->each(fn (ContentVersion $version) => $version->delete()); + $event->content->contexts()->detach(); + $event->content->tags()->detach(); $event->content->users()->detach(); diff --git a/sourcecode/hub/app/Listeners/LtiPlatformListener.php b/sourcecode/hub/app/Listeners/LtiPlatformListener.php new file mode 100644 index 000000000..9323d8a0f --- /dev/null +++ b/sourcecode/hub/app/Listeners/LtiPlatformListener.php @@ -0,0 +1,15 @@ +ltiPlatform->contexts()->detach(); + } +} diff --git a/sourcecode/hub/app/Models/Content.php b/sourcecode/hub/app/Models/Content.php index 3ef5a262d..67b223b70 100644 --- a/sourcecode/hub/app/Models/Content.php +++ b/sourcecode/hub/app/Models/Content.php @@ -4,9 +4,10 @@ namespace App\Models; -use App\Enums\ContentUserRole; +use App\Enums\ContentRole; use App\Enums\ContentViewSource; use App\Events\ContentForceDeleting; +use App\Events\ContentSaving; use App\Support\HasUlidsFromCreationDate; use Cerpus\EdlibResourceKit\Lti\Edlib\DeepLinking\EdlibLtiLinkItem; use Cerpus\EdlibResourceKit\Lti\Message\DeepLinking\ContentItem; @@ -44,6 +45,7 @@ class Content extends Model * @var array */ protected $dispatchesEvents = [ + 'saving' => ContentSaving::class, 'forceDeleting' => ContentForceDeleting::class, ]; @@ -117,7 +119,7 @@ public function createCopyBelongingTo(User $user, ContentVersion|null $version = ]); } - $copy->users()->save($user, ['role' => ContentUserRole::Owner]); + $copy->users()->save($user, ['role' => ContentRole::Owner]); $copy->save(); return $copy; @@ -286,11 +288,11 @@ public function users(): BelongsToMany } /** - * @return BelongsToMany + * @return BelongsToMany */ - public function usersWithTimestamps(): BelongsToMany + public function contexts(): BelongsToMany { - return $this->users()->withTimestamps(); + return $this->belongsToMany(Context::class); } public function hasUser(User $user): bool @@ -298,6 +300,22 @@ public function hasUser(User $user): bool return $this->users->contains($user); } + public function hasUserWithMinimumRole(User $user, ContentRole $role): bool + { + foreach ($this->users as $contentUser) { + if ($contentUser->is($user)) { + // @phpstan-ignore property.notFound + $contentUserRole = $contentUser->pivot->role; + assert($contentUserRole instanceof ContentRole); + + // User cannot be added more than once, so we return here. + return $contentUserRole->grants($role); + } + } + + return false; + } + /** * @return array */ diff --git a/sourcecode/hub/app/Models/ContentUser.php b/sourcecode/hub/app/Models/ContentUser.php index 71bd60431..0fa8d4bbb 100644 --- a/sourcecode/hub/app/Models/ContentUser.php +++ b/sourcecode/hub/app/Models/ContentUser.php @@ -4,12 +4,12 @@ namespace App\Models; -use App\Enums\ContentUserRole; +use App\Enums\ContentRole; use Illuminate\Database\Eloquent\Relations\Pivot; class ContentUser extends Pivot { protected $casts = [ - 'role' => ContentUserRole::class, + 'role' => ContentRole::class, ]; } diff --git a/sourcecode/hub/app/Models/Context.php b/sourcecode/hub/app/Models/Context.php new file mode 100644 index 000000000..0f1689bd3 --- /dev/null +++ b/sourcecode/hub/app/Models/Context.php @@ -0,0 +1,26 @@ + */ + use HasFactory; + use HasUlidsFromCreationDate; + + public const UPDATED_AT = null; + + protected $fillable = [ + 'name', + ]; +} diff --git a/sourcecode/hub/app/Models/LtiPlatform.php b/sourcecode/hub/app/Models/LtiPlatform.php index 03adc1c45..acac7f50b 100644 --- a/sourcecode/hub/app/Models/LtiPlatform.php +++ b/sourcecode/hub/app/Models/LtiPlatform.php @@ -4,15 +4,20 @@ namespace App\Models; +use App\Enums\ContentRole; +use App\Events\LtiPlatformDeleting; use Cerpus\EdlibResourceKit\Oauth1\Credentials; use Database\Factories\LtiPlatformFactory; use Illuminate\Database\Eloquent\Concerns\HasUlids; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\Eloquent\Relations\BelongsToMany; use Illuminate\Database\Eloquent\SoftDeletes; use Illuminate\Support\Str; use Random\Randomizer; +use function assert; + class LtiPlatform extends Model { /** @use HasFactory */ @@ -40,6 +45,11 @@ class LtiPlatform extends Model 'authorizes_edit' => 'boolean', ]; + /** @var array */ + protected $dispatchesEvents = [ + 'deleting' => LtiPlatformDeleting::class, + ]; + protected static function booted(): void { static::creating(function (self $ltiPlatform): void { @@ -54,6 +64,32 @@ protected static function booted(): void }); } + /** + * @return BelongsToMany + */ + public function contexts(): BelongsToMany + { + return $this->belongsToMany(Context::class, 'lti_platform_context') + ->withPivot('role') + ->using(LtiPlatformContext::class); + } + + public function hasContextWithMinimumRole(Context $context, ContentRole $role): bool + { + foreach ($this->contexts as $platformContext) { + if ($platformContext->is($context)) { + // @phpstan-ignore property.notFound + $ltiPlatformRole = $platformContext->pivot->role; + assert($ltiPlatformRole instanceof ContentRole); + + // Context cannot be added more than once, so we return here. + return $ltiPlatformRole->grants($role); + } + } + + return false; + } + public function getOauth1Credentials(): Credentials { return new Credentials($this->key, $this->secret); diff --git a/sourcecode/hub/app/Models/LtiPlatformContext.php b/sourcecode/hub/app/Models/LtiPlatformContext.php new file mode 100644 index 000000000..75f0bc4fd --- /dev/null +++ b/sourcecode/hub/app/Models/LtiPlatformContext.php @@ -0,0 +1,15 @@ + ContentRole::class, + ]; +} diff --git a/sourcecode/hub/app/Policies/ContentPolicy.php b/sourcecode/hub/app/Policies/ContentPolicy.php index f3972b4b3..dca76ce42 100644 --- a/sourcecode/hub/app/Policies/ContentPolicy.php +++ b/sourcecode/hub/app/Policies/ContentPolicy.php @@ -4,21 +4,27 @@ namespace App\Policies; +use App\Enums\ContentRole; use App\Models\Content; use App\Models\ContentVersion; use App\Models\LtiPlatform; use App\Models\User; -use Illuminate\Support\Facades\Session; +use Illuminate\Http\Request; +use LogicException; -use function request; - -class ContentPolicy +readonly class ContentPolicy { + public function __construct(private Request $request) + { + } + public function view( User|null $user, Content $content, ContentVersion|null $version = null ): bool { + $this->ensureVersionBelongsToContent($content, $version); + if ($user?->admin) { return true; } @@ -29,11 +35,20 @@ public function view( return true; } + $platform = $this->getLtiPlatform(); + if ($platform) { + foreach ($content->contexts ?? [] as $context) { + if ($platform->hasContextWithMinimumRole($context, ContentRole::Reader)) { + return true; + } + } + } + if (!$user) { return false; } - return $content->hasUser($user); + return $content->hasUserWithMinimumRole($user, ContentRole::Reader); } public function create(User $user): bool @@ -46,27 +61,30 @@ public function edit( Content $content, ContentVersion|null $version = null, ): bool { - if ($version && !$version->content()->is($content)) { - return false; - } + $this->ensureVersionBelongsToContent($content, $version); if ($user->admin) { return true; } - if (Session::has('lti.oauth_consumer_key')) { - $key = Session::get('lti.oauth_consumer_key'); - $platform = LtiPlatform::where('key', $key)->first(); + $platform = $this->getLtiPlatform(); + if ($platform) { if ( - $platform?->authorizes_edit && - Session::has('intent-to-edit.' . $content->id) + $platform->authorizes_edit && + $this->request->session()->has('intent-to-edit.' . $content->id) ) { return true; } + + foreach ($content->contexts ?? [] as $context) { + if ($platform->hasContextWithMinimumRole($context, ContentRole::Editor)) { + return true; + } + } } - return $content->hasUser($user); + return $content->hasUserWithMinimumRole($user, ContentRole::Editor); } public function copy( @@ -74,7 +92,18 @@ public function copy( Content $content, ContentVersion|null $version = null, ): bool { - if ($content->hasUser($user)) { + $this->ensureVersionBelongsToContent($content, $version); + + $platform = $this->getLtiPlatform(); + if ($platform) { + foreach ($content->contexts ?? [] as $context) { + if ($platform->hasContextWithMinimumRole($context, ContentRole::Reader)) { + return true; + } + } + } + + if ($content->hasUserWithMinimumRole($user, ContentRole::Reader)) { return true; } @@ -84,7 +113,7 @@ public function copy( $version ??= $content->latestPublishedVersion; - if ($version === null) { + if (!$version?->published) { return false; } @@ -101,15 +130,25 @@ public function delete(User $user, Content $content): bool return true; } - // TODO: check owner role - return $content->hasUser($user); + $platform = $this->getLtiPlatform(); + if ($platform) { + foreach ($content->contexts ?? [] as $context) { + if ($platform->hasContextWithMinimumRole($context, ContentRole::Owner)) { + return true; + } + } + } + + return $content->hasUserWithMinimumRole($user, ContentRole::Owner); } public function use(User|null $user, Content $content, ContentVersion $version): bool { + $this->ensureVersionBelongsToContent($content, $version); + if ( - !request()->hasPreviousSession() || - !request()->session()->has('lti.content_item_return_url') + !$this->request->hasPreviousSession() || + !$this->request->session()->has('lti.content_item_return_url') ) { // not in LTI Deep Linking context return false; @@ -125,4 +164,31 @@ public function use(User|null $user, Content $content, ContentVersion $version): return true; } + + public function manageRoles(User $user, Content $content): bool + { + if ($content->hasUserWithMinimumRole($user, ContentRole::Owner)) { + return true; + } + + return false; + } + + private function ensureVersionBelongsToContent(Content $content, ContentVersion|null $version): void + { + if ($version && !$version->content?->is($content)) { + throw new LogicException('Version does not belong to content'); + } + } + + private function getLtiPlatform(): LtiPlatform|null + { + $key = $this->request->session()->get('lti.oauth_consumer_key'); + + if (!$key) { + return null; + } + + return LtiPlatform::where('key', $key)->first(); + } } diff --git a/sourcecode/hub/database/factories/ContentFactory.php b/sourcecode/hub/database/factories/ContentFactory.php index 1b4171ec0..890bb4f43 100644 --- a/sourcecode/hub/database/factories/ContentFactory.php +++ b/sourcecode/hub/database/factories/ContentFactory.php @@ -4,10 +4,11 @@ namespace Database\Factories; -use App\Enums\ContentUserRole; +use App\Enums\ContentRole; use App\Models\Content; use App\Models\ContentVersion; use App\Models\ContentView; +use App\Models\Context; use App\Models\Tag; use App\Models\User; use DateTimeImmutable; @@ -60,11 +61,16 @@ public function trashed(DateTimeInterface|null $deletedAt = null): self public function withUser( User|UserFactory $user, - ContentUserRole|null $role = ContentUserRole::Owner, + ContentRole|null $role = ContentRole::Owner, ): self { return $this->hasAttached($user, ['role' => $role], 'users'); } + public function withContext(Context|ContextFactory $context): self + { + return $this->hasAttached($context, [], 'contexts'); + } + public function withVersion(ContentVersionFactory|null $version = null): self { $version ??= ContentVersion::factory(); diff --git a/sourcecode/hub/database/factories/ContextFactory.php b/sourcecode/hub/database/factories/ContextFactory.php new file mode 100644 index 000000000..cde9dea16 --- /dev/null +++ b/sourcecode/hub/database/factories/ContextFactory.php @@ -0,0 +1,28 @@ + + */ +final class ContextFactory extends Factory +{ + public function definition(): array + { + return [ + 'name' => $this->faker->unique()->word, + ]; + } + + public function name(string $name): self + { + return $this->state([ + 'name' => $name, + ]); + } +} diff --git a/sourcecode/hub/database/factories/LtiPlatformFactory.php b/sourcecode/hub/database/factories/LtiPlatformFactory.php index 9f5c1a0e1..c7505e331 100644 --- a/sourcecode/hub/database/factories/LtiPlatformFactory.php +++ b/sourcecode/hub/database/factories/LtiPlatformFactory.php @@ -4,6 +4,8 @@ namespace Database\Factories; +use App\Enums\ContentRole; +use App\Models\Context; use App\Models\LtiPlatform; use Illuminate\Database\Eloquent\Factories\Factory; @@ -27,4 +29,9 @@ public function name(string $name): self { return $this->state(['name' => $name]); } + + public function withContext(Context|ContextFactory $context, ContentRole $role): self + { + return $this->hasAttached($context, ['role' => $role], 'contexts'); + } } diff --git a/sourcecode/hub/database/migrations/2024_12_12_080000_add_content_context.php b/sourcecode/hub/database/migrations/2024_12_12_080000_add_content_context.php new file mode 100644 index 000000000..7272715c7 --- /dev/null +++ b/sourcecode/hub/database/migrations/2024_12_12_080000_add_content_context.php @@ -0,0 +1,44 @@ +ulid('id')->primary(); + $table->string('name')->unique(); + $table->timestampTz('created_at'); + }); + + Schema::create('content_context', function (Blueprint $table) { + $table->ulid('content_id'); + $table->ulid('context_id'); + + $table->foreign('content_id')->references('id')->on('contents'); + $table->foreign('context_id')->references('id')->on('contexts'); + $table->unique(['content_id', 'context_id']); + }); + + Schema::create('lti_platform_context', function (Blueprint $table) { + $table->ulid('lti_platform_id'); + $table->ulid('context_id'); + $table->string('role'); + + $table->foreign('lti_platform_id')->references('id')->on('lti_platforms'); + $table->foreign('context_id')->references('id')->on('contexts'); + $table->unique(['lti_platform_id', 'context_id']); + }); + } + + public function down(): void + { + Schema::drop('content_context'); + Schema::drop('lti_platform_context'); + Schema::drop('contexts'); + } +}; diff --git a/sourcecode/hub/lang/en/messages.php b/sourcecode/hub/lang/en/messages.php index d03e70c61..1bd810510 100644 --- a/sourcecode/hub/lang/en/messages.php +++ b/sourcecode/hub/lang/en/messages.php @@ -21,6 +21,7 @@ 'admin-home' => 'Admin home', 'manage-lti-platforms' => 'Manage LTI platforms', 'manage-lti-tools' => 'Manage LTI tools', + 'manage-contexts' => 'Manage contexts', 'more-details' => 'More details', 'details' => 'Details', 'preview' => 'Preview', @@ -196,7 +197,6 @@ 'editor' => 'Editor', 'reader' => 'Reader', 'extra-endpoints' => 'Extra endpoints', - 'content-has-no-roles' => 'No roles are assigned to this content.', 'statistics' => 'Statistics', 'reset-zoom' => 'Reset view', 'view-detail' => 'Detail', @@ -218,7 +218,21 @@ 'lti-platform-authorizes-edit' => 'The platform authorizes edit access', 'lti-platform-authorizes-edit-help' => 'Transfers responsibility for access control of editing from :site to the platform, i.e. :site will accept all valid edit requests from the platform', 'dangerous-lti-platform-settings' => 'These settings are dangerous, and should only be enabled for trusted platforms with email verification.', + 'users' => 'Users', + 'contexts' => 'Contexts', + 'nothing-to-display' => 'Nothing to display.', 'url-slug' => 'URL slug', 'lti-playground' => 'LTI playground', 'select-a-content-type' => 'Select a content type', + 'context' => 'Context', + 'context-help' => 'Contexts are used to grant implicit permissions for accessing content.', + 'context-name-help' => 'The name must be unique, and consist of characters from A-Z, 0-9, and underscores.', + 'add-context' => 'Add context', + 'no-available-contexts-to-add' => 'There are no available contexts to add.', + 'context-added-to-content' => 'The context was added to the content.', + 'context-added-to-lti-platform' => 'The context was added to the LTI platform.', + 'context-removed-from-content' => 'The content was removed from the content.', + 'edit-lti-platform' => 'Edit LTI platform', + 'contexts-for-lti-platform' => 'Contexts for LTI platform ":platform"', + 'context-added' => 'The context was added.', ]; diff --git a/sourcecode/hub/lang/nb/messages.php b/sourcecode/hub/lang/nb/messages.php index e20d7fc75..172054cbf 100644 --- a/sourcecode/hub/lang/nb/messages.php +++ b/sourcecode/hub/lang/nb/messages.php @@ -163,7 +163,6 @@ 'owner' => 'Eier', 'editor' => 'Redaktør', 'reader' => 'Leser', - 'content-has-no-roles' => 'Ingen roller har blitt satt for dette innholdet.', 'statistics' => 'Statistikk', 'reset-zoom' => 'Nullstill visning', 'view-detail' => 'Detalj', diff --git a/sourcecode/hub/resources/views/admin/contexts/index.blade.php b/sourcecode/hub/resources/views/admin/contexts/index.blade.php new file mode 100644 index 000000000..1a52902bd --- /dev/null +++ b/sourcecode/hub/resources/views/admin/contexts/index.blade.php @@ -0,0 +1,37 @@ + + Contexts + +

{{ trans('messages.context-help') }}

+ + @if (count($contexts) > 0) + + + + + + + + @foreach ($contexts as $context) + + + + @endforeach + +
{{ trans('messages.name') }}
{{ $context->name }}
+ @else +

{{ trans('messages.nothing-to-display') }} + @endif + +

+ + + + + {{ trans('messages.add') }} + + diff --git a/sourcecode/hub/resources/views/admin/index.blade.php b/sourcecode/hub/resources/views/admin/index.blade.php index b416aad07..199b550d8 100644 --- a/sourcecode/hub/resources/views/admin/index.blade.php +++ b/sourcecode/hub/resources/views/admin/index.blade.php @@ -14,6 +14,12 @@ +
  • + + {{ trans('messages.manage-contexts') }} + +
    • +
  • diff --git a/sourcecode/hub/resources/views/admin/lti-platforms/contexts.blade.php b/sourcecode/hub/resources/views/admin/lti-platforms/contexts.blade.php new file mode 100644 index 000000000..23c62b7f6 --- /dev/null +++ b/sourcecode/hub/resources/views/admin/lti-platforms/contexts.blade.php @@ -0,0 +1,54 @@ + + {{ trans('messages.contexts-for-lti-platform', ['platform' => $platform->name]) }} + +

    Back to LTI platforms

    + + @if (count($platform->contexts) > 0) +
      + @foreach ($platform->contexts as $context) +
    • {{ $context->name }} ({{ match ($context->pivot->role) { + \App\Enums\ContentRole::Reader => trans('messages.reader'), + \App\Enums\ContentRole::Editor => trans('messages.editor'), + \App\Enums\ContentRole::Owner => trans('messages.owner'), + } }})
      • + @endforeach +
    + @endif + + @if (count($available_contexts) > 0) + +
    +
    + +
    + +
    + +
    + +
    + {{ trans('messages.add') }} +
    +
    +     + @else +

    {{ trans('messages.no-available-contexts-to-add') }}

    + @endif +     diff --git a/sourcecode/hub/resources/views/admin/lti-platforms/edit.blade.php b/sourcecode/hub/resources/views/admin/lti-platforms/edit.blade.php index cdd8dc8f1..1fb95ea4a 100644 --- a/sourcecode/hub/resources/views/admin/lti-platforms/edit.blade.php +++ b/sourcecode/hub/resources/views/admin/lti-platforms/edit.blade.php @@ -1,5 +1,5 @@ - {{ trans('messages.edit-lti-platforms') }} + {{ trans('messages.edit-lti-platform') }} {{ $platform->enable_sso ? trans('messages.yes') : trans('messages.no') }}
    {{ trans('messages.lti-platform-authorizes-edit') }}
    {{ $platform->authorizes_edit ? trans('messages.yes') : trans('messages.no') }}
    +
    {{ trans('messages.contexts') }}
    +
    {{ count($platform->contexts) }}
    diff --git a/sourcecode/hub/resources/views/components/form/dropdown.blade.php b/sourcecode/hub/resources/views/components/form/dropdown.blade.php index cff35cc50..6b3115e2d 100644 --- a/sourcecode/hub/resources/views/components/form/dropdown.blade.php +++ b/sourcecode/hub/resources/views/components/form/dropdown.blade.php @@ -1,23 +1,14 @@ -@props(['name', 'options', 'selected', 'emptyOption' => false, 'multiple' => false]) -except(['selected', 'options'])->class(['form-select']) }}> + @if ($emptyOption ?? false) + @endif @foreach ($options as $key => $label) @endforeach diff --git a/sourcecode/hub/resources/views/components/form/field.blade.php b/sourcecode/hub/resources/views/components/form/field.blade.php index df4523b16..8b41ea6fb 100644 --- a/sourcecode/hub/resources/views/components/form/field.blade.php +++ b/sourcecode/hub/resources/views/components/form/field.blade.php @@ -4,6 +4,13 @@ 'form-check' => in_array($type ?? 'text', ['checkbox', 'radio']), ])> @switch ($type ?? 'text') + @case('select') + + {{ $label ?? $name }} + + + @break + @case('radio') @case('checkbox') diff --git a/sourcecode/hub/resources/views/content/roles.blade.php b/sourcecode/hub/resources/views/content/roles.blade.php index 1d0a8e774..973daa459 100644 --- a/sourcecode/hub/resources/views/content/roles.blade.php +++ b/sourcecode/hub/resources/views/content/roles.blade.php @@ -3,6 +3,8 @@ +

    {{ trans('messages.users') }}

    + @if (count($content->users) > 0) @@ -17,15 +19,62 @@ @endforeach
    {{ $user->name }} {{ match ($user->pivot->role) { - \App\Enums\ContentUserRole::Owner => trans('messages.owner'), - \App\Enums\ContentUserRole::Editor => trans('messages.editor'), - \App\Enums\ContentUserRole::Reader => trans('messages.reader'), + \App\Enums\ContentRole::Owner => trans('messages.owner'), + \App\Enums\ContentRole::Editor => trans('messages.editor'), + \App\Enums\ContentRole::Reader => trans('messages.reader'), } }}
    @else -

    {{ trans('messages.content-has-no-roles') }}

    +

    {{ trans('messages.nothing-to-display') }}

    + @endif + +

    {{ trans('messages.contexts') }}

    + + @if (count($content->contexts) > 0) + + + + + + + + + + @foreach ($content->contexts as $context) + + + + + @endforeach + +
    {{ trans('messages.context') }}
    {{ $context->name }} + + {{ trans('messages.remove') }} + +
    + @else +

    {{ trans('messages.nothing-to-display') }}

    @endif + + @can('manage-roles', [$content]) + @if (count($available_contexts) > 0) + + + + {{ trans('messages.add') }} + + @else +

    {{ trans('messages.no-available-contexts-to-add') }}

    + @endif + @endcan     diff --git a/sourcecode/hub/routes/web.php b/sourcecode/hub/routes/web.php index 3cf46a20b..e418acffa 100644 --- a/sourcecode/hub/routes/web.php +++ b/sourcecode/hub/routes/web.php @@ -3,6 +3,7 @@ declare(strict_types=1); use App\Http\Controllers\Admin\AdminController; +use App\Http\Controllers\Admin\ContextController; use App\Http\Controllers\Admin\LtiPlatformController; use App\Http\Controllers\Admin\LtiToolController; use App\Http\Controllers\ContentController; @@ -89,6 +90,18 @@ ->whereUlid(['content']) ->can('edit', ['content']); + Route::post('/content/{content}/roles/add-context') + ->uses([ContentController::class, 'addContext']) + ->name('content.add-context') + ->whereUlid(['content']) + ->can('manage-roles', ['content']); + + Route::delete('/content/{content}/roles/{role}') + ->uses([ContentController::class, 'removeContext']) + ->name('content.remove-context') + ->can('manage-roles', ['content']) + ->scopeBindings(); + Route::get('/content/{content}/statistics') ->name('content.statistics') ->uses([ContentController::class, 'statistics']) @@ -243,6 +256,14 @@ Route::post('/rebuild-content-index', [AdminController::class, 'rebuildContentIndex']) ->name('admin.rebuild-content-index'); + Route::get('/contexts') + ->uses([ContextController::class, 'index']) + ->name('admin.contexts.index'); + + Route::post('/contexts') + ->uses([ContextController::class, 'add']) + ->name('admin.contexts.add'); + Route::prefix('/lti-platforms')->group(function () { Route::get('') ->uses([LtiPlatformController::class, 'index']) @@ -257,6 +278,16 @@ ->name('admin.lti-platforms.edit') ->can('edit', ['platform']); + Route::get('/{platform}/contexts') + ->uses([LtiPlatformController::class, 'contexts']) + ->name('admin.lti-platforms.contexts') + ->can('edit', ['platform']); + + Route::put('/{platform}/contexts') + ->uses([LtiPlatformController::class, 'addContext']) + ->name('admin.lti-platforms.add-context') + ->can('edit', ['platform']); + Route::patch('/{platform}') ->uses([LtiPlatformController::class, 'update']) ->name('admin.lti-platforms.update') diff --git a/sourcecode/hub/tests/Browser/AdminTest.php b/sourcecode/hub/tests/Browser/AdminTest.php index 48de9edc9..b12713216 100644 --- a/sourcecode/hub/tests/Browser/AdminTest.php +++ b/sourcecode/hub/tests/Browser/AdminTest.php @@ -4,6 +4,7 @@ namespace Tests\Browser; +use App\Models\Context; use App\Models\LtiPlatform; use App\Models\LtiTool; use App\Models\User; @@ -266,4 +267,36 @@ public function testCanEditFlagsForTool(): void ) ); } + + public function testCanAddContextToLtiPlatform(): void + { + LtiPlatform::factory()->create(); + $context = Context::factory()->name('ndla_users')->create(); + $user = User::factory()->admin()->create(); + + $this->browse( + fn (Browser $browser) => $browser + ->loginAs($user->email) + ->assertAuthenticated() + ->visit('/admin') + ->clickLink('Manage LTI platforms') + ->within( + new LtiPlatformCard(), + fn (Browser $card) => $card + ->assertSeeIn('@context-count', '0') + ->clickLink('Contexts') + ) + // Dusk does not support selecting by the choice's label + ->select('context', $context->id) + ->press('Add') + ->assertSee('The context was added to the LTI platform') + ->visit('/admin') + ->clickLink('Manage LTI platforms') + ->within( + new LtiPlatformCard(), + fn (Browser $card) => $card + ->assertSeeIn('@context-count', '1') + ) + ); + } } diff --git a/sourcecode/hub/tests/Browser/Components/LtiPlatformCard.php b/sourcecode/hub/tests/Browser/Components/LtiPlatformCard.php index 4b26b8c7d..a4bf402de 100644 --- a/sourcecode/hub/tests/Browser/Components/LtiPlatformCard.php +++ b/sourcecode/hub/tests/Browser/Components/LtiPlatformCard.php @@ -28,6 +28,7 @@ public function elements(): array '@title' => '.lti-platform-card-title', '@authorizes-edit' => '.lti-platform-card-authorizes-edit', '@enable-sso' => '.lti-platform-card-enable-sso', + '@context-count' => '.lti-platform-card-context-count', ]; } } diff --git a/sourcecode/hub/tests/Browser/ContentTest.php b/sourcecode/hub/tests/Browser/ContentTest.php index 72d4a3db4..032aa38a4 100644 --- a/sourcecode/hub/tests/Browser/ContentTest.php +++ b/sourcecode/hub/tests/Browser/ContentTest.php @@ -4,12 +4,13 @@ namespace Tests\Browser; -use App\Enums\ContentUserRole; +use App\Enums\ContentRole; use App\Enums\LtiToolEditMode; use App\Jobs\RebuildContentIndex; use App\Models\Content; use App\Models\ContentVersion; use App\Models\ContentView; +use App\Models\Context; use App\Models\LtiPlatform; use App\Models\LtiTool; use App\Models\User; @@ -786,9 +787,9 @@ public function testSharingCopiesUrl(): void public function testViewsContentRoles(): void { $content = Content::factory() - ->withUser(User::factory()->name('Owner McOwnerson'), ContentUserRole::Owner) - ->withUser(User::factory()->name('Editor McEditorson'), ContentUserRole::Editor) - ->withUser(User::factory()->name('Reader McReaderson'), ContentUserRole::Reader) + ->withUser(User::factory()->name('Owner McOwnerson'), ContentRole::Owner) + ->withUser(User::factory()->name('Editor McEditorson'), ContentRole::Editor) + ->withUser(User::factory()->name('Reader McReaderson'), ContentRole::Reader) ->withPublishedVersion() ->create(); @@ -1016,4 +1017,189 @@ public function testCopiesContentWhenUpdatingWithCopyFlagViaLti(): void ->assertSeeLink('The original content') ); } + + public function testOwnerOfContentCanAssignContextToContent(): void + { + $user = User::factory()->create(); + + Context::factory()->name('filler')->create(); + $context = Context::factory()->name('desired')->create(); + Context::factory()->name('more_filler')->create(); + + Content::factory() + ->withUser($user) + ->withVersion(ContentVersion::factory()->title('my beautiful content')) + ->create(); + + $this->browse( + fn (Browser $browser) => $browser + ->loginAs($user->email) + ->assertAuthenticated() + ->visit('/content/mine') + ->clickLink('my beautiful content') + ->clickLink('Roles') + // Dusk doesn't support selecting the actual visual text + ->select('[name="context"]', $context->id) + ->press('Add') + ->assertSee('The context was added to the content.') + ->assertSeeIn('.content-contexts > tbody > tr:first-child > td:nth-child(1)', 'desired') + ); + } + + public function testContentCreatedInLtiContextInheritsPlatformRoles(): void + { + $platform = LtiPlatform::factory() + ->withContext(Context::factory()->name('ndla_people'), ContentRole::Editor) + ->create(); + LtiTool::factory() + ->withName('My tool') + ->launchUrl('https://hub-test.edlib.test/lti/samples/deep-link') + ->withCredentials($platform->getOauth1Credentials()) + ->create(); + $user = User::factory() + ->withEmail('foo@example.com') + ->create(); + + $this->browse( + fn (Browser $browser) => $browser + ->loginAs($user->email) + ->assertAuthenticated() + ->visit('/lti/playground') + ->type('launch_url', 'https://hub-test.edlib.test/lti/dl') + ->type('key', $platform->key) + ->type('secret', $platform->secret) + ->type('parameters', 'lis_person_contact_email_primary=foo@example.com') + ->press('Launch') + ->withinFrame( + 'iframe', + fn (Browser $frame) => $frame + ->clickLink('Create') + ->clickLink('My tool') + ->withinFrame( + 'iframe', + fn (Browser $tool) => $tool + ->type('payload', <<press('Send') + ) + ) + ->visit('/content/mine') + ->clickLink('My new content') + ->clickLink('Roles') + ->assertSeeIn('.content-contexts > tbody > tr:first-child > td:nth-child(1)', 'ndla_people') + ); + } + + public function testCanEditContentWhenGivenEditorRoleViaContext(): void + { + $context = Context::factory() + ->name('my_context') + ->create(); + + Content::factory() + ->shared() + ->withContext($context) + ->withVersion( + ContentVersion::factory() + ->title('The content with context') + ->published() + ) + ->create(); + + $platform = LtiPlatform::factory() + ->withContext($context, ContentRole::Editor) + ->create(); + + $user = User::factory() + ->withEmail('person@example.com') + ->create(); + + $this->browse( + fn (Browser $browser) => $browser + ->loginAs($user->email) + ->assertAuthenticated() + ->visit('/lti/playground') + ->type('launch_url', 'https://hub-test.edlib.test/lti/dl') + ->type('key', $platform->key) + ->type('secret', $platform->secret) + ->type( + 'parameters', + 'content_item_return_url=about:blank' . + '<i_message_type=ContentItemSelectionRequest' . + '&lis_person_contact_email_primary=person@example.com' + ) + ->press('Launch') + ->withinFrame( + 'iframe', + fn (Browser $edlib) => $edlib + ->with( + new ContentCard(), + fn (Browser $card) => $card + ->press('@action-menu-toggle') + ->assertPresent('@edit-link') + ) + ) + ); + } + + public function testCannotEditContentInContextsOneIsNotPartOf(): void + { + Content::factory() + ->shared() + ->withContext( + Context::factory()->name('someone_elses_context') + ) + ->withVersion( + ContentVersion::factory() + ->title('The content to not edit') + ->published() + ) + ->create(); + + $platform = LtiPlatform::factory() + ->withContext(Context::factory()->name('my_context'), ContentRole::Editor) + ->create(); + + $user = User::factory() + ->withEmail('person@example.com') + ->create(); + + $this->browse( + fn (Browser $browser) => $browser + ->loginAs($user->email) + ->assertAuthenticated() + ->visit('/lti/playground') + ->type('launch_url', 'https://hub-test.edlib.test/lti/dl') + ->type('key', $platform->key) + ->type('secret', $platform->secret) + ->type( + 'parameters', + 'content_item_return_url=about:blank' . + '<i_message_type=ContentItemSelectionRequest' . + '&lis_person_contact_email_primary=person@example.com' + ) + ->press('Launch') + ->withinFrame( + 'iframe', + fn (Browser $edlib) => $edlib + ->with( + new ContentCard(), + fn (Browser $card) => $card + ->press('@action-menu-toggle') + ->assertNotPresent('@edit-link') + ) + ) + ); + } } diff --git a/sourcecode/hub/tests/Feature/ContentTest.php b/sourcecode/hub/tests/Feature/ContentTest.php index 91473791c..e41f069fb 100644 --- a/sourcecode/hub/tests/Feature/ContentTest.php +++ b/sourcecode/hub/tests/Feature/ContentTest.php @@ -4,15 +4,19 @@ namespace Tests\Feature; +use App\Enums\ContentRole; use App\Jobs\PruneVersionlessContent; use App\Models\Content; use App\Models\ContentVersion; use App\Models\LtiPlatform; +use App\Models\User; use Carbon\Carbon; use Cerpus\EdlibResourceKit\Oauth1\Request; use Cerpus\EdlibResourceKit\Oauth1\Signer; use DomainException; use Illuminate\Foundation\Testing\RefreshDatabase; +use PHPUnit\Framework\Attributes\TestDox; +use PHPUnit\Framework\Attributes\TestWith; use Tests\TestCase; final class ContentTest extends TestCase @@ -162,4 +166,49 @@ public function testThrowsWhenGeneratingUrlToVersionlessContent(): void (new Content())->getDetailsUrl(); } + + #[TestDox('User with role $roleOfUser in content is granted $roleToCheck')] + #[TestWith([ContentRole::Owner, ContentRole::Owner], 'owner, owner')] + #[TestWith([ContentRole::Owner, ContentRole::Editor], 'owner, editor')] + #[TestWith([ContentRole::Owner, ContentRole::Reader], 'owner, reader')] + #[TestWith([ContentRole::Editor, ContentRole::Editor], 'editor, editor')] + #[TestWith([ContentRole::Editor, ContentRole::Reader], 'editor, reader')] + #[TestWith([ContentRole::Reader, ContentRole::Reader], 'reader, reader')] + public function testCheckHasUserWithMinimumRole(ContentRole $roleOfUser, ContentRole $roleToCheck): void + { + $user = User::factory() + ->create(); + $content = Content::factory() + ->withUser($user, $roleOfUser) + ->create(); + + $this->assertTrue($content->hasUserWithMinimumRole($user, $roleToCheck)); + } + + #[TestDox('User with role $roleOfUser in content is denied $roleToCheck')] + #[TestWith([ContentRole::Editor, ContentRole::Owner], 'editor, owner')] + #[TestWith([ContentRole::Reader, ContentRole::Owner], 'reader, owner')] + #[TestWith([ContentRole::Reader, ContentRole::Editor], 'reader, editor')] + public function testUserWithRoleInContentDoesNotHaveMinimumRole(ContentRole $roleOfUser, ContentRole $roleToCheck): void + { + $user = User::factory() + ->create(); + $content = Content::factory() + ->withUser($user, $roleOfUser) + ->create(); + + $this->assertFalse($content->hasUserWithMinimumRole($user, $roleToCheck)); + } + + #[TestWith([ContentRole::Owner], 'owner')] + #[TestWith([ContentRole::Editor], 'editor')] + #[TestWith([ContentRole::Reader], 'reader')] + public function testRolelessContentHasNeitherUserNorMinimumRole(ContentRole $roleToCheck): void + { + $user = User::factory()->create(); + $content = Content::factory()->create(); + + $this->assertFalse($content->hasUser($user)); + $this->assertFalse($content->hasUserWithMinimumRole($user, $roleToCheck)); + } } diff --git a/sourcecode/hub/tests/Unit/Enum/ContentRoleTest.php b/sourcecode/hub/tests/Unit/Enum/ContentRoleTest.php new file mode 100644 index 000000000..ffaedd9d5 --- /dev/null +++ b/sourcecode/hub/tests/Unit/Enum/ContentRoleTest.php @@ -0,0 +1,34 @@ +assertTrue($role->grants($roleToCheck)); + } + + #[TestDox('Role $role denies $roleToCheck')] + #[TestWith([ContentRole::Editor, ContentRole::Owner], 'editor does not have owner role')] + #[TestWith([ContentRole::Reader, ContentRole::Owner], 'reader does not have owner role')] + #[TestWith([ContentRole::Reader, ContentRole::Editor], 'reader does not have editor role')] + public function testDenies(ContentRole $role, ContentRole $roleToCheck): void + { + $this->assertFalse($role->grants($roleToCheck)); + } +} From 925626f1315351919d08bd920f8e87ca1550532e Mon Sep 17 00:00:00 2001 From: "Emma C. Hughes" <84008144+emmachughes@users.noreply.github.com> Date: Wed, 8 Jan 2025 12:21:27 +0100 Subject: [PATCH 2/3] clean up ContentPolicy --- sourcecode/hub/app/Policies/ContentPolicy.php | 76 ++++++++++--------- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/sourcecode/hub/app/Policies/ContentPolicy.php b/sourcecode/hub/app/Policies/ContentPolicy.php index dca76ce42..4ec47d873 100644 --- a/sourcecode/hub/app/Policies/ContentPolicy.php +++ b/sourcecode/hub/app/Policies/ContentPolicy.php @@ -35,20 +35,11 @@ public function view( return true; } - $platform = $this->getLtiPlatform(); - if ($platform) { - foreach ($content->contexts ?? [] as $context) { - if ($platform->hasContextWithMinimumRole($context, ContentRole::Reader)) { - return true; - } - } - } - - if (!$user) { - return false; + if ($this->hasContentRole(ContentRole::Reader, $content, $user)) { + return true; } - return $content->hasUserWithMinimumRole($user, ContentRole::Reader); + return false; } public function create(User $user): bool @@ -67,6 +58,10 @@ public function edit( return true; } + if ($content->hasUserWithMinimumRole($user, ContentRole::Editor)) { + return true; + } + $platform = $this->getLtiPlatform(); if ($platform) { @@ -77,14 +72,12 @@ public function edit( return true; } - foreach ($content->contexts ?? [] as $context) { - if ($platform->hasContextWithMinimumRole($context, ContentRole::Editor)) { - return true; - } + if ($this->hasContentRole(ContentRole::Editor, $content, $user, $platform)) { + return true; } } - return $content->hasUserWithMinimumRole($user, ContentRole::Editor); + return false; } public function copy( @@ -94,16 +87,11 @@ public function copy( ): bool { $this->ensureVersionBelongsToContent($content, $version); - $platform = $this->getLtiPlatform(); - if ($platform) { - foreach ($content->contexts ?? [] as $context) { - if ($platform->hasContextWithMinimumRole($context, ContentRole::Reader)) { - return true; - } - } + if ($user->admin) { + return true; } - if ($content->hasUserWithMinimumRole($user, ContentRole::Reader)) { + if ($this->hasContentRole(ContentRole::Reader, $content, $user)) { return true; } @@ -130,16 +118,11 @@ public function delete(User $user, Content $content): bool return true; } - $platform = $this->getLtiPlatform(); - if ($platform) { - foreach ($content->contexts ?? [] as $context) { - if ($platform->hasContextWithMinimumRole($context, ContentRole::Owner)) { - return true; - } - } + if ($this->hasContentRole(ContentRole::Owner, $content, $user)) { + return true; } - return $content->hasUserWithMinimumRole($user, ContentRole::Owner); + return false; } public function use(User|null $user, Content $content, ContentVersion $version): bool @@ -167,11 +150,11 @@ public function use(User|null $user, Content $content, ContentVersion $version): public function manageRoles(User $user, Content $content): bool { - if ($content->hasUserWithMinimumRole($user, ContentRole::Owner)) { + if ($user->admin) { return true; } - return false; + return $this->hasContentRole(ContentRole::Owner, $content, $user); } private function ensureVersionBelongsToContent(Content $content, ContentVersion|null $version): void @@ -181,6 +164,29 @@ private function ensureVersionBelongsToContent(Content $content, ContentVersion| } } + private function hasContentRole( + ContentRole $role, + Content $content, + User|null $user = null, + LtiPlatform|null $platform = null, + ): bool { + if ($user && $content->hasUserWithMinimumRole($user, $role)) { + return true; + } + + $platform ??= $this->getLtiPlatform(); + + if ($platform) { + foreach ($content->contexts as $context) { + if ($platform->hasContextWithMinimumRole($context, $role)) { + return true; + } + } + } + + return false; + } + private function getLtiPlatform(): LtiPlatform|null { $key = $this->request->session()->get('lti.oauth_consumer_key'); From e59dd1abe7ffe097aa00f1470996f3662d5b0a74 Mon Sep 17 00:00:00 2001 From: "Emma C. Hughes" <84008144+emmachughes@users.noreply.github.com> Date: Wed, 8 Jan 2025 13:17:53 +0100 Subject: [PATCH 3/3] further simplify ContentPolicy::edit --- sourcecode/hub/app/Policies/ContentPolicy.php | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/sourcecode/hub/app/Policies/ContentPolicy.php b/sourcecode/hub/app/Policies/ContentPolicy.php index 4ec47d873..46f75f78c 100644 --- a/sourcecode/hub/app/Policies/ContentPolicy.php +++ b/sourcecode/hub/app/Policies/ContentPolicy.php @@ -58,23 +58,17 @@ public function edit( return true; } - if ($content->hasUserWithMinimumRole($user, ContentRole::Editor)) { - return true; - } - $platform = $this->getLtiPlatform(); - if ($platform) { - if ( - $platform->authorizes_edit && - $this->request->session()->has('intent-to-edit.' . $content->id) - ) { - return true; - } + if ( + $platform?->authorizes_edit && + $this->request->session()->has('intent-to-edit.' . $content->id) + ) { + return true; + } - if ($this->hasContentRole(ContentRole::Editor, $content, $user, $platform)) { - return true; - } + if ($this->hasContentRole(ContentRole::Editor, $content, $user, $platform)) { + return true; } return false;