Skip to content
This repository has been archived by the owner on Mar 28, 2019. It is now read-only.

Refactor registration of protected resources (fixes #348, fixes #322) #361

Merged
merged 8 commits into from Jul 9, 2015

Conversation

leplatrem
Copy link
Contributor

r? @Natim && @ametaireau

leplatrem added a commit to mozilla-services/readinglist that referenced this pull request Jul 6, 2015
This commit follows what has been done in
mozilla-services/cliquet#361
@Natim
Copy link
Contributor

Natim commented Jul 6, 2015

Looks good to me. r+

@almet
Copy link
Contributor

almet commented Jul 6, 2015

I'm unclear about what happens if we define a resource without any factory attached.

The Authorization policy seems to require a context and as such it doesn't really make sense to be able to set the factory to None.

Another approach would be to:

  • Only check the context if there a permission required;
  • Have a separate ViewSet subclass for protected resources, which sets the factory to what we're waiting for.

@leplatrem
Copy link
Contributor Author

doesn't really make sense to be able to set the factory to None.

It only makes sense if another AuthzPolicy is used indeed.

Have a separate ViewSet subclass for protected resources

This makes a lot of sense, and would like to see it as well. But it requires a lot more efforts on several parts of the code. I wonder if we should do it in the scope of this bug fix...

I'll look at your suggestion and see if we can get rid of those configuration lines to specify principals...

@leplatrem leplatrem force-pushed the 348-default-route-factory branch 2 times, most recently from c8f2350 to cd9debb Compare July 8, 2015 14:12
@almet
Copy link
Contributor

almet commented Jul 8, 2015

Great, I prefer it much more like that, thanks!

@almet
Copy link
Contributor

almet commented Jul 8, 2015

(r+)

@leplatrem leplatrem changed the title Use Cliquet factory by default in resources (fixes #348) Refactor registration of protected resources (fixes #348, fixes #322) Jul 9, 2015
@@ -252,6 +271,9 @@ def callback(context, name, ob):
class BaseResource(object):
"""Base resource class providing every endpoint."""

default_viewset = ViewSet
Copy link
Contributor

Choose a reason for hiding this comment

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

neat.

@leplatrem leplatrem merged commit a7e22b2 into master Jul 9, 2015
@leplatrem leplatrem deleted the 348-default-route-factory branch July 9, 2015 14:19
leplatrem added a commit to mozilla-services/readinglist that referenced this pull request Jul 9, 2015
This commit follows what has been done in
mozilla-services/cliquet#361
glasserc pushed a commit that referenced this pull request May 20, 2016
Default bucket as built-in plugin (fixes #277, fixes #311)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants