Bug 14272

There is a security issue in this version; the ID supplied from the URL parameter is not checked for expiry. This means people can watch old, expired news by changing the ID. This is known as 'object reference bug'. Whether it is a problem or not is another matter.

I just realized that the ability show arbitrary news items by ID poses a vulnerability; you can view stuff with visbility other than the OPAC.
This would leak internal news to whomever has access to the OPAC.

Hi Martin, I like this feature.

Questions: 

- When I display the single item, the news header has still the same link (on itself). Could this be changed to a link back to where I came from?

- The single display shows the html from syspref OpacMainUserBlock. Is that necessary or could the single view display the contents of the news item only?

I did not sign this off. I rebased it to apply cleanly.

(In reply to M. Tompsett from comment #14)
> Created attachment 47556 [details] [review] [review]
> Bug 14272: Show single news item (tests)
> 
> Updates tests to match the modified NewsChannels module.
> 
> Sponsored-by: Halland County Library

The tests fail:

t/db_dependent/NewsChannels.t .. 1/12 # Category CAT1 not found, inserting
# Borrower BRWR1 not found, inserting

#   Failed test 'got back expected news item via get_opac_new - ID 1'
#   at t/db_dependent/NewsChannels.t line 131.
#     Structures begin differing at:
#          $got->{newdate} = Does not exist
#     $expected->{newdate} = '01/01/2000'

#   Failed test 'got back expected news item via get_opac_new - ID 2'
#   at t/db_dependent/NewsChannels.t line 158.
#     Structures begin differing at:
#          $got->{newdate} = Does not exist
#     $expected->{newdate} = '01/01/2000'
# Looks like you failed 2 tests of 12.
t/db_dependent/NewsChannels.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/12 subtests 

Test Summary Report
-------------------
t/db_dependent/NewsChannels.t (Wstat: 512 Tests: 12 Failed: 2)
  Failed tests:  8-9
  Non-zero exit status: 2
Files=1, Tests=12,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.59 cusr  0.04 csys =  0.65 CPU)
Result: FAIL

Sorry for all the comments, git kept timing out >.< so annoying! Won't change this to Signed Off as those tests don't pass, but will sign off once they're fixed to pass!

Sorry, does not apply:

Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt

I've moved this back to assigned, since I only rebased it.
The issue was .raw which was removed because the security patch which escaped all the data was removed. Also, 'new'? Can we try to have keys/fields which aren't reserved words?

Patch didn't apply, 

Some problems occurred applying patches from bug 14272:
<h1>Something went wrong !</h1>Applying: Bug 14272: Show single news item (tests)
Applying: Bug 14272: Show single news item (backend)
Using index info to reconstruct a base tree...
M	C4/NewsChannels.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/NewsChannels.pm
CONFLICT (content): Merge conflict in C4/NewsChannels.pm
Patch failed at 0001 Bug 14272: Show single news item (backend)

Just some questions, but I don't feel any of this is a blocker:

- In the breadcrumbs the link says "News item" - why not use the heading in the breadcrumbs?
- The RSS feed remains visible - is that intended?
- OpacMainUserBlock remains at the bottom - is that intended?

(In reply to Katrin Fischer from comment #42)
> Just some questions, but I don't feel any of this is a blocker:
> 
> - In the breadcrumbs the link says "News item" - why not use the heading in
> the breadcrumbs?
> - The RSS feed remains visible - is that intended?
> - OpacMainUserBlock remains at the bottom - is that intended?

- Should not we add a message if news_id is passed but wrong?

Also (blocker):
- parameter $time is added to GetNewsToDisplay but not used, we should remove it

Created attachment 76015 [details] [review]
Bug 14272: Show single news item [alternative patch]

This is a new patch for this bug that shows a single news item on the
OPAC using Koha Objects (Koha::News).

To test:
1) Go to Tools -> News on staff interface, make a news item to show on
the OPAC
2) Go to OPAC
3) Notice news item shows and title cannot be clicked
4) Apply patch, refresh page
5) Notice title is now a link. Click title
6) Confirm breadcrumbs shows title of news item
7) Confirm RSS feed and OpacMainUserBlock are not visible
8) In the URL, change the news_id param to an ID that does not exist
9) Confirm that an appropriate error message shows

Sponsored-by: Catalyst IT

Should I refrain from signing this off if the lingering issue mentionned in comment#2 is still reproducible?

I feel like it shouldn't be too hard to add a method like Koha::News::is_displayable that would filter the array from the search method, and return only what's acceptable for the given interface/lang.

Went with the path of least resistance: search query now filters based on the news_lang retrieved earlier in the script.

This should avoid retrieving news marked 'slip' or 'koha', and even those in other languages than the one currently displayed.

Also removed a lingering variable, koha_news_count, that was not used anymore, and not even passed to the template.

Test plan for this follow-up patch:

1) Make sure you have news from all different Location to display. At minimum, you should have the 'koha', 'slip' and 'en' locations readily available.
1.1) If you don't, create them at <intranet url>/cgi-bin/koha/tools/koha-news.pl
2) Apply patch "Bug 14272: Show single news item [alternative patch]"
3) Visit the OPAC
4) Click on the news' title
4.1) Not seeing any news? Go back to 1.1
5) Change the news_id parameter in the URL for another news that you've just created
5.1) You should be able to display any news, even those which you shouldn't have access to
6) Apply (follow-up) patch
7) Repeat 3) to 5), but this time, you should see an error message

Created attachment 76580 [details] [review]
Bug 14272: (follow-up) only display news available for a particular opac lang [alternative patch]

(In reply to Katrin Fischer from comment #42)

> - OpacMainUserBlock remains at the bottom - is that intended?

I think Quote of the Day and OpacMainUserBlock shouldn't show if you're looking at one news item.

A problem I found: If there is no news to display, OpacMainUserBlock and Quote of the Day don't show at all. I think the [% END %] at line 131 is misplaced.

Created attachment 76662 [details] [review]
Bug 14272: [FOLLOW-UP] Fixing template error

To test, confirm that OpacMainUserBlock and Quote of the Day do not show
if viewing a single news item, but do show otherwise.

The page layout is not working right now: The login form and user dashboard are displayed below the news (single or multiple) instead of in the sidebar.

Created attachment 77546 [details] [review]
Bug 14272: (follow-up) Fixing template error and layout

To test, confirm that OpacMainUserBlock and Quote of the Day do not show
if viewing a single news item, but do show otherwise. Confirm user
dashboard shows where expected

Created attachment 77564 [details] [review]
Bug 14272: Show single news item [alternative patch]

This is a new patch for this bug that shows a single news item on the
OPAC using Koha Objects (Koha::News).

To test:
1) Go to Tools -> News on staff interface, make a news item to show on
the OPAC
2) Go to OPAC
3) Notice news item shows and title cannot be clicked
4) Apply patch, refresh page
5) Notice title is now a link. Click title
6) Confirm breadcrumbs shows title of news item
7) Confirm RSS feed and OpacMainUserBlock are not visible
8) In the URL, change the news_id param to an ID that does not exist
9) Confirm that an appropriate error message shows

Sponsored-by: Catalyst IT

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 77565 [details] [review]
Bug 14272: (follow-up) only display news available for a particular opac lang [alternative patch]

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 77566 [details] [review]
Bug 14272: (follow-up) Fixing template error and layout

To test, confirm that OpacMainUserBlock and Quote of the Day do not show
if viewing a single news item, but do show otherwise. Confirm user
dashboard shows where expected

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

I notice that an expired news entry is still visible using the direct link to it, is this intended? 

It could be seen as a change of behaviour as right now there is no way to access an expired entry. Owen and others, what do you think?

(In reply to Katrin Fischer from comment #60)
> I notice that an expired news entry is still visible using the direct link
> to it, is this intended? 

A blog or a news site would keep articles around even after they're gone from their front page, so I think it's okay. Libraries have the ability to delete entries which they don't want anyone to see again.

It is a change of behavior, but my vote is to consider it a feature not a bug.

Created attachment 77567 [details] [review]
Bug 14272: Show single news item [alternative patch]

This is a new patch for this bug that shows a single news item on the
OPAC using Koha Objects (Koha::News).

To test:
1) Go to Tools -> News on staff interface, make a news item to show on
the OPAC
2) Go to OPAC
3) Notice news item shows and title cannot be clicked
4) Apply patch, refresh page
5) Notice title is now a link. Click title
6) Confirm breadcrumbs shows title of news item
7) Confirm RSS feed and OpacMainUserBlock are not visible
8) In the URL, change the news_id param to an ID that does not exist
9) Confirm that an appropriate error message shows

Sponsored-by: Catalyst IT

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 77568 [details] [review]
Bug 14272: (follow-up) only display news available for a particular opac lang [alternative patch]

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 77569 [details] [review]
Bug 14272: (follow-up) Fixing template error and layout

To test, confirm that OpacMainUserBlock and Quote of the Day do not show
if viewing a single news item, but do show otherwise. Confirm user
dashboard shows where expected

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

(In reply to Owen Leonard from comment #61)
> It is a change of behavior, but my vote is to consider it a feature not a
> bug.

Usually it means it would be considered as a bug for someone else :)

I would add a hint close to "Expiration date:" and highlight in the release notes.

(In reply to Jonathan Druart from comment #65)
> (In reply to Owen Leonard from comment #61)
> > It is a change of behavior, but my vote is to consider it a feature not a
> > bug.
> 
> Usually it means it would be considered as a bug for someone else :)
> 
> I would add a hint close to "Expiration date:" and highlight in the release
> notes.

Katrin updated the release notes on QAing!

Owen, can you rebase on 13618 and add the hint to expiration date as Jonathan suggested?

Created attachment 78104 [details] [review]
Bug 14272: (follow-up) Fixing template error and layout

To test, confirm that OpacMainUserBlock and Quote of the Day do not show
if viewing a single news item, but do show otherwise. Confirm user
dashboard shows where expected

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

UPDATE 24/Aug: Add expiration date text

Created attachment 78206 [details] [review]
Bug 14272: Show single news item [alternative patch]

This is a new patch for this bug that shows a single news item on the
OPAC using Koha Objects (Koha::News).

To test:
1) Go to Tools -> News on staff interface, make a news item to show on
the OPAC
2) Go to OPAC
3) Notice news item shows and title cannot be clicked
4) Apply patch, refresh page
5) Notice title is now a link. Click title
6) Confirm breadcrumbs shows title of news item
7) Confirm RSS feed and OpacMainUserBlock are not visible
8) In the URL, change the news_id param to an ID that does not exist
9) Confirm that an appropriate error message shows

Sponsored-by: Catalyst IT

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 78207 [details] [review]
Bug 14272: (follow-up) only display news available for a particular opac lang [alternative patch]

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 78208 [details] [review]
Bug 14272: (follow-up) Fixing template error and layout

To test, confirm that OpacMainUserBlock and Quote of the Day do not show
if viewing a single news item, but do show otherwise. Confirm user
dashboard shows where expected

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

UPDATE 24/Aug: Add expiration date text

Rebased, ready for testing

If it's only the hint being added, I'd be ok putting this back into the QA queue.

(In reply to Owen Leonard from comment #53)
> 
> A problem I found: If there is no news to display, OpacMainUserBlock and
> Quote of the Day don't show at all.

This problem is back.

Why do we note the expiration date of the news item? That doesn't seem relevant to me. If we must keep it the date should be formatted according to dateformat preference.

Hm, yes, I didn't sign off on that, I think? Please always do follow-ups for changes like that once a patch has reached signed-off status.

I also think this is a misunderstanding entirely. We were not asking for the expiration date to show on the news item, but for a hint on the news form in staff that the news will still be accessible by direct URL even if expired.

Created attachment 78231 [details] [review]
Bug 14272: (follow-up) Fixing layout issues and expiration date hint

Misunderstood the first time.

I've added the hint to the news form on staff side.
I've fixed the layout on OPAC.
The RSS feed shows on the main page, but is hidden if there is no news
and if viewing one news item.

Hi Aleisha, I found a little problem: When location is set to "all" (staff and opac), then the page view doesn't work. You see the news on the front page, but when you click on the headline it claims news item doesn't exist.

I think this would be nice to have, maybe it can be fixed easily?

Created attachment 81643 [details] [review]
Bug 14272: Fix missing TT filters

Created attachment 82713 [details] [review]
Bug 14272: (follow-up) Show single display of 'all' news

This patch ensures that news items with location set to 'all' can be
clicked on and shown individually also.

Created attachment 83425 [details] [review]
Bug 14272: Show single news item [alternative patch]

This is a new patch for this bug that shows a single news item on the
OPAC using Koha Objects (Koha::News).

To test:
1) Go to Tools -> News on staff interface, make a news item to show on
the OPAC
2) Go to OPAC
3) Notice news item shows and title cannot be clicked
4) Apply patch, refresh page
5) Notice title is now a link. Click title
6) Confirm breadcrumbs shows title of news item
7) Confirm RSS feed and OpacMainUserBlock are not visible
8) In the URL, change the news_id param to an ID that does not exist
9) Confirm that an appropriate error message shows

Sponsored-by: Catalyst IT

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83426 [details] [review]
Bug 14272: (follow-up) only display news available for a particular opac lang [alternative patch]

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83427 [details] [review]
Bug 14272: (follow-up) Fixing template error and layout

To test, confirm that OpacMainUserBlock and Quote of the Day do not show
if viewing a single news item, but do show otherwise. Confirm user
dashboard shows where expected

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

UPDATE 24/Aug: Add expiration date text

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83428 [details] [review]
Bug 14272: (follow-up) Fixing layout issues and expiration date hint

Misunderstood the first time.

I've added the hint to the news form on staff side.
I've fixed the layout on OPAC.
The RSS feed shows on the main page, but is hidden if there is no news
and if viewing one news item.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83429 [details] [review]
Bug 14272: Fix missing TT filters

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 83430 [details] [review]
Bug 14272: (follow-up) Show single display of 'all' news

This patch ensures that news items with location set to 'all' can be
clicked on and shown individually also.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 84868 [details] [review]
Bug 14272: Show single news item [alternative patch]

This is a new patch for this bug that shows a single news item on the
OPAC using Koha Objects (Koha::News).

To test:
1) Go to Tools -> News on staff interface, make a news item to show on
the OPAC
2) Go to OPAC
3) Notice news item shows and title cannot be clicked
4) Apply patch, refresh page
5) Notice title is now a link. Click title
6) Confirm breadcrumbs shows title of news item
7) Confirm RSS feed and OpacMainUserBlock are not visible
8) In the URL, change the news_id param to an ID that does not exist
9) Confirm that an appropriate error message shows

Sponsored-by: Catalyst IT
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 84869 [details] [review]
Bug 14272: (follow-up) only display news available for a particular opac lang [alternative patch]

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 84870 [details] [review]
Bug 14272: (follow-up) Fixing template error and layout

To test, confirm that OpacMainUserBlock and Quote of the Day do not show
if viewing a single news item, but do show otherwise. Confirm user
dashboard shows where expected

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
UPDATE 24/Aug: Add expiration date text
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 84871 [details] [review]
Bug 14272: (follow-up) Fixing layout issues and expiration date hint

Misunderstood the first time.

I've added the hint to the news form on staff side.
I've fixed the layout on OPAC.
The RSS feed shows on the main page, but is hidden if there is no news
and if viewing one news item.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 84872 [details] [review]
Bug 14272: Fix missing TT filters

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 84873 [details] [review]
Bug 14272: (follow-up) Show single display of 'all' news

This patch ensures that news items with location set to 'all' can be
clicked on and shown individually also.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 84874 [details] [review]
Bug 14272: (QA follow-up) Remove superflous filter

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Passing QA as the feature works as expected and I can't find any regressions at this time.

It would be nice to have some selenium integration tests for this feature to prevent future regressions, but as they're not yet widespread I can't fail on that point.

Also, can we get a relevant update patch submitted to the manual, please.

I can input ids manually and access staff news items via the opac interface. I think this should be prevented

Awesome work all!

Pushed to master for 19.05

(In reply to Nick Clemens from comment #94)
> I can input ids manually and access staff news items via the opac interface.
> I think this should be prevented

I pushed this one, filed a followup bug 22370

Enhancement, will not be backported to 18.11.x series.

*** Bug 14980 has been marked as a duplicate of this bug. ***