Problem/Motivation

When switching context user to retrieve a resource, the session is not getting saved. Resulting in the session being the anonymous user instead of the configured context user.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#12 3482533-12.patch2.6 KBbceyssens
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

heddn created an issue. See original summary.

loparev’s picture

Hi @heddn.

Thanks. I have a few questions though:
1. You've mentioned (in emails or in support ticket) that it doesn't work only for some exact users. Does it mean that on your Drupal instance, you have some saml/sso modules installed that serve only some users thus context user impersonating doesn't work only in those cases? I would like to reproduce and debug the issue before merging this branch, I'm not sure where exactly the session gets dropped/overridden.
2. From our logs I see you've tried the context debug module on your instances (dev and prod I believe). On prod, a request to `/global/en/admin/content/block/N` gives 403 response code with `1006*****` response body, which seems to be a Cloudflare firewall-related. Sounds like a request for grabbing context didn't even hit the Drupal instance. Just want to clarify if this is related or if you reconfigured something on your side. Otherwise, it's not clear how it works for one Drupal user and not for another.

Btw this "switcher" is based on masquerade module, it might be affected by the issue as well I believe.

heddn’s picture

1) The user being used is inconsequential. Early testing seemed to indicate it was important, but that isn't the case. Any user I use, if I use the debug module to test/confirm, it will fail if the user I am impersonating isn't also the same user as the currently logged in user.
2) Almost all of my testing was on localdev without any CF. I could reproduce it there without any issue. The MR here "fixed" the problem.

loparev’s picture

Did you try enabling "Context silent user authentication" option? If enabled it skips user login/logout hooks being called when impersonating. It might be the case that some of saml/sso modules hook into those actions and override session.

heddn’s picture

Yes, that was attempted. We also tested with users who aren't even in SAML. Same result. After debugging things, it is entirely around the fact that impersonation of another user via their session and a session cookie generated for that user doesn't work until we save the session.

loparev’s picture

> After debugging things, it is entirely around the fact that impersonation of another user via their session and a session cookie generated for that user doesn't work until we save the session.

It doesn't sound right. It works in our test envs and on other clients too. Please share your Drupal core version, I will debug on that exact core.

p.s. just curious, doesn't masquerade also work on your env? We use exactly the same approach as they use.

heddn’s picture

Status: Active » Needs review

After working on #3483245: Support layout builder translations, we aren't hitting a scenario any more where an anonymous user is trying to access a restricted block edit page. That said, I still don't think this hurts anything and will only solve a few issues for some customers. See related MR.

loparev’s picture

Status: Needs review » Postponed

I ran functional tests with this change. Unfortunately, context related tests failed (testContextSendingByCron and some others).

I'm going to postpone this issue since you are not hitting this scenario anymore. Will get back to it once I have a capacity.

bceyssens’s picture

Status: Postponed » Needs work

Hi @loparev, we're running into the same issue.

We've setup a new Drupal environment to isolate the issue:
Drupal: 11.2.2
Tmgmt: 8.x-1.17
Tmgmt Smartling: 8.x-9.17

We've used the context debug submodule which works as advertised.
The issue however only occurs when trying to upload unpublished pages, in combination with the queue worker running from command line.

It looks like the code, copied from masquerade, on its own (handled with curl afterwards) doesn't handle sessions completely. Masquerade does a redirect on which the Symfony frameworks handles the session correctly and sets the cookie in a response listener.

To dive deeper into the issue, dumping the cookies from modules/contrib/tmgmt_smartling/src/Context/ContextUserAuth.php's getCookies() looks like SSESSc2b280e207aa821b24606f896b740d6d=. As you can see it lacks the session_id value.

It's probably because the queue is handled via command line (cron)?
Let me know if you need more information to reproduce on your end.

bceyssens’s picture

@loparev I'm not sure if we really need to switch user like the masquerade module does?
The tmgmt module added an access bypass feature around the same time context user switching has been added in this project.

anonymous_access is enabled by default, that's probably why it works in most cases?
If it is disabled or the key query param is stripped before it reaches the webserver, you'll run into the tmgmt_smartling authentication issue.

bceyssens’s picture

StatusFileSize
new2.6 KB

@loparev if you agree, the user switching part can be removed completely.

The only additional thing that needs to happen is to properly check the job and job item statuses to prevent requests that will result in a 403 anyway. Attached patch is based on logic from Drupal\tmgmt_content\Access\KeyAccessCheck. There's one other place where the queue is injected (SendContextActionApproveForm.php) but it isn't used yet, so didn't touch that code.

loparev’s picture

Hi, thanks for the report/research. I will need to review and test this approach.

loparev’s picture

> I'm not sure if we really need to switch user like the masquerade module does? The tmgmt module added an access bypass feature around the same time context user switching has been added in this project.

I reviewed `anonymous_access`/`key_access` logic in TMGMT core, it seems like it's only related to previewing TMGMT Job Items content (translation content). But "Visual context" is a visual representation of the source content, not translated one.

It's interesting that TMGMT generates a link to source node with a valid `key` (`JobItem::getSourceUrl()`). in query parameters (if `anonymous_access ` is enabled and anonymous user doesn't have access to entity's `view`) but it doesn't seems like it alters node view by checking this key though, it only checks access for their preview controller which renders translated content.

> if you agree, the user switching part can be removed completely.

I would agree if mentioned logic above worked as expected in TMGMT (if TMGMT allowed anonymous users to view source content with the given valid key in query params in url) but it doesn't work. Am I missing something?

> The only additional thing that needs to happen is to properly check the job and job item statuses to prevent requests that will result in a 403 anyway.

I don't get how removing user switching will end up in this "the only additional thing to do ...". It will not work, anonymous users will not have access to entity's `view` pages if they don't have `View published content` permission. TMGMT access bypass doesn't work for source content, yes it generates links like `https://drupal10-app-new.lndo.site/en/node/55?key=...` but it doesn't bypass anonymous users. Did you try this and it worked for you?

Also, could you please explain why in your patch it's necessary to pass job item id to queue processor instead of job id? We only need settings from the provider that we fetch by the job, the job is the same for all the job items so we just pass its id, there is no need to resolve job inside queue processor. Am I missing something?

Also, did you try the solution from the comment #2? Did it work for your case? https://git.drupalcode.org/project/tmgmt_smartling/-/merge_requests/29/d...

bceyssens’s picture

Hi @loparev, thanks for having a look at this!

As you already figured out yourself, the key isn't only used for previewing but also for accessing the source URL (JobItem::getSourceUrl). Granting the access is done in tmgmt_content_entity_access.
You are correct about the "View published content" permission, when that one is disabled the tmgmt functionality isn't working either. Drupal core seems to force this behavior (NodeAccessControlHandler::access) and doesn't allow alteration. When we want to follow tmgmt strategy we should implement the same access logic on job item, thats why we need the item and not the job alone.

We already tried https://git.drupalcode.org/project/tmgmt_smartling/-/merge_requests/29/d... but that didn't work either. The session cookie that is generated is still lacking an ID, like this: SSESSc2b280e207aa821b24606f896b740d6d=.

Have you tried using the contextUsername with tmgmt anonymous_access disabled? Can you verify if the session cookie is build correctly?

loparev’s picture

Hi @bceyssens!

> Have you tried using the contextUsername with tmgmt anonymous_access disabled? Can you verify if the session cookie is build correctly?

I tested these scenarios:
1. anonymous_access off, cron is triggered from web UI by the admin user - session id is generated just fine
2. anonymous_access off, cron is triggered by the drush command `drush --uri=drupal10-app-new.lndo.site -root=/web cron` - session id is not generated
3. anonymous_access on, cron is triggered from web UI by the admin user - session id is generated just fine
4. anonymous_access on, cron is triggered by the drush command `drush --uri=drupal10-app-new.lndo.site -root=/web cron` - session id is not generated

It sounds like it has something to do with the way queue is being processed - session is not being generated if process is tarted by drush command.

loparev’s picture

I think the next steps would be to:
1. Check if current run context is cli/drush run
2. initiate anonymous user session (?)
3. pass execution to the usual flow.

researching for now.

loparev’s picture

I tested user switching and confirm it doesn't work for debug form and for processing items from queue (with retrieving cookies in both scenarios with cron run by drush or by web action/http request). It DOES work from "Send context" bulk action on tmgmt job items list page and I don't understand what's the difference. Will continue looking into that.

With cron that's totally different story - there are no sessions at all. In core session manager service there are checks "if CLI" and all the related to session management methods just do nothing (ignore save/regenerate/etc).

bceyssens’s picture

Hi @loparev, glad we're making progress here!

Triggering cron via the UI is possible, but will it ever be used that way on production environments? If I'm not mistaken, everyone using CLI to process the queues will run into the authentication issue, unless using the default tmgmt functionality.

The above works fine with the tmgmt anonymous_access expect if you really need to support uploading context for Drupal setups that have the "View published content" permission disabled.

So I think you need to decide which of these 2 ways of working you want to support in the module.
If you get the authentication working, that would be even better!

loparev’s picture

> Triggering cron via the UI is possible, but will it ever be used that way on production environments? If I'm not mistaken, everyone using CLI

Correct, agree. I mean not everyone but majority I would imagine. Also, you can setup cli to just call cron url as well. But that's another story, it will not help to solve the core issue - user switching. This will only fallback to anonymous user.

> everyone using CLI to process the queues will run into the authentication issue

Processing queue by drush will result to user switching doesn't switch user for retrieving context and context (html page) will be grabbed under anonymous user, correct.

> unless using the default tmgmt functionality.

Can't agree. This `anonymous_access ` setting from TMGMT has nothing to do with accessing nodes, it has something to do with accessing TMGMT Job Items and I don't agree that we need to bring any of those checks (is job active, is job item in proper state etc) to the context retrieving process because why does it matter in which state is job or job item - we only need to grab node's/entity's html markup.

Tested more deeply and found out that user switching does actually work but only for the next http request, context retrieving still happens under anonymous (if you run manually by http request, not by drush). Also, user is not switched back to previous one.

I tested cases with enabled/disabled tmgmt `anonymous_access` feature - no difference which is expected I believe.

Tests were:

1. Test
"View published content" for anonymous: true
"Allow access to source for translators": true

Result:
Cron by web: cookies - yes; page contents - yes; user - anonymous; context in smartling - yes; impersonated to - context_user after page reload;
Cron by drush: cookies - no; page contents - yes; user - anonymous; context in smartling - yes; impersonated to - no;
Send context bulk action: cookies - yes; page contents - yes; user - context_user; context in smartling - yes; impersonated to - context_user + back;
Context debug form: cookies - yes; page contents - yes; user - anonymous; context in smartling - yes; impersonated to - anonymous + back;

2. Test
"View published content" for anonymous: true
"Allow access to source for translators": false

Result:
Cron by web: cookies - yes; page contents - yes; user - anonymous; context in smartling - yes; impersonated to - context_user after page reload;
Cron by drush: cookies - no; page contents - yes; user - anonymous; context in smartling - yes; impersonated to - no;
Send context bulk action: cookies - yes; page contents - yes; user - context_user; context in smartling - yes; impersonated to - context_user + back;
Context debug form: cookies - yes; page contents - yes; user - anonymous; context in smartling - yes; impersonated to - anonymous + back;

As it's seen, only `Send context bulk action` works as expected.

> So I think you need to decide which of these 2 ways of working you want to support in the module. If you get the authentication working, that would be even better!

I agree. We need either remove user switching at all since it turned out it's not working properly and all the context is gathered under anonymous user or fix the switching. I'm working 2nd option.

loparev’s picture

Reworked approach how we get context html. Instead of switching uers as masquerade module does we generate one time login link for the given context user and then use cookie jar in curl to login user and then request view entity page (with cookies received after logging in). This works for every case listed above.

@bceyssens could you please confirm it works for you? drush --uri=your.domain --root=/your/web/root cron

PR: https://git.drupalcode.org/project/tmgmt_smartling/-/merge_requests/40

loparev’s picture

Status: Needs work » Fixed

Will be included to the next release

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.