Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docker: Don't update permissions of originals on startup #2371

Closed
wants to merge 1 commit into from

Conversation

BonzTM
Copy link

@BonzTM BonzTM commented May 28, 2022

Separating Originals path from Storage and Imports path for CHOWN purposes. This will allow users with specific ownership on Originals to retain those owners while allowing the rest of the pathing to be owned by the UID/GID in the container.

Photoprism states that it does NOT modify the Originals directory. This is untrue. A power user who controls the permissions of the Originals directory should be able to exclude it from being owned by container UID/GID.

Acceptance Criteria:

  • Features and improvements are fully implemented so that they can be released at any time without additional work
  • Automated unit and/or acceptance tests have been added to ensure the changes work as expected and to reduce repetitive manual work
  • User interface changes are fully responsive and have been tested on all major browsers and various devices
  • Database-related changes are compatible with SQLite and MariaDB
  • Translations have been / will be updated (specify if needed)
  • Documentation has been / will be updated (specify if needed)
  • Contributor License Agreement (CLA) has been signed

…poses. This will allow users with specific ownership on Originals to retain those owners while allowing the rest of the pathing to be owned by the UID/GID in the container.
@CLAassistant
Copy link

CLAassistant commented May 28, 2022

CLA assistant check
All committers have signed the CLA.

@BonzTM BonzTM changed the title CHOWN Originals directory separately Docker: CHOWN Originals directory separately May 28, 2022
@alexislefebvre
Copy link
Contributor

alexislefebvre commented May 28, 2022

You can mount any folder in read-only mode to restrict the access to files:

   volumes:
      - "/mnt/photos/nature:/photoprism/originals/nature:ro"

@BonzTM
Copy link
Author

BonzTM commented May 30, 2022

You can mount any folder in read-only mode to restrict the access to files:

   volumes:
      - "/mnt/photos/nature:/photoprism/originals/nature:ro"

Very true. You can do many different things.

I only opened the PR because the current default behavior goes against the "we do not ever touch originals" wording. I do understand from a non-paying-user support standpoint things are kept as simplistic as possible, but there should still be options. We don't always know the tech stack that others are using, and maybe they may not have the ability to mark something read-only. Regardless, being explicit is almost always a better thing than being implicit.

I have an OKD cluster w/ CephFS backing my originals directory. I have had to mark this path readOnly, even though I may have other paths along this one that I do not wish to be marked as readOnly.

So my manifest looks like such:

  spec:
    volumes:
      - name: originals
        cephfs:
          path: "/path/to/originals"
          readOnly: true
          monitors:
            - 10.1.2.30:6789
            - 10.1.2.31:6789
            - 10.1.2.32:6789
          user: okd-photos-user
          secretRef:
            name: password

My problem is solved, though I do not entirely like the implementation. You may close this PR if you deem it harmful.

@insomnia417
Copy link

This problem really bothered me a hundred times, either adding different user management permissions, or restricting the permission modification of my origins file by photoprism (sorry, my English is poor, from translation software)

lastzero added a commit that referenced this pull request Aug 1, 2022
Signed-off-by: Michael Mayer <michael@photoprism.app>
lastzero added a commit that referenced this pull request Aug 1, 2022
Signed-off-by: Michael Mayer <michael@photoprism.app>
@lastzero lastzero changed the title Docker: CHOWN Originals directory separately Docker: Skip changing permissions of the original folder on startup Aug 1, 2022
@lastzero lastzero changed the title Docker: Skip changing permissions of the original folder on startup Docker: Skip changing permissions of originals on startup Aug 1, 2022
@lastzero lastzero changed the title Docker: Skip changing permissions of originals on startup Docker: Don't update permissions of originals on startup Aug 1, 2022
@lastzero lastzero self-requested a review August 1, 2022 14:19
@lastzero lastzero self-assigned this Aug 1, 2022
@lastzero lastzero added the docker Docker Images, Build Scripts, Config & Deployment Examples label Aug 1, 2022
@lastzero lastzero removed their request for review August 1, 2022 14:21
@lastzero lastzero added the please-test Ready for acceptance test label Aug 1, 2022
@lastzero
Copy link
Member

lastzero commented Aug 1, 2022

Started a new preview build for testing! 👍

@lastzero lastzero added the help wanted Well suited for external contributors! label Aug 1, 2022
@lastzero lastzero removed the help wanted Well suited for external contributors! label Sep 1, 2022
@BonzTM
Copy link
Author

BonzTM commented Oct 28, 2022

@lastzero thanks for the updates! There is 1 issue that has surfaced later than your commits regarding

switching to uid 1000620000:1000620000
/opt/photoprism/bin/photoprism start
setpriv: uid 1000620000 not found, --init-groups requires an user that can be found on the system

but I can open a separate issue for that and my issue and PR can be closed

@BonzTM BonzTM closed this Oct 28, 2022
@lastzero
Copy link
Member

UID 1000620000 is not supported, see list of valid ranges in the config examples.

@BonzTM
Copy link
Author

BonzTM commented Oct 28, 2022

UID 1000620000 is not supported, see list of valid ranges in the config examples.

Unfortunately this requires me to bypass certain security contexts in OpenShift/OKD, but I'll deal with it.

@hiro5id
Copy link

hiro5id commented Oct 31, 2022

Is this a configurable ? For example, can I turn this behaviour back on if I want to ?

EDIT: Oh I see... I can set PHOTOPRISM_DISABLE_CHOWN_ORIGINALS environment variable to control this behaviour. 👍

@BonzTM
Copy link
Author

BonzTM commented Oct 31, 2022

Is this a configurable ? For example, can I turn this behaviour back on if I want to ?

EDIT: Oh I see... I can set PHOTOPRISM_DISABLE_CHOWN_ORIGINALS environment variable to control this behaviour. 👍

PHOTOPRISM_DISABLE_CHOWN_ORIGINALS was not implemented. Instead, @lastzero changed the original argument from taking ownership of the originals directory in the following commit: f06d768

@hiro5id
Copy link

hiro5id commented Nov 1, 2022

OOff..... so.... its not configurable then?

@lastzero
Copy link
Member

lastzero commented Nov 1, 2022

The originals folder itself is not touched anymore, but the behavior for the storage folder can be enabled / disabled as before.

@lastzero lastzero added released Available in the stable release and removed please-test Ready for acceptance test labels Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker Docker Images, Build Scripts, Config & Deployment Examples released Available in the stable release
Projects
Status: Release 🌈
Development

Successfully merging this pull request may close these issues.

6 participants