Re: [pdm-devel] [PATCH yew-comp v2 2/2] token panel: improve token secret dialog layout and hide password

["Lukas Wagner" <l.wagner@proxmox.com>]

On Fri Oct 17, 2025 at 12:04 PM CEST, Shannon Sterz wrote:
> On Fri Oct 17, 2025 at 11:36 AM CEST, Lukas Wagner wrote:
>> Looks good to me.
>> One comment inline, but nothing that prohibits applying this patch.
>>
>> Tested-by: Lukas Wagner <l.wagner@proxmox.com>
>> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
>>
>> On Tue Oct 14, 2025 at 4:37 PM CEST, Shannon Sterz wrote:
>>> the dialog showing the token secret now displays it like any other
>>> password. meaning that the actual secret is covered up at first but
>>> can be displayed if required. this adds protections against should
>>> surfing attacks.
>>>
>>> the copy secret button has also been moved next to the input field and
>>> is now an icon buton with a tool tip.
>>>
>>> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
>>> ---
>>>  src/token_panel.rs | 111 ++++++++++++++++++++++-----------------------
>>>  1 file changed, 55 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/src/token_panel.rs b/src/token_panel.rs
>>> index c027a32..5c8fa23 100644
>>> --- a/src/token_panel.rs
>>> +++ b/src/token_panel.rs
>>> @@ -6,6 +6,7 @@ use anyhow::Error;
>>>  use proxmox_access_control::types::{ApiToken, UserWithTokens};
>>>  use proxmox_auth_api::types::Authid;
>>>  use proxmox_client::ApiResponseData;
>>> +use pwt::css::ColorScheme;
>>>  use serde_json::{json, Value};
>>>
>>>  use yew::virtual_dom::{Key, VComp, VNode};
>>> @@ -14,7 +15,9 @@ use pwt::prelude::*;
>>>  use pwt::state::{Selection, Store};
>>>  use pwt::widget::data_table::{DataTable, DataTableColumn, DataTableHeader};
>>>  use pwt::widget::form::{Checkbox, DisplayField, Field, FormContext, InputType};
>>> -use pwt::widget::{Button, Column, Container, Dialog, InputPanel, Toolbar};
>>> +use pwt::widget::{
>>> +    Button, Column, Container, Dialog, FieldLabel, InputPanel, Row, Toolbar, Tooltip,
>>> +};
>>>
>>>  use crate::percent_encoding::percent_encode_component;
>>>  use crate::utils::{
>>> @@ -321,65 +324,61 @@ impl ProxmoxTokenView {
>>>      }
>>>
>>>      fn show_secret_dialog(&self, ctx: &LoadableComponentContext<Self>, secret: &Value) -> Html {
>>> -        let secret = secret.clone();
>>> +        let tokenid = AttrValue::from(secret["tokenid"].as_str().unwrap_or("").to_owned());
>>> +        let secret = AttrValue::from(secret["value"].as_str().unwrap_or("").to_owned());
>>>
>>>          Dialog::new(tr!("Token Secret"))
>>>              .with_child(
>>> -                Column::new()
>>> -                    .with_child(
>>> -                        InputPanel::new()
>>> -                            .padding(4)
>>> -                            .with_large_field(
>>> -                                tr!("Token ID"),
>>> -                                DisplayField::new()
>>> -                                    .value(AttrValue::from(
>>> -                                        secret["tokenid"].as_str().unwrap_or("").to_owned(),
>>> -                                    ))
>>> -                                    .border(true),
>>> -                            )
>>> -                            .with_large_field(
>>> -                                tr!("Secret"),
>>> -                                DisplayField::new()
>>> -                                    .value(AttrValue::from(
>>> -                                        secret["value"].as_str().unwrap_or("").to_owned(),
>>> -                                    ))
>>> -                                    .border(true),
>>> -                            ),
>>> -                    )
>>> -                    .with_child(
>>> -                        Container::new()
>>> -                            .style("opacity", "0")
>>> -                            .with_child(AttrValue::from(
>>> -                                secret["value"].as_str().unwrap_or("").to_owned(),
>>> -                            )),
>>> -                    )
>>> -                    .with_child(
>>> -                        Container::new()
>>> -                            .padding(4)
>>> -                            .class(pwt::css::FlexFit)
>>> -                            .class("pwt-bg-color-warning-container")
>>> -                            .class("pwt-color-on-warning-container")
>>> -                            .with_child(tr!(
>>> -                                "Please record the API token secret - it will only be displayed now"
>>> -                            )),
>>> -                    )
>>> -                    .with_child(
>>> -                        Toolbar::new()
>>> -                            .class("pwt-bg-color-surface")
>>> -                            .with_flex_spacer()
>>> -                            .with_child(
>>> -                                Button::new(tr!("Copy Secret Value"))
>>> -                                    .icon_class("fa fa-clipboard")
>>> -                                    .class("pwt-scheme-primary")
>>> -                                    .on_activate({
>>> -                                        move |_| {
>>> -                                            copy_text_to_clipboard(
>>> -                                                secret["value"].as_str().unwrap_or(""),
>>> +                Column::new().with_child(
>>> +                    InputPanel::new()
>>> +                        .padding(4)
>>> +                        .with_large_field(
>>> +                            tr!("Token ID"),
>>> +                            DisplayField::new()
>>> +                                .value(tokenid)
>>> +                                .name("tokenid")
>>> +                                .border(true),
>>> +                        )
>>> +                        .with_large_custom_child(
>>> +                            Container::new()
>>> +                                .class("pwt-form-grid-col4")
>>> +                                .with_child(FieldLabel::new(tr!("Secret")))
>>> +                                .with_child(
>>> +                                    Row::new()
>>> +                                        .class("pwt-fill-grid-row")
>>> +                                        .gap(2)
>>> +                                        .with_child(
>>> +                                            Field::new()
>>> +                                                .input_type(InputType::Password)
>>> +                                                .class(pwt::css::FlexFit)
>>> +                                                .value(secret.clone())
>>> +                                                .name("secret")
>>> +                                                .disabled(true)
>>
>> When clicking the 'show secret' button, it is not possible to select the
>> secret via the cursor and copy it by 'hand' (Ctrl-C/right click context
>> menu), which is due to this field being disabled. I know there is a
>> 'copy secret' button, but this tripped me up for a moment when trying
>> this out.
>> Was this an intentional decision, e.g. to avoid mistakes when copying
>> the value or to discourage users from using the 'show secret' button
>> altogether?
>
> thanks for the review, this was actually discussed in a previous review.
> there i used a DisplayField, but that doesn't allow for hiding the
> contents like password fields do.
>
> i decided to mark this as `disabled` so that users won't edit it on
> accident, but allowing users to select it manually certainly makes
> sense.
>

Ah, alright, thanks for the explanation!


_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel