Replace `require_login` controller filters with Pundit policies #10083

dmarcoux · 2020-08-27T12:27:11Z

Some controllers rely solely on Pundit and others on custom legacy code like require_login to authorize various actions throughout the application.

We want to rely completely on Pundit to use a widely adopted authorization system instead of our own custom solution. Replacing require_login controller filters with Pundit policies brings us a step closer to this goal.

Policies inheriting from the ApplicationPolicy can enforce that users are logged in by passing the option :ensure_logged_in. They can also allow anonymous users by instead passing the option :user_optional. Those options are passed in the initialize method of those policies.

To make this more manageable, this issue should be tackled controller by controller. See the list below.

Controllers (under src/api/app/controllers/):

The following controllers have to be tackled at the end since they are the root controllers for the web UI and API:

webui/webui_controller.rb
application_controller.rb

The list was generated at the root of the git repository with the command:

rg --files-with-matches require_login src/api/app/controllers/ | sort --reverse | cut --delimiter='/' --fields=5- | sed 's/^/- [ ] /'

The text was updated successfully, but these errors were encountered:

dmarcoux · 2020-09-16T13:59:50Z

I suggest relying on the verify_authorized method provided by Pundit, using it as an after_action controller filter. In the Pundit policies, we will check if anonymous users are authorized or not to create/update/etc...

intrip · 2020-09-21T10:58:47Z

@dmarcoux I'd like to tackle this (if possible).
I really like the idea of using the after_action :verify_authorized and extending the Policies to deal with anonymous user but there's some important details which I'd like to discuss before moving forward.

Exception class
Currently we raise an AnonymousUser error when we require_user, this exception is used in ApplicationController in order to prompt for HTTP basic auth (and in some other places too).
Since this is a refactoring I suppose we need to keep the backward compatibility and therefore a simple way to achieve this (at least initially) could be to raise Authenticator::AnonymousUser from the Policies too.
Please note that keeping or not backward compatibility is an important decision point and affect other parts of the implementation.

Currently In ApplicationPolicy we have already something in place for Anonymous users which I think it's used as secondary defence; what we'll do with that I think it's related to the above decision.

My proposal
As outlined in the issue I'd make a PR for each controller whilst keeping the require_login in the other parts of the app, eventually we'll get rid of the before_action :require_login and skip_before_action :require_login left with the last PR.
For each controller/policy I'd proceed in this way:

Add

skip_before_action :require_login
after_action :verify_authorized

Extend the Policy(details TBD) in order to deal with anonymous user
Use the Policy in every controller action
Add anonymous user tests in case they were missing for the given controller/action

wdyt? Am I overlooking at something?

dmarcoux · 2020-09-21T17:38:16Z

Hey @intrip,

After reading your comment, I've realized that I didn't mention that we have two require_login methods (in Authenticator and in Webui::WebuiController). The goal is to get rid of both. I would suggest dealing first with the method from Webui::WebuiController since this is where it will bring the most value as we work more on the web UI.

Now, why are there two require_login methods? The API is built on the ApplicationController, while the web UI is built on the Webui::WebuiController. They are both completely separated from each other. Both interfaces have their quirkiness in how they handle authorization.

From now on in this comment, I will only write about the web UI since this is what we should tackle first. We already handle the case when users are not logged-in in a concern which is included in the Webui::WebuiController. To migrate a web UI controller, so any controller inheriting from Webui::WebuiController, we have to remove require_login, then rely on verify_authorized as explained in my comment above. We should then raise Pundit::NotAuthorizedError to prevent anonymous users from doing whatever we want to block for them. This will be caught by the aforementioned concern. The ApplicationPolicy has a check for anonymous users, as you pointed out. It's pretty much what we should do in our various policies when dealing with anonymous users.

Once all controllers are migrated, we could get rid of the require_login in the Webui::WebuiController. We'll have to handle the Kerberos authentication though before removing require_login. I'm still not sure about how we'll do this right now.

I believe that I've covered everything for the web UI. Let me know if something isn't clear.

intrip · 2020-09-22T07:28:08Z

@dmarcoux thanks for for the detailed explanation and apologies for my poor knowledge of the app 😞.
It's all clear now 😄

dmarcoux · 2020-09-22T07:40:21Z

@intrip Don't excuse yourself for this, I was also in the same position when I started contributing to the project, just like anybody else. I'm glad to read that it's clearer now.

In order to increase consistency across the codebase replace `require_login` with Poundit. This commit tackle `Cloud::Azure::ConfigurationController`. Fixes openSUSE#10083

This is a PR of a series which replaces `require_login` with `Poundit`. You can find further relevant info in openSUSE#10083. Tackles [Cloud::Azure::ConfigurationController](https://github.com/openSUSE/open-build-service/blob/master/src/api/app/controllers/webui/cloud/azure/configurations_controller.rb). Fixes openSUSE#10083

This is a PR of a series which replaces `require_login` with `Poundit`. You can find further relevant info in openSUSE#10083. Tackles Cloud::Azure::ConfigurationController Fixes openSUSE#10083

This is a PR of a series which replaces `require_login` with `Poundit`. You can find further relevant info in openSUSE#10083. Tackles Cloud::Azure::ConfigurationController Ref openSUSE#10083

intrip · 2020-09-24T10:09:17Z

@dmarcoux Since we'll need to create a bunch of PRs in order to deal with this issue, WDYT of listing in the issue description all the controllers that we need to check? We could then ✅ them one by one as we go trough them.

I'm thinking on something like this (sorted alphabetically):

webui/cloud/azure/configurations_controller.rb
webui/cloud/ec2/configurations_controller.rb
...

Also we could add a checkbox related to the removal of require_login which we'll eventually do.

This is a PR of a series which replaces `require_login` with `Poundit`. You can find further relevant info in openSUSE#10083. Tackles Cloud::Azure::ConfigurationController Ref openSUSE#10083

This is a PR of a series which replaces `require_login` with `Pundit`. You can find further relevant info in openSUSE#10083. Tackles Cloud::Azure::ConfigurationController Ref openSUSE#10083

This is a PR of a series which replaces `require_login` with `Pundit`. You can find further relevant info in openSUSE#10083. Tackles Cloud::Azure::ConfigurationController. Ref openSUSE#10083

This is a PR of a series which replaces `require_login` with `Pundit`. You can find further relevant info in openSUSE#10083. Tackles `Webui::UsersController` Ref openSUSE#10083

This is a PR of a series which replaces `require_login` with `Pundit`. You can find further relevant info in openSUSE#10083. Tackles `Webui::StatusMessagesController` Ref openSUSE#10083

You can find further relevant info in openSUSE#10083. Tackles `Webui::Staging::WorkflowsController` Ref openSUSE#10083

…oller This is a PR of a series which replaces require_login with Pundit. You can find further relevant info in openSUSE#10083. Tackles `Webui::Staging::WorkflowsController` Ref openSUSE#10083

You can find further relevant info in openSUSE#10083. Tackles Webui::Staging::ProjectsController Ref openSUSE#10083

…ller This is a PR of a series which replaces require_login with Pundit. You can find further relevant info in openSUSE#10083. Tackles Webui::Staging::ProjectsController Ref openSUSE#10083

…ller This is a PR of a series which replaces require_login with Pundit. You can find further relevant info in openSUSE#10083. Tackles `Webui::Staging::ProjectsController` Ref openSUSE#10083

hennevogel · 2024-05-14T13:45:57Z

I'm going to roll this back. Pundit is for authorization a.k.a. who can do what. require_login is for ensuring someone is authenticated a.k.a User.session is set.

It should have been clear that this isn't the best idea by the amount of custom code we had to add to pundit policies and controllers

dmarcoux added Frontend Things related to the OBS RoR app Refactoring 🏭 good first issue Easy task, perfect for a first contribution labels Aug 27, 2020

intrip mentioned this issue Sep 23, 2020

Replace require_login with Pundit in Cloud::Azure::ConfigurationController #10200

Merged

intrip mentioned this issue Dec 15, 2020

Replace require_login with Pundit in Webui::StatusMessagesController #10557

Merged

5 tasks

intrip added a commit to intrip/open-build-service that referenced this issue Mar 5, 2021

This is a PR of a series which replaces require_login with Pundit.

fc1cfab

You can find further relevant info in openSUSE#10083. Tackles `Webui::Staging::WorkflowsController` Ref openSUSE#10083

intrip mentioned this issue Mar 5, 2021

Replace require_login with Pundit in Webui::Staging::WorkflowsController #10835

Merged

5 tasks

intrip added a commit to intrip/open-build-service that referenced this issue May 20, 2021

This is a PR of a series which replaces require_login with Pundit.

21bf125

You can find further relevant info in openSUSE#10083. Tackles Webui::Staging::ProjectsController Ref openSUSE#10083

intrip mentioned this issue May 20, 2021

Replace require_login with Pundit in Webui::Staging::ProjectsController #11164

Merged

5 tasks

dmarcoux mentioned this issue Feb 21, 2022

Implement the new watchlist feature #12185

Merged

hennevogel mentioned this issue May 14, 2024

Refactoring pundit for authentication #16133

Merged

hennevogel closed this as completed in #16133 May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `require_login` controller filters with Pundit policies #10083

Replace `require_login` controller filters with Pundit policies #10083

dmarcoux commented Aug 27, 2020 •

edited by saraycp

dmarcoux commented Sep 16, 2020 •

edited

intrip commented Sep 21, 2020 •

edited

dmarcoux commented Sep 21, 2020 •

edited

intrip commented Sep 22, 2020

dmarcoux commented Sep 22, 2020

intrip commented Sep 24, 2020 •

edited

hennevogel commented May 14, 2024 •

edited

Replace require_login controller filters with Pundit policies #10083

Replace require_login controller filters with Pundit policies #10083

Comments

dmarcoux commented Aug 27, 2020 • edited by saraycp

dmarcoux commented Sep 16, 2020 • edited

intrip commented Sep 21, 2020 • edited

dmarcoux commented Sep 21, 2020 • edited

intrip commented Sep 22, 2020

dmarcoux commented Sep 22, 2020

intrip commented Sep 24, 2020 • edited

hennevogel commented May 14, 2024 • edited

Replace `require_login` controller filters with Pundit policies #10083

Replace `require_login` controller filters with Pundit policies #10083

dmarcoux commented Aug 27, 2020 •

edited by saraycp

dmarcoux commented Sep 16, 2020 •

edited

intrip commented Sep 21, 2020 •

edited

dmarcoux commented Sep 21, 2020 •

edited

intrip commented Sep 24, 2020 •

edited

hennevogel commented May 14, 2024 •

edited