It would be most useful if you could set permissions per search page, instead of only for all search pages.

CommentFileSizeAuthor
#5 search_api_page-n1371482-5.patch2.01 KBdamienmckenna
#1 search_api_page-per_page_perms-1371482-1.patch1.98 KBrooby

Comments

rooby’s picture

rooby commented
Status: Active » Needs review
StatusFileSize
new search_api_page-per_page_perms-1371482-1.patch1.98 KB

Here is a patch.
It uses the existing permission as the access all pages permission so there is no need for an update to migrate existing sites' permissions.

A user is granted access if they have either the 'all' permission or the individual page permission for the given page.

ohthehugemanatee’s picture

ohthehugemanatee commented

Patch applied cleanly on current stable branch... and it appears to work. Thank you!

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Seems reasonable, the patch is clean. Good to go.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Needs work

Thanks a lot for suggesting this, great idea! Sorry for not seeing it earlier, and thanks for pusing it, Damien!

There are only a few smaller issues with the patch, apart from that it's good to go:

  1. +++ b/search_api_page.module
@@ -39,7 +39,8 @@ function search_api_page_menu() {
         'page callback' => 'search_api_page_view',
         'page arguments' => array((string) $page->machine_name),
-        'access arguments' => array('access search_api_page'),
+        'access callback' => 'search_api_page_access_callback',
+        'access arguments' => array($page),

    Please remove the _callback suffix from the function name, I don't think that's usually done.

    Also, like for the page arguments above, please just pass the page machine name, not the whole page object. 

  2. +++ b/search_api_page.module
@@ -50,6 +51,14 @@ function search_api_page_menu() {
+  return user_access('access search_api_page') || user_access('access ' . check_plain($page->index_id) . ' search_api_page');

@@ -80,12 +89,20 @@ function search_api_page_theme() {
-  return array(

    The check_plain() is completely unnecessary here (and in the permission definition below).

Thanks again!

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Needs review
StatusFileSize
new search_api_page-n1371482-5.patch2.01 KB

How about this then? I removed the check_plain and changed it to use the machine name for all internal strings rather than the index_id.

rooby’s picture

rooby commented

Beat me to it I was just about to post the same thing.
Thanks.

  • Commit 448019a on 7.x-1.x authored by rooby, committed by drunken monkey: 
    Issue #1371482 by rooby, DamienMcKenna: Added per-page permissions.
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Looks good, thanks. And thanks again to rooby, of course!
Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.