o/state,daemon: add snap-run-inhibit notice #13769

ZeyadYasser · 2024-03-28T18:30:36Z

This PR adds the snap-run-inhibit notice which is need for the "snap-run during refresh UX" flow described in SD167.

This is mostly a port of custom notices implementation from pebble canonical/pebble#296 adapted for the snap-run-inhibit notice.

This PR is a building block for #13770.

codecov-commenter · 2024-03-28T18:55:20Z

Codecov Report

Attention: Patch coverage is 70.00000% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 78.85%. Comparing base (fe5d801) to head (6b714e6).
Report is 5 commits behind head on master.

Files	Patch %	Lines
daemon/api_notices.go	69.62%	16 Missing and 8 partials ⚠️
daemon/snap.go	70.00%	2 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13769      +/-   ##
==========================================
- Coverage   78.86%   78.85%   -0.02%     
==========================================
  Files        1043     1043              
  Lines      134549   134681     +132     
==========================================
+ Hits       106109   106199      +90     
- Misses      21828    21857      +29     
- Partials     6612     6625      +13

Flag	Coverage Δ
unittests	`78.85% <70.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

olivercalder

Thanks for your work on this. I'm rather skeptical of the approach of allowing the creation of "snap-run-inhibit" notice by posting to the notices endpoint from outside of snapd. This seems to me like something which should be determined internally by snapd. As it is now, any client/user would be able to create a "snap-run-inhibit" notice by simply posting to the notices API and accepting the polkit prompt, as far as I understand it.

It really feels to me like snapd should be responsible for deciding when the "snap-run-inhibit" notice should be issued. Maybe that means posting to another API endpoint, which then tells snapd to record the notice, but posting to the notices endpoint should record a notice without any complicated checks beyond validating the type and data format. I think for this notice type deeper checks would seem to be required, which feels beyond the scope of posting to the notices endpoint, in my opinion.

Perhaps I'm misunderstanding though. Is there a reason we really need to post to the notices endpoint for this use case?

olivercalder · 2024-03-29T02:33:29Z

daemon/api_notices.go

+		GET:         getNotices,
+		POST:        postNotices,
+		ReadAccess:  interfaceOpenAccess{Interface: "snap-refresh-observe"},
+		WriteAccess: authenticatedAccess{Polkit: polkitActionManage},


I'm a bit unsure as to whether polkit authentication makes sense here... if I understand correctly, these notices are issued by the snap client when a snap is run, to inhibit refreshes? In that case, I don't think it makes sense to send a polkit prompt to the user when they try to run the snap.

The usage is confusing to me too: 63634cc#diff-2040a7ab4147eafb94c9b58c40202df922bd298e0ec3d152e322cef32f051db5R146-R163

Here, it checks whether the connection has the "snap-refresh-observe" interface connected, but the notice endpoint itself is on the usual snapd.socket, not snapd-snap.socket, and doesn't require a particular interface? I know I'm misunderstanding some of how connections work in this case, but it still seems strange to me.

I'm a bit unsure as to whether polkit authentication makes sense here

Good point regarding polkit, The polkit prompting doesn't make sense in this use case, I will remove it.

if I understand correctly, these notices are issued by the snap client when a snap is run, to inhibit refreshes?

No, They are issued by the snap client to announce that it is being inhibited from running due to an ongoing refresh, so that a desktop client (specifically snapd-desktop-integration snap) can listen for this notice so it can display notifications or refresh progress as described in SD167.

Here, it checks whether the connection has the "snap-refresh-observe" interface connected, but the notice endpoint itself is on the usual snapd.socket, not snapd-snap.socket, and doesn't require a particular interface?

Thanks for pointing this out, It does look weird without context, It warrants a comment explaining it.
This marker interface check is just to be able do UX fallback:

if no one is listening for the notices (i.e. snap-refresh-observe marker interface is not connected), snap run sends the notifications itself (63634cc#diff-2040a7ab4147eafb94c9b58c40202df922bd298e0ec3d152e322cef32f051db5R115-R131).

We could send the notice regardless of the interface check (which actually might be better for readability and consistency) that's why it is not relevant to the API implementation, but I just chose the route with less API calls.

I don't think we want authentication here as such

Updated, Thank you!

olivercalder · 2024-03-29T02:38:07Z

daemon/api_notices.go

@@ -218,6 +230,53 @@ func allowedNoticeTypesForInterface(iface string) []state.NoticeType {
 	return types
 }

+func postNotices(c *Command, r *http.Request, user *auth.UserState) Response {


If at all possible, I'd like to avoid being able to post to notices, at least for "snap-run-inhibit" notices. As far as I understand, this notice type is something produced from within snapd that a user or non-snapd client should never need to issue, so I don't think it makes sense to be created via the API. Then, a user could create a "snap-run-inhibit" notice if they post to the snapd API, triggering a polkit prompt, and it would be nonsensical.

As far as I understand, this notice type is something produced from within snapd

Not really, snap-run-inhibit indicate that snap run couldn't start because of an ongoing refresh. snapd have no way of telling when snap run run is called.

Thank you for pointing this out, it means that I should document this better.

olivercalder · 2024-03-29T02:38:46Z

daemon/api_notices.go

+	if payload.Action != "add" {
+		return BadRequest("invalid action %q", payload.Action)
+	}
+	if payload.Type != "snap-run-inhibit" {


It makes more sense in pebble, where the "custom" notice type is the only notice which can be created, and it should contain well-namespaced custom data.

I don't think "snap-run-inhibit" makes sense in the same way as a notice that any sufficiently-privileged user can create.

Actually, we were going to use custom notices for this, but the fact we had to stuff the notice type (snap run is inhibited) and key (inhibited instance name) in the custom notice key (e.g. key="snap-run-inhibited/firefox") meant that a listener (i.e. snapd-desktop-integration) won't have a way to filter for snap-run-inhibited events since we don't have pattern based matching for notices (yet). Also, we wanted to be less flexible with custom notices since it doesn't make sense for snapd, unlike pebble.

Basically, it is custom notices, but with a type field.

olivercalder

Some thoughts after discussing with @ZeyadYasser:

I don't think we want to use authenticatedAccess on any http method of the /v2/notices endpoint
- Notices should be open to anyone (up to user and interface requirements) to read, and recording notices should be similarly open
- To do otherwise would preclude future custom notice types which applications should be able to issue without authentication
- Thus, if we want to add a POST /v2/notices endpoint, it should be openAccess or interfaceOpenAccess
Since "snap-run-inhibit" notices are supposed to tell something which is reliably true about the fact that a snap cannot currently be run, a non-privileged application (snap or not) should not be able to erroneously or maliciously create a "snap-run-inhibit" notice by simply posting to the open notices API
- I believe this would be less verifiable than the current system of snap run sending a dbus message to snapd-desktop-integration, but even if not, I think we can do better at ensuring that the "snap-run-inhibit" notice is created because of snap run, not a third party
- I think snapd should verify this as well as possible, and then add the "snap-run-inhibit" notice directly/internally
I would propose creating a dedicated snapd API endpoint for the snap client to tell snapd that snap run is inhibited, and then snapd should be responsible for recording the "snap-run-inhibit" notice
- This endpoint could be, for example, /v2/snaps/{name}/inhibit
  - Here, the snap name would be included in the path, and would not need to be parsed from the request body
- Such an endpoint should be authenticatedAccess, and snapd should verify that the request comes from the snap client, not a third party
  - No polkit authentication, probably should use a macaroon, but I'm not sure how those work
    - Is authenticatedAccess with a macaroon for authentication possible, and if so, what makes it meaningfully more secure than openAccess?
- If possible, it would be great if snapd could verify that there is actually a refresh occurring for the snap in question, and if so, then it creates the "snap-run-inhibit" notice on behalf of the snap client
- This way, POST /v2/notices would still be available in the future if we want to add more permissive notice types that users/applications can create at will
- In the meantime, we can remove POST /v2/notices, client/notices.go, etc.

I'd very much appreciate hearing what @pedronis thinks of this approach, as it does contradict some of the existing RAA spec.

pedronis · 2024-04-02T11:26:39Z

@olivercalder @ZeyadYasser

the idea was always to allow to create this notice only at least if the requester /proc/PID/exe points to a snapd "snap" command, there might be more sophisticated possible checks, but that would be the minimum

ZeyadYasser · 2024-04-03T19:18:17Z

@pedronis @olivercalder Added the check for requester /proc/PID/exe as suggested here #13769 (comment)

ZeyadYasser · 2024-04-03T19:26:55Z

Rebased to resolve conflicts as they were preventing tests from running.

olivercalder

Looks really good, thanks! I think given the very special handling of "only run inhibit notice" and "only snap client", we could use a more specialized access checker than openAccess. What would you think of this @pedronis ?

olivercalder · 2024-04-03T19:36:55Z

daemon/api_notices.go

+		GET:         getNotices,
+		POST:        postNotices,
+		ReadAccess:  interfaceOpenAccess{Interfaces: []string{"snap-refresh-observe"}},
+		WriteAccess: openAccess{},


I think perhaps a different type of access, something like snapClientAccess{} might be appropriate here? Again, for the time being, we don't want to allow anyone outside of snapd/the snap client to be able to create notices. Your solution of checking /proc/<pid>/exe works, but the endpoint being openAccess suggests to me that anyone should be able to create notices of any type, neither of which are true.

Something like snapClientAccess could implement the check you added below while maintaining that this behaves differently from the way openAccess works in other places, (afaik) without restrictions about the caller (beyond it not being a snap).

I slightly disagree with forcing something like snapClientAccess{} on POST /v2/notices. We cannot rule out the possibility of having other notices that are generated outside the snap command.

I think of it more as validation per notice type, it just happens that we have only one notice type for now.

That's a good point. Furthermore, in the future, we may wish to allow snaps to create notices, in which case it would become interfaceOpenAccess, which is definitely not compatible with a snapClientAccess. Using openAccess as you suggest is analogous to the way interfaceOpenAccess grants access only to particular notice types depending on the connected interface, so there is precedent as well.

So +1 from me for your current approach. Thanks!

How is this endpoint currently protected in pebble?

@Meulengracht It is not protected, it uses an access checker that similar to openAccess.

olivercalder · 2024-04-03T19:39:22Z

daemon/api_notices.go

+	return SyncResponse(addedNotice{ID: noticeId})
+}
+
+func requestFromSnapCmd(st *state.State, r *http.Request) (bool, error) {


This is a good check, thanks! I think it would be great as a general check for a new access checker like snapClientAccess{}.

I think isRequestFrom would be a bit a clearer name for this, also it migh belong to access.go

daemon/api_notices_test.go

olivercalder

Looks good to me, thanks for the discussion.

olivercalder · 2024-04-04T18:05:59Z

daemon/api_notices.go

+		GET:         getNotices,
+		POST:        postNotices,
+		ReadAccess:  interfaceOpenAccess{Interfaces: []string{"snap-refresh-observe"}},
+		WriteAccess: openAccess{},


That's a good point. Furthermore, in the future, we may wish to allow snaps to create notices, in which case it would become interfaceOpenAccess, which is definitely not compatible with a snapClientAccess. Using openAccess as you suggest is analogous to the way interfaceOpenAccess grants access only to particular notice types depending on the connected interface, so there is precedent as well.

So +1 from me for your current approach. Thanks!

Meulengracht

I did a first pass

Meulengracht · 2024-04-05T07:19:20Z

daemon/api_notices.go

+		GET:         getNotices,
+		POST:        postNotices,
+		ReadAccess:  interfaceOpenAccess{Interfaces: []string{"snap-refresh-observe"}},
+		WriteAccess: openAccess{},


How is this endpoint currently protected in pebble?

Meulengracht · 2024-04-05T07:24:28Z

daemon/api_notices.go

+	var payload struct {
+		Action string `json:"action"`
+		Type   string `json:"type"`
+		Key    string `json:"key"`
+		// NOTE: Data and RepeatAfter fields are not needed for snap-run-inhibit notices.
+	}
+	decoder := json.NewDecoder(r.Body)
+	if err := decoder.Decode(&payload); err != nil {
+		return BadRequest("cannot decode request body: %v", err)
+	}


This is definitely not a nice solution - this distracts from the purpose of the postNotices handler, and creates a lot of noise. Please move var payload struct out of the function, give it a more meaningful name and a docstring explaining why it's needed/purpose, and then create a function called decode<>AndValidate, where you move the validation logic to as well. Having a separate decode and validate function here is acceptable as well.

this code is like this in pebble:

https://github.com/canonical/pebble/blob/master/internals/daemon/api_notices.go#L200

I have less of a strong opinion on this, as the struct is not used across many endpoints. Anyway if we change this we would need to backport the change to pebble

this style is used in a few places in daemon fwiw

This is an exact backport from pebble as @pedronis mentioned. I believe for snapd it makes more sense to have the decode<>AndValidate approach because we might have other notices (e.g. custom notices someday), but in I don't believe it make sense for pebble since the postNotices endpoint whole purpose is to support custom notices, it was built for specific purpose.

I understand the appeal of using a pattern that works for both snapd and pebble, but I think input from @benhoyt would be appreciated.

Updated, thanks!

Meulengracht · 2024-04-05T07:27:29Z

daemon/api_notices.go

+	fromSnapCmd, err := requestFromSnapCmd(st, r)
+	if err != nil {
+		return InternalError("cannot check request source")
+	}
+	if !fromSnapCmd {
+		return Forbidden("only snap command can record notices")
+	}


Since they aren't used further down, we can scope the variables here

Suggested change

fromSnapCmd, err := requestFromSnapCmd(st, r)

if err != nil {

return InternalError("cannot check request source")

}

if !fromSnapCmd {

return Forbidden("only snap command can record notices")

}

if fromSnapCmd, err := requestFromSnapCmd(st, r); err != nil {

return InternalError("cannot check request source")

} else if !fromSnapCmd {

return Forbidden("only snap command can record notices")

}

Meulengracht · 2024-04-05T07:27:54Z

daemon/api_notices.go

+	exists, err := snapInstanceExists(st, payload.Key)
+	if err != nil {
+		return InternalError("cannot check snap in state: %v", err)
+	}
+	if !exists {
+		return BadRequest("snap %q does not exist", payload.Key)
+	}


Same here

Suggested change

exists, err := snapInstanceExists(st, payload.Key)

if err != nil {

return InternalError("cannot check snap in state: %v", err)

}

if !exists {

return BadRequest("snap %q does not exist", payload.Key)

}

if exists, err := snapInstanceExists(st, payload.Key); err != nil {

return InternalError("cannot check snap in state: %v", err)

} else if !exists {

return BadRequest("snap %q does not exist", payload.Key)

}

Meulengracht · 2024-04-05T07:31:17Z

daemon/api_notices.go

+	maxNoticeKeyLength = 256
+)
+
+type addedNotice struct {


This needs a docstring

Good point, thanks! updated

Meulengracht · 2024-04-05T07:32:07Z

daemon/api_notices.go

@@ -47,6 +53,14 @@ var (
 	}
 )

+const (
+	maxNoticeKeyLength = 256


Should this be a more common place for notices? I'm not sure I feel like this would be the correct place to have it. Also do we have some more notice-key validation logic somewhere it could fit?

This is an exact backport from pebble https://github.com/canonical/pebble/blob/master/internals/daemon/api_notices.go#L37.

We only check that the key is not empty. I don't mind moving the key validation there but would we need to backported it pebble as well?
https://github.com/snapcore/snapd/blob/8e088a787eee8e041780fc3848f72442f5d7ba0d/overlord/state/notices.go#L282-L293

I moved the validation to state/notices.go and shared it with the daemon, I like this better, Thanks!

pedronis

did a first pass

pedronis · 2024-04-05T11:32:32Z

daemon/api_notices.go

+		return BadRequest("invalid action %q", payload.Action)
+	}
+	if payload.Type != "snap-run-inhibit" {
+		return BadRequest(`invalid type %q (can only add "snap-run-inhibit" notices)`, payload.Type)


this error is probably confusing, we can only decide what to do with the type once we have check the command things are coming from below

Good point, Thanks! updated to move the check up.

pedronis · 2024-04-05T11:34:22Z

daemon/api_notices.go

+	return SyncResponse(addedNotice{ID: noticeId})
+}
+
+func requestFromSnapCmd(st *state.State, r *http.Request) (bool, error) {


I think isRequestFrom would be a bit a clearer name for this, also it migh belong to access.go

pedronis · 2024-04-05T11:36:02Z

daemon/request.go

 	"time"
 )

+var osReadlink = os.Readlink


unless it is used in multiple places this should go in the file that needs it

good point, thanks! updated

bboozzoo · 2024-04-08T10:48:30Z

daemon/access.go

+		return false, err
+	}
+	if err == nil {
+		snapdPath := fmt.Sprintf("snapd/%s/usr/bin/snap", snapst.CurrentSideInfo().Revision)


does it have to be current, or would any installed revision be acceptable? i.e. what if snapd is refreshing just now.

Good point, Thank you! Would it be better to use a wildcard here?

CC: @zyga @pedronis

a wildcard would be safer yes, given our purposes. The doc comment of the helper should probably clarify that this not a security-oriented check.

updated to use wildcard matching, great catch, thanks!

pedronis

couple small comments

pedronis · 2024-04-08T16:46:50Z

daemon/access.go

+		return false, err
+	}
+	if err == nil {
+		snapdPath := fmt.Sprintf("snapd/%s/usr/bin/snap", snapst.CurrentSideInfo().Revision)


a wildcard would be safer yes, given our purposes. The doc comment of the helper should probably clarify that this not a security-oriented check.

pedronis · 2024-04-08T16:49:46Z

overlord/state/notices.go

 	if !noticeType.Valid() {
-		return fmt.Errorf("internal error: attempted to add notice with invalid type %q", noticeType)
+		return fmt.Errorf("attempted to add notice with invalid type %q", noticeType)


I think all the "attempted to" here should be turned into "cannot" if we remove the "internal error" prefix

updated, thanks!

pedronis

thank you

bboozzoo · 2024-04-09T09:48:10Z

daemon/api_notices.go

+		return Forbidden("only snap command can record notices")
+	}
+
+	if exists, err := snapInstanceExists(st, inst.Key); err != nil {


by the time we reach this point, notice key is assumed to be a snap name, so perhaps this should run naming.ValidateInstance(inst.Key)

just FYI, feel free to push this change in a followup

Meulengracht

Thank you

@olivercalder

* daemon: remove polkit check from POST /v2/notices (thanks @olivercalder) * o/state: update snap-run-inhibit notice doc comment * daemon: allow snap command only to add snap-run-inhibit notices Check that the call to /v2/notices to create snap-run-inhibit notices is coming from an expected source by checking the symlink target of /proc/<PID>/exe is one of the known locations: - /usr/bin/snap - /snap/snapd/current/usr/bin/snap - /snap/core/current/usr/bin/snap * daemon/api_notices_test.go: fix typo (thanks @olivercalder) * daemon: address review comments * daemon: extract validation out of postNotices * o/state,daemon: use common notice validation (thanks @Meulengracht) * p/state,daemon: s/attempted to/cannot/ in notice validation errors (thanks @pedronis) * daemon: use wildcard matching for snap binary location (thanks @bboozzoo) * daemon: simplify snap-run-inhibit snap validation * daemon: remove unused snapInstanceExists Signed-off-by: Zeyad Gouda <[email protected]>

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Mar 28, 2024

ZeyadYasser force-pushed the add-snap-run-inhibit-notice branch from 95c405e to 0e74216 Compare March 28, 2024 18:35

ZeyadYasser marked this pull request as ready for review March 28, 2024 18:38

ZeyadYasser mentioned this pull request Mar 28, 2024

cmd/snap: record snap-run-inhibit notice #13770

Merged

ZeyadYasser requested a review from olivercalder March 28, 2024 18:41

ZeyadYasser changed the title ~~cmd/snap,client: record snap-run-inhibit notice~~ o/state,daemon: add snap-run-inhibit notice Mar 28, 2024

ZeyadYasser requested a review from Meulengracht March 28, 2024 18:43

olivercalder requested changes Mar 29, 2024

View reviewed changes

ZeyadYasser added this to the 2.63 milestone Mar 29, 2024

olivercalder self-requested a review March 29, 2024 15:57

olivercalder reviewed Mar 29, 2024

View reviewed changes

ZeyadYasser requested a review from pedronis April 1, 2024 08:44

ZeyadYasser added the Needs Samuele review Needs a review from Samuele before it can land label Apr 1, 2024

ZeyadYasser mentioned this pull request Apr 1, 2024

many: refresh-app-awareness UX improvement/enablement [WIP] #13066

Closed

30 tasks

ZeyadYasser force-pushed the add-snap-run-inhibit-notice branch from a9d2bff to 2589e42 Compare April 3, 2024 19:26

ZeyadYasser requested a review from olivercalder April 3, 2024 19:27

olivercalder reviewed Apr 3, 2024

View reviewed changes

ZeyadYasser requested a review from olivercalder April 4, 2024 12:37

olivercalder approved these changes Apr 4, 2024

View reviewed changes

Meulengracht reviewed Apr 5, 2024

View reviewed changes

pedronis reviewed Apr 5, 2024

View reviewed changes

pedronis requested a review from bboozzoo April 5, 2024 11:37

bboozzoo reviewed Apr 8, 2024

View reviewed changes

ZeyadYasser requested review from Meulengracht and pedronis April 8, 2024 11:07

pedronis reviewed Apr 8, 2024

View reviewed changes

ZeyadYasser requested review from bboozzoo and pedronis April 8, 2024 18:41

pedronis approved these changes Apr 9, 2024

View reviewed changes

bboozzoo reviewed Apr 9, 2024

View reviewed changes

Meulengracht approved these changes Apr 9, 2024

View reviewed changes

ZeyadYasser force-pushed the add-snap-run-inhibit-notice branch 2 times, most recently from 9825be8 to 56b0f52 Compare April 9, 2024 10:28

ZeyadYasser force-pushed the add-snap-run-inhibit-notice branch from 56b0f52 to 2e63a5a Compare April 9, 2024 10:54

Meulengracht merged commit 57624c4 into canonical:master Apr 10, 2024
42 of 44 checks passed

o/state,daemon: add snap-run-inhibit notice #13769

o/state,daemon: add snap-run-inhibit notice #13769

Conversation

ZeyadYasser commented Mar 28, 2024 • edited Loading

codecov-commenter commented Mar 28, 2024 • edited Loading

Codecov Report

olivercalder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivercalder left a comment

Choose a reason for hiding this comment

pedronis commented Apr 2, 2024

ZeyadYasser commented Apr 3, 2024

ZeyadYasser commented Apr 3, 2024

olivercalder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivercalder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meulengracht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meulengracht left a comment

Choose a reason for hiding this comment

ZeyadYasser commented Mar 28, 2024 •

edited

Loading

codecov-commenter commented Mar 28, 2024 •

edited

Loading

ZeyadYasser Apr 4, 2024 •

edited

Loading

ZeyadYasser Apr 8, 2024 •

edited

Loading

ZeyadYasser Apr 8, 2024 •

edited

Loading