RSDK-14414 Add UserPermission list to AuthConfig

[Benjamin Rewis (benjirewis)]

RSDK-14414

Adds the API specified in the scope.

[Daniel Botros (danielbotros)]

LGTM

[Josh Matthews (jmatth)]

If this is a known set should it be an enum instead of stringly typed?

Could also do a oneof but the code that generates has a kind of disguisting level of nesting:

Oneof with sub messages

message UserPermission {
  repeated Permission permissions = 1;

  message EmailUser {
    string email = 1;
  }

  message APIKeyUser {
    string key = 1;
  }

  message DefaultUser {}

  // user is the User this UserPermission applies to. A User can only be
  // listed in a single UserPermission for a set of UserPermissions.
  oneof user {
    EmailUser email = 2;
    APIKeyUser api = 3;
    DefaultUser default = 4;
  }
}

func test() {
	up := UserPermission{
		User: &UserPermission_Api{
			Api: &UserPermission_APIKeyUser{
				Key: "",
			},
		},
	}
}

Some of that nesting can be removed by not defining custom messages but that would make it annoying to update if the api case needed extra fields in the future or something:

Oneof with a level of nesting removed

message UserPermission {
  repeated Permission permissions = 1;

  // user is the User this UserPermission applies to. A User can only be
  // listed in a single UserPermission for a set of UserPermissions.
  oneof user {
    string email = 2;
    string api = 3;
    google.protobuf.Empty default = 4;
  }
}

func test() {
	up := UserPermission{
		User: &UserPermission_Api{
			Api: "",
		},
	}
}

I really want oneof to be useful but enum is probably good enough.

[Benjamin Rewis (benjirewis)]

It's nice to restrict the app.viam.com -> viam-server communication to the set of known user types.

The API I have now does mirror the proposed JSON, which is a property I'd like to maintain:

"user_permissions": [
    {
      "user": { "type": "email", "id": "benji@viam.com" },
      "permissions": [
        {
          "resources": ["cam1", "cam2"],
          "allowed_methods": ["/viam.component.camera.v1.CameraService/GetImages"]
        }
      ]
    },
    {
      "user": { "type": "api-key-id", "id": "c6f77790-5405-488a-9c9c-9612402fb9b0" },
      "permissions": [
        {
          "resources": ["sensor1"],
          "allowed_methods": ["/viam.component.sensor.v1.SensorService/GetReadings"]
        }
      ]
    },
    {
      "user": { "type": "default" },
      "permissions": [
        {
          "resources": ["_machine"],
          "allowed_methods": ["/viam.robot.v1.RobotService/ResourceNames"]
        }
      ]
    }
  ]

We could do something like this to mimic your proposed API:

"user_permissions": [
    {
      "email": "benji@viam.com",
      "permissions": [
        {
          "resources": ["cam1", "cam2"],
          "allowed_methods": ["/viam.component.camera.v1.CameraService/GetImages"]
        }
      ]
    },
    {
      "api_key_id": "c6f77790-5405-488a-9c9c-9612402fb9b0",
      "permissions": [
        {
          "resources": ["sensor1"],
          "allowed_methods": ["/viam.component.sensor.v1.SensorService/GetReadings"]
        }
      ]
    },
    {
      "default": true,
      "permissions": [
        {
          "resources": ["_machine"],
          "allowed_methods": ["/viam.robot.v1.RobotService/ResourceNames"]
        }
      ]
    }
  ]

But the value of default is weird there (could also be {} 🤷🏻 ), and I don't love that there's nothing really stopping you from specifying both email and api_key_id (and maybe thinking that could represent a single user's email and API key ID or something).

We have to render user_permissions in both JSON and Proto, so I'd like to keep their formats as close as possible so that there's very little transformation logic between JSON -> Golang -> Proto, e.g..

[Josh Matthews (jmatth)]

I don't think it's actually worth the effort to use oneof but the technical discussion is interesting, so: a good JSON version of using a oneof would probably look more like this:

Oneof json representation

"user_permissions": [
    {
      "user": {
        "_t": "email",
        "email": "benji@viam.com"
      },
      "permissions": [
        {
          "resources": ["cam1", "cam2"],
          "allowed_methods": ["/viam.component.camera.v1.CameraService/GetImages"]
        }
      ]
    },
    {
      "user": {
        "_t": "key",
        "api_key_id": "benji@viam.com"
      },
      "permissions": [
        {
          "resources": ["sensor1"],
          "allowed_methods": ["/viam.component.sensor.v1.SensorService/GetReadings"]
        }
      ]
    },
    {
      "user": {
        "_t": "default"
      },
      "permissions": [
        {
          "resources": ["_machine"],
          "allowed_methods": ["/viam.robot.v1.RobotService/ResourceNames"]
        }
      ]
    }
  ]

Here _t acts as a discriminator so code can figure out which type to deserialize to without guessing based on field name and presence. Unfortunately it doesn't look proto's built in json support does thing this way.

I do think it would be worth using an enum, and protos json support uses the field name rather than integer value by default, at least in go:

{
  "user": {
    "type": "USER_TYPE_EMAIL",
    "id": "josh.matthews@viam.com"
  }
}

That said, I don't feel strongly enough to actually hold up this PR.

[Josh Matthews (jmatth)]

Sorry, wrong button on that last one

[Josh Matthews (jmatth)]

I don't think it's actually worth the effort to use oneof but the technical discussion is interesting, so: a good JSON version of using a oneof would probably look more like this:

Oneof json representation

"user_permissions": [
    {
      "user": {
        "_t": "email",
        "email": "benji@viam.com"
      },
      "permissions": [
        {
          "resources": ["cam1", "cam2"],
          "allowed_methods": ["/viam.component.camera.v1.CameraService/GetImages"]
        }
      ]
    },
    {
      "user": {
        "_t": "key",
        "api_key_id": "benji@viam.com"
      },
      "permissions": [
        {
          "resources": ["sensor1"],
          "allowed_methods": ["/viam.component.sensor.v1.SensorService/GetReadings"]
        }
      ]
    },
    {
      "user": {
        "_t": "default"
      },
      "permissions": [
        {
          "resources": ["_machine"],
          "allowed_methods": ["/viam.robot.v1.RobotService/ResourceNames"]
        }
      ]
    }
  ]

Here _t acts as a discriminator so code can figure out which type to deserialize to without guessing based on field name and presence. Unfortunately it doesn't look proto's built in json support does thing this way.

I do think it would be worth using an enum, and protos json support uses the field name rather than integer value by default, at least in go:

{
  "user": {
    "type": "USER_TYPE_EMAIL",
    "id": "josh.matthews@viam.com"
  }
}

That said, I don't feel strongly enough to actually hold up this PR.

[viam-overwatch]

Hey Benjamin Rewis (@benjirewis) — this PR has been approved and CI has been green for 4+ business days. Ready to merge?

Auto-comment from overwatch. Will not re-nudge for 7 days.