Refactor file access #2726
Refactor file access #2726
Conversation
…tor-file-access-2
…tor-file-access-2
samuelwei
force-pushed
the
1409-refactor-file-access-2
branch
from
8797230 to
632dec7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
resources/js/components/RoomTabPersonalizedLinksEditButton.vue (1)
181-195:⚠️ Potential issue | 🟡 MinorFix modal close on 404 — wrong ref is updated.
Line 194 sets
showModal.value = false, butshowModalis a function defined on line 157. The dialog won't close on a 404. UsemodalVisible.value = falseinstead.🛠️ Proposed fix
- showModal.value = false; + modalVisible.value = false;lang/de-gender/rooms.php (1)
426-431:⚠️ Potential issue | 🟡 MinorFix typo in personalized link copied message.
“persionalisierte” should be “personalisierte”.
📝 Suggested fix
- 'room_link_copied' => 'Der persionalisierte Raumlink für :firstname :lastname wurde in Ihre Zwischenablage kopiert.', + 'room_link_copied' => 'Der personalisierte Raumlink für :firstname :lastname wurde in Ihre Zwischenablage kopiert.',lang/de/rooms.php (1)
470-489:⚠️ Potential issue | 🟡 MinorFix typo in German copy for “room_link_copied”.
“persionalisierte” → “personalisierte”.✏️ Suggested fix
- 'room_link_copied' => 'Der persionalisierte Raumlink für :firstname :lastname wurde in Ihre Zwischenablage kopiert.', + 'room_link_copied' => 'Der personalisierte Raumlink für :firstname :lastname wurde in Ihre Zwischenablage kopiert.',
🤖 Fix all issues with AI agents
In `@app/Http/Middleware/RoomAuthenticate.php`:
- Around line 79-80: The inline comment is inaccurate: update the comment in
RoomAuthenticate.php next to the if checking $providedRoomAuthTokenType ===
RoomAuthTokenType::PERSONALIZED_LINK || $room->access_code != null so it states
that the provided invalid room auth token was of type PERSONALIZED_LINK (or room
has an access code set), matching the actual condition that checks
RoomAuthTokenType::PERSONALIZED_LINK and $room->access_code.
In `@app/Settings/RoomSettings.php`:
- Line 12: The 'room' expectedProperties in ProvisioningService (the validation
rule that lists room properties) still uses the old 'token_expiration' name;
update that entry to 'personalized_link_expiration' so it matches the renamed
property public TimePeriod $personalized_link_expiration in RoomSettings.php and
allows provisioning to set the field correctly.
In
`@database/migrations/2025_11_16_075447_add_room_personalized_links_table_indexes.php`:
- Around line 26-29: The down() migration for
Schema::table('room_personalized_links') drops the id column while it is still
the primary key; modify the down() logic to drop the primary key first by
calling $table->dropPrimary() before $table->dropColumn(['id']) so the rollback
won't fail, keeping the existing drops for foreign keys and uniques (e.g., the
block that calls $table->dropForeign(['room_id']) and
$table->dropUnique(['token'])) in the same closure.
In `@lang/en/admin.php`:
- Around line 387-389: The description for the
'room_personalized_link_expiration' entry currently reads "Time period since
last usage, after which personalized room links gets automatically removed." Fix
the grammar by updating the 'description' value to use "get" instead of "gets"
so it reads "...after which personalized room links get automatically removed."
Locate the 'room_personalized_link_expiration' array and update its
'description' string accordingly.
In `@resources/js/components/RoomTabPersonalizedLinksDeleteButton.vue`:
- Around line 120-126: The code incorrectly assigns to showModal.value even
though showModal is a function; update the 404 error branch in the delete error
handler to close the dialog by setting modalVisible.value = false (or calling
showModal(false)) instead of showModal.value = false; keep the emit("notFound")
and toast.error(t(...)) behavior intact so the handler calls modalVisible.value
= false (or showModal(false)), toast.error(...), and emit("notFound").
In `@resources/js/views/RoomsView.vue`:
- Around line 381-391: handleInvalidRoomAuthToken currently calls
window.location.reload() for ROOM_AUTH_TOKEN_TYPE_PERSONALIZED_LINK which can
loop; instead clear the token and set the existing tokenInvalid UI state so the
“invalid personalized link” message is shown (e.g., set tokenInvalid.value =
ROOM_AUTH_TOKEN_TYPE_PERSONALIZED_LINK or tokenInvalid.value = true), or
alternatively navigate to the base room URL using router.replace('/') if you
prefer redirecting; update handleInvalidRoomAuthToken to remove
window.location.reload() and trigger the tokenInvalid state (or router.replace)
so the UI displays the invalid-token message.
In `@tests/Frontend/e2e/AdminSettingsEdit.cy.js`:
- Around line 3139-3141: The test stubs/assertions referencing the validation
key room_personalized_link_expiration still use the old phrase "room token
expiration"; update those error message strings to match the personalized-link
naming used by the backend (e.g., change "The selected room token expiration is
invalid." to "The selected personalized link expiration is invalid.") in the
three places flagged (the occurrences referencing
room_personalized_link_expiration around the asserted/stubbed error messages).
In `@tests/Frontend/e2e/RoomsViewFilesFileActions.cy.js`:
- Around line 1120-1138: The test incorrectly asserts no network reload by
checking the intercept alias value (reloadRoomFilesRequest) for null; intercept
always yields null so replace that check with an assertion against the
intercepted request list using the alias.all (e.g., use
"@reloadRoomFilesRequest.all" and assert its length is 0 or its array is empty)
to ensure no GET "api/v1/rooms/abc-def-123/files*" requests were made after
posting the file_not_found message.
🧹 Nitpick comments (8)
app/Observers/RoomPersonalizedLinkObserver.php (1)
14-23: Make token generation race-safe and bounded.
Line 16 uses an unbounded loop plus a non-atomicdoesntExist()check; if a unique index onroom_personalized_links.tokenis missing, concurrent creates could still collide. Please confirm the DB-level unique constraint and consider a bounded retry (and respecting any pre-set token) to avoid infinite looping.Suggested hardening
public function creating(RoomPersonalizedLink $personalizedLink): void { - while (true) { - $token = Str::random(100); - if (DB::table('room_personalized_links')->where('token', '=', $token)->doesntExist()) { - $personalizedLink->token = $token; - - break; - } - } + if ($personalizedLink->token) { + return; + } + for ($attempts = 0; $attempts < 5; $attempts++) { + $token = Str::random(100); + if (DB::table('room_personalized_links')->where('token', $token)->doesntExist()) { + $personalizedLink->token = $token; + return; + } + } + throw new \RuntimeException('Unable to generate unique room personalized link token.'); }app/Http/Resources/Room.php (1)
56-56: Consider simplifying the conditional.The
!empty($this->personalizedLink)check is evaluated twice - once for thewhen()condition and again in the ternary operator. While functionally correct, this could be simplified.♻️ Optional simplification
- 'username' => $this->when(! empty($this->personalizedLink), ! empty($this->personalizedLink) ? $this->personalizedLink->fullname : null), + 'username' => $this->when($this->personalizedLink !== null, fn() => $this->personalizedLink->fullname),app/Http/Middleware/RoomAuthenticate.php (1)
51-68: UseRule::enum()for consistency with the enum validation pattern used elsewhere.While Laravel 12.3's backed int enums may handle the string-to-int conversion implicitly, the validation pattern here differs from
RoomAuthRequestwhich usesRule::enum(RoomAuthTokenType::class)for the same type of validation. For clarity and consistency across the codebase, change line 55 to use the dedicated enum validator:'room_auth_token_type' => ['required', Rule::enum(RoomAuthTokenType::class)],This eliminates potential type coercion edge cases and aligns with Laravel's recommended enum validation approach.
tests/Backend/Unit/Console/DeleteObsoletePersonalizedLinksTest.php (2)
19-22: RedundantsetUp()method.This
setUp()method only callsparent::setUp()and adds no additional setup logic. It can be safely removed.♻️ Proposed fix
- /** - * `@see` TestCase::setUp() - */ - protected function setUp(): void - { - parent::setUp(); - } -
35-35: Inconsistent test method name.The method name
test_deletion_of_expired_room_tokensstill uses "tokens" terminology, which is inconsistent with the refactoring to "personalized links".♻️ Proposed fix
- public function test_deletion_of_expired_room_tokens() + public function test_deletion_of_expired_room_links()tests/Backend/Feature/api/v1/Room/MembershipTest.php (1)
95-100: Consider using explicit->valuefor consistency with other test files.While both
RoomAuthTokenType::PERSONALIZED_LINKandRoomAuthTokenType::PERSONALIZED_LINK->valuework correctly in query parameters (Laravel 12.3 automatically serializes enums), other test files explicitly use->valuefor this parameter. For consistency, align with the pattern used inRoomDescriptionTest.phpandRoomTest.php.tests/Frontend/e2e/RoomsViewMeetings.cy.js (1)
2853-2854: Missingcy.wait("@roomRequest")after initial visit in personalized link test.In the "start meeting load requirements errors" test,
cy.interceptRoomFilesRequest()is called but there's no explicit room request intercept set up before the personalized link visit at Line 2963. This could cause flaky tests if the room request timing varies.tests/Backend/Feature/api/v1/Room/FileTest.php (1)
431-432: Drop redundantsave()aftercreate().
RoomAuthToken::factory()->create(...)already persists the model.♻️ Suggested cleanup
- $roomAuthToken->save(); ... - $roomAuthToken->save();Also applies to: 503-504
| Schema::table('room_personalized_links', function (Blueprint $table) { | ||
| $table->dropColumn(['id']); | ||
| $table->dropForeign(['room_id']); | ||
| $table->dropUnique(['token']); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the migration file
fd "2025_11_16_075447_add_room_personalized_links_table_indexes.php" --type fRepository: THM-Health/PILOS
Length of output: 143
🏁 Script executed:
# Read the migration file
cat -n database/migrations/2025_11_16_075447_add_room_personalized_links_table_indexes.phpRepository: THM-Health/PILOS
Length of output: 1112
🏁 Script executed:
# Find the migration that created room_personalized_links table
fd "create.*room_personalized_links" --type fRepository: THM-Health/PILOS
Length of output: 42
🏁 Script executed:
# Search for room_personalized_links in migration files
rg "room_personalized_links" database/migrations --type php -B 2 -A 2Repository: THM-Health/PILOS
Length of output: 2239
🏁 Script executed:
# Find the migration that created room_tokens table originally
rg "room_tokens" database/migrations --type php -lRepository: THM-Health/PILOS
Length of output: 334
🏁 Script executed:
# Check the original room_tokens creation migration
cat -n database/migrations/2022_07_21_000015_create_room_tokens_table.phpRepository: THM-Health/PILOS
Length of output: 1336
🏁 Script executed:
# Check other migrations to see how they handle dropping primary key columns
rg "dropColumn.*id" database/migrations --type php -B 3 -A 1Repository: THM-Health/PILOS
Length of output: 2787
🏁 Script executed:
# Check the migration that drops indexes to understand the sequence
cat -n database/migrations/2025_11_16_075445_drop_room_token_table_indexes.phpRepository: THM-Health/PILOS
Length of output: 1099
The down() method will fail during rollback: the id column cannot be dropped without first removing the primary key constraint.
In MySQL, you cannot drop a column that is part of a primary key without first dropping the primary key. Add $table->dropPrimary() before $table->dropColumn(['id']) to ensure rollbacks work correctly.
🛠️ Suggested fix
public function down(): void
{
Schema::table('room_personalized_links', function (Blueprint $table) {
+ $table->dropPrimary();
$table->dropColumn(['id']);
$table->dropForeign(['room_id']);
$table->dropUnique(['token']);
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Schema::table('room_personalized_links', function (Blueprint $table) { | |
| $table->dropColumn(['id']); | |
| $table->dropForeign(['room_id']); | |
| $table->dropUnique(['token']); | |
| Schema::table('room_personalized_links', function (Blueprint $table) { | |
| $table->dropPrimary(); | |
| $table->dropColumn(['id']); | |
| $table->dropForeign(['room_id']); | |
| $table->dropUnique(['token']); |
🤖 Prompt for AI Agents
In
`@database/migrations/2025_11_16_075447_add_room_personalized_links_table_indexes.php`
around lines 26 - 29, The down() migration for
Schema::table('room_personalized_links') drops the id column while it is still
the primary key; modify the down() logic to drop the primary key first by
calling $table->dropPrimary() before $table->dropColumn(['id']) so the rollback
won't fail, keeping the existing drops for foreign keys and uniques (e.g., the
block that calls $table->dropForeign(['room_id']) and
$table->dropUnique(['token'])) in the same closure.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@app/Http/Middleware/RoomAuthenticate.php`:
- Around line 51-70: The validator in the RoomAuthenticate middleware is
comparing incoming string query values to enum objects causing valid inputs to
fail; update the validation rule for 'room_auth_token_type' to validate against
the enum backing values or use Laravel's Rule::enum(RoomAuthTokenType::class),
and then ensure you convert/cast the validated value before calling
RoomAuthTokenType::from (e.g., cast to int when using backing values); locate
the validator creation in RoomAuthenticate (the Validator::make call) and
replace Rule::in([RoomAuthTokenType::CODE,
RoomAuthTokenType::PERSONALIZED_LINK]) with either
Rule::enum(RoomAuthTokenType::class) or
Rule::in([RoomAuthTokenType::CODE->value,
RoomAuthTokenType::PERSONALIZED_LINK->value]) and if you choose the latter cast
the validated 'room_auth_token_type' to the enum's backing type before calling
RoomAuthTokenType::from.
In `@resources/js/components/RoomTabFiles.vue`:
- Around line 443-465: The handler function handleFileErrorMessages should
validate event.data before accessing event.data.type to avoid runtime errors
from unrelated postMessage traffic; add an early guard (e.g., if (!event.data ||
typeof event.data !== "object" || !("type" in event.data)) return) immediately
after the origin check, then proceed with the existing checks for
HTTP_ROOM_FILE_NOT_FOUND, HTTP_ROOM_INVALID_TOKEN, HTTP_ROOM_REQUIRE_CODE,
HTTP_ROOM_FILE_FORBIDDEN, HTTP_ROOM_GUESTS_NOT_ALLOWED, and
HTTP_ROOM_GUESTS_ONLY so loadData(), emit("invalidRoomAuthToken"),
EventBus.emit(EVENT_FORBIDDEN), and api.handleGuestsOnly() are only called when
a well-formed payload is present.
In `@resources/js/views/RoomsView.vue`:
- Around line 645-648: When handling the /auth response in RoomsView.vue, ensure
stale tokens are cleared when the server returns 204: currently the block only
sets roomAuthToken.value when response.status !== 204, but it does not
explicitly clear the token on 204; update the logic around response.status and
roomAuthToken (the reactive variable roomAuthToken.value) so that when
response.status === 204 you set roomAuthToken.value to null (or empty string) to
avoid re-sending an invalid token, and keep the existing behavior of assigning
response.data.data when status !== 204.
In `@tests/Backend/Feature/api/v1/RecordingTest.php`:
- Around line 186-190: The route call is passing the enum constant
RoomAuthTokenType::PERSONALIZED_LINK directly; change it to use the backed enum
string value (RoomAuthTokenType::PERSONALIZED_LINK->value) wherever used in this
test (notably in the api.v1.rooms.recordings.index call and the other
occurrences around the specified ranges) so the route parameter gets the correct
string; update each occurrence to use the enum's ->value (matching how
RoomAuthTokenType::CODE->value is used elsewhere).
In `@tests/Backend/Feature/api/v1/Room/FileTest.php`:
- Around line 989-992: Replace the thrown-exception assertion with a response
status assertion: remove the expectException(NotFoundHttpException::class) call
and call assertNotFound() on the response returned by $this->get((new
RoomFileService($room_file))->url()) (either by chaining ->assertNotFound() or
storing the response and calling $response->assertNotFound()); this ensures the
test checks the 404 HTTP response produced by RoomFileService::url() under
Laravel's exception handling.
In `@tests/Backend/Feature/api/v1/Room/RoomTest.php`:
- Around line 650-738: The failing tests pass enum instances directly as the
room_auth_token_type query param which serializes incorrectly; update all usages
in tests (e.g., inside test_room_auth_token_access_code_guests and every call to
route('api.v1.rooms.show') and other room route calls) to pass RoomAuthTokenType
enum values by appending ->value (e.g., RoomAuthTokenType::CODE->value) wherever
room_auth_token_type is supplied; search for RoomAuthTokenType occurrences
across the test file and replace the enum instance arguments with their ->value
string form so query strings serialize correctly.
In `@tests/Frontend/e2e/RoomsViewFilesFileActions.cy.js`:
- Around line 753-760: The test currently asserts the href has a specific value
which is ambiguous; update the assertion on the element selected by
cy.get('[data-test="room-file-item"]').eq(0).find('[data-test="room-files-view-button"]')
to explicitly assert the href attribute is absent by using
.should("not.have.attr", "href") instead of .should("not.have.attr", "href",
"https://example.com/files/File1.pdf?signature=abc123"), ensuring the link is
truly not present before terms-of-use acceptance.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/Http/Middleware/RoomAuthenticate.php`:
- Around line 54-69: The validated room_auth_token_type comes back as a string
from the request so calling
RoomAuthTokenType::from($validated['room_auth_token_type']) can throw ValueError
for an int-backed enum; fix by casting the validated value to int before calling
RoomAuthTokenType::from (e.g., use (int)$validated['room_auth_token_type'] when
assigning $providedRoomAuthTokenType) so the enum conversion succeeds; adjust
the assignment near the validator/validated block that uses
Rule::enum(RoomAuthTokenType::class) and $validated['room_auth_token_type'].
In `@resources/js/components/RoomTabFiles.vue`:
- Around line 392-395: The file list doesn't reload when the prop roomAuthToken
changes; add a watcher on props.roomAuthToken that calls the existing loadData()
to refresh files whenever the token changes (including when it becomes set
asynchronously or cleared). Locate the component's loadData function and the
current room change/mount logic, then add a Vue watch on the roomAuthToken prop
(or watch(() => props.roomAuthToken, ...)) that invokes loadData() whenever the
value changes, ensuring you use the same signature/context as existing calls so
auth params (config.params.room_auth_token and room_auth_token_type) are
applied.
| $validator = Validator::make($request->all(), [ | ||
| 'room_auth_token' => ['required', 'uuid'], | ||
| 'room_auth_token_type' => ['required', Rule::enum(RoomAuthTokenType::class)], | ||
| ]); | ||
|
|
||
| if ($token == null) { | ||
| Counter::get('room_authentication_errors_total')->inc('token'); | ||
| Log::notice('Room token authentication failed for room {room}', ['room' => $room->getLogLabel()]); | ||
| abort(401, 'invalid_token'); | ||
| if ($validator->stopOnFirstFailure()->fails()) { | ||
| Counter::get('room_authentication_errors_total')->inc('room_auth_token_invalid'); | ||
| Log::notice('Room auth token authentication failed for room {room} (Input validation failed)', ['room' => $room->getLogLabel()]); | ||
|
|
||
| return $this->handleError(CustomErrorMessages::ROOM_INVALID_TOKEN->value, 401, 'Invalid token', __('rooms.flash.auth_token_invalid')); | ||
| } | ||
|
|
||
| $token->last_usage = now(); | ||
| $token->save(); | ||
| // Retrieve the validated input | ||
| $validated = $validator->validated(); | ||
| $providedRoomAuthToken = $validated['room_auth_token']; | ||
| $providedRoomAuthTokenType = RoomAuthTokenType::from($validated['room_auth_token_type']); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the RoomAuthTokenType enum definition
cat app/Enums/RoomAuthTokenType.php
# Search for room_auth_token_type usage to understand how it's sent
rg -n "room_auth_token_type" -S --type=php --type=ts --type=js --type=vueRepository: THM-Health/PILOS
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Search more broadly for room_auth_token_type usage
rg -n "room_auth_token_type" -S --type=php
# Also check for test files that might show how this is used
find . -name "*.php" -path "*/tests/*" | xargs grep -l "room_auth_token_type" 2>/dev/null | head -5Repository: THM-Health/PILOS
Length of output: 19459
Cast to int before RoomAuthTokenType::from() on line 69.
The Rule::enum(RoomAuthTokenType::class) validates the input but does not cast it. Since the enum is int-backed and HTTP requests transmit values as strings, $validated['room_auth_token_type'] will be a string (e.g., "0" or "1"). Calling from() with a string argument on an int-backed enum will throw ValueError. Cast to int before calling from():
- $providedRoomAuthTokenType = RoomAuthTokenType::from($validated['room_auth_token_type']);
+ $providedRoomAuthTokenType = RoomAuthTokenType::from((int) $validated['room_auth_token_type']);🤖 Prompt for AI Agents
In `@app/Http/Middleware/RoomAuthenticate.php` around lines 54 - 69, The validated
room_auth_token_type comes back as a string from the request so calling
RoomAuthTokenType::from($validated['room_auth_token_type']) can throw ValueError
for an int-backed enum; fix by casting the validated value to int before calling
RoomAuthTokenType::from (e.g., use (int)$validated['room_auth_token_type'] when
assigning $providedRoomAuthTokenType) so the enum conversion succeeds; adjust
the assignment near the validator/validated block that uses
Rule::enum(RoomAuthTokenType::class) and $validated['room_auth_token_type'].
There was a problem hiding this comment.
Currently seems to work. ::from seems to also accept and cast string arguments, but not completely sure about that.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHP 8.1 backed enum from() method type coercion string to int
💡 Result:
BackedEnum::from() is not “always string-or-int tolerant”; it follows the enum’s backing type, and strict_types changes whether cross-type coercion is allowed.
- Weak typing (default, no
declare(strict_types=1);): passing a numeric string to an int-backed enum will be coerced to int and can match a case (and vice‑versa for string-backed enums). Floats may also be coerced. [1] - Strict typing (
declare(strict_types=1);): passing the “other” scalar type (e.g., string to an int-backed enum, or int to a string-backed enum) throws aTypeError. [1] - If the (possibly coerced) value still doesn’t match any case,
from()throwsValueError. [2]
Example (int-backed):
declare(strict_types=1);
enum Status: int { case Draft = 1; }
Status::from("1"); // TypeError in strict mode
Status::from((int)"1"); // OKSources: [1] (PHP manual: Backed enumerations – coercion + strict typing rules), [2] (PHP manual: BackedEnum::from() – ValueError behavior).
🏁 Script executed:
#!/bin/bash
# Check if this specific file uses strict_types
head -10 app/Http/Middleware/RoomAuthenticate.php | grep -i "strict_types"
# Check how many PHP files in the project use strict_types
rg "declare\(strict_types\s*=\s*1\)" --type=php -c | head -20
# Check if any middleware files use strict_types
rg "declare\(strict_types\s*=\s*1\)" app/Http/Middleware/ --type=php
Length of output: 0
@Sabr1n4W, you're correct! The code works as-is.
PHP's BackedEnum::from() behavior depends on the strict_types setting:
- Weak typing (default): Numeric strings like
"0"and"1"are automatically coerced to integers when callingfrom()on an int-backed enum. - Strict typing (
declare(strict_types=1);): Would throw aTypeErrorwithout an explicit cast.
Your project doesn't use strict type declarations, so PHP automatically coerces the validated string values to integers, and the enum conversion succeeds. The explicit cast (int) is not required in your current setup.
That said, if you ever introduce strict typing in the future, the explicit cast would become necessary. But for now, the code is functioning correctly without it.
🧬 Code Graph Analysis Results
<filename: app/Models/RoomAuthToken.php>
class RoomAuthToken extends Model
{
use HasFactory;
use HasUuids;
protected $casts = [
'type' => RoomAuthTokenType::class,
];
protected $fillable = [
'type',
'room_id',
'room_personalized_link_id',
'session_id',
];
public function room()
{
return $this->belongsTo(Room::class);
}
public function personalizedLink()
{
return $this->belongsTo(RoomPersonalizedLink::class, 'room_personalized_link_id');
}
public function session()
{
return $this->belongsTo(Session::class);
}
}
| if (props.roomAuthToken) { | ||
| config.params.room_auth_token = props.roomAuthToken.id; | ||
| config.params.room_auth_token_type = props.roomAuthToken.type; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where RoomTabFiles is used and how roomAuthToken is passed/updated
echo "=== RoomTabFiles usage ==="
rg -n 'RoomTabFiles' --type=vue
echo -e "\n=== roomAuthToken references ==="
rg -n 'roomAuthToken' --type=vueRepository: THM-Health/PILOS
Length of output: 193
🏁 Script executed:
#!/bin/bash
echo "=== RoomTabFiles usage in .vue files ==="
rg -n 'RoomTabFiles' -g '*.vue'
echo -e "\n=== roomAuthToken references in .vue files ==="
rg -n 'roomAuthToken' -g '*.vue'
echo -e "\n=== Locate RoomTabFiles.vue file ==="
fd -n 'RoomTabFiles.vue'Repository: THM-Health/PILOS
Length of output: 5528
🏁 Script executed:
#!/bin/bash
echo "=== RoomTabFiles.vue - Check watchers and loadData calls ==="
cat resources/js/components/RoomTabFiles.vue | head -400 | tail -100
echo -e "\n=== Check for watch/computed in RoomTabFiles ==="
rg -n 'watch|computed|onMounted|onRoomHasChanged' resources/js/components/RoomTabFiles.vueRepository: THM-Health/PILOS
Length of output: 3265
🏁 Script executed:
#!/bin/bash
echo "=== onMounted and onRoomHasChanged implementation ==="
sed -n '469,510p' resources/js/components/RoomTabFiles.vue
echo -e "\n=== Check if there's a watch for roomAuthToken ==="
rg -A 5 'watch.*roomAuthToken' resources/js/components/RoomTabFiles.vueRepository: THM-Health/PILOS
Length of output: 476
Add watcher for roomAuthToken to reload file list when token changes.
The component loads files only on mount and when the room changes, but not when roomAuthToken changes. If the token is obtained asynchronously (e.g., user enters an access code), the file list won't refresh because loadData() uses the token but has no reactive dependency on prop changes.
Proposed fix
-import { computed, onMounted, onUnmounted, ref } from "vue";
+import { computed, onMounted, onUnmounted, ref, watch } from "vue";
@@
onUnmounted(() => {
window.removeEventListener("message", handleFileErrorMessages);
});
+watch(
+ () => props.roomAuthToken,
+ (next, prev) => {
+ if (next?.id !== prev?.id || next?.type !== prev?.type) {
+ loadData(1);
+ }
+ },
+);
+
onRoomHasChanged(
() => props.room,
() => loadData(),
);🤖 Prompt for AI Agents
In `@resources/js/components/RoomTabFiles.vue` around lines 392 - 395, The file
list doesn't reload when the prop roomAuthToken changes; add a watcher on
props.roomAuthToken that calls the existing loadData() to refresh files whenever
the token changes (including when it becomes set asynchronously or cleared).
Locate the component's loadData function and the current room change/mount
logic, then add a Vue watch on the roomAuthToken prop (or watch(() =>
props.roomAuthToken, ...)) that invokes loadData() whenever the value changes,
ensuring you use the same signature/context as existing calls so auth params
(config.params.room_auth_token and room_auth_token_type) are applied.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/Backend/Feature/api/v1/Room/RoomPersonalizedLinkTest.php`:
- Around line 47-53: The RoomPersonalizedLink factory calls pass the whole model
($this->room) into the 'room_id' field which can serialize the model and break
inserts; update each occurrence (e.g., the RoomPersonalizedLink::factory() calls
that create test records including the $moderatorLink) to provide the actual
foreign key (use 'room_id' => $this->room->id) or switch to Laravel factory
relation syntax (->for($this->room)) so the factories set the room relationship
correctly.
In `@tests/Backend/Feature/api/v1/Room/RoomTest.php`:
- Around line 821-884: The test_auth_with_access_code_user contains several POST
requests that run unauthenticated because they omit actingAs($this->user), which
makes assertions about guest vs authenticated behavior unreliable; fix by making
all requests within this test authenticate as the same user—either call
$this->actingAs($this->user) once at the start of the test or prepend
->actingAs($this->user) to every postJson(...) that should run as the user
(targets: postJson(route('api.v1.rooms.authenticate', ['room' => $room])) and
any subsequent postJson calls), ensuring guest-only assertions remain explicit
and isolated.
In `@tests/Frontend/e2e/RoomsViewGeneral.cy.js`:
- Around line 1560-1583: Replace the incorrect assertion that checks the alias
directly with the Cypress idiom that inspects the intercepted requests array:
after the reload and roomRequest wait, change the final assertion using the
alias differentRoomAuthRequest to use
cy.get('@differentRoomAuthRequest.all').should('have.length', 0) so the test
verifies no requests matched the intercept; locate the usage of
cy.get("@differentRoomAuthRequest").should("be.null") and update it to the .all
length check on the differentRoomAuthRequest alias.
| RoomPersonalizedLink::factory()->create(['firstname' => 'John', 'lastname' => 'Doe', 'role' => RoomUserRole::USER, 'last_usage' => '2024-04-01 08:00', 'room_id' => $this->room]); | ||
| RoomPersonalizedLink::factory()->create(['firstname' => 'Daniel', 'lastname' => 'Osorio', 'role' => RoomUserRole::USER, 'last_usage' => '2024-04-01 09:00', 'room_id' => $this->room]); | ||
| RoomPersonalizedLink::factory()->create(['firstname' => 'Angela', 'lastname' => 'Jones', 'role' => RoomUserRole::USER, 'last_usage' => null, 'room_id' => $this->room]); | ||
| RoomPersonalizedLink::factory()->create(['firstname' => 'Thomas', 'lastname' => 'Bolden', 'role' => RoomUserRole::USER, 'last_usage' => '2024-04-01 10:00', 'room_id' => $this->room]); | ||
| RoomPersonalizedLink::factory()->create(['firstname' => 'Hoyt', 'lastname' => 'Hastings', 'role' => RoomUserRole::MODERATOR, 'last_usage' => '2024-04-01 11:00', 'room_id' => $this->room]); | ||
| $moderatorLink = RoomPersonalizedLink::factory()->create(['firstname' => 'William', 'lastname' => 'White', 'role' => RoomUserRole::MODERATOR, 'last_usage' => null, 'room_id' => $this->room]); | ||
|
|
There was a problem hiding this comment.
Fix room_id assignments in factories.
Line 47 and the other listed locations pass $this->room into a foreign key field; this can serialize the model or break inserts. Use $this->room->id (or ->for($this->room)) consistently.
🔧 Suggested fix (apply to each occurrence)
-RoomPersonalizedLink::factory()->create([..., 'room_id' => $this->room]);
+RoomPersonalizedLink::factory()->create([..., 'room_id' => $this->room->id]);Also applies to: 213-216, 302-309, 411-417
🤖 Prompt for AI Agents
In `@tests/Backend/Feature/api/v1/Room/RoomPersonalizedLinkTest.php` around lines
47 - 53, The RoomPersonalizedLink factory calls pass the whole model
($this->room) into the 'room_id' field which can serialize the model and break
inserts; update each occurrence (e.g., the RoomPersonalizedLink::factory() calls
that create test records including the $moderatorLink) to provide the actual
foreign key (use 'room_id' => $this->room->id) or switch to Laravel factory
relation syntax (->for($this->room)) so the factories set the room relationship
correctly.
| public function test_auth_with_access_code_user() | ||
| { | ||
| $room = Room::factory()->create([ | ||
| 'allow_guests' => true, | ||
| 'access_code' => $this->createAccessCode(), | ||
| ]); | ||
|
|
||
| $currentSession = $this->startNewSession(); | ||
|
|
||
| // Try without access code | ||
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room])) | ||
| ->assertStatus(422) | ||
| ->assertJsonValidationErrors(['type']); | ||
|
|
||
| // Try with missing access code | ||
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value]) | ||
| ->assertStatus(422) | ||
| ->assertJsonValidationErrors(['access_code']); | ||
|
|
||
| // Try with empty acces code | ||
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => '']) | ||
| ->assertStatus(422) | ||
| ->assertJsonValidationErrors(['access_code']); | ||
|
|
||
| // Try with random access code | ||
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $this->createAccessCode()]) | ||
| ->assertUnauthorized() | ||
| ->assertJsonFragment(['message' => CustomErrorMessages::ROOM_INVALID_CODE->value]); | ||
|
|
||
| // Try with correct access code | ||
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code]) | ||
| ->assertStatus(201); | ||
|
|
||
| // Check that room auth token was created | ||
| $this->assertDatabaseCount('room_auth_tokens', 1); | ||
| $this->assertDatabaseHas('room_auth_tokens', [ | ||
| 'room_id' => $room->id, | ||
| 'session_id' => $currentSession->id, | ||
| 'type' => RoomAuthTokenType::CODE->value, | ||
| ]); | ||
|
|
||
| // Try with guests not allowed | ||
| $room->allow_guests = false; | ||
| $room->save(); | ||
|
|
||
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code]) | ||
| ->assertStatus(200); | ||
|
|
||
| // Try with access code but room does not require access code | ||
| $room->access_code = null; | ||
| $room->save(); | ||
|
|
||
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => '999999999']) | ||
| ->assertStatus(204); | ||
|
|
||
| // Try with access code but user is member (user) | ||
| $room->access_code = $this->createAccessCode(); | ||
| $room->save(); | ||
|
|
||
| $room->members()->sync([$this->user->id => ['role' => RoomUserRole::USER]]); | ||
|
|
||
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code]) | ||
| ->assertStatus(204); | ||
| } |
There was a problem hiding this comment.
Keep user-auth flow consistently authenticated in this test
In test_auth_with_access_code_user, several requests drop actingAs($this->user), so they run as guests. That makes assertions like “guests not allowed → 200” unreliable and can mask regressions.
✅ Suggested fix (apply consistently in this test)
- $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value])
+ $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value])
- $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => ''])
+ $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => ''])
- $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $this->createAccessCode()])
+ $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $this->createAccessCode()])
- $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code])
+ $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code])
- $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code])
+ $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code])
- $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => '999999999'])
+ $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => '999999999'])
- $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code])
+ $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function test_auth_with_access_code_user() | |
| { | |
| $room = Room::factory()->create([ | |
| 'allow_guests' => true, | |
| 'access_code' => $this->createAccessCode(), | |
| ]); | |
| $currentSession = $this->startNewSession(); | |
| // Try without access code | |
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room])) | |
| ->assertStatus(422) | |
| ->assertJsonValidationErrors(['type']); | |
| // Try with missing access code | |
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value]) | |
| ->assertStatus(422) | |
| ->assertJsonValidationErrors(['access_code']); | |
| // Try with empty acces code | |
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => '']) | |
| ->assertStatus(422) | |
| ->assertJsonValidationErrors(['access_code']); | |
| // Try with random access code | |
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $this->createAccessCode()]) | |
| ->assertUnauthorized() | |
| ->assertJsonFragment(['message' => CustomErrorMessages::ROOM_INVALID_CODE->value]); | |
| // Try with correct access code | |
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code]) | |
| ->assertStatus(201); | |
| // Check that room auth token was created | |
| $this->assertDatabaseCount('room_auth_tokens', 1); | |
| $this->assertDatabaseHas('room_auth_tokens', [ | |
| 'room_id' => $room->id, | |
| 'session_id' => $currentSession->id, | |
| 'type' => RoomAuthTokenType::CODE->value, | |
| ]); | |
| // Try with guests not allowed | |
| $room->allow_guests = false; | |
| $room->save(); | |
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code]) | |
| ->assertStatus(200); | |
| // Try with access code but room does not require access code | |
| $room->access_code = null; | |
| $room->save(); | |
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => '999999999']) | |
| ->assertStatus(204); | |
| // Try with access code but user is member (user) | |
| $room->access_code = $this->createAccessCode(); | |
| $room->save(); | |
| $room->members()->sync([$this->user->id => ['role' => RoomUserRole::USER]]); | |
| $this->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code]) | |
| ->assertStatus(204); | |
| } | |
| public function test_auth_with_access_code_user() | |
| { | |
| $room = Room::factory()->create([ | |
| 'allow_guests' => true, | |
| 'access_code' => $this->createAccessCode(), | |
| ]); | |
| $currentSession = $this->startNewSession(); | |
| // Try without access code | |
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room])) | |
| ->assertStatus(422) | |
| ->assertJsonValidationErrors(['type']); | |
| // Try with missing access code | |
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value]) | |
| ->assertStatus(422) | |
| ->assertJsonValidationErrors(['access_code']); | |
| // Try with empty acces code | |
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => '']) | |
| ->assertStatus(422) | |
| ->assertJsonValidationErrors(['access_code']); | |
| // Try with random access code | |
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $this->createAccessCode()]) | |
| ->assertUnauthorized() | |
| ->assertJsonFragment(['message' => CustomErrorMessages::ROOM_INVALID_CODE->value]); | |
| // Try with correct access code | |
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code]) | |
| ->assertStatus(201); | |
| // Check that room auth token was created | |
| $this->assertDatabaseCount('room_auth_tokens', 1); | |
| $this->assertDatabaseHas('room_auth_tokens', [ | |
| 'room_id' => $room->id, | |
| 'session_id' => $currentSession->id, | |
| 'type' => RoomAuthTokenType::CODE->value, | |
| ]); | |
| // Try with guests not allowed | |
| $room->allow_guests = false; | |
| $room->save(); | |
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code]) | |
| ->assertStatus(200); | |
| // Try with access code but room does not require access code | |
| $room->access_code = null; | |
| $room->save(); | |
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => '999999999']) | |
| ->assertStatus(204); | |
| // Try with access code but user is member (user) | |
| $room->access_code = $this->createAccessCode(); | |
| $room->save(); | |
| $room->members()->sync([$this->user->id => ['role' => RoomUserRole::USER]]); | |
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.authenticate', ['room' => $room]), ['type' => RoomAuthTokenType::CODE->value, 'access_code' => $room->access_code]) | |
| ->assertStatus(204); | |
| } |
🤖 Prompt for AI Agents
In `@tests/Backend/Feature/api/v1/Room/RoomTest.php` around lines 821 - 884, The
test_auth_with_access_code_user contains several POST requests that run
unauthenticated because they omit actingAs($this->user), which makes assertions
about guest vs authenticated behavior unreliable; fix by making all requests
within this test authenticate as the same user—either call
$this->actingAs($this->user) once at the start of the test or prepend
->actingAs($this->user) to every postJson(...) that should run as the user
(targets: postJson(route('api.v1.rooms.authenticate', ['room' => $room])) and
any subsequent postJson calls), ensuring guest-only assertions remain explicit
and isolated.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/Frontend/e2e/RoomsViewGeneral.cy.js`:
- Around line 701-703: Two tests share the identical title string "room view
with access code errors", which makes test output ambiguous; locate the
duplicate it(...) blocks (the one currently titled "room view with access code
errors") and rename one to a unique, descriptive title (for example "room view
with access code errors - owner" or "room view with access code errors
(alternate scenario)") so both it() descriptions are distinct and clearly
indicate which scenario they cover.
…tor-file-access-2 # Conflicts: # tests/Frontend/e2e/RoomsViewFilesFileActions.cy.js
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@tests/Backend/Feature/api/v1/Room/FileTest.php`:
- Around line 200-206: The test is passing the wrong identifier for room auth
tokens: replace uses of $roomAuthToken->id with the actual token value (e.g.,
$roomAuthToken->token or $roomAuthToken->getKey()) when populating the
'room_auth_token' route parameter (seen in the getJson call to
route('api.v1.rooms.files.get') and other occurrences), and update any related
assertions or ordering logic that assumed an integer id to use the token (and
room_id where applicable) since the room_tokens table uses a composite primary
key (token, room_id).
In `@tests/Backend/Feature/api/v1/Room/RoomTest.php`:
- Around line 3189-3207: The second POST to route('api.v1.rooms.start') is
missing the authenticated context and is testing a guest path; update the call
so it uses the same authenticated actor as the first request by chaining
actingAs($this->user) before postJson (keep the same payload and room_auth_token
parameters), ensuring the test exercises the authenticated flow involving
startNewSession, RoomAuthToken::factory(...) and RoomAuthTokenType::CODE.
- Around line 3360-3379: The failing assertions are using unauthenticated
requests for the "authorized user with invalid token" paths; update the two
postJson calls after the first to be authenticated by chaining
->actingAs($this->user) (same as the first request) so they run as the intended
user, keeping the route('api.v1.rooms.start', [...]) params and the body payload
unchanged and preserving the assertions that expect ROOM_INVALID_AUTH_TOKEN via
CustomErrorMessages and RoomAuthTokenType::CODE references.
In `@tests/Frontend/e2e/RoomsViewMeetings.cy.js`:
- Line 2271: Update the inline comment in the test "RoomsViewMeetings.cy.js"
that currently reads "// Check thet room auth token is reset" to correct the
typo to "// Check that room auth token is reset" so the comment is grammatically
correct and clear; locate the comment near the test assertion that verifies the
room auth token reset and replace "thet" with "that".
🧹 Nitpick comments (3)
tests/Backend/Feature/api/v1/RecordingTest.php (1)
711-743: Consider using a constant for the unauthorized message.The error message
'This action is unauthorized.'is hardcoded while other error assertions in this file use theCustomErrorMessagesenum (e.g.,CustomErrorMessages::ROOM_REQUIRE_CODE->value).If this is Laravel's default authorization message, consider either:
- Extracting it to a constant for maintainability
- Adding a comment explaining this is Laravel's default message
This improves consistency and makes future message changes easier to track.
♻️ Example refactor
- ->assertJsonFragment(['message' => 'This action is unauthorized.']); + ->assertJsonFragment(['message' => __('This action is unauthorized.')]);Or if a constant exists/should be added:
- ->assertJsonFragment(['message' => 'This action is unauthorized.']); + ->assertJsonFragment(['message' => CustomErrorMessages::UNAUTHORIZED->value]);tests/Frontend/e2e/RoomsViewRecordingsRecordingActions.cy.js (1)
131-134: Scope room GET intercepts to avoid overlapping recordings routes.The wildcard pattern
api/v1/rooms/abc-def-123*matches recording paths like/api/v1/rooms/abc-def-123/recordings/...because minimatch treats*as matching any character including/. In Cypress, when multiple intercepts match the same request, the most recently defined one runs first and stops propagation if it returns a response. If the room intercept is defined after a recordings intercept, it will intercept recording requests incorrectly. Constraining the room intercept to its exact pathname prevents this order-dependent brittleness.♻️ Suggested pattern (apply similarly to all room GET intercepts)
- cy.intercept("GET", "api/v1/rooms/abc-def-123*", { + cy.intercept( + { method: "GET", pathname: "/api/v1/rooms/abc-def-123" }, + { statusCode: 200, body: room, - }).as("roomRequest"); + }, + ).as("roomRequest");Also applies to: 232-235, 260-263, 299-302, 327-330, 370-373, 457-460
tests/Frontend/e2e/RoomsViewMeetings.cy.js (1)
822-825: Consider using a consistent assertion style for verifying tokens are cleared.At lines 888-890, you use
.to.be.undefinedchecks which explicitly verify the query parameters don't exist. Here you use.to.not.contain()which would also pass if the keys exist with different values. For consistency and precision, consider using the same approach throughout.Suggested change for consistency
- cy.wait("@roomRequest").then((interception) => { - expect(interception.request.query).to.not.contain({ - room_auth_token: "roomAuthToken", - room_auth_token_type: "0", - }); - }); + cy.wait("@roomRequest").then((interception) => { + expect(interception.request.query.room_auth_token).to.be.undefined; + expect(interception.request.query.room_auth_token_type).to.be.undefined; + });
| $currentSession = $this->startNewSession($this->user); | ||
|
|
||
| $roomAuthToken = RoomAuthToken::factory()->create([ | ||
| 'session_id' => $currentSession->id, | ||
| 'room_id' => $room->id, | ||
| 'type' => RoomAuthTokenType::CODE, | ||
| ]); | ||
|
|
||
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.start', ['room' => $room]), ['consent_record_attendance' => false, 'consent_record' => false, 'consent_record_video' => false]) | ||
| ->assertForbidden(); | ||
| $this->withHeaders(['Access-Code' => $room->access_code])->postJson(route('api.v1.rooms.start', ['room' => $room]), ['consent_record_attendance' => false, 'consent_record' => false, 'consent_record_video' => false]) | ||
| ->assertForbidden(); | ||
| $this->flushHeaders(); | ||
| ->assertForbidden() | ||
| ->assertJsonFragment(['message' => CustomErrorMessages::ROOM_REQUIRE_CODE->value]); | ||
|
|
||
| $this->postJson(route('api.v1.rooms.start', [ | ||
| 'room' => $room, | ||
| 'room_auth_token' => $roomAuthToken->id, | ||
| 'room_auth_token_type' => RoomAuthTokenType::CODE->value, | ||
| ]), ['consent_record_attendance' => false, 'consent_record' => false, 'consent_record_video' => false]) | ||
| ->assertForbidden() | ||
| ->assertJsonFragment(['message' => 'This action is unauthorized.']); |
There was a problem hiding this comment.
Use authenticated requests in the “authorized users” block.
The second request in this section drops actingAs($this->user), so it tests a guest path rather than the intended authenticated flow.
Proposed fix
- $this->postJson(route('api.v1.rooms.start', [
+ $this->actingAs($this->user)->postJson(route('api.v1.rooms.start', [
'room' => $room,
'room_auth_token' => $roomAuthToken->id,
'room_auth_token_type' => RoomAuthTokenType::CODE->value,
]), ['consent_record_attendance' => false, 'consent_record' => false, 'consent_record_video' => false])🤖 Prompt for AI Agents
In `@tests/Backend/Feature/api/v1/Room/RoomTest.php` around lines 3189 - 3207, The
second POST to route('api.v1.rooms.start') is missing the authenticated context
and is testing a guest path; update the call so it uses the same authenticated
actor as the first request by chaining actingAs($this->user) before postJson
(keep the same payload and room_auth_token parameters), ensuring the test
exercises the authenticated flow involving startNewSession,
RoomAuthToken::factory(...) and RoomAuthTokenType::CODE.
| $this->actingAs($this->user)->postJson(route('api.v1.rooms.start', ['room' => $room]), ['consent_record_attendance' => false, 'consent_record' => false, 'consent_record_video' => false]) | ||
| ->assertForbidden(); | ||
| $this->withHeaders(['Access-Code' => $this->createAccessCode()])->postJson(route('api.v1.rooms.start', ['room' => $room]), ['consent_record_attendance' => false, 'consent_record' => false, 'consent_record_video' => false]) | ||
| ->assertUnauthorized(); | ||
| $this->withHeaders(['Access-Code' => $room->access_code])->postJson(route('api.v1.rooms.start', ['room' => $room]), ['consent_record_attendance' => false, 'consent_record' => false, 'consent_record_video' => false]) | ||
| ->assertForbidden() | ||
| ->assertJsonFragment(['message' => CustomErrorMessages::ROOM_REQUIRE_CODE->value]); | ||
|
|
||
| // Testing authorized user with invalid token | ||
| $this->postJson(route('api.v1.rooms.start', [ | ||
| 'room' => $room, | ||
| 'room_auth_token' => 'invalidToken', | ||
| 'room_auth_token_type' => RoomAuthTokenType::CODE->value, | ||
| ]), ['consent_record_attendance' => false, 'consent_record' => false, 'consent_record_video' => false]) | ||
| ->assertUnauthorized() | ||
| ->assertJsonFragment(['message' => CustomErrorMessages::ROOM_INVALID_AUTH_TOKEN->value]); | ||
|
|
||
| $this->postJson(route('api.v1.rooms.start', [ | ||
| 'room' => $room, | ||
| 'room_auth_token' => $this->faker->uuid, | ||
| 'room_auth_token_type' => RoomAuthTokenType::CODE->value, | ||
| ]), ['consent_record_attendance' => false, 'consent_record' => false, 'consent_record_video' => false]) | ||
| ->assertUnauthorized() | ||
| ->assertJsonFragment(['message' => CustomErrorMessages::ROOM_INVALID_AUTH_TOKEN->value]); |
There was a problem hiding this comment.
Same auth mismatch in the “authorized user with invalid token” path.
These requests are unauthenticated, so they don’t exercise the intended user scenario.
Proposed fix
- $this->postJson(route('api.v1.rooms.start', [
+ $this->actingAs($this->user)->postJson(route('api.v1.rooms.start', [
'room' => $room,
'room_auth_token' => 'invalidToken',
'room_auth_token_type' => RoomAuthTokenType::CODE->value,
]), ['consent_record_attendance' => false, 'consent_record' => false, 'consent_record_video' => false])🤖 Prompt for AI Agents
In `@tests/Backend/Feature/api/v1/Room/RoomTest.php` around lines 3360 - 3379, The
failing assertions are using unauthenticated requests for the "authorized user
with invalid token" paths; update the two postJson calls after the first to be
authenticated by chaining ->actingAs($this->user) (same as the first request) so
they run as the intended user, keeping the route('api.v1.rooms.start', [...])
params and the body payload unchanged and preserving the assertions that expect
ROOM_INVALID_AUTH_TOKEN via CustomErrorMessages and RoomAuthTokenType::CODE
references.
| }); | ||
|
|
||
| // Check that access code header is reset | ||
| // Check thet room auth token is reset |
There was a problem hiding this comment.
Typo in comment: "thet" should be "that".
Fix typo
- // Check thet room auth token is reset
+ // Check that room auth token is reset📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check thet room auth token is reset | |
| // Check that room auth token is reset |
🤖 Prompt for AI Agents
In `@tests/Frontend/e2e/RoomsViewMeetings.cy.js` at line 2271, Update the inline
comment in the test "RoomsViewMeetings.cy.js" that currently reads "// Check
thet room auth token is reset" to correct the typo to "// Check that room auth
token is reset" so the comment is grammatically correct and clear; locate the
comment near the test assertion that verifies the room auth token reset and
replace "thet" with "that".
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| abort(401, CustomErrorMessages::ROOM_INVALID_CODE->value); | ||
| } | ||
| } elseif ($request->type === RoomAuthTokenType::PERSONALIZED_LINK->value) { | ||
| if (! Auth::guest()) { |
There was a problem hiding this comment.
| if (! Auth::guest()) { | |
| if (Auth::user()) { |
| } | ||
|
|
||
| if ($request->type === RoomAuthTokenType::CODE->value) { | ||
| if (! $room->getRoomSetting('allow_guests') && ! Auth::user()) { |
There was a problem hiding this comment.
| if (! $room->getRoomSetting('allow_guests') && ! Auth::user()) { | |
| if (! $room->getRoomSetting('allow_guests') && Auth::guest()) { |
| } else if (event.data.type === HTTP_ROOM_FILE_FORBIDDEN) { | ||
| // Forbidden, not allowed to view file | ||
| toast.error(t("rooms.flash.file_forbidden")); | ||
| EventBus.emit(EVENT_FORBIDDEN); |
There was a problem hiding this comment.
| EventBus.emit(EVENT_FORBIDDEN); | |
| EventBus.emit(EVENT_FORBIDDEN); | |
| loadData(); |
Maybe also reload file list, in case the permissions to view the files have changed (Download is no longer allowed)
There was a problem hiding this comment.
I agree that reloading the file list, in case the permissions to view the files have changed (Download is no longer allowed) would be a good idea.
But I am currently not sure if it is that simple or if it could lead to problems with other reloads of the file list.
One scenario where this would result in several reloads is the following (not sure if there are others):
- User was a co-owner of the room but role was changed to something else while the user is on the file list
- The user tries to download a file which is not downloadable anymore because of the role change (previously had access to it as a co-owner but now is not able to download the file anymore -> error is triggered)
- This would trigger several reloads of the file list (with the changes above):
- One because of the error handling
- Another because of
onRoomHasChanged(user role changed from co-owner to something else)
Personally, I think reloading the file list several times in some cases would be better than not reloading when permissions to view the files have changed, but a solution that avoids triggering multiple reloads would be preferable.
|
|
||
| return [ | ||
| 'username' => $this->when(! empty($this->token), ! empty($this->token) ? $this->token->fullname : null), | ||
| 'username' => $this->when(! empty($this->personalizedLink), ! empty($this->personalizedLink) ? $this->personalizedLink->fullname : null), |
There was a problem hiding this comment.
| 'username' => $this->when(! empty($this->personalizedLink), ! empty($this->personalizedLink) ? $this->personalizedLink->fullname : null), | |
| 'username' => $this->when(! empty($this->personalizedLink), $this->personalizedLink?->fullname), |
| $room->personalizedLinks()->save($link); | ||
|
|
||
| Log::info('Created new room token for guest {name} with the role {role} for room {room}', ['room' => $room->getLogLabel(), 'role' => $token->role->label(), 'name' => $token->firstname.' '.$token->lastname]); | ||
| Log::info('Created new personalized room link for guest {name} with the role {role} for room {room}', ['room' => $room->getLogLabel(), 'role' => $link->role->label(), 'name' => $link->firstname.' '.$link->lastname]); |
There was a problem hiding this comment.
| Log::info('Created new personalized room link for guest {name} with the role {role} for room {room}', ['room' => $room->getLogLabel(), 'role' => $link->role->label(), 'name' => $link->firstname.' '.$link->lastname]); | |
| Log::info('Created new personalized room link for guest {name} with the role {role} for room {room}', ['room' => $room->getLogLabel(), 'role' => $link->role->label(), 'name' => $link->fullname]); |
| /** | ||
| * Create download link | ||
| */ | ||
| public function url(): string |
There was a problem hiding this comment.
This method is now only used in the MeetingService. For the download url generation in the HTTP Resources we do not use this method. I therefor suggest removing this method and creating the download url directly inside of the MeetingService.
The current solution would also result in the start meeting request to exit with 404 in case a file doesn't exists
| ->nullable() | ||
| ->constrained() | ||
| ->onDelete('cascade'); | ||
| $table->timestamps(); |
There was a problem hiding this comment.
Do we need or use the timestamps ?
There was a problem hiding this comment.
To my knowledge the timestamps are not used anywhere and not necessarily needed here.
I think I added them because they were also added in the older branch and forgot to check if we still need them.
If we remove them, we’ll need to update the model accordingly.
| isLoadingAction.value = true; | ||
| loadingError.value = false; | ||
|
|
||
| // Build url, add accessCode and token if needed |
| @@ -100,10 +101,17 @@ function joinMembership() { | |||
| isLoadingAction.value = true; | |||
|
|
|||
| // Join room as member, send access code if needed | |||
| if (props.token && !roomAuthToken.value) { | ||
| authenticate(ROOM_AUTH_TOKEN_TYPE_PERSONALIZED_LINK, props.token).then( |
There was a problem hiding this comment.
This is needed for the following case:
- User tries to access room with a personalized room link but auth request that would return the room auth token fails with e.g. 500 error code.
- This results in
props.tokenbeing set androomAuthToken.valuebeing null
After that the reload button is shown and can be clicked by the user to retry accessing the room.
When retrying to access the room it is necessary (for users that try to access using a personalized room link) to send the auth request again to retrieve a valid room auth token before trying to reload / load the room.
The frontend test "room auth with personalized link errors" in the file RoomsViewGeneral.cy.js contains checks that simulate this case (ll. 1362 - 1438)
This was fixed after a comment from coderabbit (#2726 (comment)) in the commits 4c001ec and f486036. (Tests were added and improved alongside other tests in other commits)
Only reloading / loading the room would not be enough because the user would have no room auth token after the auth request failed and therefore the reload / load room request would fail in most cases.
If the room is accessible for guests without access code the request would be successful but the personalized link would not be used.
An alternative for this would be to reload the whole page instead of only reloading the necessary requests.
There was a problem hiding this comment.
props.token is the personalized room link token provided by the route
Could maybe be renamed to make this clearer
2 participants
Implements #1409
Type
Checklist
Changes
Summary by CodeRabbit
New Features
Bug Fixes
UI/Documentation Updates