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

Fix authentication error viewing ZMI with a user defined outside of zope root #1196

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

perrinjerome
Copy link
Contributor

Fixes #1195

@d-maurer
Copy link
Contributor

d-maurer commented Feb 17, 2024 via email

@perrinjerome
Copy link
Contributor Author

Thank you

it seems suboptimal to me: quite complex and less HTTP cache efficient. I would prefer a different approach: make the necessary resources public.

Yes, this is a bit complex. Frankly, I would also prefer to make the resources public, I don't see why they need to be protected.

@dataflake
Copy link
Member

I absolutely don't like publishing a whole bunch of JS out of a basic Zope install that none of us can audit for issues.

@dataflake
Copy link
Member

To expand on my earlier answer, I don't care about the CSS. I am concerned about the JS. If this was just a few script files we maintained ourselves that would be OK. But we have several megabytes of stuff in there, including the full Ace editor package, bootstrap, jquery and fontawesome. None of us has any idea what all that code does. Just publishing it freely for convenience seems a bad trade-off for me.

Jerome's solution looks OK to me, it preserves the safety barrier. I don't think caching is really a valid concern. We're talking a ZMI, an application that requires authentication everywhere. It doesn't make much sense to try and cache things there.

@perrinjerome
Copy link
Contributor Author

You probably understood that, but I want to clarify this about the caching concerns: My first naive idea was to make the resources relatives to the current folder. This would have fixed the original issue, but it would have been really bad for cache, because each folder visited in the ZMI would use different URLs for js and css. By making the resources relative to the authentication path like it's implemented here, each admin user always uses the same URLs for js and css, so they can be cached in the browser cache.

dataflake
dataflake previously approved these changes Feb 20, 2024
@perrinjerome
Copy link
Contributor Author

I resolved a conflict, it would be good to decide something here. @dataflake approved a previous version of the changes. The argument that it's better to be safe and not publish javascript that we don't know sounds reasonable (at least the risk is limited to ZMI).

@d-maurer you're also OK ?

I don't really have another idea how to fix this.

@d-maurer
Copy link
Contributor

d-maurer commented Feb 27, 2024 via email

@perrinjerome perrinjerome merged commit 7f6a4d1 into zopefoundation:master Feb 27, 2024
21 checks passed
@perrinjerome perrinjerome deleted the fix/zmi_sub_folder branch February 27, 2024 09:44
@perrinjerome
Copy link
Contributor Author

Thank both of you

@mauritsvanrees
Copy link
Member

This fixes the problem for the normal case, so thanks. But this gives wrong urls when using virtual hosting. A Zope Manager gets this link in the head:

href="/plone/virtual_hosting/++resource++zmi/zmi_base.css"

A Plone manager gets this link in the head:

href="/plone/virtual_hosting///++resource++zmi/zmi_base.css"

I opened a new issue #1203 and show a partial fix so the urls are correct.

But even with correct links, I get an Unauthorized when using virtual hosting, so the root cause is not really this PR.

perrinjerome added a commit to perrinjerome/Zope that referenced this pull request Apr 12, 2024
The previous approach from zopefoundation#1196 was not correct when using virtual host,
because AUTHENTICATION_PATH is not usable in virtual host contexts.

This uses a different approach, by making the js and css path
subscribers take care of generating the URLs with the authentication
path prepended using request API aware of virtual hosting.
perrinjerome added a commit that referenced this pull request Apr 12, 2024
The previous approach from #1196 was not correct when using virtual host,
because AUTHENTICATION_PATH is not usable in virtual host contexts.

This uses a different approach, by making the js and css path
subscribers take care of generating the URLs with the authentication
path prepended using request API aware of virtual hosting.
perrinjerome added a commit that referenced this pull request Apr 14, 2024
The previous approach from #1196 was not correct when using virtual host,
because AUTHENTICATION_PATH is not usable in virtual host contexts.

This uses a different approach, by making the js and css path
subscribers take care of generating the URLs with the authentication
path prepended using request API aware of virtual hosting.
perrinjerome added a commit that referenced this pull request Apr 24, 2024
…1204)

* ZMI: re-implement the logic to prepend authentication path

The previous approach from #1196 was not correct when using virtual host,
because AUTHENTICATION_PATH is not usable in virtual host contexts.

This uses a different approach, by making the js and css path
subscribers take care of generating the URLs with the authentication
path prepended using request API aware of virtual hosting.

* ZMI: use /++resource++logo/ for public resources

/++resource++zmi/logo/ and /++resource++logo/ is the same folder on disk,
the former is protected by Manage portal and the later is public.
Protected resources were problematic, because we want to serve them from
the root, but the manager user might not have permission on the root.

* ZMI: include zmi.localstorage.api.js using js subscriber

Using this approach we serve a resource relative to the path where user
is authenticated.

* copyright.dtml: update bootstrap URL

This was referencing an old URL.

Co-authored-by: Maurits van Rees <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authentication error viewing ZMI with a user defined outside of zope root
5 participants