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

Default to authenticated required #103

Merged
merged 2 commits into from
Sep 13, 2024
Merged

Default to authenticated required #103

merged 2 commits into from
Sep 13, 2024

Conversation

JPrevost
Copy link
Member

@JPrevost JPrevost commented Sep 11, 2024

Why are these changes being introduced:

  • Defaulting to secure allows less worry that a future action will inadvertantly be left unsecured

Relevant ticket(s):

How does this address that need:

  • Adds a user check in the main application controller before_action
  • Safelists specific actions that do not require authentication
  • Moves GraphQL playground from static HTML to a rails route to allow for requiring authentication
  • Adds tests to ensure authentication is being required

Document any side effects to this change:

  • Some routes will get specific role requirements and not just authentication in the near future. This is a step towards where we are heading, not the full solution.

Summary of changes (please refer to commit messages for full details)

Developer

  • ANDI or Wave has been run in accordance to our guide and
    all issues introduced by these changes have been resolved or opened
    as new issues (link to those issues in the Pull Request details above)
  • There are no accessibility implications to this change

Documentation

  • Project documentation has been updated, and yard output previewed
  • No documentation changes are needed

ENV

  • All new ENV is documented in README.
  • All new ENV has been added to Heroku Pipeline, Staging and Prod.
  • ENV has not changed.

Stakeholders

  • Stakeholder approval has been confirmed
  • Stakeholder approval is not needed

Dependencies and migrations

NO dependencies are updated

NO migrations are included

Reviewer

Code

  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.

Documentation

  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.
  • New dependencies are appropriate or there were no changes.

Testing

  • There are appropriate tests covering any new functionality.
  • No additional test coverage is required.

@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-103 September 11, 2024 14:51 Inactive
@JPrevost JPrevost temporarily deployed to tacos-api-pipeline-pr-103 September 11, 2024 15:04 Inactive
@matt-bernhardt matt-bernhardt self-assigned this Sep 12, 2024
Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

Adopting security-by-default through the require_user method makes a lot of sense, and I see how routes like the homepage and the actual API are exempted from that.

This posture might make more sense to me when we have more parts of the application in place, because right now there are only four, I think?

  1. The public UI (right now only the homepage)
  2. The playground (I'll have to think about what sort of security I'd prefer for this, but I'm not requesting a change now)
  3. The GraphQL API
  4. The administration area

Seeing the ways that this approach plays out, my question boils down to how a requirement for basic authentication should be implemented (i.e. something that can be accessed by anyone who can log in, regardless of their authorizations).

It seems like two options are possible, as I poke on branch locally:

  1. Drop the authorize! line from the static controller (this still leaves the require_user check in place, but the check of the ability model never happens)
  2. Uncomment the can :view, :playground line from the ability model (explicitly assigns view permission to everyone, so the permission check is performed, but is always successful).

The existence of both options tells me that we're authenticating by default, but not authorizing by default? I"m not entirely sure how an authorize-by-default posture would be implemented, but I wanted to ask here in case I'm missing something.

@JPrevost
Copy link
Member Author

Authorization by default will happen as well, but I'm much less concerned with that aspect as the only people that can login are explicitly added to a specific Moira list. The ability model will just allow any authorized user to do certain things (tbd what exactly) and then start restricting other things based on roles. Does that make sense?

We'll make sure the correct authorization is in place by creating specific tests for each role with each new feature, but if we get that wrong the worst case is one of our staff can do something they weren't supposed to in a probably not awful way. If we get authorization wrong, any random person can do something... which is a much larger pool of potential bad actors.

@JPrevost
Copy link
Member Author

Addendum. Authorization will likely not happen by default at the top level application controller. It will be set on each controller. So it won't truly be "by default" in the same way authorization will work. I believe the rest of what I said holds true (i.e. authentication is way more important to us than authorization for this particular application)

Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

Yeah, that helps. I don't have any blocking concerns with this approach, and agree with the biggest focus here.

:shipit:

Why are these changes being introduced:

* Defaulting to secure allows less worry that a future action will
  inadvertantly be left unsecured

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/TCO-86

How does this address that need:

* Adds a user check in the main application controller before_action
* Safelists specific actions that do not require authentication
* Moves GraphQL playground from static HTML to a rails route to allow
  for requiring authentication
* Adds tests to ensure authentication is being required

Document any side effects to this change:

* Some routes will get specific role requirements and not just
  authentication in the near future. This is a step towards where we are
  heading, not the full solution.
@JPrevost JPrevost temporarily deployed to tacos-api-pipeline-pr-103 September 13, 2024 18:39 Inactive
@JPrevost JPrevost merged commit 8300e0e into main Sep 13, 2024
2 checks passed
@JPrevost JPrevost deleted the tco-76-permissions branch September 13, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants