server: Add service method Lock and Unlock implementation #150

warusadura · 2024-10-31T16:55:37Z

This change adds Secret Service method Lock and Unlock implementation.

warusadura · 2024-10-31T17:05:19Z

Service method Lock should emit PropertiesChanged signal for each Locked item and collection. Currently oo7_daemon can't do this with this change. Can you help me figure out how to do this. So far I have tried moving set methods (set_locked) inside the Service impl (inside interface attribute) and also marking them with zbus properties attribute. None of these approaches worked so far. @bilelmoussaoui

server/src/collection.rs

bilelmoussaoui · 2024-10-31T17:42:01Z

server/src/service.rs

    ) -> Result<(Vec<OwnedObjectPath>, ObjectPath), ServiceError> {
-        todo!()
+        let mut locked: Vec<OwnedObjectPath> = Vec::new();
+        let prompt = ObjectPath::default();


Use Prompt::default() so we can remove the Default impl later and track all the mis-usages left.

Yeah I agree. But, a prompt isn't required here, like at all. So, returning an empty objectpath: '/' is enough. And we don't have to worry about cleaning up later :)

Where did you got this information from ?

Based on three things:

There is nothing on the spec about keeping track of the objects that are locked with a prompt and a way to return them. This can be consider as a hint there is nothing related to actually prompting a system promt here.

gnome-keyring also returns an empty object path as the prompt: https://gitlab.gnome.org/GNOME/gnome-keyring/-/blob/master/daemon/dbus/gkd-secret-service.c#L609

I have tested this call many time. And never found any use case involving a prompt.

gnome-keyring-daemon is the least respectful for the secrets spec, i wouldn't follow it "blindly"

There is nothing on the spec about keeping track of the objects that are locked with a prompt and a way to return them. This can be consider as a hint there is nothing related to actually prompting a system prompt here.

There is, the prompt has a details return variable that is a zvariant::Value, it can contain anything, even a list of object paths ;)

yes, there is no way to pass a list of items to unlock into prompt & then get the result of what got locked and what wasn't. But there should definitely be a prompt used here otherwise the spec is wrong which I don't think it is...

server/src/service.rs

bilelmoussaoui · 2024-10-31T17:47:41Z

Service method Lock should emit PropertiesChanged signal for each Locked item and collection

That is normal, because you have to make use of the auto-generated method called $property_changed. Which you should call after you updated the value of locked in the set_locked method...

warusadura · 2024-10-31T17:54:16Z

Service method Lock should emit PropertiesChanged signal for each Locked item and collection

That is normal, because you have to make use of the auto-generated method called $property_changed. Which you should call after you updated the value of locked in the set_locked method...

Understood, thanks!

server/src/service.rs

server/src/collection.rs

server/src/service_manager.rs

server/src/collection.rs

server/src/service.rs

server/src/collection.rs

server/src/item.rs

server/src/service.rs

server/src/collection.rs

server/src/item.rs

server/src/service.rs

server/src/collection.rs

server/src/service.rs

bilelmoussaoui · 2024-11-19T14:30:24Z

Can you add the unlock implementation as well? Without the prompt bits obviously. Given that it will just be calling the same functions added here

warusadura · 2024-11-19T14:31:42Z

Can you add the unlock implementation as well? Without the prompt bits obviously. Given that it will just be calling the same functions added here

Yeah, of course. Will do.

bilelmoussaoui · 2024-11-19T16:44:16Z

server/src/service.rs

    ) -> Result<(Vec<OwnedObjectPath>, ObjectPath), ServiceError> {
-        todo!()
+        let mut locked = Vec::new();


It is the exact same implementation, with only difference being the state of locked, put the common code in a function.

Yeah I have thought about this. But, will this function get messy when adding the Prompt related things during Unlock?

Should I still follow this and move this to a new function?

basically share what is common between both :)

will do, thanks!

bilelmoussaoui · 2024-11-19T16:44:49Z

server/src/service.rs

