collective / sc.social.like

Social: Like Actions is a Plone package (add-on) providing simple Google+, Twitter and Facebook integration for Plone Content Types.
7 stars 23 forks source link

Canonical domain could contain a Plone site id #119

Open hvelarde opened 7 years ago

hvelarde commented 7 years ago

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.

rodfersou commented 7 years ago

I sniff a bug related to cache here...

hvelarde commented 7 years ago

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

rodfersou commented 7 years ago

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

hvelarde commented 7 years ago

see the tests I commented in the other PR: https://github.com/collective/sc.social.like/pull/118/commits/7a8770b167304e27d10187917b9aa1452f6b623a

tcurvelo commented 7 years ago

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 commented 7 years ago

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

hvelarde commented 7 years ago

@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.