Various API Fixes #1538
Various API Fixes #1538
Conversation
0xemc
had a problem deploying
to
Preview – carbonmark-api
GitHub Actions
Failure
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
0xemc
had a problem deploying
to
Preview – carbonmark-api
GitHub Actions
Failure
0xemc
temporarily deployed
to
Preview – carbonmark-api
GitHub Actions
Inactive
| if (user.exists) { | ||
| return reply.code(403).send({ | ||
| code: 403, | ||
| error: "This user is already registered!", | ||
| error: "This wallet address is already registered!", | ||
| }); | ||
| } | ||
|
|
||
| // Check if the handle already exists in our database | ||
| const usersRef = fastify.firebase.firestore().collection("users"); | ||
|
|
||
| const userSnapshot = await usersRef | ||
| .where("handle", "==", handle.toLowerCase()) | ||
| .limit(1) | ||
| .get(); | ||
| // If no documents are found, return a 404 error | ||
|
|
||
| if (!userSnapshot.empty) { | ||
| return reply.code(403).send({ | ||
| code: 403, | ||
| error: "This user is already registered!", | ||
| error: "A user with this handle is already registered!", | ||
| }); | ||
| } |
There was a problem hiding this comment.
I find these checks a little confusing:
user.exists == true=> wallet already registered!userSnapshot.empty == true=> user with this handle is already registered
Especially since the former checks explicitly if user.exists…
There was a problem hiding this comment.
I can see why, in the first case we're confirming that no Document in firebase exists with that wallet (this is the primary key).
In the second we want to make sure that no one is creating a new account with a new address (primary key) but using a handle that someone else has already set on their Document.
Does that make sense?
0xemc
temporarily deployed
to
Preview – carbonmark-api
GitHub Actions
Inactive
0xemc
force-pushed
the
emc/fix/various-fixes
branch
from
578eea1
to
7f9a1f2
Compare
0xemc
temporarily deployed
to
Preview – carbonmark-api
GitHub Actions
Inactive
0xemc
temporarily deployed
to
Preview – carbonmark-api
GitHub Actions
Inactive
0xemc
temporarily deployed
to
Preview – carbonmark-api
GitHub Actions
Inactive
| .firestore() | ||
| .collection("users") | ||
| .doc(wallet.toUpperCase()) | ||
| .set(createData); | ||
|
|
||
| // If the document is successfully created, return the request body | ||
| return reply.send(request.body); | ||
| return reply.send(document); |
There was a problem hiding this comment.
This isn't a document it is actually a WriteResult interface. Don't think we want to return that.
it may make more sense to return createData rather than request.body however that is out of scope for this PR
There was a problem hiding this comment.
Good catch, reverted to request body for now
| @@ -21,6 +21,19 @@ const handler = (fastify: FastifyInstance) => | |||
| profileImgUrl && profileImgUrl.length ? profileImgUrl : null, | |||
| }; | |||
|
|
|||
| // Check if the handle already exists in our database | |||
There was a problem hiding this comment.
we only want to do this for POST (new user creation). PUT is for edits, this will incorrectly throw for all edits because the handle already exists.
Actually this draws my attention to an existing mistake in this PUT endpoint-- we shouldn't pass handle to firestore.update()-- the original requirement was to not support handle changes after profile creation
There was a problem hiding this comment.
Fair enough, updated and removed handle.
0xemc
temporarily deployed
to
Preview – carbonmark-api
GitHub Actions
Inactive
0xemc
force-pushed
the
emc/fix/various-fixes
branch
from
61d1a05
to
343a232
Compare
0xemc
temporarily deployed
to
Preview – carbonmark-api
GitHub Actions
Inactive
Description
This PR refines the user registration and update process in the Carbonmark API. It adds checks for JWT_SECRET, improves error messages and adds pattern validation for user handles.
Changes