Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds an RSS feed via pages/all.rss #225

Closed
wants to merge 17 commits into from
Closed

Conversation

fsmanuel
Copy link
Contributor

addresses #223
not yet ready to merge. we need a way to authenticate the user (e.g. a token)

def authenticate(role = 'any')
# RSS works with a token
return true if authenticated_for_rss?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm wouldn't it be better to use skip_before_filter :authenticate, :only => :whatever and add the authenticated_for_rss filter for the rss feed? I'd find it a bit scary to allow using this token so generally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well it's not so generall. authenticated_for_rss? checks the controller and action but i see your point.

@fsmanuel
Copy link
Contributor Author

@wvengen merged your PR

@wvengen
Copy link
Member

wvengen commented Dec 30, 2013

Thanks. Little oops: when the token is wrong, the exception ActiveSupport::MessageVerifier::InvalidSignature is thrown (instead of false being returned). For the rest it seems good!

@fsmanuel
Copy link
Contributor Author

fsmanuel commented Jan 1, 2014

@wvengen updated

@wvengen
Copy link
Member

wvengen commented Jan 2, 2014

Thanks! I did some more research on the HMAC-SHA1 key concatenation. When there are many different foodcoop-prefix combinations this may not be safe. We'd either need to use HMAC for concatenation, or put this information in the message itself. Working on the latter now.

@wvengen
Copy link
Member

wvengen commented Jan 2, 2014

I'm seeing the meta tag in the dashboard too - wouldn't restricting it to the page controller be clearer?

@wvengen
Copy link
Member

wvengen commented Jan 2, 2014

regarding the key concatenation - see fsmanuel#3

@fsmanuel
Copy link
Contributor Author

fsmanuel commented Jan 2, 2014

  1. concerning the SHA1 problem. that was my feeling but wasn't sure about it. will merge your PR.
  2. meta tag. i see a yield(:head) in the header.html.haml but i'm not sure where it is used. i can not find any content_for :head so we could use it BUT content_for only works in the view context and i don't think that we should put it in all the pages views... a RSS meta tag should inform about RSS feeds of the app not of an controller, doesn't it?

fsmanuel and others added 2 commits January 2, 2014 03:56
@fsmanuel
Copy link
Contributor Author

fsmanuel commented Jan 2, 2014

@wvengen i should do a rebase before we merge

@wvengen
Copy link
Member

wvengen commented Jan 2, 2014

  1. spec pending
  2. :head it's used in plugins (e.g. the signup plugin), but it's been there for a while.
    • I'd expect the dashboard RSS to show changes related to all of foodsoft not just the pages.
    • Generating the token on every request adds another bit to the load time.
    • Don't know yet how to do this for the page controller only cleanly ... reviewing your code, I don't think rss_meta_tag would survive multiple definitions. The layout could check for it being defined in the current controller?
  3. Rebase - to get rid of the file-added-and-removed-again changes, you mean? Would be clean, if you know how to do that without a lot of manual work.

@fsmanuel
Copy link
Contributor Author

fsmanuel commented Jan 2, 2014

found a way:
https://gist.github.com/hiroshi/985457
https://github.com/clm-a/content_for_in_controllers

should we go for the gem or add it to ApplicationController?

rebase - will open a new PR when we are done

@fsmanuel
Copy link
Contributor Author

fsmanuel commented Jan 2, 2014

@wvengen with the gem it would look like that

@wvengen
Copy link
Member

wvengen commented Jan 2, 2014

Cool -> gem! Put it in wiki gemspec, perhaps?

@fsmanuel
Copy link
Contributor Author

fsmanuel commented Jan 4, 2014

@wvengen moved it. if we are done i'll open a new PR with one commit for all the changes.

@wvengen
Copy link
Member

wvengen commented Jan 4, 2014

Great!

@fsmanuel fsmanuel closed this Jan 4, 2014
@fsmanuel fsmanuel deleted the master branch January 4, 2014 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants