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

If a project is not collaborative, don't check user access #5072

Merged
merged 6 commits into from
Mar 19, 2024

Conversation

ruggi
Copy link
Contributor

@ruggi ruggi commented Mar 18, 2024

Fix #5071

Problem:

Projects that are not collaborative (or public for that matter) should not be accessible by anybody (except owners), even if those users were given access permissions while the project was collaborative.

Fix:

  • Just don't run the request access controls if the project is not collaborative, and just return a 404

On the side…

  • Add full testing for the project validator, which was untested
  • Adjust the validators implementation so it's a lot easier to test them and to follow their flow (which can (and will!) be/get complicated)

Copy link
Contributor

github-actions bot commented Mar 18, 2024

Try me

Copy link

relativeci bot commented Mar 18, 2024

Job #11117: Bundle Size — 63.25MiB (~-0.01%).

f4a27f3(current) vs 2296c28 master#11106(baseline)

Warning

Bundle contains 58 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Regression 1 regression
                 Current
Job #11117
     Baseline
Job #11106
Regression  Initial JS 50.43MiB(~+0.01%) 50.43MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 18.9% 21.42%
No change  Chunks 40 40
No change  Assets 44 44
No change  Modules 4467 4467
No change  Duplicate Modules 605 605
No change  Duplicate Code 31.97% 31.97%
No change  Packages 467 467
No change  Duplicate Packages 58 58
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
Job #11117
     Baseline
Job #11106
Regression  JS 63.24MiB (~+0.01%) 63.24MiB
Improvement  HTML 14.15KiB (-0.44%) 14.21KiB

View job #11117 reportView feat/project-access-collaborativ... branch activityView project dashboard

Copy link
Contributor

github-actions bot commented Mar 18, 2024

Performance test results:
(Chart1)
(Chart2)

Copy link
Contributor

@liady liady left a comment

Choose a reason for hiding this comment

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

I think we also need to discuss how much we want to rely on FGA permissions system and how much on our DB access levels (since we need to keep them in sync).
Today changing access levels toggles on/off permissions for user:*, and that's the only "source of truth" for validation. If we want to also add the layer of actually checking project access level in the DB, we just need to see that we don't have edge cases there (for example maybe we'll have a permission that we do want to allow for private projects in the future)

}

// if the project is collaborative…
if (ownership.accessLevel === AccessLevel.COLLABORATIVE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also want a WITH_LINK access level?

@ruggi ruggi marked this pull request as draft March 18, 2024 21:03
@ruggi ruggi mentioned this pull request Mar 18, 2024
ruggi added 3 commits March 19, 2024 11:34
# Conflicts:
#	utopia-remix/app/handlers/validators.spec.ts
#	utopia-remix/app/handlers/validators.ts
@ruggi ruggi marked this pull request as ready for review March 19, 2024 10:55
@ruggi ruggi merged commit 1fb4bca into master Mar 19, 2024
15 checks passed
@ruggi ruggi deleted the feat/project-access-collaborative-403 branch March 19, 2024 11:54
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.

Don't even do access checks if the project is not collaborative
4 participants