Events received from the Bus should identify the user who published them

[dustmop]

Problem Statement

We want to have events sent on the event.Bus to have user's profileID attached to them. This way, subsystems that receive events, in a multi-tenant system, know which user performed them, and thus how and where to apply them.

One of the first places we're running into this is the collection.Set, which receives an event when a dataset is deleted. It wants to know which user deleted it, and only remove it from the appropriate collection list.

Scope questions

Is this a security problem?

Or just about code convenience, and avoiding error prone code

What are the use cases?

Example: are we worried about a user interacting with a multi-tenant instance of core, using a modified qri instance to delete other users data?

That would imply that published events need to be signed and authenticated, as if they were API calls.

I'm assuming the answer here is no, and rather it's a code convenience issue. Because otherwise solving it is out of scope and needs a larger design.

(or rather, the answer is not yet. we'll get to that eventually, but first just want the logic to work correctly)

Calling Code

What we want to avoid:

// Get the instance's global bus
bus := scope.Bus()

// do some work
...

// Send an event, make sure to include our profile ID
bus.Publish(
  MyEvent{
    ProfileID: scope.ActiveProfile().ID,
    Type: "update",
    Name: "my_ds",
  }
)

// do some more work
...

// Oops! Send another event but forgot the profile ID
bus.Publish(
  MyEvent{
    Type: "delete",
    Name: "my_ds"
  }
)

Above is how things currently work.

What we want:

bus := scope.Bus()

...

// The bus *itself* knows the user identity and adds the profileID
bus.Publish(
  MyEvent{
    Type: "delete",
    Name: "my_ds"
  }
)

The caller of this code just gets an interface, and isn't even aware that it's scoped to their user, and that events they send are id'd to them. In actually, they don't have the instance's global bus, but rather a struct which wraps the global bus, and always sends events with the scope's user's profileID.

In the current prototype branch is looks like this:

// Bus returns a wrapped bus that uses the scope's user identity
func (s *scope) Bus() event.Bus {
	if s.bus == nil {
		pid := s.ActiveProfile().ID
		s.bus = &perUserBus{
			profileID: pid,
			mainBus:   s.inst.bus,
		}
	}
	return s.bus
}

So, when code using this perUserBus implementation calls .Publish, the profileID is added to the event, and then sent to subscribers.

But how is this last part going to work?

Interfaces and structs

Event

Subscribers to the bus get an event that looks like this:

type Event struct {
	Type      Type
	Timestamp int64
	SessionID string
	Payload   interface{}
}

Seems clear we want to add a User profile.ID or something to this, which seems easy enough.

Bus

Time to look at the interface of Bus in event/event.go

type Publisher interface {
	Publish(context.Context, Type, payload interface{}) error
	PublishID(context.Context, Type, sessionID string, payload interface{}) error
}

The sessionID tends to be something like a runID.

What we want is to keep this above interface for code that uses the bus, to let callsites be ignorant of profileIDs. However, the implementation of the scoped perUserBus wants to be able to wrap the global bus, so it also needs to use this same interface.

Here is where the problem starts:

func (p *perUserBus) Publish(ctx context.Context, t event.Type, data interface{}) error {
	// Somehow need to add the profileID, and call
	// p.mainBus.publish, passing it in
	return fmt.Errorf("Implement me!")
}

What do we do to pass the profile.ID to this wrapped bus, while still keeping the interface the same for both. How to do so?

--

There are some options:

1. Add another method to type Bus interface

PublishSessionAndProfile(context.Context, Type, sessionID, profileID string, payload interface{})

Add another method to the interface, with a superset of all other fields, plus the profileID. The perUserBus (from scope) throws an error if this is called with an invalid profileId, but it will only ever call this method on p.mainBus sending along the scope's profileID.

2. A second interface:

type UserEventPublisher interface {
  PublishUserEvent(context.Context, Type, sessionID, profileID, payload)
}

This is implemented by the global bus, but doesn't exist on the actual type Bus interface. The implementation of perUserBus.Publish type asserts this interface and calls this method.

3. A special data type that wraps the payload

Create a type like:

type UserWrappedPayload struct {
  user profile.ID
  data interface{}
}

perUserBus.Publish wraps the payload it is given with this type. The implementation of Bus.publish will unwrap this value, assign EventID.User = payload.user, then continue publishing.

--

Interested to hear other's thoughts. I am leaning towards (2), but maybe there's another way.

[b5]

ok, here's an idea: what if we just used the profile IDs that are usually stored in the contexts we pass around for publishing?

here's an example from local collection:

	case event.ETDatasetDeleteAll:
		// TODO(b5): need user-scoped events for this, unless we want one user
		// removing a dataset to remove it from all collections of all users
		if initID, ok := e.Payload.(string); ok {
			pid := profile.IDFromCtx(ctx)
			if pid == "" {
				// define a new error in profile package so we can fish this out later if needed
				// returning an error here needs to interrupt the publisher anyway, so this error
				// won't get swallowed
				return profile.ErrNoUser
			}
			if err := s.deleteForUser(pid, initID); err != nil {
				log.Debugw("removing dataset for user collections", "initID", initID, "err", err)
			}
		}

we already construct and populate a context with a profile id on request creation as part of access control, so the only places where a profileID stored on a context currently isn't a hard requirement is within the automation package, and we can make it work that way relatively easily.

Upside: very few code changes. Downside: relies on a magic black box called ctx. But also: we can at least error within handlers if it isn't set

[dustmop]

Even better idea, inspired by your mention of context. The scoped perUserBus could assign the profileID to the context, and the main bus could get it from there to add it to the event struct that gets published. Kind of the best of all worlds.

[b5]

Love it. At that point do we even need the perUserBus struct? Or could the event package just pull the profile from context & populate the event struct field?

On Thu, Jul 29, 2021 at 6:46 PM Dustin Long @.***> wrote:

Even better idea, inspired by your mention of context. The scoped perUserBus could assign the profileID to the context, and the main bus could get it from there to add it to the event struct that gets published. Kind of the best of all worlds.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/qri-io/qri/issues/1850#issuecomment-889508911, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIZ2VQ3TUWYHQAEVS3I7Q3T2HK5TANCNFSM5BHB3FFA .

--

Brendan O'Brien caretaker, qri.io twitter.com/b_fiive https://calendly.com/b_five

[dustmop]

Seems like you're correct. perUserBus is no longer needed. The desired functionality matches very closely to how automation is adding the profile.ID to the context, so I refactored that part first: https://github.com/qri-io/qri/pull/1859/