We only have one reference to the UnauthorizedError, which is shadows
the ApplicationPolicy::NotAuthorizedError. This commit removes the
exception.
Related to forem/forem#16985 but only barely
As I'm looking at the dashboard, there's lots of small duplication.
This refactor is a way to help me collect my thoughts regarding how to
approach the larger issue at hand.
There is no reason to create an instance variable as we don't pass this
to the view. Further, by adding this as a before_action there's a
disconnect in logic.
This commit attempts to address those issues.
What follows is a three-fold change:
1. Removing the before action (let's just call the method)
2. Reworking the method to reduce, just a bit, the method cost
3. Removing an unused instance variable
Here are the benchmarks. Note the "follows_limit" as written below is
the "Proposed" route.
```ruby
require 'benchmark'
def follows_limit_original(params:, default: 80, max: 1000)
per_page = (params[:per_page] || default).to_i
@follows_limit = [per_page, max].min
end
def follows_limit(params:, default: 80, max: 1000)
return default unless params.key?(:per_page)
per_page = params[:per_page].to_i
return max
per_page
end
def follows_limit_alt(params:, default: 80, max: 1000)
per_page = params.fetch(:per_page, default).to_i
return max if per_page > max
per_page
end
TIMES = 10_000
Benchmark.bmbm do |b|
b.report("Original no params") { 1000.times { follows_limit_original(params: {}) } }
b.report("Original less than max") { 1000.times { follows_limit_original(params: {per_page: 90 }) } }
b.report("Original greater than max") { 1000.times { follows_limit_original(params: {per_page: 9000 }) } }
b.report("Proposed no params") { 1000.times { follows_limit(params: {}) } }
b.report("Proposed less than max") { 1000.times { follows_limit(params: {per_page: 90 }) } }
b.report("Proposed greater than max") { 1000.times { follows_limit(params: {per_page: 9000 }) } }
b.report("Alt no params") { 1000.times { follows_limit_alt(params: {}) } }
b.report("Alt less than max") { 1000.times { follows_limit_alt(params: {per_page: 90 }) } }
b.report("Alt greater than max") { 1000.times { follows_limit_alt(params: {per_page: 9000 }) } }
end
```
```shell
$ ruby /Users/jfriesen/git/forem/bench.rb
Rehearsal -------------------------------------------------------------
Original no params 0.000403 0.000002 0.000405 ( 0.000405)
Original less than max 0.000109 0.000005 0.000114 ( 0.000114)
Original greater than max 0.000161 0.000006 0.000167 ( 0.000166)
Proposed no params 0.000076 0.000001 0.000077 ( 0.000076)
Proposed less than max 0.000114 0.000009 0.000123 ( 0.000126)
Proposed greater than max 0.000112 0.000011 0.000123 ( 0.000123)
Alt no params 0.000104 0.000007 0.000111 ( 0.000114)
Alt less than max 0.000113 0.000009 0.000122 ( 0.000122)
Alt greater than max 0.000111 0.000001 0.000112 ( 0.000115)
---------------------------------------------------- total: 0.001354sec
user system total real
Original no params 0.000108 0.000000 0.000108 ( 0.000110)
Original less than max 0.000109 0.000001 0.000110 ( 0.000111)
Original greater than max 0.000109 0.000000 0.000109 ( 0.000109)
Proposed no params 0.000071 0.000000 0.000071 ( 0.000071)
Proposed less than max 0.000103 0.000001 0.000104 ( 0.000103)
Proposed greater than max 0.000102 0.000000 0.000102 ( 0.000103)
Alt no params 0.000093 0.000000 0.000093 ( 0.000093)
Alt less than max 0.000103 0.000000 0.000103 ( 0.000104)
Alt greater than max 0.000102 0.000000 0.000102 ( 0.000103)
```
There is an incredible amount of conditionals in play within this
controller. I'm working to disentangle the logic so we can introduce
unified authorization policies.
The first step is removing the quasi-opaque "before_action" behavior
related to authorization. My strong preference is to make that explicit
and in doing so begin to see how to adjust the policy enforcement/creation.
This relates to forem/forem#16913
While exploring the DashboardsController, I came across method calls to
`not_found`. Idiomatically, I assumed that these methods were returning
a value. However, in looking at the code, it raises an exception.
_Note: I excpect methods that raise exceptions, especially as the only
thing they do, to end in a `!`._
By adding the documentation my "IntelliSense" provides insight into the
expected behavior of this function (e.g. "Raises an exception").
Without the documentation, I don't see any useful information.
* Penciling in a Default Spaces section
This delivers two primary things:
1. Extracting shared policy examples
2. Providing a functioning skeleton for toggling on the
"limit_post_creation_to_admins" feature.
Important is that once merged, it would be possible for this code to
"leak" out. But how that leaks out is also how you can test that the
feature works.
First, to orient, there is a constraint for a new route. That
constraint checks if the feature flag exists (e.g. has been explicitly
enabled or explicitly disabled). In other words, at this point, you
can't accidentally enable the feature via the UI. But once you have
enabled the feature, you can then toggle the feature on and off.
So, to test this in the browser:
1. Pull down this branch
2. Run `rails runner "FeatureFlag.disable(:limit_post_creation_to_admins)"`
3. Startup your the web server.
4. Login as an administrator
5. Goto /admin/content_manager/spaces
a. Bask in the glory of a plain HTML form
6. With another browser, login as another non-admin user
7. Go to /settings/extensions, you should see a section Publishing from RSS.
8. Now with the admin's session, update the form to turn on
"limit_post_creation_to_admins"
9. Back to the non-admin browser, refresh /settings/extensions, you
should no longer see the section Publishing from RSS.
Note: I have not included an admin menu item as that is related to and
dependent on some refactors I'm working on (see forem/forem#16888 and
forem/forem#16847). So a bit of "security through obsurity"
Note: In the future, once we resolveforem/forem#16490, we'll start
toggling the "Create a Post" button.
Note: I am not including Cypress tests nor request tests for this
feature because the implementation details related to the testing via
that approach are a little too volitale.
Related to forem/forem#16842
* Updating copy based on forem/forem#16893
* styles
* styles
* dark styles
Co-authored-by: Paweł Ludwiczak <ludwiczakpawel@gmail.com>
* feat: add the feature flag route and controller
* feat: add a ff page
* feat: update to a post with a success banner
* feat: test the feature
* feat: update the styling
* feat: change feature flag to extension witha few more improvements like adding a model not backed by a table
* feat: update the text
* refactor: if you enable a FF, it gets added under the hood as well
* feat: accessibility name for label
* use keyword args
* use locales
* Update app/views/admin/extensions/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/views/admin/extensions/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/views/admin/extensions/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update config/locales/controllers/admin/fr.yml
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/controllers/admin/extensions_controller.rb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* fix: rubucop
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Refactors _banish.html.erb by removing duplicate code
* Refactors _banish.html.erb to account for new and old users
* Moves conditional into Admin::UsersController#set_user_to_be_banished
* Reverts last commit, as it didnt work as expected
* Fixes last commit, which reverted to the incorrect code
* Pulls banishable user into instance var and fixes button txt
* Removes admin_member_view feature flags and refactors conditionals
* Removes unused admin/users/_email_tools partial and user_email_tools_spec
* Actually remove the admin/user_email_tools_spec this time
* Removes the edit route from admin.rb users
* Adds a DUS (and spec) to disable and remove admin_member_view flag
* Fixes admin/users_spec.rb failures by updating spec with Member Detail changes
* Fixes spec failures within admin_bans_or_warns_user_spec.rb
* Refactors Admin::UsersController#credit_params due to failing specs
* Removes last traces of edit_admin_user_path and fixes buttons, specs, etc.
```shell
❯ rg "@notifications_index"
```
The above shell command exits status code 1, which means there are no
results found.
This relates to exploratory work for forem/forem#16821
While in essence an idempotent change the purpose of this PR is to also
favor explicit method calls over callbacks. Note, we don't have an
associated model (e.g. Video) and instead rely on `:video` which Pundit
converts to VideoPolicy.
From my experience in rails it can become challenging to mentally parse
the various before/after action paired with the only and except. As
well as considering the timing of what happens when.
Further this commit helps provide a record of "work" towards an issue.
Closesforem/forem#16729
Reverses forem/forem#3806
As part of the [Authorization System Use Case
1:1](https://github.com/orgs/forem/projects/46) project, we are driving
towards the feature of: "Only admin's may post articles in this Forem."
This commit ensures that the API's "create an article" end-point
delivers on that feature.
Along the way, I've added reading notes and comments, to help us further
flex in the future (namely move all authorization checks into a policy
object).
Closesforem/forem#16488
* Trigger Build
* Remove cache header before_action and add ability to search by tag name
* Remove cache header before_action and add ability to search by tag name
* Add internal bulk tag endpoint to get tags by array of names or ids
* Restore V0 tags controller
In conversations with citizen428, this method looked to be a holdover
from a past approach. Reviewing the code, we can get the same behavior
with other methods.
Further, I added some comments for future considerations, and refactored
for more readily scannable guard clauses.
Related to forem/forem#16488, forem/forem#16681, and forem/forem#6255
As I was investigating an approach for #16488, I stumbled upon two
methods partially doing the same thing. This helps consolidate the
logic and provides some guiding documentation.
* Adjusting ArticlePolicy for admin only posting
The goal of this commit is actually two fold:
1) To add documentation regarding my current emerging understanding of
our caching implementation as it relates to our authorization and
authentication.
2) Flippiing "on" the feature's core authorization check.
Buried within this is the desired normalization of the authorization
between the `ArticlePolicy`'s `#create?`, `#preview?`, `#new?`.
My testing plan for this is to ask for SRE to spin-up a canary, then
test. What does that look like? I'm uncertain because this is nudge
closer towards our edge-caching strategy. Which makes robust testing
more difficult.
Closes forem/forem#16483
Related to #16529, #16571, #16536, #16529
Informs #16490, #16606
* Update spec/requests/editor_spec.rb
Co-authored-by: Jamie Gaskins <jgaskins@hey.com>
Co-authored-by: Jamie Gaskins <jgaskins@hey.com>
* WIP: Adds tabs to the Admin Member Detail View
* WIP: Adds tabs in the least elegant way to Admin Member Detail View
* WIP: Comments out unused code
* WIP: Initial code set up for tabs
* feat: add a tab param to the route
* feat: set the current tab and direct to the overview tab in all other cases
* chore: remove all other unused code
* Renames @tab to @current_tab and MemberDetails to UserDetails
* Removes unused route and reverts renaming of params
* Removes unused route from admin.rb
* Adds an E2E test that tests tabbing through Admin Member Detail tabs
* Updates navigateTabs E2E and _tabs.html.erb per feedback
* Updates tnavigateTabs.spec.js
* Updates navigateTabs.spec.js per Suzannes suggestion :)
* Removes useless comment from Admin::UsersController
Co-authored-by: Ridhwana <ridhwana.khan16@gmail.com>
* Initial hack
* Begin functionality implementation
* Add proper route
* Finish up
* Fix test and formatting
* Fix test vars
* Update app/views/moderations/actions_panel.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/views/moderations/actions_panel.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/views/moderations/actions_panel.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/controllers/articles_controller.rb
Co-authored-by: Michael Kohl <me@citizen428.net>
* Update file to not use keys that are not needed
* Finalize featured logic
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Co-authored-by: Michael Kohl <me@citizen428.net>
* feat: allow reply_to and email_from to be set for an SMTP config
* feat: use these SMTP values in the mailers
* spec: test the application_mailer
* fix: validate with email, and not url
* feat: mimic macs changes from https://github.com/forem/forem/pull/16216 to use in this PR
* setup packs for admin
* refactor: order the keys and add a const for the auth methods
* feat: rename the header to a more user friendlly name
* chore: move the section with Emails
* feat: add a toggle that will show and hide the SMTP form under certain conditions
* feat: add the javaScript to handle the toggles
* feat: add a better description until we convert to a dropdown
* feat: ensure that we have declared sendgrid_enabled
* chore: add anote to the config controller
* chore: remove references of the email addresses to keep brnach scoped
* feat: tweak js
* test: cypress workflow to update smtp settings
* feat : update the smtp tests
* remove comments
* update test
* chore: rename NOTE
* feat: polisha dn test ForemInstance.only_sendgrid_enabled?
* chore: remove specs
* Update app/lib/constants/settings/smtp.rb
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* Update spec/system/admin/config/admin_updates_smtp_settings_spec.rb
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* Update spec/system/admin/config/admin_updates_smtp_settings_spec.rb
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* Update spec/system/admin/config/admin_updates_smtp_settings_spec.rb
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* Update app/javascript/packs/admin/config/smtp.js
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
* refactor js as per comments
* refactor as per comments
* Update cypress/integration/seededFlows/adminFlows/config/emailServerSettingsSection.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* refactor: update the Cypress tests
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Initialize compare hex once in controller, fallback if color missing
This PR adds a fallback color choice when there are missing
bg_color_hex or text_color_hex (this is likely going to become more
common as we only show the bg_color_hex in the form).
The choices (black and white) may not be correct - this "custom, or
default" value choice should likely be made in a decorator or on the
model instead.
Addresses an error reported in
https://app.honeybadger.io/projects/66984/faults/83671065
* typo: Attribute name is text_color_hex
* feat: remove the default email and cobine the periodic digest and the contact email under the Email section
* refactor: rename the email_link to contact link and use the contact_email as a default and fallback to the ForemInstance.email
* chore: alignment
* feat: use the contact_email helper
* feat: move the contact_email to the ForemInstance model
* feat: use ForemInstance.contact_email instead of the application helper method
* removed the application Helper
* feat: set the dafault on the contact_email
* fix: cypress tests
* Update app/lib/constants/settings/general.rb
Co-authored-by: Michael Kohl <me@citizen428.net>
Co-authored-by: Michael Kohl <me@citizen428.net>
Prior to this commit, I saw the following in the Rails test log:
```
DEPRECATION WARNING: 'include Pundit' is deprecated. Please use 'include Pundit::Authorization' instead.
(called from include at ./app/controllers/application_controller.rb:12)
```
* move script to packs, strip ids
* make sure userdata is available before determining ui
* stop working around old html, fix duplicate ID errors
* skip login modal
* remove DEV hard coding
* replace system spec with cypress spec
* update base_data to include apple auth info, add to cypress tests
* add initial version of DUS to resave HTML
* remove data update script
* Removing Articles::Builder making policy decision
This change is a refactoring through triangulation. Given that
`ArticlePolicy#new?` returned true, I'm prepared to assume that calling
`authorize(Article)` in all cases is acceptable.
So to narrow the Builder making a policy decision I renamed the returned
value to reflect what it was actually doing in the logic. And in
renaming, flipped the polarity of the boolean. Why the flip? Because
`needs_authorization == !store_location`.
In consultation with Allison and Jennie, I'm proceeding with a short-cut
to get me unstuck. That unstuck is namely "I need to ensure that the
articles#new action can go through authorization."
Given that I'll be spending time in the authorization layer, I hope
these noted short-cuts and comments will be useful in future spelunking
efforts regarding authorization.
Closes#16529
* Update app/policies/article_policy.rb
Co-authored-by: Michael Kohl <me@citizen428.net>
* Disabling spec
* Update app/policies/article_policy.rb
Co-authored-by: Dwight Scott <dwight@forem.com>
* Update app/policies/article_policy.rb
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Michael Kohl <me@citizen428.net>
Co-authored-by: Dwight Scott <dwight@forem.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* Removes code behind new_admin_members feature flag
* Removes components/admin/users/tools/* and the tools components
* Removes unused /admin/users/tools/* controllers, comments, and routes
* Removes New Member View-related E2E and RSpec specs
* Remove admin_users_tools.rb frin spec/support/shared_examples/
* Removes remaining component-related specs
* Removes the view_component gem and test helper
* Resolve merge conflicts in Admin::UsersController
* Removes the view_component gem, as it is no longer used
* Removes view_component from Gemfile.lock
Prior to this commit, the admin could delete the post but were blocked
by the "delete confirm". The blocker was because we scoped the finding
of an article to the current user.
With this commit, the admin should be able to confirm the deletion of an
article.
Closes#16461
* init
* .
* partials
* few extras
* revert indicators in favor of separate PR
* stuff
* .
* stuff
* whoops
* .
* .
* .
* .
* opacity
* partial
* cosmetics
* move actions to a partial
* whoopsy
* Added an @admin alias for the frontend and removed the @admin-controllers alias.
* Added modal support for admin user actions.
* Added modal support for more parts of the user admin.
* pills
* pills
* pills
* Added optional chaining for when the overview panel isn't loaded.
* styling updates
* fix
* modals
* pass size
* tags
* .
* balance modal
* comment
* fixes
* Improved modal trigger implementation.
* Removed default value for dataset.
* Moved prevent default to after modal trigger check so forms still submit.
* Update app/views/admin/users/widgets/_credits.html.erb
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* Update app/views/admin/users/reports/_index.html.erb
* Apply suggestions from code review
* Apply suggestions from code review
* Update app/views/admin/users/actions/_export.html.erb
* Update app/views/admin/users/widgets/_tag_moderation.html.erb
* Update app/views/admin/users/actions/_merge.html.erb
* Removed <AdminUserActionsModal /> component as it wasn't necessary.
* Fixed duplicate ID errors detected by Axe.
* Redirects to the Show page rather than the Edit page
* Adjusts quotations within _social_accounts.html.erb
* Ensures that Learn More links open in new tab
* updates with latests and greatest
* .
* fixes
* wip. Fixed some issues in the credits form.
* Changed Subtract to Remove for credits dropdown.
* Cleaned up the edits form.
* Wraps functionality in feature flag and calls #set_user_details
* Wraps /admin/users/show.html.erb in feature flag
* wip: Got credit params form working.
* empty select
* responsiveness
* empty commiit
* Updates redirects in spec and controller to show rather than edit
* Updates tag moderation copy in Admin Member Detail View
* Now the note is saved when changing credits.
* Fixed label for attributes as it was affecting writing tests and a11y.
* Removes unused index.js component
* Refactored credit params.
* chore: move method under private and update comment
* verified email responsiveness
* better opacity variables
* better org logo
* indicators
* refactor the credits method
Co-authored-by: Nick Taylor <nick@dev.to>
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
Co-authored-by: Nick Taylor <nick@forem.com>
Co-authored-by: Ridhwana <ridhwana.khan16@gmail.com>
* Removes FeatureFlag.enabled?(:creator_onboarding) from codebase
* Removes FeatureFlag.enabled?(:creator_onboarding) from specs
* Further cleanup, removal, and spec fixes
* Fixes the user_request_confirmation_spec.rb
* Reverts change to confirmation email button
* Revert revert after looking at designs again :(
* Removes redundant logo_png field from config + fixes test
* Rewords an expectation in user_uses_the_editor_spec.rb
* Revert removal of logo_png from Config images
* Removes CSS class from user_uses_the_editor_spec.rb
* Removes test from user_uses_the_editor_sepc.rb
* Removes unnecessary else from _logo.html.erb
* Adds back removed system spec
* Removes SVG-related code from _logo.html.erb
* Removes AsyncInfoController#use_creator_onboarding
* Fixes spec failues due to removed code
* Removes svg-related code (that I thought I removed already :/ )
* Re-removes FeatureFlag and logo_svg from _images.html.erb
* Remove newest instances of FeatureFlag(:creator_onboarding)
* remove instances where we use the creator_onboarding field from the base_data
* fix: redirect to the correct path in the reguistrations controller based on whether the user is a creator or not
Co-authored-by: Ridhwana <ridhwana.khan16@gmail.com>
The hotness score is an "opaque to our users" score, it reflects
"activity" on the tag. However, by also showing the number of tags on
the associated view, there's a bit of a head scratcher when a "hotter"
tag with less tags is rendered ahead of a popular tag.
Closes#16321
* Added tag search to nav menu
* Added tag search
* Improved tags search results view
* Removed commented lines from the controller
* Fix specs for Search::Tag
* Prepare for tags search pagination
* Fixed Search::Tag specs
* styling
Co-authored-by: Paweł Ludwiczak <ludwiczakpawel@gmail.com>
* Convert symbol hash keys to strings when calling .perform_async
Fixes a warning from Sidekiq 6.4.0+ about perform_async arguments
which are not equal when passed to perform (`JSON.parse(JSON.dump(arg))`
should equal arg).
This is a safety measure to prevent passing objects (like classes, or
model instances) rather than their representations (like a class name,
or a model's attributes hash).
This warning will be an error in sidekiq 7
* Turn warning into an error in non-production environments
* Use string keys for reaction notification and article fetched
Missed these two on the first pass
* Update example argument hashes for #enqueues_on_correct_queue
Since this calls perform_async under the hood we need to pass json
safe hashes in the test cases as well.
https://github.com/forem/forem/blob/main/spec/workers/shared_examples/enqueues_on_correct_queue.rb
* Convert keys from FollowData#to_h to string before perform_async
I'm not sure enough where else (outside of notification) to_h is being
called, so I'm converting here when building args, rather than in
FollowData#to_h, which might be my next step.
* Let FollowData#to_h return a hash with string keys
Update spec to use string keys as well.
* Make to_h return string keys for ReactionData
Like FollowData, the #to_h method is only used to call
notifications (this is used to enqueue sidekiq jobs).
* Remove a key that was in the hash
Since reaction_data calls to_h, it gets string and not symbol,
keys. Call Hash#except with a key that was actually there.
* Refactoring cached_followed_tags
This refactor consolidates two queries and a loop into a single query.
One thing that is unclear is if we're ever really clearing the cache of
the user by id?
What follows is un-related to the refactor but appears to be related to #14937.
> First, I see we set a cache here:
>
> ebdaaaf15b/app/decorators/user_decorator.rb (L23-L34)
>
> But when I look at what busts the user cache, it doesn’t look like we’re busting it for the above cached location:
>
> ebdaaaf15b/app/services/edge_cache/bust_user.rb (L3-L22)
>
> What if anything am I missing in regards to cache busting?
* Adding some deprecated methods
* Updating documentation
* Bump for travis
* Bump for travis
* Updating comments to fix confusion
NOTE: I might be introducing more confusion, but at least
I'm addressing the prior confusion.
* Bump for travis
* Updating so we don't cache ActiveRecord objects
* Selecting only applicable attributes for caching
* Apply suggestions from code review
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Adjusting code based on feedback
* Bump for travis
* Bump for travis
* Update app/decorators/user_decorator.rb
Co-authored-by: Michael Kohl <citizen428@forem.com>
Co-authored-by: Michael Kohl <citizen428@forem.com>
