Add contact-info view

[cekk]

Seems working good. The only problem that i have, is with the formData. I need to pass formData={{ tilesLayoutFieldname: {} }} to avoid some errors in the Form component.

[coveralls]

Pull Request Test Coverage Report for Build 2730

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at ?%

---

Totals
Change from base Build 2726:	0.0%
Covered Lines:
Relevant Lines:	0

---

💛 - Coveralls

[cekk]

@tisto did you received some notifications about my replies on your reviews? Because i didn't get any email from github for yours.

[tisto]

@cekk yep. Got your comments via email...

[sneridagh]

@cekk Just merged latest master into the branch. Overall it LGTM, but one question, I was considering two things:

• Check if the backend has a correct mail setup (Plone already does that)
• Add a captcha (like https://github.com/dozoisch/react-google-recaptcha)

I know that the last one might be an opt-in, in case you configure it, but still we can warn integrators about the fact if they do not configure one...

/cc @tisto @robgietema @cekk what do you think?

[cekk]

If i remember correctly, i tried it but i couldn't check email config because i needed to call a "private" endpoint: the registry one. I tried to bypass this thing with a better error handling on plone.restapi.

For captcha field, we need to get the value from registry (from collective.recaptcha), right?

[sneridagh]

@cekk I would follow Marie Kondo's philosophy in that kind of questions about if use an existing Plone addon or not. So, does collective.recaptcha will spark joy to Volto if added as an addon in the Plone backend? 😂

My answer would be not, because it won't add any value to it except saving the recaptcha token... and then, it will force us to make a API call to get it...

Then we should thank collective.recaptcha, and discard it 🤣

So my first shot will be add the token to the Volto's config (with an empty default), then get it from there. We are already using a similar approach for Raven, and Google Analytics (not yet in Volto, though, still have to backport it from projects).

/cc @tisto @robgietema @bloodbare @vangheem what do you think about following the Kondo's policy on using Plone addons for Volto features or not from now on? 😛

[sneridagh]

@cekk regarding the mail test, let's leave it as it is, so no check is made. However, I think the recaptcha is required.

[vangheem]

If you stored the captcha secret in settings you'd be able to find it looking at the js source wouldn't you? Seems like that check needs to be in server implemention of volto only if possible.

But otherwise, yes, ideally we don't need plone add-ons. Maybe there is something more clever we can think of...

[sneridagh]

It’s the usual chicken-egg problem when going full frontend for public websites... you will need anonymous access to that endpoint anyways, so the token can be easily acquired, even more easily than de-uglyfying the code.

There are lots of these ones :( but agreed: let’s see if we can think on something better in the future On Wed, 20 Feb 2019 at 13:25, Nathan Van Gheem notifications@github.com wrote:

If you stored the captcha secret in settings you'd be able to find it looking at the js source wouldn't you? Seems like that check needs to be in server implemention of volto only if possible.

But otherwise, yes, ideally we don't need plone add-ons. Maybe there is something more clever we can think of...

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/plone/volto/pull/312#issuecomment-465552997, or mute the thread https://github.com/notifications/unsubscribe-auth/AAduD-kohyeBGy429GdIcklrK7k346z4ks5vPT6jgaJpZM4YYfpd .

[vangheem]

Nah, that's not what I'm suggesting.

You need server side code that does the check, hiding the private key.

Public side has the public key only.

[cekk]

with server-side you mean Plone or Volto's node instance?

Because with the component proposed by Victor, we only need public key (site key) for the check.

I'm trying it with another project (ploneconf site fyi) and it seems working. Like Nathan says, if we set this key on an env variable, then this key will be visible reading the source code. Is it ok exposing this key?

[sneridagh]

@vangheem @cekk how about to create a file (let's called secrets.js) that is only pulled from server.jsx, then it (hopefully, we have to double check) will only be included in the server bundle.

From server.jsx, we create the code regarding the recaptcha validation server side by creating a special route for it in express, and then call the route from the component to validate the code with the secret token.

Only one consideration here: If we start to hack into the SSR (I already started with https://github.com/plone/volto/pull/653), the code might become unmanageable soon. So we have to come up with a way to tidy those special SSR calls. I'm pretty new to express, but I'm sure there's a right way(TM) to do this properly and keep the normal routes working as well.

/cc @robgietema @tisto @bloodbare

[cekk]

This looks too much complex to me. I don't have the complete vision of this project, but for what I understand, Volto should be a new skin for Plone. That means (for me) that all the logic should be handled in Plone, not in a third layer (the node server for ssr).

I also don't understand why we don't need Plone add-ons. Plone already provides several resources with restapi. Is it a bad thing installing a plugin that enables a route to check the captcha?

[vangheem]

@sneridagh yes, that is a good idea.

[sneridagh]

@cekk having a Plone add-on only to store a string in the registry, then add a service (and a use an XHR call) to retrieve it, and after that validate it (you should also do from the SSR process to not expose the private token), then talk again to the component to give the ok for the code, seems to me like just too much. I don't think that would be that much use cases like this one, but this kind of decisions are the ones that should avoid in the future and we should take it as a strategic policy.

It's true that we can still do the validation on Plone, then add a service for it, but it's that worthy?

/cc @tisto @robgietema @vangheem @bloodbare what do you think?

Probably I can draft something in the next days implementing the solution that I proposed.

[cekk]

Ok, as I said you're more into Volto than me, so you better know what's the vision/policy about its features ;)

[sneridagh]

@cekk @vangheem ok, first implementation in place.

I added a new tokens stash where the private ones are not exposed to the client bundle (tested and works well).

I added the recaptcha feature to be generic for all <Form> component and API-less dependent via a prop recaptcha.

However, after having all implemented, I realised that the mail endpoint is still not protected "as must be" (something that we have in mind), and we probably require to implement the recaptcha validation on it directly for this special case. @bloodbare told me that Guillotina provides this OOTB :) The problem is that we are in a point were doing changes in Plone core is difficult (since 5.2 is feature freezed), although for plone.restapi things are a bit different we will have to introduce a few dependencies (at least the recaptcha verification library) which I don't know if the FWT will be up to it...

Anyways, I would go with this implementation and leave the endpoint improvement for some of the upcoming sprints. For now it works for all forms if the prop is enabled, which it already a great feature for me. What do you think?

/cc @tisto @bloodbare @robgietema

[tisto]

@sneridagh how would the user add the recaptcha token? Do I need a create-volto-app app to do this? I think we are missing some docs here. Though, maybe we should agree if that's the way to go before putting more effort into it.

@bloodbare @robgietema @vangheem opinions?

[vangheem]

woah, https://plonerestapi.readthedocs.io/en/latest/email-send.html isn't protected?

Can we just schedule a team call to discuss these issues? I feel like at this point, we need a longer form discussion to get through everything that is going on related to integration.

[sneridagh]

@vangheem it is, because by default only "Manager" role can send mails. We know that it's not the best solution, but for sure it won't be the last implementation. It's on the list of "things to improve".

@bloodbare told me how you do it in Guillotina, I'm +1 for using the same approach and expand it to use it in "all forms". However, not the best moment to introduce changes into the core :) then there's also the inclusion of a third party tool in the core is also to be considered carefully.

@tisto I was waiting to see if that implementation that I did was good enough, and I was waiting for feedback before documenting it. My idea is to use that approach until we have something better in the back. Also, I was -1 in making the config on the Plone side.

[vangheem]

Could we schedule a call about this to discuss?

[tisto]

Yep. Let's do that.

[sneridagh]

@cekk @robgietema and me had a talk about this yesterday, and some weeks ago I discussed this with @vangheem So we concluded:

1. We ship this for now in the same shape that Plone currently provides (a non "secure" form) not using any captcha mechanism.
2. We will investigate and develop a way to do client-server validation, ideally without relying a third party service (Google recaptcha) to do that during the Toulouse and Bonn sprints.

So I would merge it right away.