Refactored Channel classes' authorization

[mrafie1]

Proposed Changes

(Describe your changes here. Also describe the motivation for your changes: what problem do they solve, or how do they improve the application or codebase? If this pull request fixes an open issue, use a keyword to link this pull request to the issue.)

• Updated policies to include the record and context validation checks that were previously handled within subscribed methods in channel child classes.
• Refactored the channel superclass:
  • Added automatic auth handling for children and auth state checks.
    • Runs authorization automatically with before_subscribe :authorize_channel
    • Checks if authorization occurs with before_subscribe :verify_authorized and custom verify_authorized method
  • Introduced methods for setting up context, defining target records, performing auth, and verifying auth completion.
• Refactored channel child classes:
  • Removed auth logic from subscribed methods.
  • Added dedicated methods for defining context and auth target records.

UPDATE JUL 16

• Moved Assignment method to classes that only required it
  • Additionally, moved authorize:... context to classes that only required it
• Removed record from assignment_policy.rb and replaced it with assignment that is passed in as context
• Updated all relevant policy files to accept context passed in with authorize!
  • Example line 9 of app/policies/assignment_policy.rb explicitly accepts :assignment
• Changed channel.rb's authorize_channel method to reject if current_role is nil. See response to third comment for more information as I found this change to be a little complicated
• Changed channel.rb definition of real_user to use connection.real_user instead of connection.current_user
• Changed connection.rb to fit the definitions in session_handler.rb
• Changed all channel tests to now use real_user whose values are the same as current_user
  • Reasoning: No rspec test seemed to be testing for when real_user is different than current_user (in cases of impersonation)

Screenshots of your changes (if applicable)

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)	x
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
📖 Documentation update (change that updates documentation)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • This is required for new features.
• If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

• I have updated the project Changelog (this is required for all changes).
• I have verified that the pre-commit.ci checks have passed.
• I have verified that the CI tests have passed.
• I have reviewed the test coverage changes reported by Coveralls.
• I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

About your comment regarding verify_authorized:

• The function exists in ActionPolicy::Controller link
• It is specific to Controllers only; "Controller concern..."
• ActionPolicy::Behaviour link can be used anywhere; "..Could be included anywhere to perform authorization..." however does not include verify_authorized

It would be wiser to implement our own verify_authorized method, which only checks if authorization occurs, and not perform authorization itself. (In the context of controllers, before_action { authorize! } automatically authorizes, and verify_authorized checks to see if that function is called`

Questions:

• In my channel.rb file, what are your opinions on the methods defined such as current_role, real_user, etc? I feel like there must be a better way to define variables without using many 'one-line methods'

[david-yz-liu]

Seems like you should put this after the authorize! call

[mrafie1]

From the documentation, it seems that verify_authorized only checks that authorization occurs (Link). In the context of the controllers, it seems that it is there to make sure each child controller calls before_action {authorize!}

But since we already 'automatically' call authorization from the parent, the children don't really need to do anything. Is it safe to remove verify_authorized entirely?

[david-yz-liu]

Hmm okay, you can go ahead and remove the @authorization_checked variable and verify_authorized entirely.

[david-yz-liu]

I'm a bit surprised by this check. I thought ApplicationPolicy already has a pre_check :role_exists? that will handle this?

[mrafie1]

I am deleting this from all policies. This is because it seems to be necessary to have this 'logic' inside the authorize_channel function, as some test cases in test_runs_channel_spec.rb test whether giving a course_id value of nil (which causes role to be nil) will stop the channel from being subscribed to.

This means I would have to go through all (relevant) policies to add this 'nil checking' so I ended up keeping the 'role nil checking' inside the authorize_channel function.

[mrafie1]

UPDATE: However, I think I will keep the logic to check if 'assignment' is nil in the file assignment_policy.rb.

This is because if I were to put this logic inside the parent channel.rb file, not all channel actions require the assignment to not be nil, which could cause some connections to be rejected when not intended to be rejected.

[david-yz-liu]

See my comment below about role.nil?.

For record.nil?, can you dig deeper into how recordis defined here? There's one definition insession_handler.rb` (again, for controllers); it seems like maybe we'd need to define this in the relevant channels as well?

[mrafie1]

For this, I ended up removing record and replacing it with assignment instead and updated the policy to declare that they need :assignment

My initial thought about using record was because originally, we checked if assignment is nil. Since we end up passing it into our authorize! function, record is representing assignment (link)

[mrafie1]

Hi David, took a look at some of the feedback. I wrote down some comments on some of the feedback comments. I will let you know when I update my PR with the new changes.

[mrafie1]

Hi David. I pushed all of my changes and addressed all of your comments. I tried my best to thoroughly explain all of my changes, but let me know if there's something in particular that needs to be addressed.

Note: I will change the changelog description to be more descriptive.

One question: I ended up changing the definition of real_user from connection.current_user to connection.real_user. Then I ended up changing the channel tests by adding real_user in the connection stub and setting its value to be identical to current_user

I wanted to know if this is okay. My understanding of the differentiation between current/real users is to handle impersonation logins. The channel tests don't handle this kind of case.

[david-yz-liu]

Hi @mrafie1, good progress, and I left a few more comments. The changes to the tests are fine.

[david-yz-liu]

Hmm okay, you can go ahead and remove the @authorization_checked variable and verify_authorized entirely.

[coveralls]

Coverage Report for CI Build 29784276163

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage remained the same at 90.411%

Details

• Coverage remained the same as the base build.
• Patch coverage: 2 uncovered changes across 1 file (76 of 78 lines covered, 97.44%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
app/channels/application_cable/channel.rb	23	21	91.3%
Total (10 files)	78	76	97.44%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	51281
Covered Lines:	47379
Line Coverage:	92.39%
Relevant Branches:	2470
Covered Branches:	1218
Branch Coverage:	49.31%
Branches in Coverage %:	Yes
Coverage Strength:	127.83 hits per line

---

💛 - Coveralls

[david-yz-liu]

Nice work, @mrafie1!