Errors when clicking on source

still getting this error - anybody else having this problem or know what to do?

thanks,

tobias

I am getting:

* warning: Missing argument 2 for theme_feed_icon() in /srv/www/testsystem/includes/theme.inc on line 1537.
* warning: Invalid argument supplied for foreach() in /srv/www/testsystem/modules/aggregator/aggregator.pages.inc on line 122.

when clicking on the source in node/x/aggregator. So confirming there are problems with the source-view.

P.S.: At least with these errors the sources page does not check for correct permissions (i.e. whether the user is member of the groupe where the source was defined).

Please check this is working in the latest release, all green from my end.

Firstly: Thank you for your fast response and action Taz. But then ...
I installed 6.x-1.3 and ran some tests. Problem persists. I tested with admin and an authenticated user, both the same:

I then tried switching from my custom theme to Garland with the admin. This changed above into:
Fatal error: Call to undefined function _aggregator_page_list() in /srv/www/testsystem/sites/all/modules/og_aggregator/og_aggregator.module on line 796
on a empty page.
In this setting neither the feed-page, nor the sources-page do a proper check for group-permissions (i.e. if the user is member of the group where the source has been added). This means by knowing the nid of a group and manually constructing the according URL for a feed let's you access any group-feed or source-page. I changed this to critical - this way the module doesn't respect og-permissions.
Sidenote: The "From Group: " on the source-view has nothing behind it, maybe that's an indicator what's problem?
If you need to know any more about the configuration, just let me know.

First part, looks like the commit only updated the install file and not the module itself. A shame, it is actually fixed on my dev site.

Your point about the security is true to a certain extent. No you can't just pass the nid for the group, that function access the unique feed id. Each OG RSS feed you add gets its own ID, regardless of the group. The only security flaw is that it will display which group this feed is part of.

One can argue that there doesn't need to be security around it because RSS fees are typically publically available.

It had the 'access og_aggregator' permission on it before. I will modify it to check users have node access 'view' permission for the group that feed is part of. Will post back when i'm done.

Latest module is attached, otherwise grab it from the DEV download. Whichever is easiest.
Tim, when you confirm it's working i'll make a rerelease.

Attachment	Size
og_aggregator.txt	34.08 KB

First of all: Thank you for the fast and good work.

A short explanation why this is important concerning security ... I don't like facebook, but some friends write there - and that is available as a feed. This feed is highly private in its content but only protected by a key in the URL, I even hate that it's transmitted unencrypted, but at least I want to guarantee the no one can read all the others peoples facebook stuff on my site, right? Further the key contained in the URL might even give access to other functions - I don't know as I don't want to deal with facebook ;) It might even make one suable if you make others private postings available publicly?

Then a question on the priority. Was it wrong to set this to critical? You changed it back. I thought as an OG-module a main premise would be to honor the groups set up - and of course their access-rights? Just trying to learn how to contribute in a proper manner, so please give me some advise on this. I am still unsure on the handling of this ...

Then a question about your changes. Why not a patch-file? I wanted to have a look how you changed this - mainly to learn something. You submitted a .txt file now containing the module. So I tried to make a diff myself. I noticed I had to use the additional option "--strip-trailing-cr" for diff, for it to work (otherwise all lines are treated as changed). You will find the patch-file against 6.x-1.3 enclosed.

From this I see these changes:
1) "acess og_aggregator" instead of "access content" for view sources
2) new perm "view all og_aggregator sources"
3) for a specific source: loading feed-node, checking perm "('view all og_aggregator sources') || node_access('view', $node))" on it in function og_aggregator_page_sources($fid=NULL) and according handling
4) for all sources: checking perm 'view all og_aggregator sources' in function og_aggregator_page_sources($fid=NULL) and according handling
5) checking for if($feed->nid) in function theme_og_aggregator_feed($feed)

A remark: I don't see why 2) (and the follow-ups to it) are needed. You still have the admin, and you have the 'administer og_aggregator' permission. But I assume you have a scenario in mind where you want certain user-roles to be not administering all group-feeds but still being able to access all groups sources? Otherwise you could drop that new permission and instead check for 'administer og_aggregator' in 3/4?

I assume 5) is to solve the errors/warning originally reported here.

So I tested that patch by applying my patch-file then. Clicking on Sources now doesn't yield any errors/warnings anymore, neither for admin, nor for authenticated users. This works with Garland as well as with my customized theme. The sources page now does respect og-access-permissions and shows a "Access denied" correctly. Good, so far ... But ... the feed-page itself doesn't. An authenticated user can still access the feed-page at node/x/aggregator from any group if he only knows the nid of the group. Now this is beyond the scope of the originally reported error, isn't it? Should we make a new issue out of this or what? I guess I should report this as an security issue, but now that it is mentioned here anyways ... Any advise on further steps Taz?

Anyways, I tried looking further into the code. My first analysis:
* $items['node/%node/aggregator'] = array(
* $items['node/%node/aggregator/feed'] = array(
don't honor permissions correctly. These call "function og_aggregator_page($og = NULL, $option = 'newsstream') {", in which "_og_aggregator_newsstream($og ? $og : NULL);" gets called. Could this be solved by applying a logic like in 3) above within _og_aggregator_newsstream?

And the all over conclusion from my side: While the reported problem is solved there still is a (important!) problem. Either we fix it here as we are on the go (bad practise I assume as this is going under the wrong title then) or you finish this up and we make a new (security-?) issue out of the rest. Let me know how I can help in any further steps ...

Attachment	Size
og_aggregator.module-patched-by-taz.patch	6.61 KB

Tim,

Patching was more effort at the time than just uploading the file I was working on. Also, CVS provides a patching system anyway through the web interface

You can track CVS messages for Drupal projects on the project homepage.

The particular line is:
http://drupal.org/project/cvs/100574

With the actual commit being here:
http://cvs.drupal.org/viewvc/drupal/contributions/modules/og_aggregator/...

Responding to the rest...

Added 'view all og_aggregator sources' as a flexibility, you are correct so someone doesn't have to be an admin.
Your original post made it seem as if og group members needed to see the feed, hence all the extra checking i've put in not just check for 'administer og_aggregator'. How it's done now is the most abiding to OG permissions - as it is an OG module.

I will open another issue for the other node/access bug. Good practice for larger modules that have more bugs coming in. Easier to track.

Good to see you're keen to help with the Drupal community.

Firstly, on the issue itself: Far as I can tell this issue here is solved then, at least I don't get any errors on clicking on sources anymore. I set this back to needs (more) review for now. The access problems have their own issue now: #628168: Access .

Then, on the other stuff. Oh, ok! :) These are cool links/sites, I didn't know about them at all. I searched for something like this in the blocks, but didn't pay enough attention to the links at the bottom of the projects home. Thanx-a-lot for pointing this out to me! Will use this in the future :)

This works for me on dev now. Many thanks! Your work on this module is much appreciated.

-T

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