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.

Sign up for GitHub

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

Jump to bottom

Make sure routes/actions are locked down correctly #166

Closed
3 tasks done
reefdog opened this issue Mar 31, 2022 · 1 comment · Fixed by #359
Closed
3 tasks done

Make sure routes/actions are locked down correctly #166

reefdog opened this issue Mar 31, 2022 · 1 comment · Fixed by #359
Assignees
@reefdog
Labels
security
Milestone
Sprint 4

Comments

@reefdog
Copy link
Contributor

reefdog commented Mar 31, 2022 •
edited
Loading

I noticed some funny behavior when clicking around the site while logged in and not logged in:

While logged in:

  • Everything seems to work fine, except the "All Collected Media" placeholder (which goes for everyone of course).

While not logged in:

  • Clicking "Zenodotus" (root) spawns the login flash, which is a little weird.
  • "Text Search" is visible, but clicking it spawns the login flash. (It should just be hidden, right?)
  • 🚨 The following pages don't appear on the navbar, but their pages are accessible and usable if you visit them manually:
    • /image_search
    • /twitter_users/*

So I think the things we need to do:

  • Hide Text Search (and maybe All Collected Media?) from unauthenticated users
  • Make sure the image_search and twitter_users routes require authentication
  • 🤔 about whether we have a fundamental security model problem if we missed those (should our route authentication be opt-out instead of opt-in?)
@reefdog reefdog mentioned this issue Apr 28, 2022
Closed
3 tasks
reefdog added a commit that referenced this issue Apr 29, 2022
@reefdog
Redesign primary navbar
b1a847c 
This commit redesigns the primary navbar and fixes some broken links.

- Implement a cleaner look
- Nest user/org information in a user menu
- Nest regular links in a mobile menu at small breakpoints
- Remove placeholder links (related to #166)

#198
@reefdog reefdog changed the title Make sure routes are locked down correctly Make sure routes/actions are locked down correctly Aug 24, 2022
@reefdog
Copy link
Contributor Author

reefdog commented Aug 24, 2022

Also: the Jobs Status page (/jobs) isn't locked down at all, and is accessible even while logged-out.

@reefdog reefdog added this to the Sprint 6 / Soft Launch milestone Aug 24, 2022
@reefdog reefdog self-assigned this Sep 20, 2022
@reefdog reefdog mentioned this issue Sep 20, 2022
Closed
reefdog added a commit that referenced this issue Sep 21, 2022
@reefdog
Remove unnecessary duckpunching of sessions
fde7136 
Two separate problems going on here:

1. We were recreating `SessionsController#create` unnecessarily, as the
   code in our method was exactly the same as the normal Devise method.
   Removed that.
2. `SessionsController#new` incorrectly set the @title_tag right in the
   controller, when we can just add it in the template itself.

This commit fixes both of those, and also restores the commented-out
`destroy` method for future reference.

Issue #166
reefdog added a commit that referenced this issue Sep 21, 2022
@reefdog
Require being logged-out to access login page
73f301a 
Users don’t need to get to the login page or submit the login form while
already logged in.

Issue #166
reefdog added a commit that referenced this issue Sep 21, 2022
@reefdog
Remove unused Devise registration routes
445f4c6 
Finally! This commit stops letting Devise generate the registration-
related routes that we weren’t using anyway. They were around because of
some testing requirements that I couldn’t pin down, but are now no more.

Issue #166
reefdog added a commit that referenced this issue Sep 21, 2022
@reefdog
Fix routes for applicants and add tests
b33f15e 
- Use string, not symbol, for naming `new_applicant` path for
  consistency with the other routes
- Replace the unnecessary `resources :applicants` with the more explicit
  `post “/“`, since we’re only using a single resource route
- Add tests for the applicant routes to make sure all of the above
  didn’t break; this should’ve been in place already but oops

Issue #166
reefdog added a commit that referenced this issue Sep 21, 2022
@reefdog
Require user to be logged in for image-searching
2a8995f 
Besides adding the `before_action`, this commit adds some controller
tests and stubs out further ones that we should fill in.

Issue #166
reefdog added a commit that referenced this issue Sep 21, 2022
@reefdog
Add authentication tests for text search
6d56a55 
Issue #166
reefdog added a commit that referenced this issue Sep 21, 2022
@reefdog
Tweak language for authenticate_super_user error
537a6b3 
Issue #166
reefdog added a commit that referenced this issue Sep 21, 2022
@reefdog
Restrict jobs status pages to admins
6084e42 
Prior to this commit, the Jobs page was hidden in the nav from non-admin
users, but if they knew the URL they could still get to it.

This commit locks that down and adds tests to verify. It also renames
the relevant test file to bring it in alignment with the actual
controller being tested, and cleans up one of the tests a bit.

Issue #166
reefdog added a commit that referenced this issue Sep 21, 2022
@reefdog
Authenticate media source controllers
5f09e24 
Previously, the controllers for our various media sources (Twitter,
YouTube, etc.) weren’t authenticated, and thus could be accessed even
while logged out.

This commit:

- Adds authentication to all of them
- Fixes an inconsistency in the strict-typing declaration between them
- Adds fixtures and tests for all sources

Issue #166
reefdog added a commit that referenced this issue Sep 21, 2022
@reefdog
Move comment to correct place in ArchiveController
13a450e 
Issue #166
reefdog added a commit that referenced this issue Sep 21, 2022
@reefdog
Remove unused login and settings controllers
eee7ca1 
AFAIK these are unused. Login is handled in
`users/sessions_controller.rb` and settings in `accounts_controller.rb`.

Issue #166
@reefdog reefdog mentioned this issue Sep 21, 2022
Merged
reefdog added a commit that referenced this issue Sep 22, 2022
@reefdog
Require being logged-out to access login page
a842d45 
Users don’t need to get to the login page while already logged in.

This explicitly *doesn’t* block the other actions. `create` because
blocking it causes some weird behavior (and who cares if people want to
force a new authentication, really; we just want to stop them from
accidentally stumbling in), and `destroy` because of course.

Issue #166
reefdog added a commit that referenced this issue Sep 22, 2022
@reefdog
Remove unused Devise registration routes
3012dd8 
Finally! This commit stops letting Devise generate the registration-
related routes that we weren’t using anyway. They were around because of
some testing requirements that I couldn’t pin down, but are now no more.

Issue #166
reefdog added a commit that referenced this issue Sep 22, 2022
@reefdog
Fix routes for applicants and add tests
ac87141 
- Use string, not symbol, for naming `new_applicant` path for
  consistency with the other routes
- Replace the unnecessary `resources :applicants` with the more explicit
  `post “/“`, since we’re only using a single resource route
- Add tests for the applicant routes to make sure all of the above
  didn’t break; this should’ve been in place already but oops

Issue #166
reefdog added a commit that referenced this issue Sep 22, 2022
@reefdog
Require user to be logged in for image-searching
fb686d5 
Besides adding the `before_action`, this commit adds some controller
tests and stubs out further ones that we should fill in.

Issue #166
reefdog added a commit that referenced this issue Sep 22, 2022
@reefdog
Add authentication tests for text search
c92c7c1 
Issue #166
reefdog added a commit that referenced this issue Sep 22, 2022
@reefdog
Tweak language for authenticate_super_user error
ba07136 
Issue #166
reefdog added a commit that referenced this issue Sep 22, 2022
@reefdog
Restrict jobs status pages to admins
cfc6121 
Prior to this commit, the Jobs page was hidden in the nav from non-admin
users, but if they knew the URL they could still get to it.

This commit locks that down and adds tests to verify. It also renames
the relevant test file to bring it in alignment with the actual
controller being tested, and cleans up one of the tests a bit.

Issue #166
reefdog added a commit that referenced this issue Sep 22, 2022
@reefdog
Authenticate media source controllers
f228cfb 
Previously, the controllers for our various media sources (Twitter,
YouTube, etc.) weren’t authenticated, and thus could be accessed even
while logged out.

This commit:

- Adds authentication to all of them
- Fixes an inconsistency in the strict-typing declaration between them
- Adds fixtures and tests for all sources

Issue #166
reefdog added a commit that referenced this issue Sep 22, 2022
@reefdog
Move comment to correct place in ArchiveController
bcd3d18 
Issue #166
reefdog added a commit that referenced this issue Sep 22, 2022
@reefdog
Remove unused login and settings controllers
590e2aa 
AFAIK these are unused. Login is handled in
`users/sessions_controller.rb` and settings in `accounts_controller.rb`.

Issue #166
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@reefdog reefdog

Labels
security
Projects
None yet
Milestone
Sprint 4
Development

Successfully merging a pull request may close this issue.

Fix authentication on all controller methods TechAndCheck/zenodotus
1 participant
@reefdog