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

Link to file in Email is missing when submitting while not logged in #1225

Closed
baukezwaan opened this issue Aug 7, 2023 · 24 comments
Closed

Comments

@baukezwaan
Copy link
Contributor

baukezwaan commented Aug 7, 2023

When a not-loggedin user submits a form, with a file-upload-field... the link to the file is missing in the email. Instead a message is shown: 'You don't have the right permissions to download this file'

This seems to be introduced since #1158 (tested on Silverstripe 4 - userforms 5.15.3)

Steps to reproduce:

  • Create new UserForm
  • Add one filefield
  • Add one Email Recipient
  • Test setup, by submitting form in fronted while logged in (observe a link to the file is present in the email)
  • Open incognito browser
  • Submit the form again and check the generated Email:
  • Instead of a link the file, the text 'You don't have the right permissions to download this file' is shown

Acceptance criteria

  • The link to the file is always included in the email.
  • If the file is protected, the email tells you that you have to be logged in to see the file.
  • The message in the email matches the default locale of the site which may differ from the locale of the person submitting the form.

Slightly related to #1217

PRs

@GuySartorelli
Copy link
Member

Hi, thanks for reporting this.
Can you please confirm the following:

  1. The recipient(s) is a Member in the database, who has read access to the folder the submission upload files are being saved into
  2. The file is correctly linked in the email if an authenticated member uploads the form - and that's the only difference between it being linked and not being linked

@michalkleiner
Copy link
Contributor

We've seen this issue as well, I'll try to confirm the replication steps.

@baukezwaan
Copy link
Contributor Author

@GuySartorelli thanks for looking into this

  1. The recipient(s) is a Member in the database, who has read access to the folder the submission upload files are being saved into

The recipient is not a Member, it is just an emailaddress. This is not what the issue is about, the body of the email send to this recipient is faulty.

  1. The file is correctly linked in the email if an authenticated member uploads the form - and that's the only difference between it being linked and not being linked

That is indeed the case. The email is correct when an authenticated users submits the form. But is incorrect when a anonymous visitor submits the form.

@michalkleiner
Copy link
Contributor

And when an anonymous user submits the form, the expectation is the link will display the file if you're logged in. We had a custom enhancement that was altering these links to go via the login mechanism with the file link being the backUrl so that it would prompt you to log in, but that would be out of scope for this fix.

@baukezwaan
Copy link
Contributor Author

For now I have temporary worked around this, by changing the canView of the uploaded file for UserDefinedForm-users:
https://gist.github.com/baukezwaan/87d1db1a72251c0ee588c26be6ad5cc0

@michalkleiner
Copy link
Contributor

I don't think we can do that on an MBIE site as the submissions need to stay private and I wouldn't want to risk it that someone somehow gets a file via UDF controller that they are not supposed to.

baukezwaan added a commit to hamaka/silverstripe-userforms that referenced this issue Aug 11, 2023
@baukezwaan
Copy link
Contributor Author

I don't think we can do that on an MBIE site as the submissions need to stay private and I wouldn't want to risk it that someone somehow gets a file via UDF controller that they are not supposed to.

@michalkleiner You are right, it is too broad to apply this to all files. Maybe this PR solution is more specific and aligns better with the previous behaviour.

@michalkleiner
Copy link
Contributor

I can confirm this is an issue for a large MBIE site and the replication steps in the issue description are accurate.

@michalkleiner
Copy link
Contributor

Adding impact/high as for large files the file link is the only way how some UDF recipients can get to downloading the file for their further use when working with the submission.

@michalkleiner
Copy link
Contributor

@GuySartorelli the second AC 'If the file is protected, the email tells you that you have to be logged in to see the file.' can probably be removed as there have been enhancements ensuring the submitted files are always placed in a protected folder, so it's always the case that you need to be logged in to view it.

@baukezwaan
Copy link
Contributor Author

@GuySartorelli the second AC 'If the file is protected, the email tells you that you have to be logged in to see the file.' can probably be removed as there have been enhancements ensuring the submitted files are always placed in a protected folder, so it's always the case that you need to be logged in to view it.

I agree.
And if that's agreed upon: the third AC will become redundant, since there is no message to be shown.

@GuySartorelli
Copy link
Member

GuySartorelli commented Aug 16, 2023

I think it's still suitable to have a message to that effect - if the folder is always protected, that just means there is always a message in the email.

The idea is that this will (hopefully) help to avoid any confusion in the event someone hasn't logged in yet and clicks one of these links - I can absolutely see some content managers raising alarms that "oh I can't access the file", so if the email says "you must be logged in" then a) they will (hopefully) not raise the alarm in the first place, but if they do, whoever responds can simply say "as stated in the email, you must be logged in".

@baukezwaan
Copy link
Contributor Author

baukezwaan commented Aug 17, 2023

Fair point. However it brings some more complexity to something that was fairly simple.