+        for object in &objects {
+            for collection in collections.iter() {
+                if object == collection.path() {
+                    collection.set_locked(true).await?;


Why is this not doing the same check as unlock? See above for fixing that

For unlock we need a way to identify when to issue a Prompt. But, for Lock we don't need a prompt at all. This I'm sure :)

For unlock we need a way to identify when to issue a Prompt. But, for Lock we #150 (comment) need a prompt at all. This I'm sure :)

The spec disagree with you.

The current implementations don't use a prompt here, we might not need it if we agree with both the KDE/GNOME implementors, then it can easily be disabled. But for now, let us assume it is required to be fully compatible with the published specification.

But anyways, the lock/unlock DBus functions would call the "inner" one, so the Unlock could grow some functionality around it and the Lock one won't.

Understood, thanks!

hey, sorry for the delay.

I just realized that my implementation is wrong :) I think we shouldn't do the actual Lock/Unlock (calling collection.set_locked() and item.set_locked()) here. The Lock implementation used to be correct. But, since now both calls should have a prompt, the actual Lock/Unlock should happen inside Prompt. Specifically inside prompt_ready() call. Not inside the service methods. The purpose of service methods (Lock & Unlock) are to filter the objects locked/unlocked without prompt and send them out or send out a prompt without the objects.

question: how do we filter out which objects that we lock/unlock without a prompt?

Unlock:

if the objects are already unlocked, then thses object are unlocked without a prompt. So, we return the unlocked objecpaths here and no prompt.

if the objects are locked we return a prompt here and no unlocked objecpaths.

Lock:

Shoud we do the same for lock?

Thanks for the reply.

Yeah it's incomplete. But,

The purpose of service methods (Lock & Unlock) are to filter the objects locked/unlocked without prompt and send them out or send out a prompt without the objects.

And currently we're setting the locked status inside service methods. We shouldn't do this here. All we need to do this return objectspaths (locked/unlocked objects list or a prompt).

Since I can't get a feel of Lock with a prompt, I'm not sure when to issue a prompt and when to return locked objectpaths.

Who is supposed to modify the collection/item state then? It is supposed to be done once the prompt is finished successfully but still part of the code in Lock/Unlock.

Who is supposed to modify the collection/item state then?

org.gnome.keyring.internal.Prompter.Callback PromptReady

That will execute a task, the task will be created inside Lock and Unlock.

Updated the implementation.

Signed-off-by: Dhanuka Warusadura <dhanuka@gnome.org>

bilelmoussaoui · 2024-11-22T07:46:14Z

server/src/collection.rs

@@ -205,6 +205,34 @@ impl Collection {
        None
    }

+    #[allow(unused)]


what is this for?

bilelmoussaoui · 2024-11-22T07:47:01Z

server/src/service.rs

@@ -28,6 +29,7 @@ pub struct Service {
    collections: Arc<Mutex<Vec<Collection>>>,
    // Other attributes
    manager: Arc<Mutex<ServiceManager>>,
+    prompt_index: Arc<RwLock<u32>>,


Would put this in the ServiceManager instead and would pass that to newly created prompts as well

bilelmoussaoui · 2024-11-22T07:47:57Z

server/src/service.rs

+    pub async fn set_locked(
+        &self,
+        locked: bool,
+        objects: &Vec<OwnedObjectPath>,


Suggested change

objects: &Vec<OwnedObjectPath>,

objects: &[OwnedObjectPath],

bilelmoussaoui · 2024-11-22T07:48:53Z

server/src/service.rs

+        for object in objects {
+            for collection in collections.iter() {
+                if object == collection.path() {
+                    if !collection.is_locked().await {


shouldn't this compare it to the value of locked passed to it?

bilelmoussaoui · 2024-11-22T07:49:13Z

server/src/service.rs

+                    }
+                    break;
+                } else if let Some(item) = collection.item_from_path(object).await {
+                    if !item.is_locked().await {


bilelmoussaoui · 2024-11-22T07:49:39Z

server/src/service.rs

+                    }
+                    break;
+                }
+                tracing::info!("Object: {} does not exist.", object);


Maybe warn here instead

bilelmoussaoui · 2024-11-22T07:52:08Z

server/src/service.rs

+        #[zbus(object_server)] object_server: &zbus::ObjectServer,
+    ) -> Result<(Vec<OwnedObjectPath>, OwnedObjectPath), ServiceError> {
+        let unlocked = self.set_locked(false, &objects).await?;
+        if unlocked.is_empty() {


The condition here seems wrong, you would use a prompt only if there are items which don't have the expected state.

bilelmoussaoui · 2024-11-22T08:00:11Z

server/src/service.rs

+            let n_prompt = *self.prompt_index.read().await + 1;
+            let prompt = Prompt::new(n_prompt);
+            *self.prompt_index.write().await = n_prompt;


If you pass the service manager when creating the prompt, you could deal with incrementing the prompt index there

warusadura marked this pull request as draft October 31, 2024 16:56

bilelmoussaoui reviewed Oct 31, 2024

View reviewed changes

warusadura force-pushed the some_service_methods branch from f3de5f6 to acc5c86 Compare November 2, 2024 14:06

bilelmoussaoui reviewed Nov 2, 2024

View reviewed changes

server/src/service.rs Outdated Show resolved Hide resolved

warusadura force-pushed the some_service_methods branch 2 times, most recently from b414c91 to ef6929c Compare November 3, 2024 03:12

warusadura marked this pull request as ready for review November 3, 2024 03:24

warusadura force-pushed the some_service_methods branch from ef6929c to cd2a4cb Compare November 3, 2024 03:28

bilelmoussaoui reviewed Nov 3, 2024

View reviewed changes

server/src/service.rs Outdated Show resolved Hide resolved

warusadura force-pushed the some_service_methods branch 3 times, most recently from 0f2313a to 13e500c Compare November 3, 2024 16:01

bilelmoussaoui reviewed Nov 4, 2024

View reviewed changes

warusadura force-pushed the some_service_methods branch 2 times, most recently from 374bd6d to 0d7b53b Compare November 17, 2024 15:12

bilelmoussaoui reviewed Nov 17, 2024

View reviewed changes

server/src/collection.rs Outdated Show resolved Hide resolved

warusadura force-pushed the some_service_methods branch 2 times, most recently from a7d8efc to 4dc75dd Compare November 18, 2024 14:44

bilelmoussaoui reviewed Nov 18, 2024

View reviewed changes

server/src/collection.rs Outdated Show resolved Hide resolved

server/src/collection.rs Show resolved Hide resolved

server/src/collection.rs Outdated Show resolved Hide resolved

server/src/item.rs Outdated Show resolved Hide resolved

server/src/service.rs Outdated Show resolved Hide resolved

warusadura force-pushed the some_service_methods branch from 4dc75dd to 748d0c4 Compare November 18, 2024 16:27

bilelmoussaoui reviewed Nov 18, 2024

View reviewed changes

server/src/collection.rs Outdated Show resolved Hide resolved

bilelmoussaoui reviewed Nov 18, 2024

View reviewed changes

server/src/item.rs Show resolved Hide resolved

bilelmoussaoui reviewed Nov 18, 2024

View reviewed changes

server/src/service.rs Outdated Show resolved Hide resolved

warusadura force-pushed the some_service_methods branch from 748d0c4 to 73676bd Compare November 19, 2024 13:48

bilelmoussaoui reviewed Nov 19, 2024

View reviewed changes

server/src/collection.rs Show resolved Hide resolved

server/src/service.rs Outdated Show resolved Hide resolved

warusadura force-pushed the some_service_methods branch from 73676bd to ab88be5 Compare November 19, 2024 16:41

warusadura changed the title ~~server: Add service method Lock implementation~~ server: Add service method Lock and Unlock implementation Nov 19, 2024

bilelmoussaoui reviewed Nov 19, 2024

View reviewed changes

server: Add Lock and Unlock implementation

baa3d25

Signed-off-by: Dhanuka Warusadura <dhanuka@gnome.org>

warusadura force-pushed the some_service_methods branch from ab88be5 to baa3d25 Compare November 21, 2024 17:33

bilelmoussaoui reviewed Nov 22, 2024

View reviewed changes

server: Add service method Lock and Unlock implementation #150

Are you sure you want to change the base?

server: Add service method Lock and Unlock implementation #150

Conversation

warusadura commented Oct 31, 2024 • edited Loading

warusadura commented Oct 31, 2024

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

bilelmoussaoui commented Oct 31, 2024

warusadura commented Oct 31, 2024

bilelmoussaoui commented Nov 19, 2024

warusadura commented Nov 19, 2024

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

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

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

warusadura commented Oct 31, 2024 •

edited

Loading