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

Add permission to bypass throttling #1938

Merged
merged 1 commit into from Feb 7, 2020

Conversation

@datitisev
Copy link
Member

datitisev commented Nov 17, 2019

Fixes #1255.
Continues #1586.

  • Permission is called floodgate.postWithoutThrottle
    • Should it go under "moderation" instead? Seems to be more of a utility for those with power than normal members...
    • Should there be a different permission for starting discussions?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run php vendor/bin/phpunit).

Required changes:

  • Related core extension PRs: flarum/english (not done yet, waiting for confirmation)
@datitisev datitisev mentioned this pull request Nov 17, 2019
2 of 3 tasks complete
@datitisev datitisev requested a review from franzliedke Nov 17, 2019
@luceos

This comment has been minimized.

Copy link
Member

luceos commented Nov 18, 2019

  • Yes under moderation
  • No not for discussion. If memory serves me right, the floodgate is only active when posting.
@datitisev

This comment has been minimized.

Copy link
Member Author

datitisev commented Nov 18, 2019

@luceos Ok, I'll move it to moderation.

If the discussion posting doesn't have a floodgate, why does the controller assert that it shouldn't throttle?

@luceos luceos added this to the 0.1.0-beta.11 milestone Nov 20, 2019
@luceos

This comment has been minimized.

Copy link
Member

luceos commented Nov 20, 2019

Feel free to merge once the pr is merged to English.

@datitisev

This comment has been minimized.

Copy link
Member Author

datitisev commented Nov 20, 2019

@luceos 👍 What should the wording be? "Bypass posting floodgate"/"Bypass post throttling"...?

@jordanjay29

This comment has been minimized.

Copy link
Member

jordanjay29 commented Nov 20, 2019

@luceos 👍 What should the wording be? "Bypass posting floodgate"/"Bypass post throttling"...?

How about "Reply multiple times without waiting" in keeping with our other Permission names?

@franzliedke franzliedke removed this from the 0.1.0-beta.11 milestone Nov 20, 2019
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Nov 20, 2019

Hmm, what about the permission name? That floodgate. prefix is new, not sure what the convention is there. But the discussion.replyWithoutApproval seems to be the closest existing thing, so maybe we should scope this one under discussion, too? 🤔

@tobyzerner If you happen to read this... can you explain the idea behind the naming scheme? Why are some of the permission names scoped, and others aren't?

@tobyzerner

This comment has been minimized.

Copy link
Contributor

tobyzerner commented Nov 20, 2019

They are scoped so that specific policies can check them. eg. DiscussionPolicy's can method.

@luceos

This comment has been minimized.

Copy link
Member

luceos commented Nov 21, 2019

Also only those permissions prefixed with discussion can be configured per tag.

@Ralkage

This comment has been minimized.

Copy link
Member

Ralkage commented Dec 13, 2019

I actually pondered on the idea of changing the default hard-coded time limit for the floodgate to something configurable as a group permission to either set it to 0 (bypass floodgate) or a set value (i.e. 30 seconds, 2 minutes, etc) but that may be more work to do I assume.

I'm referencing

public function isFlooding(User $actor): bool
{
$isFlooding = $this->events->until(
new CheckingForFlooding($actor)
);
return $isFlooding ?? Post::where('user_id', $actor->id)->where('created_at', '>=', new DateTime('-10 seconds'))->exists();
}
this method of course.

I like the approach to provide as much flexibility with permissions as possible as it can also be used as an award or restricted group permission respectively.

@datitisev

This comment has been minimized.

Copy link
Member Author

datitisev commented Dec 13, 2019

@Ralkage There's probably an issue for that already, and it's outside the scope of this PR.

@franzliedke franzliedke force-pushed the ds/1255-throttling-bypass-permission branch 2 times, most recently from af8775d to b5c1e78 Feb 7, 2020
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Feb 7, 2020

I removed the prefix in the permission name, and encapsulated the permission check in the Floodgate class itself.

Copy link
Member

franzliedke left a comment

Happy now! 😀

@franzliedke franzliedke force-pushed the ds/1255-throttling-bypass-permission branch from b5c1e78 to 711e775 Feb 7, 2020
@franzliedke franzliedke merged commit b91e903 into master Feb 7, 2020
19 checks passed
19 checks passed
PHP 7.1 / MySQL
Details
PHP 7.1 / MySQL
Details
PHP 7.1 / MariaDB
Details
PHP 7.1 / MariaDB
Details
PHP 7.2 / MySQL
Details
PHP 7.2 / MySQL
Details
PHP 7.2 / MariaDB
Details
PHP 7.2 / MariaDB
Details
PHP 7.3 / MySQL
Details
PHP 7.3 / MySQL
Details
PHP 7.3 / MySQL (prefix)
Details
PHP 7.3 / MySQL (prefix)
Details
PHP 7.3 / MariaDB
Details
PHP 7.3 / MariaDB
Details
PHP 7.3 / MariaDB (prefix)
Details
PHP 7.3 / MariaDB (prefix)
Details
WIP Ready for review
Details
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/styleci/push The analysis has passed
Details
@franzliedke franzliedke deleted the ds/1255-throttling-bypass-permission branch Feb 7, 2020
franzliedke added a commit to flarum/lang-english that referenced this pull request Feb 7, 2020
@timozander timozander mentioned this pull request Mar 20, 2020
2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.