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

Add DTO for users table for example 5 #29

Open
dschwarz91 opened this issue Oct 23, 2022 · 8 comments
Open

Add DTO for users table for example 5 #29

dschwarz91 opened this issue Oct 23, 2022 · 8 comments

Comments

@dschwarz91
Copy link
Contributor

at the moment, different SQL-Queries are used to either include or exclude the user pwd hash.
Best practice would be to use a DTO to make unintentional mistakes less common

@karlhorky
Copy link
Member

Interesting idea! Do you think you could edit the description above to also include 2 code blocks for "Before" and "After"? I would be interested to see what you mean from a code perspective.

@dschwarz91
Copy link
Contributor Author

@dschwarz91
Copy link
Contributor Author

so we could do something like this for the user object/class in example 5:

interface userDTO {
  id: Number;
  name: String;
}

class userMap {
  public static toDTO(user: User) {
    return {
      id: user.id,
      name: user.username
    } as userDTO
  }

}

I'm just having some trouble with the handling of the multiple rows....

@karlhorky
Copy link
Member

Looks complex... what's the benefit? (and how would you explain this benefit to beginners?)

Usually we try to avoid any layers of indirection unless they are absolutely necessary.

@dschwarz91
Copy link
Contributor Author

The idea is, that the DTO more or less defines the structure of your API (like an API contract).
Advantages:

  • for security: you can make sure, that no unwanted data gets leaked at read-requests (e.g. the password-hash in our example 5) and no unwanted fields can be overwritten in write-requests (mass assignment)
  • besides security: you separate the outfacing API from your internal data structure - so if you change your internal data structures, your API doesn't break

but I also think about skipping this part.

@karlhorky
Copy link
Member

Hm... not sure how a DTO by itself would prevent exposure of an API key like this:

export async function getServerSideProps(): Promise<
GetServerSidePropsResult<Props>
> {
const users = await getUsersWithPasswordHash();
return {
props: {
apiKey: process.env.API_KEY!,
users: users,
},
};
}

In this case our Props type in this file acts as our "DTO", if I'm understanding correctly. And in this case, apiKey is part of the Props (meaning that it's part of the DTO).

A DTO by itself probably doesn't inherently increase security, if you were to have the secret data defined in that DTO accidentally (like in this Props example).


I'm also similar skeptical about the value of a DTO in keeping public-facing APIs stable over time (if you change your internal data structure, then you may similarly mess up exposing it properly via the DTO). I've gone this route of abstraction before, but in the last years, I've been writing my code simpler and with stronger guarantees like full-stack type safety, and it seems to work pretty well to avoid these problems.

@dschwarz91
Copy link
Contributor Author

Didn't talk about the API key, but about the password-hash contained in the user-objects in line 38.
If you define which attributes are exposed via the API (with the DTO), it doesn't matter if the database-query returns additional data fields or not, you can be sure that only the intended data gets exposed.

DTOs don't help you with the whole API-key problem, because this is actually a generic design issue (in my opinion) if you need an API key for a backend-service in the frontend.

But I absolutely get your point regarding the additional complexity...

Let's skip this whole DTO thing for now... This is maybe a little bit too complex/controversial for such a short beginners course.

@karlhorky
Copy link
Member

karlhorky commented Nov 3, 2022

Ok, I guess my point is similar with the users with password hash though: if the DTO (Props can be seen like this here), define a permissive data structure like User (allowing a password hash), then you would still have the problem. The DTO itself doesn't solve the problem, it just makes it more clear to see the error and centralizes the place where problems should be looked for (although really, you probably want to look for problems on all layers anyway).

Instead of adding these extra abstractions, another (simpler) solution would be on the type level in the database file:

export type User = {
  id: number;
  username: string;
+ passwordHash: never;
};

-export type UserWithPasswordHash = User & {
+export type UserWithPasswordHash = Omit<User, 'passwordHash'> & {
  passwordHash: string;
};

Then your code doesn't even compile if a user object has a passwordHash property and there's no way that you make this mistake, all without extra abstraction layers.

That could be added as part of one of the existing solutions, or added as a new solution too.

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

No branches or pull requests

2 participants