Adobe-Consulting-Services / acs-aem-commons

http://adobe-consulting-services.github.io/acs-aem-commons/
Apache License 2.0
455 stars 603 forks source link

REDIRECT MANAGER - None of the OPTIONS Context Aware Configs are working (ISSUE ROOT CAUSED IN AEM CLOUD) #3284

Closed thcharan closed 8 months ago

thcharan commented 8 months ago

This is a major issue with the current version of ACS v6.4.0.,.. but possibly many older versions as well. Issue is reproducible on localhost publish on the latest AEM Cloud SDK. Issue is reproducible on all AEMasCloud Service environments.

Note - It is possible that - Same issue may exist in AMS OR ON PREM provided if the Everyone group permissions is same as in the screenshot provided below from aem-cloud. ( I did not validate the permissions setup on AMS or on-prem-aem65 environments )

@YegorKozlov - So, My Team investigated this issue by debugging runtime on a AEM Cloud SDK on localhost. Below is our findings. It's a major issue.

In a nutshell, None of the "Context Aware Cfg Options" is working in AEM-Cloud. So technically this is not just impacting "Ignore Selectors" option, but impacts all the CA options. Findings below

com.adobe.acs.commons.redirects.filter.RedirectFilter.java is having this below line of code, @reference ConfigurationResourceResolver configResolver;

The configResolver is injected with a valid configResolver object --- no issues with that. However important thing to note here -- this configResolver is still operating with a anonymous user context ( NOT AS A SERVICE USER CONTEXT WITH ELEVATED PERMISSIONS).

when code flow control reaches the match() method,

we have this line, which works fine, returning the path of an valid context aware path applicable to that site. String configPath = configResource.getPath();

The code fails here -- returns an empty valuemap (size=0) since the anonymous user context didn't have permissions on that node. it was not able to read any context aware properties stored on that node. "redirects". ValueMap properties = configResource.getValueMap();

On local, when we are logged in as admin on that browser session -- the same code worked,...that's self explanatory.

Solution - Any Read operations on the Redirect configs under /conf -- always rely on resource resolver of the SERVICE USER ACCOUNT.

Note - Anonymous is part of everyone group... the Out of the box permissions in the below screenshot should make it clear of the issue described above. We should not be modifying the ACL's of the everyone group as short fix.

image

YegorKozlov commented 8 months ago

@thcharan I can't reproduce it. ACS Commons grants read access to redirects in the repo-init script:

https://github.com/Adobe-Consulting-Services/acs-aem-commons/blob/3d725707497a09f45ba3f31f0294554784c03718/all/src/main/content/jcr_root/apps/acs-commons/config/org.apache.sling.jcr.repoinit.RepositoryInitializer-acs-commons-all.config#L46

image

I tried with aem-sdk-2023.12.14697.20231215T125030Z-231200 and 6.5.20 and redirects worked fine to me. Do you have any ACL customizations on the /conf node?

thcharan commented 8 months ago

@YegorKozlov :

Ok -- we did further test on a vanilla localhost publish instance (aem-sdk-quickstart-2023.12.14697.20231215T125030Z-231200) with ACS 6.4.0 pkg deployed and (no other custom code on it) --- The issue is still reproducible.

Also - i checked if the ACL's of "anonymous" user acc in the above vanilla localhost with acs 6.4.0 -> They look exact same as 'what's in your screenshot'.

Can you double check ?

1) Specifically - The ignore selector functionality working for you in localhost aem sdk publish (as anonymous user). ? 2) Also to check the permissions - can you hit this endpoint...see if any prop vals returned in that json..as anonymous user context. (ensure you are not logged in to crx/de as admin).

http://localhost:4503/conf/global/settings/redirects.json

I can definitely say "anonymous" being part of "everyone" group... the out of the box acl restrictions defined at the "everyone" group is conflicting with the ACL defined at the 'anonymous' user acc which is being modified by acs-repoinit --- that is completely possible.

I'm very certain this is a issue.

vc-architha commented 8 months ago

@YegorKozlov :

I'm able to reproduce the issue with vanilla aem sdk local setup(aem-sdk-quickstart-2023.12.14697.20231215T125030Z-231200) and acs 6.4.0.

Also checked conf/global/settings/redirects.json value as an anonymous user and an admin. Attaching the screenshot of the same

Anonymous User

Screen Shot 2024-03-08 at 6 33 34 PM

Admin

Screen Shot 2024-03-08 at 6 32 26 PM

Screenshot of localhost 4503 configs and acs version used-

Screen Shot 2024-03-05 at 10 03 25 PM Screen Shot 2024-03-05 at 10 07 38 PM Screen Shot 2024-03-08 at 6 42 38 PM
YegorKozlov commented 8 months ago

@thcharan @vc-architha I was able to reproduce it on my local. Thanks for spotting it out.

@thcharan the statement that none of the context-aware configs are working is not quite correct . Regular redirects are working fine. The issue only impacts the functionality that reads optional configuration parameters from the context, i.e. from this tab:

image

that's why I could not reproduce it first.

The fix seems to be easy, we need to set another ACL for /conf :

# web requests need read access to redirect configurations, e.g. /conf/global/settings/redirects
set ACL for anonymous
    allow jcr:read on /conf  restriction(rep:glob,/*/settings/redirects)
    allow jcr:read on /conf  restriction(rep:glob,/*/settings/redirects/*)
end

I tried it on my local and with these two rules it works properly. The PR with the fix is coming.

thcharan commented 8 months ago

@YegorKozlov / @vc-architha - Thank you for checking on it. Glad to know that it's reproducible on your end.

@YegorKozlov -

Yes Apologies - I will update the title of this "Issue" to make it clear.. i probably did not phrase that statement correctly - Yes. Any of the CA Option params directly stored on that specific /redirects node being the only problem was my intended statement.

Out of curiosity -- I have this Question in my mind .... why not completely rely on user service account ? "acs-commons-manage-redirects-service" which is already being used in RedirectFilter.java .. This already gives you read permission on complete /conf node?

Screen Shot 2024-03-08 at 11 23 09 AM Screen Shot 2024-03-08 at 11 18 43 AM Screen Shot 2024-03-08 at 11 18 27 AM

any specific reasons not to ? was wondering -- why take the painful approach of modifying the 'anonymous' user ACL on publish..as i know it is a very sensitive user acc. and potentially can cause some conflicts with a custom code base of a specific client... IF we rely on our own ACS user service acc "acs-commons-manage-redirects-service" ... may be it provides better insulation in a customer envt where custom code won't interfere with acs-commons-manage-redirects-service.

Another reason to consider the user service acc usage is that... IF we modify the "Configuration Name" and "Configuration Bucket Name"... your ACL's which assumes /settings/redirects node being the bucket won't work...that would in-turn require customer implementing their own ACL and troubleshooting cycles that it would cause and all that down the road.

MicrosoftTeams-image (1)

Any thoughts ? I am sure you may have some valid reasons not to rely user service account..would like to know what could be that.. ?

YegorKozlov commented 8 months ago

Out of curiosity -- I have this Question in my mind .... why not completely rely on user service account ? "acs-commons-manage-redirects-service" which is already being used in RedirectFilter.java .. This already gives you read permission on complete /conf node?

Performance is one of the reasons. Redirect Manager was designed to work in high-load environments. Opening a service resolver on every request would add a delta , so why not to use the resolver from the request?
In early versions of the Redirect Manager this wasn't a problem. Redirects were loaded on startup/on-change into a mem-cache and everything was super-fast. Then we added stuff that required evaluation in the context and this extra bit of ACL was introduced . Changing the bucket would break it, it's a good catch, although I'm not sure if we need to be that flexible. I'd say we need to hide this configuration and have it static.

thcharan commented 8 months ago

@YegorKozlov 👍 Ok. Thank you for clarifying the context there.

davidjgonzalez commented 8 months ago

Fixed in 6.5.0

josh-ellingsworth commented 7 months ago

@davidjgonzalez -

Redirect manager works well for anonymous users, but the same issue occurs and the config is unable to be accessed for logged in user. I am working on a site where a portion is behind login and am exploring using redirect manager. Once a user logs in none, of the redirects are processed. As a short term workaround, I am looking to give everyone jcr:read for redirect folder that is applied to anonymous per this fix. A better fix would seem to be to use a service user as mentioned by @YegorKozlov Thanks

YegorKozlov commented 7 months ago

@josh-ellingsworth I was able to reproduce it in WKND. It's a different issue that the one reported in this ticket and we've always had it.

We allow anonymous to read redirect rules, and my assumption was if anonymous could read redirects, then any other user would be able too. It's not correct and like you said, we need to grant read access to anonymous and any other users/groups that access the site.

It's a good argument for using a service user. The fix might take some time though.

kwin commented 7 months ago

Please don't use service users but just grant the necessary permissions to the everyone group (every user, even anonymous is implicitly member of it) in the repoinit script.

YegorKozlov commented 7 months ago

@kwin thanks for the tip. we are never late to learn:)

I see what's wrong with the current version of repo-init:

set ACL for anonymous
    allow jcr:read on /conf  restriction(rep:glob,/*/settings/redirects)
    allow jcr:read on /conf  restriction(rep:glob,/*/settings/redirects/*)
end

it should grant it to everyone instead.

josh-ellingsworth commented 7 months ago

@YegorKozlov, @kwin - Yes, granting everyone access also fixes the issue. This is the temporary workaround I am implementing on our onPrem installation. I think this makes sense as an exception, but Adobe clearly documents that it is not recommended to change ACLs on everyone.