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

chore: New Builder Auth #4010

Merged
merged 21 commits into from
Aug 29, 2024
Merged

chore: New Builder Auth #4010

merged 21 commits into from
Aug 29, 2024

Conversation

istarkov
Copy link
Member

@istarkov istarkov commented Aug 25, 2024

Description

Full track

https://github.com/webstudio-is/webstudio-saas/issues/317#issuecomment-2289241819

So thoughts about session
https://github.com/webstudio-is/webstudio-saas/issues/317#issuecomment-2314515772

Some explanations what is fixed and how
https://github.com/webstudio-is/webstudio-saas/issues/317#issue-2465048823

Next PR:

  • - Old share links auto redirect
  • - Try make better error message having that current PR will logout all users.

Steps for reproduction

  1. Login using google login or dev login try to open projects on dahsboard, create new, clone delete etc.
  2. Try to open projects on dahboard, see each is on it's own domain and work
  3. Try to open project link (without share token) with other user - should fail.
  4. Try publish/clone any links and operations.
  5. Share project from userA:
  • try to open it with other userB
    • see userB is not logged in to that project
    • try to get using fetch from console anything about userB or userA i.e rest etc endpoints on apps.webstudio.is domain
  • try to open incognito, check it works.
  1. Open few project, then logout from dashboard, reload project page
  • See login page
  • Try to login as same uer - see it works
  • Or try to login as another user - see auth error

Code Review

  • hi @kof, I need you to do
    • conceptual review (architecture, feature-correctness)
    • detailed review (read every line)
    • test it on preview

Before requesting a review

  • made a self-review
  • added inline comments where things may be not obvious (the "why", not "what")

Before merging

  • tested locally and on preview environment (preview dev login: 5de6)
  • updated test cases document
  • added tests
  • if any new env variables are added, added them to .env file

@istarkov
Copy link
Member Author

istarkov commented Aug 26, 2024

Tests:

  1. Login via google
  2. Create new Project, created, Im redirected to a new project.
  3. Clone project using project menu. Cloned/redirected.
  4. Edit text - working
  5. Create new page and links - preview working.
  6. Publish - working
  7. create custom domain - working, publish working https://hello.poke-method.work/
  8. goto dahsboard
  9. Delete - working
  10. clone template working - got error on canvas Cannot read properties of undefined (reading 'get') (Same on Main)
  11. clone anouther template - Sheet error - (Same on main)

@istarkov istarkov force-pushed the auth.staging branch 2 times, most recently from 981d263 to f2a1a3b Compare August 27, 2024 05:34
Make builder the root path

cleanups

Add everything

Remove tmp

Remove

Fix

Fix layout

Fix

Fix url

Add cross-origin check

Fix stale data

dashboardPath

Fix canvas-url path

Improve logout

Add bloom filter

Commit projects to the session

Add leak

Change templates

Trigger Build

rename

throw to prevent any cross origin

Add log

Change session cookie

Add comments

cleanup + comments

Fix

Test delete deployments

Test call

Try delete all

fix

Trigger Build

Fix

Add cleanup after r2

potpone after test
@istarkov istarkov marked this pull request as ready for review August 27, 2024 20:55
@istarkov istarkov requested review from kof and TrySound August 27, 2024 20:55
@istarkov istarkov merged commit f7487fd into main Aug 29, 2024
24 of 26 checks passed
@istarkov istarkov deleted the auth.staging branch August 29, 2024 09:38
kof pushed a commit that referenced this pull request Aug 30, 2024
## Description

### Full track


webstudio-is/webstudio-saas#317 (comment)

So thoughts about session

webstudio-is/webstudio-saas#317 (comment)

Some explanations what is fixed and how

webstudio-is/webstudio-saas#317 (comment)

Next PR:
- [x] - Old share links auto redirect
- [x] - Try make better error message having that current PR will logout
all users.

## Steps for reproduction

1. Login using `google login` or `dev login` try to open projects on
dahsboard, create new, clone delete etc.
2. Try to open projects on dahboard, see each is on it's own domain and
work
3. Try to open project link (without share token) with other user -
should fail.
4. Try publish/clone any links and operations.
5. Share project from `userA`:
 - try to open it with other `userB`
   - see `userB` is not logged in to that project
- try to get using fetch from console anything about `userB` or `userA`
i.e rest etc endpoints on apps.webstudio.is domain
 - try to open incognito, check it works.
6. Open few project, then logout from dashboard, reload project page
 - See login page
 - Try to login as same uer - see it works
 - Or try to login as another user - see auth error


## Code Review

- [ ] hi @kof, I need you to do
  - conceptual review (architecture, feature-correctness)
  - detailed review (read every line)
  - test it on preview

## Before requesting a review

- [ ] made a self-review
- [ ] added inline comments where things may be not obvious (the "why",
not "what")

## Before merging

- [ ] tested locally and on preview environment (preview dev login:
5de6)
- [ ] updated [test
cases](https://github.com/webstudio-is/webstudio/blob/main/apps/builder/docs/test-cases.md)
document
- [ ] added tests
- [ ] if any new env variables are added, added them to `.env` file
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