getgrav / grav-plugin-login

Grav Login Plugin
http://getgrav.org
MIT License
44 stars 54 forks source link

Saving a page in a "normal" mode messes up visibility for logged-in users #228

Closed michalby closed 4 years ago

michalby commented 4 years ago

When visibility_requires access is set in a page frontmatter using an "expert" editing mode in admin plugin, everything works fine. Here's a working example:

title: Private
body_classes: 'title-center title-h1h2'
login:
    visibility_requires_access: true
access:
    site.login: true
    admin.login: true
metadata:
    robots: noindex

However, when an expert mode is switched to normal, then the page is saved (with or without making any changes to its contents), it starts to appear in menu for all guests. Frontmatter is automatically being changed to include overrides:

title: Private
metadata:
    robots: noindex
login:
    merged:
        enabled: true
        built_in_css: true
        route: null
        redirect_to_login: true
        redirect_after_login: null
        redirect_after_logout: /
        route_activate: /activate_user
        route_forgot: /forgot_password
        route_reset: /reset_password
        route_profile: /user_profile
        route_register: /user_register
        route_unauthorized: /user_unauthorized
        twofa_enabled: false
        dynamic_page_visibility: true
        parent_acl: false
        protect_protected_page_media: false
        rememberme:
            enabled: true
            timeout: 604800
            name: grav-rememberme
        max_pw_resets_count: 2
        max_pw_resets_interval: 60
        max_login_count: 5
        max_login_interval: 10
        ipv6_subnet_size: 64
        user_registration:
            enabled: false
            fields:
                - username
                - password
                - email
                - fullname
                - title
                - level
                - twofa_enabled
            default_values:
                level: Newbie
            access:
                site:
                    login: 'true'
            redirect_after_registration: ''
            redirect_after_activation: ''
            options:
                validate_password1_and_password2: true
                set_user_disabled: false
                login_after_registration: false
                send_activation_email: false
                manually_enable: false
                send_notification_email: false
                send_welcome_email: false
        visibility_requires_access: true
access:
    site.login: true
    admin.login: true

A temp fix is to switch back to expert mode, delete four spaces in front of

        visibility_requires_access

leaving only four, and save again, or remove everything in a login: section leaving only

    visibility_requires_access:

with four spaces.

The config that is pasted into the page frontmatter is coming from user/config/plugins/login.yaml Similar behaviour was also described in this issue: https://github.com/getgrav/grav-plugin-login/issues/166#issuecomment-484317114

rhukster commented 4 years ago

Not sure what is merging in the configuration. Are you running any extra plugins?

michalby commented 4 years ago

I first noticed this behavior using SEO plugin (that's why I was switching from expert to normal and back), but I managed to replicate this bug using a fresh grav instance, so Admin Panel, Email, Error, Form, Login, Markdown Notices and Problems plugins, nothing extra.

rhukster commented 4 years ago

I will try to reproduce but that merge behavior is definitely weird. Does look like a 3rd party plug-in is doing it.

michalby commented 4 years ago

That bug is not present in 1.7.0-beta.10 with Login v3.0.4 but Grav 1.6.16 with Login v3.0.3 and 3.0.4 both have this issue.

pospiemi commented 4 years ago

Same issue here with Grav v1.6.24, Admin v1.9.14 and Login v3.3.0.

Looks like it happens in "mergeConfig" used in "pageVisibility" called during "onPagesInitialized":

Changing the line https://github.com/getgrav/grav-plugin-login/blob/73c22387900c7b234d8ad09536417418e7672dda/login.php#L213

to

$config = null;

the issue is gone, but it breaks "parent_acl" check for page visibiltiy:

https://github.com/getgrav/grav-plugin-login/blob/740b1fdd7b9d03ac9db5edbe274985ee4e5f05d9/classes/Login.php#L617

Hence this is a hint, not a fix...

rhukster commented 4 years ago

I'm unable to recreate this in 1.6 or 1.7rc with Login v3.4.0. I really think that it's the fault of the SEO plugin you guys are using. Probably it doesn't happen in 1.7 because it doesn't fire the same events that 1.6 does on page save, and the SEO plugin is making it's changes.

rhukster commented 4 years ago

Arggg.. was able to reproduce it and fix.

Only happened in Grav 1.6 BTW, Grav 1.7 handles this better.