Sy 877 server api middleware #731

LeonLiur · 2024-07-18T00:45:34Z

Feature Pull Request Template

Key Information

Linear Issue: SY-878

Description

API middleware & client libraries supporting user registration and policy management.

Please review and consider these three questions I had while in development:

In our typescript client, the auth field can be undefined if the user did not provide username & password upon starting the client. Should this still be the case? Shouldn't we allow auth to always be an available service so the user can register even if they did not log in?
Should register be a public route? This is fine because even if a user registers itself, it does not have the privilege to do anything else. However this might leave vulnerabilities like DDoS? (which we can address in other ways than making it a private endpoint).
Minor security issue: If a user has channel CREATE privileges but not RETRIEVE, they can still RETRIEVE channels they created because of the client's channel cache. Is this worth addressing?

Basic Readiness Checklist

I have performed a self-review of my code.
I have added relevant tests to cover the changes.
I have updated user facing documentation accordingly.

Migrations

Console - I have ensured that previous versions of stored data structures are
properly migrated to new formats.
Server - I have ensured that previous versions of stored data structures are
properly migrated to new formats.

Manual QA Additions

I have updated the Release Candidate template
with necessary manual QA steps to test my change.

Breaking Changes

/api/v1/auth/protected/change-password -> /api/v1/user/protected/change-password
/api/v1/auth/protected/change-username -> /api/v1/user/protected/change-username
/api/v1/auth/register -> /api/v1/user/register

vercel · 2024-07-18T00:45:37Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 26, 2024 6:13pm

codecov · 2024-07-18T00:47:39Z

Codecov Report

Attention: Patch coverage is 35.24138% with 939 lines in your changes missing coverage. Please review.

Project coverage is 50.98%. Comparing base (0ae52e0) to head (f106c83).

Files	Patch %	Lines
synnax/pkg/api/framer.go	0.00%	150 Missing ⚠️
synnax/pkg/api/ranger.go	0.00%	105 Missing ⚠️
synnax/pkg/api/hardware.go	0.00%	76 Missing ⚠️
synnax/pkg/api/user.go	0.00%	70 Missing ⚠️
synnax/pkg/api/access.go	0.00%	61 Missing ⚠️
synnax/pkg/api/ontology.go	0.00%	61 Missing ⚠️
synnax/pkg/api/channel.go	0.00%	55 Missing ⚠️
synnax/pkg/api/schematic.go	0.00%	49 Missing ⚠️
synnax/pkg/api/line_plot.go	0.00%	39 Missing ⚠️
synnax/pkg/api/label.go	0.00%	38 Missing ⚠️
... and 37 more

Additional details and impacted files

@@            Coverage Diff             @@
##               rc     #731      +/-   ##
==========================================
- Coverage   51.31%   50.98%   -0.34%     
==========================================
  Files        1055     1066      +11     
  Lines       81142    82146    +1004     
  Branches     3288     3297       +9     
==========================================
+ Hits        41638    41880     +242     
- Misses      38484    39258     +774     
+ Partials     1020     1008      -12

Flag	Coverage Δ
aspen	`50.18% <ø> (ø)`
cesium	`76.18% <ø> (ø)`
clientpy	`81.97% <100.00%> (ø)`
clientts	`87.56% <96.83%> (+0.49%)`	⬆️
freighterpy	`77.81% <100.00%> (ø)`
pluto	`44.74% <0.00%> (+<0.01%)`	⬆️
synnax	`27.10% <8.15%> (-1.75%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

emilbon99

See my comments. Also, please do a review of all endpoints to make sure we're not accidentally leaking information by using named return values after checking access.

client/py/synnax/ontology/id.py

client/ts/package.json

client/py/synnax/auth.py

synnax/cmd/start.go

client/py/synnax/ontology/__init__.py

client/ts/src/auth/auth.ts

emilbon99 · 2024-07-19T23:35:00Z

synnax/pkg/api/access.go

+	ctx context.Context,
+	req AccessRetrievePolicyRequest,
+) (res AccessRetrievePolicyResponse, err error) {
+	res.Policies = make([]rbac.Policy, 0)


Useless allocation. Just use nil.

If this is nil then upon encoding as json it won't get encoded into an empty list

emilbon99 · 2024-07-20T14:57:16Z

Note that Pluto tests do not pass because the client does not compile due to type errors

LeonLiur · 2024-07-20T16:19:59Z

Note that Pluto tests do not pass because the client does not compile due to type errors

addressed

synnax/cmd/start.go

synnax/pkg/distribution/channel/channel.go

pnpm-lock.yaml

synnax/pkg/access/rbac/policy.go

synnax/pkg/api/api.go

synnax/pkg/api/channel_test.go

emilbon99

Address my comments. Also freighter test is failing - flaky or did the formatting tests mess something up?

client/ts/src/user/client.ts

synnax/pkg/access/rbac/policy.go

synnax/pkg/api/http/http.go

synnax/pkg/api/user.go

LeonLiur · 2024-07-25T17:30:17Z

Address my comments. Also freighter test is failing - flaky or did the formatting tests mess something up?

Seemed flakey, passing now

client/py/tests/conftest.py

Lham42

Is there functionality here we should also test in QA?

LeonLiur · 2024-07-26T16:52:07Z

Is there functionality here we should also test in QA?

Yes, we should probably test the ability to register new users

emilbon99 · 2024-07-26T16:55:26Z

@LeonLiur @Lham42 We'll probably add QA changes with @pjdotson goes through and implements the console stuff. The changes in this PR can all be easily unit tested.

Lham42

LGTM

emilbon99 and others added 12 commits July 4, 2024 14:08

[synnax] - implemented access control across various APIs

0695435

[synnax] - implemented basic access control APIs

444929e

backmerge rc

57d2a65

[server] - access checkpoint

027d6dc

[access] - checkpoint

589880a

[access] - checkpoint

615ee5e

backmerge rc

412dbd2

[access] - checkpoint

5288f2a

[access] - checkpoint TS client

80f848b

[client] - ALL TESTS PASS

895dcad

backmerge rc

678e523

[ts client] - import cleanup

58e5484

LeonLiur requested review from Lham42 and emilbon99 July 18, 2024 00:45

emilbon99 added the do not merge label Jul 18, 2024

pjdotson removed the do not merge label Jul 18, 2024

LeonLiur added 2 commits July 19, 2024 16:56

[client] - re-structured access client in py and ts

f9cd64a

backmerge rc

610a304

vercel bot had a problem deploying to Preview July 19, 2024 21:00 Failure

[ops] - formatted code

acb5a38

vercel bot had a problem deploying to Preview July 19, 2024 23:19 Failure

emilbon99 requested changes Jul 19, 2024

View reviewed changes

LeonLiur added 2 commits July 20, 2024 12:15

[client/server] - addressed PR comments

32d0c84

[client/server] - backmerge

2b955f2

vercel bot deployed to Preview July 20, 2024 16:20 View deployment

[client] - fix type errors

b746f3d

vercel bot deployed to Preview July 24, 2024 14:07 View deployment

pjdotson reviewed Jul 24, 2024

View reviewed changes

[server] - addressed PR comments

c8067eb

vercel bot deployed to Preview July 24, 2024 15:21 View deployment

[server] - added back old endpoints for replaced endpoitns

60b902b

vercel bot deployed to Preview July 24, 2024 15:28 View deployment

[server] - deleted channel api tests

7b6356b

vercel bot deployed to Preview July 24, 2024 15:54 View deployment

[server] - fixed a panic in starting grpc server

0682274

vercel bot deployed to Preview July 24, 2024 16:21 View deployment

emilbon99 requested changes Jul 25, 2024

View reviewed changes

[server/client] - addressed PR comments

1b9dd8e

vercel bot deployed to Preview July 25, 2024 17:26 View deployment

LeonLiur requested review from pjdotson, emilbon99 and Lham42 July 25, 2024 17:30

pjdotson approved these changes Jul 26, 2024

View reviewed changes

client/py/tests/conftest.py Outdated Show resolved Hide resolved

Lham42 approved these changes Jul 26, 2024

View reviewed changes

Lham42 requested changes Jul 26, 2024

View reviewed changes

[client] - changed tests t o use variable for port and hostname

a2f9344

vercel bot deployed to Preview July 26, 2024 17:06 View deployment

Lham42 approved these changes Jul 26, 2024

View reviewed changes

emilbon99 approved these changes Jul 26, 2024

View reviewed changes

Backmerge rc

f106c83

vercel bot deployed to Preview July 26, 2024 18:13 View deployment

LeonLiur merged commit d9fa728 into rc Jul 26, 2024
46 of 49 checks passed

LeonLiur deleted the sy-877-server-api-middleware branch July 26, 2024 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sy 877 server api middleware #731

Sy 877 server api middleware #731

LeonLiur commented Jul 18, 2024 •

edited

Loading

vercel bot commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 18, 2024 •

edited

Loading

emilbon99 left a comment

emilbon99 Jul 19, 2024

LeonLiur Jul 20, 2024

emilbon99 commented Jul 20, 2024

LeonLiur commented Jul 20, 2024

emilbon99 left a comment

LeonLiur commented Jul 25, 2024

Lham42 left a comment

LeonLiur commented Jul 26, 2024

emilbon99 commented Jul 26, 2024

Lham42 left a comment

Sy 877 server api middleware #731

Sy 877 server api middleware #731

Conversation

LeonLiur commented Jul 18, 2024 • edited Loading

Feature Pull Request Template

Key Information

Description

Basic Readiness Checklist

Migrations

Manual QA Additions

Breaking Changes

vercel bot commented Jul 18, 2024 • edited Loading

codecov bot commented Jul 18, 2024 • edited Loading

Codecov Report

emilbon99 left a comment

Choose a reason for hiding this comment

emilbon99 Jul 19, 2024

Choose a reason for hiding this comment

LeonLiur Jul 20, 2024

Choose a reason for hiding this comment

emilbon99 commented Jul 20, 2024

LeonLiur commented Jul 20, 2024

emilbon99 left a comment

Choose a reason for hiding this comment

LeonLiur commented Jul 25, 2024

Lham42 left a comment

Choose a reason for hiding this comment

LeonLiur commented Jul 26, 2024

emilbon99 commented Jul 26, 2024

Lham42 left a comment

Choose a reason for hiding this comment

LeonLiur commented Jul 18, 2024 •

edited

Loading

vercel bot commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 18, 2024 •

edited

Loading