Bug 17109 - sendbasket: Remove second authentication, add CSRF token
Summary: sendbasket: Remove second authentication, add CSRF token
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 16929
Blocks: 17096 17110
  Show dependency treegraph
 
Reported: 2016-08-11 11:40 UTC by Marcel de Rooy
Modified: 2017-12-07 22:15 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17109: Remove second authentication from (opac-)sendbasket (3.52 KB, patch)
2016-08-11 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17109: Add CSRF token to [opac-]sendbasket (7.99 KB, patch)
2016-08-11 12:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17109: Use Koha.Preference in sendbasket template (3.58 KB, patch)
2016-08-12 06:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17109: Remove second authentication from (opac-)sendbasket (3.65 KB, patch)
2016-08-15 13:02 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17109: Add CSRF token to [opac-]sendbasket (8.08 KB, patch)
2016-08-15 13:05 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17109: Use Koha.Preference in sendbasket template (3.66 KB, patch)
2016-08-15 13:07 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17109: [QA Follow-up] Die when wrong token (4.14 KB, patch)
2016-08-16 12:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17109: Remove second authentication from (opac-)sendbasket (3.72 KB, patch)
2016-08-22 14:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17109: Add CSRF token to [opac-]sendbasket (8.15 KB, patch)
2016-08-22 14:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17109: Use Koha.Preference in sendbasket template (3.73 KB, patch)
2016-08-22 14:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17109: [QA Follow-up] Die when wrong token (4.17 KB, patch)
2016-08-22 14:15 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2016-08-11 11:40:44 UTC

    
Comment 1 Marcel de Rooy 2016-08-11 12:53:48 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2016-08-11 12:54:14 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-08-12 06:34:16 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2016-08-15 13:02:35 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2016-08-15 13:05:52 UTC Comment hidden (obsolete)
Comment 6 Marc Véron 2016-08-15 13:07:15 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-08-15 13:49:35 UTC
Comment on attachment 54471 [details] [review]
Bug 17109: Add CSRF token to [opac-]sendbasket

Review of attachment 54471 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-sendbasketform.tt
@@ +20,5 @@
>                              [% END %]
>  
> +                            [% IF csrf_error %]
> +                                <p>No valid CSRF token!</p>
> +                                <p><a class="focus close" href="#">Close window</a></p>

If you do that here, I will have to do it in all other places (and really don't want to! :) )
Are you sure we really want to stylish it?
Comment 8 Marcel de Rooy 2016-08-16 12:28:15 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2016-08-16 12:28:56 UTC
(In reply to Jonathan Druart from comment #7)
> If you do that here, I will have to do it in all other places (and really
> don't want to! :) )
> Are you sure we really want to stylish it?

No problem
Comment 10 Jonathan Druart 2016-08-22 14:15:40 UTC
Created attachment 54705 [details] [review]
Bug 17109: Remove second authentication from (opac-)sendbasket

Patch deals with opac and intranet variant.
If we authenticated the first time, it is not necessary to do it
a second time rightaway.

Replaces a call to get_template_and_user (including checkauth) by
gettemplate.

Also removes duplicate use C4::Biblio statements.

Test plan:
[1] Put a few books in the cart.
[2] Send the cart from OPAC.
[3] Send the cart from intranet.

Tested 3 patches together. Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2016-08-22 14:15:44 UTC
Created attachment 54706 [details] [review]
Bug 17109: Add CSRF token to [opac-]sendbasket

If you have no (valid) token, you will not be able to send the message.

Test plan:
[1] Verify if you can still send the cart from opac and intranet.
[2] While still being logged in, try to send the cart from opac by
    using the following URL:
    /cgi-bin/koha/opac-sendbasket.pl?email_add=you@somedomain.com&comment=csrf_test&bib_list=doesnotmatter&csrf_token=justsomeguess12345
    This should now result in a csrf error.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2016-08-22 14:15:48 UTC
Created attachment 54707 [details] [review]
Bug 17109: Use Koha.Preference in sendbasket template

No need to send OPACBaseURL to the template, if you load the Koha TT
plugin inside the template.

Test plan:
Send a few items in your cart from OPAC and intranet.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2016-08-22 14:15:55 UTC
Created attachment 54708 [details] [review]
Bug 17109: [QA Follow-up] Die when wrong token

Removes template var csrf_error and associated handling.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Restested with opac and intranet: Still sends or dies elegantly..

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Kyle M Hall 2016-09-09 14:38:19 UTC
Pushed to master for 16.11, thanks Marcel!
Comment 15 Mason James 2017-05-03 02:26:39 UTC
Pushed to 16.05.x, for 16.05.12 release