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

BUGFIX: Use a dynamic URL for user impersonation #4875

Merged
merged 2 commits into from
Feb 11, 2024

Conversation

markusguenther
Copy link
Member

@markusguenther markusguenther commented Feb 6, 2024

As described in the issue, the impersonation does not work when Neos is running in a subfolder. This change adds a data attribute with a dynamic URL to the DOM, and the user impersonation is using this module URL as base.

Fixes: #4797

Review instructions

Use the user impersonation in the Backend modules (User Management and other) when Neos is running in a subfolder.
e.g. BASEURL.com/cms/neos

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

As described in the issue, the impersonation does not work when Neos is running in a subfolder. This change adds a data attribute with a dynamic URL to the DOM, and the user impersonation is using this module URL as base.

Fixes: #4797
@github-actions github-actions bot added the Bug label Feb 6, 2024
@markusguenther
Copy link
Member Author

This PR needs also an adjustment in the Neos-UI and will be linked soon.

@github-actions github-actions bot added the 8.0 label Feb 6, 2024
@ahaeslich ahaeslich self-requested a review February 6, 2024 15:56
@mhsdesign
Copy link
Member

Hmm usually we avoid string concatenating routes in the frontend (see neos ui)
so i would prefer to boot the impersonation with server calculated endpoints instead of this as this is always rather flaky and also hard to track usages of this endpoint that way:

this._basePath + 'user-change'
this._basePath + 'status'
this._basePath + 'restore'

but its your feature do as you feel fit :D

@markusguenther
Copy link
Member Author

markusguenther commented Feb 6, 2024

@mhsdesign Yes that would be better but with Fluid you just have the <f:uri.action controller="Impersonate" package="Unikka.LoginAs" action="impersonateUserWithResponse" format="json" /> and this will lead to a not so nice URL. So I thought it would work better with that. I also thought about a new Helper for _getBasePath but I think the little duplication is ok here, as we don't use it somewhere else.

@markusguenther markusguenther changed the title BUGFIX: Use a dynamic url for user impersonation BUGFIX: Use a dynamic URL for user impersonation Feb 6, 2024
@markusguenther
Copy link
Member Author

Ok, we don't need the UI adjustment as we already use the dynamic URL here.
https://github.com/markusguenther/neos-ui/blob/b575442d2700b429485c06b90119182d50720124/Resources/Private/Fusion/Backend/Root.fusion

@ahaeslich
Copy link
Member

Just tested it in a project using this "subfolder" setup:

  • Changing the user: ✅ works
  • Redirect to Neos backend afterwards: ❌ doesn't work in both places (menu button and user administration)

We need to change the usage of window.location.pathname = '/neos' in both JS files:

@ahaeslich
Copy link
Member

@markusguenther I also found window.location.pathname = '/neos' in the UI:

https://github.com/neos/neos-ui/blob/38774df6064c977026b1bd518b90aa12ee2e9ef9/packages/neos-ui-sagas/src/UI/Impersonate/index.js#L41

@ahaeslich ahaeslich linked an issue Feb 6, 2024 that may be closed by this pull request
1 task
@markusguenther
Copy link
Member Author

We now have another PR in the Neos-UI neos/neos-ui#3713

Copy link
Member

@ahaeslich ahaeslich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it with success in combination with neos/neos-ui#3713.

I agree with @mhsdesign about the server calculated endpoints. We for sure can change this when we refactor the user backend module to not use fluid anymore.

@mhsdesign
Copy link
Member

Thanks :D I dont know fluid well enough but i trust you so consider this as half approval ;)

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By reading ;)

Copy link
Contributor

@dlubitz dlubitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good by reading!

@mhsdesign
Copy link
Member

Still not a fan of the uri concatenation on the client, even if the uri would be uglier if generated from the server.
Buuut i never stumbled upon this know and never before and as this is just a bugfix lets do it :D

@mhsdesign mhsdesign merged commit 4b137ed into 8.0 Feb 11, 2024
10 checks passed
@mhsdesign mhsdesign deleted the bugfix/user-impersonation-with-subfolders branch February 11, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Login As (user impersonation) not working with subfolders
4 participants