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

API for Asset CRUD Operations and Authentication Middleware #14

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

petpalioudakis
Copy link

Assumptions:

All users are assumed to have administrative privileges, granting them access to all asset CRUD operations without ownership verification.

@wiz-gwi
Copy link

wiz-gwi bot commented Jun 2, 2024

Wiz Scan Summary

IaC Misconfigurations 0C 1H 0M 1L 0I
Vulnerabilities 0C 1H 0M 0L 0I
Sensitive Data 0C 0H 0M 0L 0I
Total 0C 2H 0M 1L 0I
Secrets 2🔑

@gvasilei gvasilei requested review from apoeco and NikosMas June 4, 2024 11:25
Copy link

Choose a reason for hiding this comment

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

Good job with the readme, very descriptive 👍

"github.com/dgrijalva/jwt-go"
)

func extractAssetIDFromPath(r *http.Request) string {
Copy link

Choose a reason for hiding this comment

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

I can't find where this method is being used


var JwtKey = []byte(os.Getenv("SECRET_KEY"))

type Claims struct {
Copy link

Choose a reason for hiding this comment

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

What approach would you choose to avoid including JWT definitions and processes in the repository layer?

Copy link
Author

Choose a reason for hiding this comment

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

I would have used a service layer approach so I could adhere to the single responsibility principle. This pattern, utilized in many frameworks, including NestJs which I am familiar with, enforces good design practices.

  • All JWT-related operations would have been moved to a separate file.
  • A new file containing a service would have been responsible for handling the business logic.
  • I would have retained the store/store.go, which in my case represents the repository layer, for database-related operations only.

@NikosMas
Copy link

NikosMas commented Jun 5, 2024

Hello @petpalioudakis 👋
First of all, thank you for your time and effort to complete the assignment!
I have a few questions about your implementation:

  1. Regarding your assumption of not applying ownership verifications on the CRUD operations
    • what are your thoughts on this assumption?
    • what would you do if you had to remove this assumption?
  2. I can see that you're using PostgreSQL
    • why didn't you choose a NoSQL database, such as MongoDB?
    • I see you used one table for assets and favorites. How would you handle the assets entity if you had to split it from the favorites one?
    • also what actions would you take to secure the scalability of the database without consequences?
  3. Regarding the error handling, Is there anything that you would do to avoid SQL errors like this one ERROR: insert or update on table "assets" violates foreign key constraint "assets_user_id_fkey" (SQLSTATE 23503) reaching the API response?
  4. If you had to add tests, which part of your implementation would you choose to test first?

@petpalioudakis
Copy link
Author

Hello @NikosMas

First of all, I would like to thank you for taking the time to review my code. I have to admit that I am not very familiar with Go. Due to my busy schedule, I implemented patterns based on my knowledge of other programming languages in the limited time I had available. I will try to answer the questions you asked me in order.

1. Regarding your assumption of not applying ownership verifications on the CRUD operations:

What are your thoughts on this assumption?

This assumption was made to simplify the implementation for the initial assignment. By assuming all users have administrative privileges, it allowed for a straightforward demonstration of basic CRUD operations without incorporating complex authorization logic.

What would you do if you had to remove this assumption?

If ownership verification had to be implemented, the following steps would be taken:

  • Modify the Asset Model: Add a user_id field to the assets table to associate each asset with its owner.
    Implement Middleware: Create middleware to verify that the authenticated user is the owner of the asset before allowing any modifications. This middleware would check the user_id field of the asset against the authenticated user's ID. I began implementing the middleware in the jwt.go file, which is why you saw the extractAssetIDFromPath method in the jwt.go file.
  • Update Handlers: Modify CRUD operation handlers to incorporate ownership verification, ensuring that only the owner can modify or delete their assets.

  1. I can see that you're using PostgreSQL:

Why didn't you choose a NoSQL database, such as MongoDB?

  • I chose PostgreSQL for convenience and familiarity. There was no particular reason beyond that. PostgreSQL offers robust support for structured data and complex queries, which fit well within the assignment's requirements.

3. I see you used one table for assets and favorites:

How would you handle the assets entity if you had to split it from the favorites one?

To separate the assets and favorites, I would:

  • Create Separate Tables:
    • Assets Table: Store all asset details.
    • Favorites Table: Store user-specific favorite mappings.

Here a Schema Example:

CREATE TABLE assets (
    id SERIAL PRIMARY KEY,
    type VARCHAR(50) NOT NULL,
    description TEXT NOT NULL,
    data TEXT NOT NULL
);

CREATE TABLE favorites (
    id SERIAL PRIMARY KEY,
    user_id INT NOT NULL,
    asset_id INT NOT NULL,
    FOREIGN KEY (user_id) REFERENCES users(id),
    FOREIGN KEY (asset_id) REFERENCES assets(id)
);

Advantages:

  • Separation of Concerns: Clear distinction between assets and user favorites.
  • Scalability: Easier to manage and scale the tables independently.

Scalability Considerations:

  • Indexing: Ensure proper indexing on frequently queried fields.
  • Partitioning: Use table partitioning for large datasets to improve query performance.
  • Replication and Sharding: Implement database replication and sharding strategies to distribute load and ensure high availability.
    I know the process might be a bit more involved for PostgresSQL compared to some other databases that have built-in support for these features.

4. Regarding the error handling:

Is there anything that you would do to avoid SQL errors like this one reaching the API response?

To improve error handling and prevent SQL errors from propagating to the API response:

  • Error Handling Middleware: Implement middleware to handle and format errors consistently.
  • Database Constraints Checks: Validate data before performing database operations to catch potential constraint violations early.

5. If you had to add tests, which part of your implementation would you choose to test first?

Priority Areas for Testing:

  • Test login with valid and invalid credentials.
  • Verify that an authenticated user can obtain a JWT token.
  • Test creating an asset and verifying that it belongs to the user.
  • Attempt to update/delete an asset owned by another user and expect an unauthorized error.
  • Test adding a favorite with a non-existent asset ID and check for a user-friendly error message.
  • Verify that SQL constraint violations do not leak implementation details to the API response.

@NikosMas
Copy link

NikosMas commented Jun 7, 2024

Hi again @petpalioudakis, thank you for your detailed responses and the time you spent on my comments!

I have one last question though:
I checked the library you used for the JWT (github.com/dgrijalva/jwt-go) and it seems that it was archived 2 years ago, is there any specific reason you chose this one?

@petpalioudakis
Copy link
Author

Hello @NikosMas

I am aware that this library has been archived, but as it was an assignment and not a production-level project, I decided to use it to save time. I have already implemented the JWT-related code in an older project I worked on three years ago when I was testing migration from PHP-implemented APIs to Go or Python.

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