Repository secure by design: how to sleep better without fear of security vulnerabilities

Thanks to tests and code review, you can make your project bug-free. Right? Well… actually, probably not. That would be too easy. 😉 These techniques lower the chance of bugs, but they can’t eliminate them entirely. Does that mean we need to live with the risk of bugs until the end of our lives?

Over a year ago, I found a pretty interesting PR in the harbor project. It was a fix for an issue that allowed a regular user to create an admin user. This was obviously a severe security issue. Of course, automated tests didn’t find this bug earlier.

This is what the bugfix looks like:

		ua.RenderError(http.StatusBadRequest, "register error:"+err.Error())
		return
	}
+
+	if !ua.IsAdmin && user.HasAdminRole {
+		msg := "Non-admin cannot create an admin user."
+		log.Errorf(msg)
+		ua.SendForbiddenError(errors.New(msg))
+		return
+	}
+
	userExist, err := dao.UserExists(user, "username")
	if err != nil {

One if statement fixed the bug. Adding new tests also should ensure that there will be no regression in the future. Is it enough? Did it secure the application from a similar bug in the future? I’m pretty sure it didn’t.

The problem becomes bigger in complex systems with a large team working on them. What if someone is new to the project and forgets to add this if statement? Even if you aren’t hiring new people now, you may hire them in the future. You’ll probably be surprised how long the code you’ve written will live. We should not trust people to use our code the way we intended: they won’t.

In some cases, the solution that protects us from issues like this is good design. Good design should not allow our code to be used in an invalid way. Good design should guarantee that you can modify existing code without fear. People new to the project will feel safer introducing changes.

In this article, I’ll show how I ensured that only authorized people can see and edit a training. In our case, a training can only be seen by the training owner (the attendee) and the trainer. I will implement it in a way that prevents our code from being misused. By design.

Our current application assumes that a repository is the only way to access data. Because of that, I will add authorization at the repository level. This ensures that unauthorized users cannot access this data.

But wait, is the repository the right place to manage authorization? Well, I can imagine some people may be skeptical about this approach. Of course, we could start a philosophical discussion on what belongs in the repository and what doesn’t. Also, the actual logic of who can see the training will be placed in the domain layer. I don’t see any significant downsides, and the advantages are clear. In my opinion, pragmatism should win here.

Show me the code, please!

To achieve our robust design, we need to implement three things:

1. Logic for who can see the training (domain layer),
2. Functions to get the training (GetTraining in the repository),
3. Functions to update the training (UpdateTraining in the repository).

Domain layer

The first part is the logic responsible for deciding if someone can see the training. Because it is part of the domain logic (you can discuss who can see the training with your business or product team), it should go in the domain layer. It’s implemented with the CanUserSeeTraining function.

It is also acceptable to keep it at the repository level, but it’s harder to reuse. I don’t see any advantage to that approach: especially since putting it in the domain doesn’t cost anything. 😉

package training

// ...

type User struct {
	userUUID string
	userType UserType
}

// ...

type ForbiddenToSeeTrainingError struct {
	RequestingUserUUID string
	TrainingOwnerUUID  string
}

func (f ForbiddenToSeeTrainingError) Error() string {
	return fmt.Sprintf(
		"user '%s' can't see user '%s' training",
		f.RequestingUserUUID, f.TrainingOwnerUUID,
	)
}

func CanUserSeeTraining(user User, training Training) error {
	if user.Type() == Trainer {
		return nil
	}
	if user.UUID() == training.UserUUID() {
		return nil
	}

	return ForbiddenToSeeTrainingError{user.UUID(), training.UserUUID()}
}

Repository

Now that we have the CanUserSeeTraining function, we need to use it. Simple as that.

func (r TrainingsFirestoreRepository) GetTraining(
	ctx context.Context,
	trainingUUID string,
+	user training.User,
) (*training.Training, error) {
	firestoreTraining, err := r.trainingsCollection().Doc(trainingUUID).Get(ctx)

	if status.Code(err) == codes.NotFound {
		return nil, training.NotFoundError{trainingUUID}
	}
	if err != nil {
		return nil, errors.Wrap(err, "unable to get actual docs")
	}

	tr, err := r.unmarshalTraining(firestoreTraining)
	if err != nil {
		return nil, err
	}
+
+	if err := training.CanUserSeeTraining(user, *tr); err != nil {
+		return nil, err
+	}
+
	return tr, nil
}

Isn’t this too simple? Our goal is to create simple, not complex, design and code. This is an excellent sign that it’s dead simple.

We are changing UpdateTraining in the same way.

func (r TrainingsFirestoreRepository) UpdateTraining(
	ctx context.Context,
	trainingUUID string,
+	user training.User,
	updateFn func(ctx context.Context, tr *training.Training) (*training.Training, error),
) error {
	trainingsCollection := r.trainingsCollection()

	return r.firestoreClient.RunTransaction(ctx, func(ctx context.Context, tx *firestore.Transaction) error {
		documentRef := trainingsCollection.Doc(trainingUUID)

		firestoreTraining, err := tx.Get(documentRef)
		if err != nil {
			return errors.Wrap(err, "unable to get actual docs")
		}

		tr, err := r.unmarshalTraining(firestoreTraining)
		if err != nil {
			return err
		}
+
+		if err := training.CanUserSeeTraining(user, *tr); err != nil {
+			return err
+		}
+
		updatedTraining, err := updateFn(ctx, tr)
		if err != nil {
			return err
		}

		return tx.Set(documentRef, r.marshalTraining(updatedTraining))
	})
}

And… that’s all! Is there any way someone can misuse this? As long as the User is valid: no.

This approach is similar to the method presented in the DDD Lite introduction article. It’s all about creating code that can’t be misused.

This is what using UpdateTraining now looks like:

func (h ApproveTrainingRescheduleHandler) Handle(ctx context.Context, cmd ApproveTrainingReschedule) (err error) {
	defer func() {
		logs.LogCommandExecution("ApproveTrainingReschedule", cmd, err)
	}()

	return h.repo.UpdateTraining(
		ctx,
		cmd.TrainingUUID,
		cmd.User,
		func(ctx context.Context, tr *training.Training) (*training.Training, error) {
			originalTrainingTime := tr.Time()

			if err := tr.ApproveReschedule(cmd.User.Type()); err != nil {
				return nil, err
			}

			err := h.trainerService.MoveTraining(ctx, tr.Time(), originalTrainingTime)
			if err != nil {
				return nil, err
			}

			return tr, nil
		},
	)
}

Of course, there are still some rules if Training can be rescheduled, but this is handled by the Training domain type. It’s covered in details in the DDD Lite introduction article. 😉

Handling collections

Even though this approach works perfectly for operating on a single training, you need to ensure that access to a collection of trainings is properly secured. There’s no magic here:

func (r TrainingsFirestoreRepository) FindTrainingsForUser(ctx context.Context, userUUID string) ([]query.Training, error) {
	query := r.trainingsCollection().Query.
		Where("Time", ">=", time.Now().Add(-time.Hour*24)).
		Where("UserUuid", "==", userUUID).
		Where("Canceled", "==", false)

	iter := query.Documents(ctx)

	return r.trainingModelsToQuery(iter)
}

Doing this at the application layer with the CanUserSeeTraining function would be very expensive and slow. It’s better to create a bit of logic duplication.

If this logic is more complex in your application, you can try abstracting it in the domain layer to a format that you can convert to query parameters in your database driver. I did this once, and it worked pretty nicely.

But in Wild Workouts, that would add unnecessary complexity. Let’s Keep It Simple, Stupid.

Handling internal updates

We often want to have endpoints that allow a developer or your company’s operations department to make some “backdoor” changes. The worst thing you can do in this case is create any kind of “fake user” and hacks.

From my experience, this ends with a lot of if statements added to the code. It also obfuscates the audit log (if you have one). Instead of a “fake user”, it’s better to create a special role and explicitly define that role’s permissions.

If you need repository methods that don’t require any user (for Pub/Sub message handlers or migrations), it’s better to create separate repository methods. In that case, naming is essential: we need to be sure that the person who uses that method understands the security implications.

From my experience, if updates are becoming much different for different actors, it’s worth introducing separate CQRS Commands per actor. In our case, it might be UpdateTrainingByOperations.

Passing authentication via context.Context

As far as I know, some people are passing authentication details via context.Context.

I highly recommend not passing anything required by your application to work correctly via context.Context. The reason is simple: when passing values via context.Context, we lose one of Go’s most significant advantages: static typing. It also hides what exactly the input for your functions is.

If you need to pass values via context for some reason, it may be a symptom of bad design somewhere in your service. Maybe the function is doing too much, and it’s hard to pass all arguments there? Perhaps it’s time to decompose it?

And that’s all for today!

As you can see, the presented approach is straightforward to implement quickly.

I hope that it will help you with your project and give you more confidence in future development.

Do you see that it can help in your project? Do you think that it may help your colleagues? Don’t forget to share it with them!