search

headwirecom / peregrine-cms

an api first, head optional cms with based on vuejs and apache sling
Apache License 2.0
52 stars 31 forks source link

Login does not work for certain paths #887

Open golinski opened 3 years ago

golinski commented 3 years ago

When in private mode one goes to etc. http://localhost:8080/system/console/bundles, we are presented with the normal login form at http://localhost:8080/system/sling/form/login?resource=%2Fsystem%2Fconsole%2Fbundles, but after entering admin/admin we are redirected to http://localhost:8080/system/sling/form/login?resource=%2Fsystem%2Fconsole%2Fbundles%2Fj_security_check. Each subsequent login attempt just adds %2Fj_security_check.

reggie7 commented 3 years ago

@golinski please check if this covers #33 too. If so - let's close that one too. #721 is the original for it.

golinski commented 3 years ago

No, https://github.com/peregrine-cms/enhancements/issues/33 works this way because /content is anonymously readable, therefore no authentication is performed (and the form is not displayed) and markup is simply returned. Afterwards the /perapi endpoints are not visible to the felibs (because we are an anonymous user), and the page stays blank.

reggie7 commented 3 years ago

Great analysis @golinski, thank you for that 👍🏼

cmrockwell commented 3 years ago

Indeed. Thanks both to Reggie and Michał for investigating the issues related to expired login. To summarize the discussion from the water-cooler meeting, there were a few points...

  1. Whether anonymous users should have access to /content for default localFS replication use-case. Instances with author runmode for the remote use-case do not grant 'everyone' access.
  2. Sling distinguishes between anonymous access and granting access to the everyone group. I think that Peregrine CMS does not give read access on /content to anonymous users, rather it grants read access to the everyone group. That is to say that org.apache.sling.engine.impl.auth.SlingAuthenticator is not configured to grant access to anonymous unauthenticated users. But any authenticated user (members of everyone) has read access. image
  3. Another facet is that "content" is optional as is apparent upon login the path is /admin/pages/index.html which is enabled by the Resource Resolver Factor configuration URL Mappings. I don't this makes ant difference to the issue per se, and rather I want to make sure the team understands

image

  1. The question about what to do about this came up in the meeting. We brainstormed a few possible solutions to consider:
    • Maybe remove the ACL granting read for everyone group on /content would be a good idea. The remote use case and this condition, and the login experience seems a but little nicer. It could be a good idea for the security of the system as well if tenant users are not totally friendly.
    • The option of a frontend approach was discussed whereby the servlet /perapi/admin/access.json is already polled at regular intervals. If user should lose their sessions, then the response should not be 404 as is the current state. Rather the response could be userID: "anonymous" and the JS could redirect the page to /system/sling/form/login?resource=
    • The option of a backend approach, where the user may be expected to refresh the screen upon observing the whitescreen. The redirect would be initiated form the backend.
reusr1 commented 3 years ago

@golinski @cmrockwell @reggie7 great discussion here - one comment on the access servlet: that is probably a permission issue in /perapi/admin/access.json that one should allow read for everyone - since the original issue was fixed and this relates more to https://github.com/peregrine-cms/enhancements/issues/33 - should we take the discussion there?

also, I think we should turn off the default /content/:/ mapping in our launcher - it is the default in sling but it seems confusing?

cmrockwell commented 3 years ago

Hi @reusr1 Peregrine CMS would greatly benefit from friendlier URL's generally. The default content mapping offers a simple way to have that not just for the CMS pages, but also the tenant pages. No site owner wants to look at /content/ at root constantly. As CMS developers, we should not settle on that either. I mentioned the default content mapping in the context of this issue only to ensure the team was aware about it. But it is a separate topic, not related inherently related to these login issues. If you want to propose changes to the content mapping, could we discuss in a separate ticket? For what it's worth, I do not feel turning off the default /content/:/ mapping is for the better.

reusr1 commented 3 years ago

@cmrockwell I think that is what I was trying to say - we should split the discussion into a different ticket - this issue is about login does not work for certain paths and from what I see that was fixed.

cmrockwell commented 3 years ago

Ah I see. My comments on this issue 887 were a bit off topic. They were more relevant to issue 33 which I had read before the water-cooler meeting. It was on my mind, so maybe I delete the comments move them. Because I was mistaken about what issue 887 was really about. My apologies!