We have to take into account:

  • the folder where the file will be stored can in some cases also be public accessible, depending on the CMS settings. This needs to be checked, before inserting the message
  • the language of the translation should be site-default, not the locale from the submitting user (as you already previously mentioned)

And don't know if it's an argument; but it always worked this way, without an extra message in the email.

@michalkleiner
Copy link
Contributor

I would vote for not making this more complex than it needs to be. In most cases people who receive these submission emails (if configured) are aware of these things. The submitting user usually gets a copy as well without the links, or doesn't get a copy at all based on how the form is configured.
The scope of this ticket was originally "this stopped working with the latest changes", not that anyone complained about it before, so I'd rather not inflate this too much.

For our project, we override the SubmittedFileField and we actually update the link to go via the backUrl of the login form, which makes the person log in if they're not or just simply shows the file if they are logged in and have permissions to see it.

That can be a potential enhancement to the module, but now we should just focus on the permissions issue in isolation.

@maxime-rainville
Copy link

Played with this a bit. If you have a link like this, you'll be prompted to log in if you are not already. Otherwise, you'll be redirected to the file right away.

http://cms5x-link-sandbox.max.silverstripe.com/Security/login?BackURL=%2Fassets%2F20210219_152148.jpg

Maybe we strike those last two ACs and just say "You are prompted to login if needed".

@michalkleiner
Copy link
Contributor

Yes, that format of a link is what we're using through a custom modification, however if you have a standard link to a protected file, you'll be redirected to the file only when you are already logged in. Otherwise it just plain 'four-oh-fours' (totally a verb that exists).

@baukezwaan
Copy link
Contributor Author

@emteknetnz

Thanks, but this does not solve the original issue properly if you ask me.

The issue is the link in the Email not clickable anymore (when the submitting user is not authenticated eg.: always),
The solution should not be "change the label for this mail", but to restore the previous functionality.

Furthermore, this message is shown in two scenario's:

  1. when a authenticated user visits the CMS to browse submissions (the the new text doesn't make sense there, since you are already in the CMS)
  2. when someone receives an e-mail

So, I totally agree that the original PR didn't get much activity recently, but it's not because the solution does not work. The discussion was about introducing new functionality by changing texts, which IMO should be another issue.

@michalkleiner
Copy link
Contributor

The main issue is the email displays different info per submitted file field (link/no link) based on whether the front-end UDF was submitted by an authenticated user or public user. It should behave the same regardless of whether the form submitting user is logged into the CMS or not.

@emteknetnz
Copy link
Member

emteknetnz commented Sep 4, 2023

The pull-request I submitted should show a link if it was submitted by an unauthenticated public user (unless my PR is implemented wrong)

Previous behaviour at 5.15.6

File submitted by someone that passes canView() permission: (i.e. admin logged into cms)
[filename] - <a href="[link-to-file]" target="_blank">Download file</a>

File submitted by someone that fails canView() permission: (i.e. virtually everyone)
<i class="icon font-icon-lock"></i> [filename] You don't have the right permissions to download this file

Pull-request behaviour

File submitted by someone that passes canView() permission: (i.e. admin logged into cms)
[filename] - <a href="[link-to-file]" target="_blank">Download file</a>

File submitted by someone that fails canView() permission: (i.e. virtually everyone)
<i class="icon font-icon-lock"></i> [filename] <a href="[link-to-file]" target="_blank">Download file</a> - <em>You must be logged in to view this file</em>

@GuySartorelli
Copy link
Member

I think the behaviour needs to be the same regardless of who submits the file - since the person viewing the link is the one who does or doesn't need permissions. There shouldn't be any canView() check for the submission at all, I think.

@emteknetnz
Copy link
Member

Yeah that makes sense, have updated PR

@sabina-talipova sabina-talipova self-assigned this Sep 5, 2023
@sabina-talipova
Copy link
Contributor

PR merged. Issue was solved. This will be automatically tagged. Close.

@sabina-talipova sabina-talipova removed their assignment Sep 5, 2023
@baukezwaan
Copy link
Contributor Author

Thanks for the work on the solution.

I still don't really understand, why you would want another message in the email for authenticated users submitting the form. But this is for us rather an edge case, since 99% of the users of the front-end forms won't be a CMS user at all.

The only problem probably is when testing the form as a content editor. They might trigger the notification mail as authenticated user, and generate an e-mail without a working link to the file.

So again, rather an edge case, but can lead to unexpected behavior. This thread can help other developers as to understand why these emails might differ when testing.

@emteknetnz
Copy link
Member

@baukezwaan That particular method is used to generated a string used in two different places

  • The email message
  • Some text that shows in the CMS to logged in user when viewing a submission

Initially I thought it was only used by the email message, though during peer review it was pointed out that it's also used within the CMS

I hope that clarifies things.

The only problem probably is when testing the form as a content editor. They might trigger the notification mail as authenticated user, and generate an e-mail without a working link to the file.

I've spun up a new issue for this. I've marked it as low priority so I can't give any sort of ETA when it gets picked up, though feel free to crate a PR for this yourself if you'd like #1239

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants