Bug 24477 - No permissions check for POS feature
Summary: No permissions check for POS feature
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 23354
Blocks: 23355
  Show dependency treegraph
 
Reported: 2020-01-22 07:23 UTC by Katrin Fischer
Modified: 2020-03-05 13:21 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24477: Require `takepayment` permission in pos/pay.pl (1.57 KB, patch)
2020-01-22 10:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24477: Require `takepayment` permission in pos/pay.pl (949 bytes, patch)
2020-01-22 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24477: Require `takepayment` permission in pos/pay.pl (1016 bytes, patch)
2020-01-22 13:57 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24478: Use `EnablePointOfSale` preference (3.31 KB, patch)
2020-01-22 14:04 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24477: Require `takepayment` permission in pos/pay.pl (1.06 KB, patch)
2020-01-22 14:04 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24477: Require `takepayment` permission in pos/pay.pl (1.05 KB, patch)
2020-01-23 07:27 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2020-01-22 07:23:08 UTC
At the moment it appears that POS can be used without any permission - This means a patron with no permissions can access the page!

my ( $template, $loggedinuser, $cookie, $user_flags ) = get_template_and_user(
    {
        template_name   => 'pos/pay.tt',
        query           => $q,
        type            => 'intranet',
        authnotrequired => 0,
    }
);
Comment 1 Martin Renvoize 2020-01-22 10:28:00 UTC
Created attachment 97697 [details] [review]
Bug 24477: Require `takepayment` permission in pos/pay.pl

When adding the point of sale take payment page in bug 23354 we missed
the permission check script side along with adding the permission at
install time (update was caught).
Comment 2 Martin Renvoize 2020-01-22 13:50:37 UTC
Created attachment 97727 [details] [review]
Bug 24477: Require `takepayment` permission in pos/pay.pl

When adding the point of sale take payment page in bug 23354 we missed
the permission check script side along with adding the permission at
install time (update was caught).
Comment 3 ByWater Sandboxes 2020-01-22 13:57:52 UTC
Created attachment 97728 [details] [review]
Bug 24477: Require `takepayment` permission in pos/pay.pl

When adding the point of sale take payment page in bug 23354 we missed
the permission check script side along with adding the permission at
install time (update was caught).

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 4 ByWater Sandboxes 2020-01-22 14:04:25 UTC
Created attachment 97729 [details] [review]
Bug 24478: Use `EnablePointOfSale` preference

This patch adds the `EnablePointOfSale` preference to the accounting tab
on the preferences editor and ties the display of the Point of Sale
module to it.

Test plan:
1) Enable both `UseCashRegisters` and `EnablePointOfSale` preferences.
2) Ensure you are a superuser or your user has the
   takepayment permission.
3) The Point of Sale module should be present on your staff home page
   with the above configuration, otherwhise it should not.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 5 ByWater Sandboxes 2020-01-22 14:04:27 UTC
Created attachment 97730 [details] [review]
Bug 24477: Require `takepayment` permission in pos/pay.pl

When adding the point of sale take payment page in bug 23354 we missed
the permission check script side along with adding the permission at
install time (update was caught).

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 6 Andrew Fuerste-Henry 2020-01-22 14:06:32 UTC
Apologies for signoff confusion from sandboxes!
Comment 7 Martin Renvoize 2020-01-22 15:14:14 UTC
No apology necessary, thanks for taking the time to look at it :)
Comment 8 Katrin Fischer 2020-01-23 07:27:56 UTC
Created attachment 97780 [details] [review]
Bug 24477: Require `takepayment` permission in pos/pay.pl

When adding the point of sale take payment page in bug 23354 we missed
the permission check script side along with adding the permission at
install time (update was caught).

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2020-01-23 07:29:34 UTC
Totally unrelated... but as I am running out of time: Would you be opposed to rename Purchase items to the "Debit types". I think "Purchase items" is quite confusing for translation (probably going to get mixed up with suggestions) and it seems strange to have 2 different names for the same thing. If you just go to admin, you won't find anything named this there.
Comment 10 Martin Renvoize 2020-01-23 08:53:43 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 11 Martin Renvoize 2020-01-23 09:42:28 UTC
(In reply to Katrin Fischer from comment #9)
> Totally unrelated... but as I am running out of time: Would you be opposed
> to rename Purchase items to the "Debit types". I think "Purchase items" is
> quite confusing for translation (probably going to get mixed up with
> suggestions) and it seems strange to have 2 different names for the same
> thing. If you just go to admin, you won't find anything named this there.

Hmm, feel free to open that as a new bug else I'll completely forget about it..

I can see where you're coming from though I'm not entirely sure about the change.  What I'd rather do is try to match the term to the flag that sets whether a debit type can be sold via the POS system vs the patron accounts pages... 'Item can be sold' I think it is.
Comment 12 Katrin Fischer 2020-01-23 09:52:43 UTC
(In reply to Martin Renvoize from comment #11)
> (In reply to Katrin Fischer from comment #9)
> > Totally unrelated... but as I am running out of time: Would you be opposed
> > to rename Purchase items to the "Debit types". I think "Purchase items" is
> > quite confusing for translation (probably going to get mixed up with
> > suggestions) and it seems strange to have 2 different names for the same
> > thing. If you just go to admin, you won't find anything named this there.
> 
> Hmm, feel free to open that as a new bug else I'll completely forget about
> it..
> 
> I can see where you're coming from though I'm not entirely sure about the
> change.  What I'd rather do is try to match the term to the flag that sets
> whether a debit type can be sold via the POS system vs the patron accounts
> pages... 'Item can be sold' I think it is.

No problem with opening a new bug - set this mostly as a reminder to myself. But not sure I can follow your line of thought there?
Comment 13 Andrew Fuerste-Henry 2020-01-23 13:07:24 UTC
I opened bug 24490 for the wording of that link and then bug 24491 suggesting some filtering options on the debit types page.
Comment 14 Katrin Fischer 2020-01-23 13:35:26 UTC
(In reply to Andrew Fuerste-Henry from comment #13)
> I opened bug 24490 for the wording of that link and then bug 24491
> suggesting some filtering options on the debit types page.

Thx!
Comment 15 Martin Renvoize 2020-01-23 14:07:54 UTC
Thanks guys, this is all really helpful feedback :)
Comment 16 Joy Nelson 2020-02-06 22:44:47 UTC
does not apply to 19.11.x.  Not backported