Skip to content

Sub-folder sharing fix#1148

Closed
alexkoon wants to merge 1 commit into
bpatrik:masterfrom
alexkoon:master
Closed

Sub-folder sharing fix#1148
alexkoon wants to merge 1 commit into
bpatrik:masterfrom
alexkoon:master

Conversation

@alexkoon

Copy link
Copy Markdown

WARNING - This was vibe coded. I am a Java dev, and don't really know typescript. I had a Claude sub so wanted to apply it to this as I use pigallery2. I have tested (not extensively) via a local build docker image.

Fix: Subfolder navigation now works for shared folder links

This fixes the regression reported in Discussion #1068 where shared folder links no longer allowed recipients to browse into subfolders.

What was broken and what was fixed:

  1. Subfolder visibility (backend) — The sharing projection query in SearchManager only matched the exact shared directory, so subdirectories were invisible to sharing users. Added recursive directory matching so that
    when a folder is shared, all its subdirectories are included in the allowed content projection.
  2. Password-protected share login (frontend + backend) — Visiting a shared link (/share/) with passwordRequired: true produced a blank screen and a spinning loop of 401 errors in the browser console. Two root
    causes:
    - NavigationService.toLogin() was awaiting the currentSharing observable to determine whether to show the password prompt — but that observable only emits after authentication, creating a deadlock. Fixed by exposing
    passwordProtected from the public /share//key endpoint (no auth required) so the redirect decision can be made synchronously.
    - error.interceptor.ts was calling logout() unconditionally on every HTTP 401, even when the user was already unauthenticated, causing an infinite logout→getSessionUser→401 loop. Fixed to only call logout() when a
    user is actually authenticated; otherwise navigates to the login/share-login page directly.
  3. Directory listing access for sharing users (backend) — After successful share password entry, the gallery content endpoint returned 401 NOT_AUTHORISED. The route required UserRoles.Guest but sharing users are
    assigned UserRoles.LimitedGuest. Changed the role check to LimitedGuest — the existing session projectionQuery already restricts sharing users to only see content within their shared directory, so there is no security
    regression.
  4. Breadcrumb navigation for sharing users (frontend) — Once inside a shared folder, there was no way to navigate back up through the folder hierarchy (only the browser back button worked). The breadcrumb bar now
    renders each folder at or below the share root as a clickable link, while folders above the share root remain plain text so users cannot navigate outside their share. The sharing key is preserved in all breadcrumb
    links automatically.
  5. Share dialog cleanup — Removed the now-inaccurate notice "Folders are not shared." from the share creation dialog.

@alexkoon alexkoon force-pushed the master branch 7 times, most recently from 5fe9410 to 4834d6b Compare April 27, 2026 07:00
@alexkoon

Copy link
Copy Markdown
Author

Force pushed over with fixes for the tests - these were the changes to the tests

Test fixes

src/frontend/app/ui/gallery/gallery.component.spec.ts
Added waitForSessionUser to MockAuthenticationService (jasmine.createSpy(...).and.returnValue(Promise.resolve())). Without this the CI Jasmine tests threw TypeError: this.authService.waitForSessionUser is not a
function when ngOnInit was triggered.

test/cypress/e2e/share.cy.ts
Updated the three share E2E tests to intercept /pgapi/gallery/content/* instead of /pgapi/search/*, and to assert on #directory-path (the breadcrumb) instead of the old search-result nav link — matching the new gallery-view navigation.

@bpatrik

bpatrik commented May 30, 2026

Copy link
Copy Markdown
Owner

thanks but I'm not confident about this PR. it touches files, I do not think it should for solving this issue.

Also this issue is complex as sharing if actually a extra layer of allow/deny list on the top of the user's approve/deny list.
I forget about the full picture of issues, but not done right can be a significant security issue, where a random sharing can give access to non-shared part of the gallery.
I remember the tricky part of the sharing was that empty folder I do not want to share for search results (what is the point its empty). Even if there is photo within the folder, but the search query or the user's deny list would filter it out. On the other hand it should show up when navigating the directory, as this is how the user can trigger the re-indexing.

@alexkoon

alexkoon commented Jun 4, 2026

Copy link
Copy Markdown
Author

appreciate the feedback and understand your concerns. Would you be amenable if I was to throw your reply to Claude and have it address those concerns and see if a different approach works?

Also understand if there is skepticism with using AI aid. I am not a fan of blindly using it as well in a professional setting without some form of review. I will try and review and read the code, but lacking typescript experience I wouldn't really know whats good or bad - just that the code reads coherently and sensible.

I love to be able to get multiple folders sharing working again.

Comment thread src/backend/middlewares/user/AuthenticationMWs.ts Fixed
Comment thread src/backend/middlewares/user/AuthenticationMWs.ts Fixed
Fixes issue bpatrik#1068 — subfolders inaccessible when browsing a shared folder link.

Root causes fixed:
- SearchManager: recursiveDir flag adds path LIKE clause so subdirectory
  media and directory listings are visible under a share root
- SessionManager: passes recursiveDir=true to both projection query builders
- GalleryRouter + AuthenticationMWs: LimitedGuest role now reaches the
  directory listing route; authoriseSharingDirectory enforces that the
  requested path is the share root or a descendant — unrelated paths get 401
- SharingMWs + SharingDTO: /share/<key>/key endpoint returns passwordProtected
  flag so the frontend can decide whether to show a password prompt
- navigation.service / error.interceptor / authentication.service: fixed
  blank-screen deadlock and 401 redirect loop on password-protected shares
- gallery.component: directory shares navigate to gallery/<dir> view so
  subfolder cards are clickable, not to the flat search results view
- navigator: breadcrumb links enabled for paths at or below the share root;
  routes observable now uses combineLatest so breadcrumbs recompute when
  share metadata arrives (fixes stale-null race on cached content loads)
- share.service: removed debug console.log

Security:
- authoriseSharingDirectory fails closed — unrecognised AND query shapes
  return 401 rather than silently granting access
- normalizeDirPath canonicalises '.' (Utils.concatUrls root form), '/' and ''
  to the same empty string so root-directory shares are not falsely rejected
- Non-directory shares (date/person) permitted at the route level; content
  scope is enforced by the DB-layer projectionQuery

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bpatrik

bpatrik commented Jun 6, 2026

Copy link
Copy Markdown
Owner

Hi,

Sorry but I can also run AI to build this. I would like to see a solid implementation plan before giving it to AI to execute on it, due to the complexity of the problem and security risks.

I think one subtask that is needed to make this work is to support search queries that only fixed at the beginning or at the end, eg we have now directory:"fixed text", d..y:(open text), I would like to have d..y:(open start" d..y:"open end).

I tried to implement this with AI about a month ago, and it already failed on it. Although It was not the latest claud.

tracking bug for the feature: #1133

@alexkoon

alexkoon commented Jun 6, 2026

Copy link
Copy Markdown
Author

Closing this PR for now to avoid notification noise. I'm going to move this to a personal fork to map out my thoughts and descriptions freely. Let me see if I can come up with a plan with your 'start/end' query support.

If you're open to the idea, you can contact me on my Hotmail account (same username as this one) so we can communicate and get feedback more easily. Alternatively, I can just reach out here before raising a new PR. Thanks!"

@alexkoon alexkoon closed this Jun 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants