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

Fixes multiple assignment bug #680

Merged
merged 2 commits into from
Aug 8, 2024
Merged

Fixes multiple assignment bug #680

merged 2 commits into from
Aug 8, 2024

Conversation

atruskie
Copy link
Member

@atruskie atruskie commented Aug 8, 2024

Fixes #679

<< instantly saves the added association. We were adding the project from the route parameter onto the site object temporarily, assuming the site would not be saved.

Instead whenever the route was accessed with any project id, that project id was added to the list of available projects - creating an security bypass.

I also took this opportunity to disallow the creation of new sites that belong to multiple projects. It is a feature we plan to remove anyway.

We still have to allow updating multi-project sites - at least until we can ensure there are no examples of them in the database.

Fixes #679

`<<` instantly saves the added association. We were adding the project from the route parameter onto the site object temporarily, assuming the site would not be saved.

Instead whenever the route was accessed with any project id, that project id was added to the list of available projects - creating an security bypass.

I also took this opportunity to disallow the creation of new sites that belong to multiple projects. It is a feature we plan to remove anyway.

We still have to allow updating multi-project sites - at least until we can ensure there are no examples of them in the database.
@atruskie atruskie requested a review from hudson-newey August 8, 2024 08:37
Copy link
Member

@hudson-newey hudson-newey left a comment

Choose a reason for hiding this comment

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

Looks good

app/models/site.rb Show resolved Hide resolved
@@ -35,7 +35,8 @@
expect_error(400, 'Site testy test site () is not in any projects.')
end

example 'can create an a site that belongs to multiple projects' do
# AT 2024: soft-deprecating the many to many project-site relationship
example 'canNOT create an a site that belongs to multiple projects' do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
example 'canNOT create an a site that belongs to multiple projects' do
example 'cannot create an a site that belongs to multiple projects' do

I'm debating whether this was purposeful or a typo

I've had a look at other examples where we have used "cannot" inside tests, and this doesn't seem to be the convention

it 'cannot be created with no studies' do

it 'cannot delete account' do

it 'cannot be created when status is not new' do

Copy link
Member Author

Choose a reason for hiding this comment

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

it wa purposeful - i'll make it more grammatically correct

app/controllers/sites_controller.rb Outdated Show resolved Hide resolved
@atruskie atruskie merged commit 0617d04 into master Aug 8, 2024
1 of 3 checks passed
@atruskie atruskie deleted the accidental-assignment-bug branch August 8, 2024 23:57
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.

Incorrect association assignment for nested site routes
2 participants