Skip to content

Adds class level permission requiring authenticated user #893

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Dec 3, 2016
Merged

Adds class level permission requiring authenticated user #893

merged 8 commits into from
Dec 3, 2016

Conversation

flovilmart
Copy link
Contributor

@flovilmart flovilmart commented Mar 7, 2016 •
edited
Loading

@flovilmart flovilmart force-pushed the flovilmart.requiresAuthCLP branch from cf86774 to a82af38 Compare March 7, 2016 17:54
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@codecov-io
Copy link

Current coverage is 91.76%

Merging #893 into master will increase coverage by +0.06% as of 7f0fd88

@@            master    #893   diff @@
======================================
  Files           71      71       
  Stmts         4110    4117     +7
  Branches       849     853     +4
  Methods          0       0       
======================================
+ Hit           3769    3778     +9
  Partial         10      10       
+ Missed         331     329     -2

Review entire Coverage Diff as of 7f0fd88

Powered by Codecov. Updated on successful CI builds.

@drew-gross
Copy link
Contributor

I'm guessing the use case for this is when the app developer is issuing accounts only to trusted people? If anyone can sign up for an account, then this feature doesn't really seem to add any security.

@flovilmart
Copy link
Contributor Author

we discussed it with @gfosco this morning, and some users want to restrict globally access to the content. It really doesn't add any security, but if s/o wants to implement it, it's still faster than having so add users to a role etc...

@drew-gross
Copy link
Contributor

I'm not sure we want to add features that just create a false sense of security. I can see how it's useful in the case of e.g. internal apps though.

Either way, this will need documentation.

@nitrag
Copy link

nitrag commented Mar 14, 2016

:) I'm a fan of this.

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart
Copy link
Contributor Author

@drew-gross you're right, we need to carefully document it, as well as implement on parse-dashboard.

@gfosco what do you think? merge now, and document later in the wiki?

@drew-gross
Copy link
Contributor

The dashboard implementation can probably wait, as long as you can modify the setting without having to dig around in the source. The documentation is more important, I'd say. So far we've been adding stuff without documentation only if it's hidden behind an experimental flag but I don't want to get too much into that habit and have tons of stuff languishing in experimental for ages.

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@drew-gross
Copy link
Contributor

Awaiting docs and rebase.

@flovilmart
Copy link
Contributor Author

@drew-gross yeah... I need to take some time on that :)

@drew-gross
Copy link
Contributor

No worries. I think this is a rarely requested feature anyway.

@cherukumilli
Copy link
Contributor

@flovilmart @drew-gross
if it is not too much to ask can one of you please explain this "creates a false sense of security"?
Is there a way to bypass requiresAuthentication to get access to the content?

@flovilmart
Copy link
Contributor Author

If anyone can signup, and create an account, then this protection dont really protect anything

@flovilmart flovilmart force-pushed the flovilmart.requiresAuthCLP branch from a82af38 to b55e8e6 Compare July 19, 2016 00:32
@ghost
Copy link

ghost commented Jul 19, 2016

@flovilmart updated the pull request.

@ghost
Copy link

ghost commented Jul 19, 2016

@flovilmart updated the pull request.

2 similar comments
@ghost
Copy link

ghost commented Jul 19, 2016

@flovilmart updated the pull request.

@ghost
Copy link

ghost commented Jul 19, 2016

@flovilmart updated the pull request.

@flovilmart flovilmart added this to the 2.3.0 milestone Jul 23, 2016
@ghost
Copy link

ghost commented Aug 18, 2016

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @drew-gross as a potential reviewer. Could you take a look please or cc someone with more context?

@nitrag
Copy link

nitrag commented Aug 31, 2016

The main benefit (for me at least) is the minimizes the potential for someone coming along and scraping your data. At least they'd have to put some effort into it with this feature.

@flovilmart flovilmart mentioned this pull request Oct 14, 2016
Closed
@flovilmart flovilmart mentioned this pull request Dec 1, 2016
Closed
@flovilmart flovilmart force-pushed the flovilmart.requiresAuthCLP branch from 73a1b1e to 153655b Compare December 1, 2016 22:54
@facebook-github-bot
Copy link

@flovilmart updated the pull request - view changes

@flovilmart flovilmart closed this Dec 1, 2016
@flovilmart flovilmart reopened this Dec 1, 2016
steven-supersolid
steven-supersolid approved these changes Dec 2, 2016
View reviewed changes
Copy link
Contributor

@steven-supersolid steven-supersolid left a comment

Choose a reason for hiding this comment

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

LGTM. Agree will need solid docs though as this is a new feature

@flovilmart
Copy link
Contributor Author

See: https://github.com/ParsePlatform/parse-server/wiki/NEW:-Class-Level-Permission:-requireAuthentication

@facebook-github-bot
Copy link

@flovilmart updated the pull request - view changes

@facebook-github-bot
Copy link

@flovilmart updated the pull request - view changes

@facebook-github-bot
Copy link

@flovilmart updated the pull request - view changes

@flovilmart
Copy link
Contributor Author

@steven-supersolid merging as it's been approved, thoroughly tested. We'll add the docs before 2.3.0

@flovilmart flovilmart merged commit e0704b4 into master Dec 3, 2016
@flovilmart flovilmart deleted the flovilmart.requiresAuthCLP branch December 3, 2016 00:47
rsouzas pushed a commit to back4app/parse-server that referenced this pull request Dec 3, 2016
@flovilmart
Adds class level permission requiring authenticated user (parse-commu…
f18560c 
…nity#893)

* Adds class level permission requiring authenticated user

* Updates to latest schema permissions syntax

* fix flaky test

* Exclude PG

* Rebased and nitted

* lints
Jcarlosjunior pushed a commit to back4app/parse-server that referenced this pull request Dec 13, 2016
@flovilmart @Jcarlosjunior
Adds class level permission requiring authenticated user (parse-commu…
8ab0865 
…nity#893)

* Adds class level permission requiring authenticated user

* Updates to latest schema permissions syntax

* fix flaky test

* Exclude PG

* Rebased and nitted

* lints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steven-supersolid steven-supersolid steven-supersolid approved these changes

Assignees

@drew-gross drew-gross

Labels
None yet
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.
8 participants
@flovilmart @facebook-github-bot @codecov-io @drew-gross @nitrag @cherukumilli @steven-supersolid @gfosco