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

Feature/create claim tester account endpoints 420 #440

Merged
merged 11 commits into from
Feb 17, 2025

Conversation

PlayJeri
Copy link
Collaborator

Implemented the logic for claiming tester accounts.

The draw.io showed that the device identifier should be saved to box metadata in getDeviceIdentifier endpoint, but my implementation does not do that. The getDeviceIdentifier endpoint creates an identifier from hash values of request user-agent and ip and sets that as a deviceIdentifier cookie. The device identifier is only saved in to the db's accountClaimersIds array when an account it successfully claimed.

The claimAccount finds a box based on the password and checks if the deviceIdentifier has already claimed an account if not then updates the box by adding the deviceIdentifier to the accountClaimersIds and sets a claimed to true on one unclaimed tester. Then it signs an access token and returns the playerData, profile_id, accessToken and password.

Since the basicServices includeRefs has type ModelName I added the refs with
this.refsInModel as string[] as ModelName. That way the options includeRefs type doesn't need to be changed.

Enhancements to box module:

  • src/box/box.controller.ts: Added new endpoints getDeviceIdentifier and claimAccount with corresponding decorators and request handlers. These endpoints handle device identification and account claiming processes.
  • src/box/box.service.ts: Introduced methods for setting device identifiers, claiming accounts, and handling related errors. These methods include setDeviceIdentifier, createDeviceIdentifier, getBoxWithTesters, updateBoxIdentifierAndTesters, claimAccount, getTesterPlayerData, and getTesterAccount.

Dependency updates:

  • package.json: Added cookie-parser and its type definitions to the dependencies list. [1] [2]

Error handling:

Type definitions:

Cookie parsing:

  • src/main.ts: Enabled cookie parsing in the main application by using the cookie-parser middleware.

@MikhailDeriabin MikhailDeriabin added feature New feature to add PR Pull request labels Feb 14, 2025
@MikhailDeriabin MikhailDeriabin added this to the Testing group milestone Feb 14, 2025
@MikhailDeriabin MikhailDeriabin linked an issue Feb 14, 2025 that may be closed by this pull request
2 tasks
Copy link
Member

@MikhailDeriabin MikhailDeriabin left a comment

Choose a reason for hiding this comment

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

Overall looks ok.

Does the claiming mechanism works as this:

  1. Client makes a request to the /box/identifier endpoint and gets a cookie set to it
  2. Client makes a request to /box/claim-account and get the account

Things to change:

  1. Box.service.ts
    • [Line 65] The unique identifiers generation logic is not sufficient, because the ip address can be the same (if testers are on the same network) and the user agent is most likely to be chrome and samsung. I would suggest instead of that use the mongo _id generation mechanism, which will ensure the uniqueness of the value and in this case there are no need to hash anything:
const identifier = new ObjectId().toString();
  • I have noticed that a lot of methods added to the BoxService can be moved to separate class, which handles the account claiming logic. What do you think?
  • [Line 118], [Line 157], [Line 184] Please instead of returning : Promise<any> or any add some type to it or just remove type definition completely, TS should be able to do it for you.
  1. BoxWithPopulatedRefs.type.ts

    • Did you forget to rename this file, since it is in folder types and have .type. in file name, but is an interface. You can name it for example IBoxWithPopulatedRefs.
  2. accountClaimed.error.ts:

    • May be the 403 error would be better here? At least from the client point of view. What you might want to do here is to throw or return a ServiceError from the BoxService, that have field: password and reason NOT_UNIQUE and on the controller level return or throw the NOT_AUTHORIZED APIError. Or even move the if-statement to the controller.
  3. player.dto.ts:

    • Why did you add extends Document here? Some tests are failing now due to it

@PlayJeri
Copy link
Collaborator Author

@MikhailDeriabin

  1. I thought the point of the identifier was to be unique based on the requester. If we use the mongoId then it is completely random. User could claim multiple accounts by resetting the cookies. If the idea behind this identifier is only to block accidental claiming of multiple accounts we don't need the endpoint for unique identifier at all. We could just set accountClaimed cookie on the claim account endpoint and block the claiming of multiple accounts by checking if the accountClaimed cookie exists when request comes to that endpoint.

  2. I named it like that on purpose the types is more describing to the use case than interface but since the "type" extends the existing dto type can't be used.

  3. My bad for not running the tests here. The extends document is there because that makes it more convenient to extract fields from the PlayerDto object. The returned playerDto can now be made to plain object with the .toObject() function and then the "const { extractField, extractField2, ...dataToKeep } = plainObject" to extract fields. Otherwise it is needed to type all the included fields which is more verbose. Since the playerDto needed two fields extracted from return type I thought this would be the easiest way. I'll try a different approach.

@MikhailDeriabin
Copy link
Member

@PlayJeri

  1. Well the idea behind unique identifiers logic is that we trust that cookie will not be reset. So that it's more a helping mechanism, if for example on the client side somebody clicks "claim account" button many times in a row or if web app team will forget to remove this button after account already claimed and user click "claim account" by accident again.
    It is not really a security measure since this mechanism is password protected, so nobody outside the group could not claim multiple accounts.
    Yes, I think that you can just set the accountClaimed cookie, at the time I wrote the plan, I did not know about this HTTP only thing. It should work fine, but I still wonder about the situation if a user hits the button like 5 times => the API sets 5 profiles as claimed then since it does not have the identifier, but if at the moment of claiming the user already has the identifier API can ignore extra calls.
    If you are sure that the logic with one request and accountClaimed cookie will work, so do it.

  2. If you mean that it could be an interface for API internal use only on the services level, then I would suggest the interfaces name for the folder and no extensions for the file, because type is kinda taken term by TS. All the classes used in controllers for external communications can be in dto folder, and all for the internal use could be just placed and named to any other folders such as interfaces, schemas etc.

  3. If you mean that you need to convert object coming from some service level class, you can try to change the return type of a method from PlayerDTO to Player. Run the tests after thought, I am not sure if it won't break something.

@PlayJeri
Copy link
Collaborator Author

@MikhailDeriabin

I made some changes to the logic and to the types and dtos.

The controller now only has the claim-account endpoint. The prevention of claiming multiple accounts now works like this. If the request is successful an accountClaimed cookie with 15min maxAge is set. The endpoint checks if the request has the accountClaimed cookie set and returns 403 error if so. The unique identifier no longer exists with this logic so the accountClaimersIds prop in the box schema might be unnecessary.

I removed the document extension from the player dto and the boxdto extension interface. I also removed the helper methods for unique identifier creation from service.

Added the explicit return types to the service methods.

@MikhailDeriabin MikhailDeriabin merged commit 5c98143 into dev Feb 17, 2025
@MikhailDeriabin MikhailDeriabin deleted the feature/create-claim-tester-account-endpoints-420 branch February 17, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature to add PR Pull request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

create endpoints for claim tester account process
2 participants