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

Align with legacy db #31

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Align with legacy db #31

wants to merge 16 commits into from

Conversation

sisygoboom
Copy link
Collaborator

@sisygoboom sisygoboom commented Jul 11, 2024

This large pr serves the purpose of aligning the code with the existing data on MongoDB. To do this I have:

  • aligned the schemas with what exists on mongo
  • ensure the correct database and collection names were used
  • Update code so that new users and account linking is consistent with the old data structure
  • Ensure tests are still working
  • sub is now used in the same way email address is used and a uuid I used as the main id

Comment on lines -122 to -126
if (this.hiveChainRepository.verifyPostingAuth(accountDetails) === false) {
const reason = `Hive Account @${hiveUsername} has not granted posting authority to @threespeak`;
const errorType = 'MISSING_POSTING_AUTHORITY';
throw new HttpException({ reason: reason, errorType: errorType }, HttpStatus.FORBIDDEN);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be removed, as this is when posting authority is required. Users should not be required to give posting authority to log in to Acela Core.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hive chain will just throw this same error if we don't have authority, What is the point of wasting an extra request checking?

sub,
}: {
hiveAccount: string;
sub: string;
user_id: string;
sub?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes the tech debt. Probably fine for now, but probably should be removed in the future I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean? we use this further down to work out if they have authority over an account

if (linkedAccount) {
throw new HttpException({ reason: 'Hive account already linked' }, HttpStatus.BAD_REQUEST);
}
const hiveAccount = await this.#hiveChainRepository.getAccount(hiveUsername);
if (!hiveAccount)
throw new NotFoundException(`Requested hive account (${hiveAccount}) could not be found.`);
await this.#hiveChainRepository.verifyHiveMessage(
`${sub} is the owner of @${hiveUsername}`,
`${user_id} is the owner of @${hiveUsername}`,
Copy link
Contributor

@Geo25rey Geo25rey Jul 22, 2024

Choose a reason for hiding this comment

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

Does this mean we need to change the message we are signing on the frontend? What should it look like? Do you have an example?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, you'd have to sign with the user id, it can be found in the token and there's also a /user endpoint which exposes it

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.

2 participants