Skip to content
This repository has been archived by the owner on Dec 18, 2024. It is now read-only.

Enforce permissions #507

Merged

Conversation

argerus
Copy link
Contributor

@argerus argerus commented Mar 12, 2023

Parsing JWT access tokens and making them available as Permissions in the GRPC handling code of databroker was previously implemented in #501.

This PR adds enforcement of these permissions throughout databroker.

This is accomplished by introducing two new interfaces DatabaseReadAccess and DatabaseWriteAccess that can only be created by supplying a Permissions struct.

All functions that read / write or create entries can only be accessed through these interfaces (structs) and that's where the enforcement happens.

In addition, authorized_access(Permissions) -> AuthorizedAccess has been added to the top level DataBroker interface in order to provide a convenient way to set the permissions once, and use the wrapper functions (in AuthorizedAccess) to interact with the previously mentioned interfaces.

@argerus argerus force-pushed the topic/implement_authorization branch 3 times, most recently from 3e4897b to 5f04343 Compare March 13, 2023 10:54
@argerus argerus marked this pull request as ready for review March 13, 2023 10:57
@argerus argerus force-pushed the topic/implement_authorization branch from 5f04343 to 6889fbf Compare March 13, 2023 12:56
Copy link
Contributor

@lukasmittag lukasmittag left a comment

Choose a reason for hiding this comment

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

Error output varies (only loveliness issue): Error setting id 51: AccessDenied ehicle.Body.Lights.IsRunningOn: ( AccessDenied ) Error setting Vehicle.Body.Trunk.Front.IsOpen: AccessDenied

kuksa_databroker/databroker/src/broker.rs Show resolved Hide resolved
kuksa_databroker/databroker/src/broker.rs Outdated Show resolved Hide resolved
kuksa_databroker/databroker/src/broker.rs Show resolved Hide resolved
@argerus argerus force-pushed the topic/implement_authorization branch from 6fd42e8 to 985f231 Compare March 13, 2023 20:52
@argerus
Copy link
Contributor Author

argerus commented Mar 13, 2023

Error output varies (only loveliness issue): Error setting id 51: AccessDenied ehicle.Body.Lights.IsRunningOn: ( AccessDenied ) Error setting Vehicle.Body.Trunk.Front.IsOpen: AccessDenied

That's an artifact of the fact that the Collector and Broker interfaces use path and i32 respectively to refer to the entries / datapoints / signals.

Anyway, I changed the error messages, since it's a lot more user friendly to refer to them by path.

@argerus argerus force-pushed the topic/implement_authorization branch from 985f231 to 8e77419 Compare March 15, 2023 12:56
argerus added 3 commits March 20, 2023 15:31
Parsing JWT access tokens and making them available as `Permissions` in
the GRPC handling code of databroker was previously implemented.

This commit adds the enforcement of these permissions throughout databroker.

This was accomplished by introducing two new interfaces
`DatabaseReadAccess` and `DatabaseWriteAccess` that can only be created
by supplying a `Permissions` struct.

All functions that read / write or create entries can only be accessed
through these interfaces (structs).

In addition, `authorized_access(Permissions) -> AuthorizedAccess` has
been added to the top level `DataBroker` interface in order to provide a
convenient way to set the permissions once, and use the wrapper
functions (in `AuthorizedAccess`) to interact with the previously
mentioned interfaces.
@argerus argerus changed the title Enforce authorization Enforce permissions Mar 20, 2023
@argerus argerus force-pushed the topic/implement_authorization branch from 8e77419 to 0044463 Compare March 20, 2023 14:40
@argerus argerus requested a review from lukasmittag March 20, 2023 15:09

// Create error stream (to be returned)
let (error_sender, error_receiver) = mpsc::channel(10);

// Listening on stream
tokio::spawn(async move {
let permissions = permissions;
let broker = broker.authorized_access(&permissions);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the naming is a bit confusing here. I see what you did, but the change in 102 did confuse me at first.

}

#[inline]
pub fn expired(&self) -> Result<(), PermissionError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this function be above the first use of it? For reading and understanding purposes.

@@ -64,6 +71,9 @@ async fn run_streaming_set_test(iterations: i32, n_th_message: i32) {
}
};

if datapoint1_id == -1 {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this be handeled in line 70?

@lukasmittag
Copy link
Contributor

Just did some minor comments. Nothing that is necessary so approved it. @SebastianSchildt now it's your turn :)

@SebastianSchildt SebastianSchildt merged commit a3fd887 into eclipse:master Mar 21, 2023
@erikbosch erikbosch deleted the topic/implement_authorization branch February 27, 2024 11:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants