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

Canonical domain could contain a Plone site id #119

Open
hvelarde opened this issue Jul 21, 2017 · 7 comments
Open

Canonical domain could contain a Plone site id #119

hvelarde opened this issue Jul 21, 2017 · 7 comments
Assignees
Labels

Comments

@hvelarde
Copy link
Member

hvelarde commented Jul 21, 2017

Currently we validate a canonical domain by checking it contains only scheme and netloc:

def validate_canonical_domain(value):
    """Check if the value is a URI containing only scheme and netloc."""
    _ = urlparse(value)
    if not all([_.scheme, _.netloc]) or any([_.path, _.params, _.query, _.fragment]):
        raise Invalid(
            u'Canonical domain should only include scheme and netloc (e.g. <strong>http://www.example.org</strong>)')
    return True

That's fine as long as the Plone site id is removed on a rewrite rule on a front end proxy like nginx or Varnish, but from time to time we need to expose the Plone site id on some sites.

We need to allow the inclusion of at least one path element.

@hvelarde hvelarde added the bug label Jul 21, 2017
@hvelarde hvelarde self-assigned this Jul 21, 2017
hvelarde added a commit that referenced this issue Jul 21, 2017
@rodfersou
Copy link
Member

I sniff a bug related to cache here...

@hvelarde
Copy link
Member Author

I think you're not understanding the issue; this is not related with caching in any way.

@rodfersou
Copy link
Member

You are right.., I still don't understand what is going on here

@hvelarde
Copy link
Member Author

see the tests I commented in the other PR: 7a8770b

@tcurvelo
Copy link
Member

IMHO, validate_canonical_domain checks if a domain is a domain and doesn't need change.
What should be fixed is how object's path is obtained.

I think we should get the object's path from the url in the request. Instead of obj.getPhysicalPath()[2:], we could use:

path = '/'.join(self.request.physicalPathToVirtualPath(obj.getPhysicalPath())

In that case, it will be transparent if the site id is relevant or not, since physicalPathToVirtualPath works well with Zope's VHM. Ex:

item requested url path
http://localhost:8080/Plone/foo 'Plone/foo'
http://localhost:8080/VirtualHostBase/https/foo.com/Plone/VirtualHostRoot/bar 'bar'

@tcurvelo
Copy link
Member

tcurvelo commented Jul 24, 2017

I'm not sure from where I should have branched out. Let me know if you need to rebase it.

@hvelarde
Copy link
Member Author

@tcurvelo thanks! that's the solution I was looking for at the beginning but my knowledge about VHM is weak; you branch has to be based on master branch.

hvelarde added a commit that referenced this issue Jul 24, 2017
* Fix Canonical URL updater form

A new upgrade step is also provided to update the objects_provides catalog index.

* Skip S001 caused by bug in code analysis

We need to find out where to open an issue about this.

* Run tests only if Dexterity-based content types are installed

* Fix test

* Skip tests related with #119

* Document issue with code analysis

Refs. gforcada/flake8-pep3101#16

* Disable catalog queue on tests

See: https://community.plone.org/t/strange-catalog-behavior-on-plone-5-1/4582/3?u=hvelarde
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants