feat(auth): add subscription items endpoint #1478

oddgrd · 2023-12-08T14:39:38Z

Description of change

Add an endpoint to auth so we can add subscription items when we provision resources. This will add RDS (or other price_ids) as a product to the existing subscription, as a recurring item. If it is added in the middle of a subscription period, only the cost of the remaining days will be added to the invoice.

An endpoint to remove items from the subscription will follow in a subsequent PR.

How has this been tested? (if applicable)

I have tested this manually on staging, it deployed correctly with the new arg, it increased the subscription correctly and returned the expected message for the initial deployment of the resource.

I also refactored the tests to make it a bit easier to mock stripe, and added a test for the new endpoint.

…s-provisioning

deployer/src/deployment/run.rs

chesedo

Left a few comments 😄

auth/src/api/builder.rs

auth/src/args.rs

auth/src/user.rs

chesedo · 2023-12-13T14:58:30Z

auth/tests/api/users.rs

+
+        let response = app.send_request(request).await;
+
+        assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR);


Hmm, since the bearer is missing this should be 'Unauthorized' right? (old bug)

The layer is designed to return 400 (although it should be 401) for this, but since there is no token in the request axum returns a 500 due to failing to set the extension. When we pass an invalid token in the next test, however, the extension should not be set either, but that returns the expected 400. 🤔

but that returns the expected 400.

You mean 401?

Should we open a ticket for this bug if it is unrelated to the code changes, and fix it separately? It is important for our monitoring. @oddgrd you can assign it to me if you do it.

Expected in the sense that the current jwt layer implementation would return a 400 for an invalid jwt. I'll open a ticket for it, since I am a bit confused about the code path that leads to this behavior: https://github.com/shuttle-hq/shuttle/blob/main/common/src/backends/auth.rs#L351-L355.

It looks like we just proceed with the request, calling inner if there is no bearer token, which seems incorrect since that should return a 401.

This comment #1478 (comment) prompted me to add a scopedlayer to the endpoint, and now it does return 401, since the jwt auth layer calls the next layer, which is the scopedlayer, and that correctly returns 401 for missing token.

common/src/constants.rs

deployer/src/deployment/run.rs

jonaro00 · 2023-12-13T15:03:16Z

auth/src/api/builder.rs

+                    add_subscription_items.layer(JwtAuthenticationLayer::new(move || {
+                        let public_key = public_key.clone();
+                        async move { public_key.clone() }
+                    })),


Why is this needed only on this endpoint?

The auth API has only dealt with API keys or cookies so far, but rather than creating an admin user for provisioner and using that api key to call auth, we decided to just pass on the JWT of the user. This will be the only endpoint that needs to take a JWT, so we're just putting the layer on this handler for now.

So what happens if there is no cached JWT and the first request made is to this endpoint?

Hmm, what do you mean by that? The provisioner needs a JWT to provision RDS, and it then uses this JWT to call the subscription update. It could be problematic if the JWT expires while provisioning the RDS instance, though, but in that case (which should be rare) we may just delete the RDS instance and the user needs to try again.

auth/src/api/builder.rs

deployer/src/deployment/run.rs

dash in auth uri

I was incorrectly assuming that there was no valid subscription if it returned false

provisioner/src/lib.rs

…s-provisioning

auth/src/subscription.rs

rather than hardcoding the price id, pass it as startup arg, and fetch it from router state when updating subscriptions

iulianbarbu

First part of the review (auth and common realated). Continuing tomorrow with the deployer and provisioner parts.

iulianbarbu · 2023-12-18T11:45:29Z

.circleci/config.yml

+          # filters:
+          #   branches:
+          #     only: main


reminder: to be removed before merging.

auth/README.md

auth/src/user.rs

Makefile

auth/src/api/handlers.rs

auth/src/user.rs

iulianbarbu · 2023-12-18T16:24:31Z

auth/tests/api/users.rs

+
+        let response = app.send_request(request).await;
+
+        assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR);


but that returns the expected 400.

You mean 401?

Should we open a ticket for this bug if it is unrelated to the code changes, and fix it separately? It is important for our monitoring. @oddgrd you can assign it to me if you do it.

provisioner/src/lib.rs

chesedo

Looking good!

auth/src/api/handlers.rs

auth/src/user.rs

…ndler

chesedo

LGTM!

…ioning rds (#1478)" This reverts commit 657815d.

* Revert "feat: add subscription items endpoint and call it when provisioning rds (#1478)" This reverts commit 657815d. * Revert "fix(provisioner): only delete new rds on failed subscription update (#1488)" This reverts commit f81b5ef. * fix: clippy

oddgrd added 5 commits December 8, 2023 15:36

feat(auth): add subscription items endpoint

d3809f2

feat(auth): move subscription item model to common

2820a16

refactor(common): move stripe models to backend

05574fa

feat(auth): guard sub items endpoint with jwt layer

c90bc91

wip(provisioner): update subscription on provisioning rds

2a24abb

oddgrd force-pushed the feature/eng-1946-increase-subscription-on-rds-provisioning branch from 4fb5e2a to 2a24abb Compare December 12, 2023 13:24

oddgrd added 7 commits December 13, 2023 11:37

Merge branch 'main' into feature/eng-1946-increase-subscription-on-rd…

7ea8662

…s-provisioning

revert(auth): fromrow changes, no longer needed with pg

950433d

docs(auth): add steps for running binary locally

c4b32d0

wip(deployer): send message on subscription increase

4340985

fix(auth): invalid postgres query

f1ceb4d

fix(deployer): missing field in test insert query

c478b66

fix(cargo-shuttle): display message on deploy and status

d2971de

oddgrd commented Dec 13, 2023

View reviewed changes

deployer/src/deployment/run.rs Outdated Show resolved Hide resolved

chesedo reviewed Dec 13, 2023

View reviewed changes

jonaro00 reviewed Dec 13, 2023

View reviewed changes

oddgrd added 11 commits December 13, 2023 17:17

fix(auth): remove unused endpoint

a83666e

refactor(auth): pass stripe client to sub update method

b9cbd62

refactor(deployer): make cost increase msg generic

18d9730

refactor: simplify subscription dto, move price_id to auth

c352e7b

feat: error handling, tests, comment deploy filter

a5ddd1b

fix: clippy

3a3c287

fix(provisioner): extraneous

d38a53f

dash in auth uri

fix(provisioner): send item as json

0028291

tests(provisioner): mock auth server

e78d42b

fix(auth): sync_tier returns false if it didn't sync

e5e653e

I was incorrectly assuming that there was no valid subscription if it returned false

fix(auth): test returns 400 due to missing sub id

a8793c8

oddgrd commented Dec 14, 2023

View reviewed changes

provisioner/src/lib.rs Outdated Show resolved Hide resolved

misc: cleanup, instrumentation

130ee9d

oddgrd marked this pull request as ready for review December 14, 2023 19:36

oddgrd added 4 commits December 15, 2023 11:50

Merge branch 'main' into feature/eng-1946-increase-subscription-on-rd…

099f59d

…s-provisioning

refactor: improve provisioner auth client error handling

4a500ae

refactor(provisioner): retry suggestion in expired jwt error

e508559

refactor(provisioner): remove unused unsafe impl

1dc1fd3

oddgrd commented Dec 15, 2023

View reviewed changes

auth/src/subscription.rs Outdated Show resolved Hide resolved

oddgrd added 7 commits December 18, 2023 10:01

tests(auth): refactor stripe tests to wiremock

8eb24d7

tests(auth): remove duplicate test

6499db2

misc(auth): cleanup tests

574a486

misc: remove todos, guard against account downgrade

4d2b659

refactor(auth): make price_id fetching env dependent

44381de

feat(auth): pass price_id as arg

0a081da

rather than hardcoding the price id, pass it as startup arg, and fetch it from router state when updating subscriptions

feat(auth): add price id arg to makefile, dockerfile

76ddf5b

iulianbarbu reviewed Dec 18, 2023

View reviewed changes

docs(auth): add price id arg to readme guide

9bbd36e

iulianbarbu reviewed Dec 19, 2023

View reviewed changes

provisioner/src/lib.rs Show resolved Hide resolved

feat(auth): add scopedlayer to update sub endpoint

6835532

chesedo reviewed Dec 19, 2023

View reviewed changes

auth/src/api/handlers.rs Outdated Show resolved Hide resolved

auth/src/user.rs Outdated Show resolved Hide resolved

oddgrd added 4 commits December 19, 2023 12:27

ci: add stripe-rds-price-id arg

4317ee6

misc(auth): remove commented debug_handler

39f0897

refactor(auth): sync and check tier in handler

2029032

refactor(auth): remove user add_sub_items method, do all checks in ha…

89f8b3b

…ndler

chesedo approved these changes Dec 19, 2023

View reviewed changes

oddgrd merged commit 657815d into main Dec 20, 2023

oddgrd deleted the feature/eng-1946-increase-subscription-on-rds-provisioning branch December 20, 2023 07:41

oddgrd added a commit that referenced this pull request Jan 8, 2024

Revert "feat: add subscription items endpoint and call it when provis…

fa09548

…ioning rds (#1478)" This reverts commit 657815d.

oddgrd mentioned this pull request Jan 9, 2024

tests(auth): simplify auth service tests with wiremock #1514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): add subscription items endpoint #1478

feat(auth): add subscription items endpoint #1478

oddgrd commented Dec 8, 2023 •

edited

Loading

chesedo left a comment

chesedo Dec 13, 2023

oddgrd Dec 13, 2023

iulianbarbu Dec 18, 2023

oddgrd Dec 18, 2023 •

edited

Loading

oddgrd Dec 18, 2023

oddgrd Dec 19, 2023

jonaro00 Dec 13, 2023

oddgrd Dec 13, 2023

jonaro00 Dec 13, 2023

oddgrd Dec 13, 2023

iulianbarbu left a comment

iulianbarbu Dec 18, 2023

iulianbarbu Dec 18, 2023

chesedo left a comment

chesedo left a comment


		let response = app.send_request(request).await;

		assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR);

feat(auth): add subscription items endpoint #1478

feat(auth): add subscription items endpoint #1478

Conversation

oddgrd commented Dec 8, 2023 • edited Loading

Description of change

How has this been tested? (if applicable)

chesedo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oddgrd Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iulianbarbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chesedo left a comment

Choose a reason for hiding this comment

chesedo left a comment

Choose a reason for hiding this comment

oddgrd commented Dec 8, 2023 •

edited

Loading

oddgrd Dec 18, 2023 •

edited

Loading