D18845: authority: add support for passing details to polkit

[Nathaniel Graham]

ngraham added a comment.


  Landed and changed the comment. @chinmoyr, what's next for T8075: Fix 
security issues with KAuth support in KIO ?

REPOSITORY
  R563 Polkit-1 Qt Library

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns, davidedmundson
Cc: elvisangelaccio, ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R563:f26ada834aea: authority: add support for passing details 
to polkit (authored by mgerstner, committed by ngraham).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D18845?vs=54931=55297#toc

REPOSITORY
  R563 Polkit-1 Qt Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18845?vs=54931=55297

REVISION DETAIL
  https://phabricator.kde.org/D18845

AFFECTED FILES
  core/polkitqt1-authority.cpp
  core/polkitqt1-authority.h

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns, davidedmundson
Cc: elvisangelaccio, ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[David Edmundson]

davidedmundson accepted this revision.

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns, davidedmundson
Cc: elvisangelaccio, ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Nathaniel Graham]

ngraham added a comment.


  So what's next for this? Can this land now or do we need to change the 
comment first?

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: elvisangelaccio, ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> polkitqt1-authority.h:226
> + */
> +// TODO KF6: merge with checkAuthorization
> +void checkAuthorizationWithDetails(

@bruns The `polkit-qt-1` repository is not a framework, so "KF6"  could be 
misleading here.

What about something like

  // TODO: merge with checkAuthorization when we decide to break binary 
compatibility.

?

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: elvisangelaccio, ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Matthias Gerstner]

mgerstner added a comment.


  In D18845#439430 , @ngraham wrote:
  
  > @mgerstner can you provide your email address so we can land this patch 
with correct authorship information?
  
  
  It's matthias.gerst...@suse.com.

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Nathaniel Graham]

ngraham added a comment.


  @mgerstner can you provide your email address so we can land this patch with 
correct authorship information?

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Stefan Brüns]

bruns accepted this revision.
bruns added a comment.
This revision is now accepted and ready to land.


  Thanks

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Matthias Gerstner]

mgerstner updated this revision to Diff 54931.
mgerstner added a comment.


  Now using `constData()` as suggest by chinmoyr.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18845?vs=51753=54931

REVISION DETAIL
  https://phabricator.kde.org/D18845

AFFECTED FILES
  core/polkitqt1-authority.cpp
  core/polkitqt1-authority.h

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Matthias Gerstner]

mgerstner added inline comments.

INLINE COMMENTS

> chinmoyr wrote in polkitqt1-authority.cpp:336
> Nitpick; constData() because the API seems to take const gchar*

Strictly spoken it already returns `const gchar*`, since `toUtf8()` returns a 
`const QByteArray` temporary object and thus the `const char* data() const` 
member function should be called here.

Anways I will make it explicit like you suggest.

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Stefan Brüns]

bruns requested changes to this revision.
bruns added a comment.
This revision now requires changes to proceed.


  Can you address the last nitpick from @chinmoyr  ?

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Nathaniel Graham]

ngraham added a comment.


  Three LGTMs and +1 are pretty good :)

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: ngraham, mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Chinmoy Ranjan Pradhan]

chinmoyr added a comment.


  I would like this patch to land so +1 from me. But I am not familiar with 
this code so I can't give any meaningful feedback. Sorry.

INLINE COMMENTS

> polkitqt1-authority.cpp:336
> +
> +polkit_details_insert(ret, key.toUtf8().data(), 
> value.toUtf8().data());
> +}

Nitpick; constData() because the API seems to take const gchar*

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Fabian Vogt]

fvogt added a comment.


  LGTM to me as well - I'm not familiar with this code at all though.

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Stefan Brüns]

bruns added a comment.


  LGTM - @fvogt , @chinmoyr  ?

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: mati865, kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Matthias Gerstner]

mgerstner added a comment.


  In D18845#416305 , @bruns wrote:
  
  > Does this solve part of T8075 ?
  
  
  Part of a part it seems. I am currently working towards T10480 
. To actually add polkit details to 
authentication messages further changes in the kauth component are required 
once this change here is accepted.

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Stefan Brüns]

bruns added a comment.


  Does this solve part of T8075 ?

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Matthias Gerstner]

mgerstner added inline comments.

INLINE COMMENTS

> bruns wrote in polkitqt1-authority.cpp:328
> nullptr

I didn't want to mix styles in the source files. It's adjusted now.

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Matthias Gerstner]

mgerstner updated this revision to Diff 51753.
mgerstner added a comment.


  Incorporated review comments: replaced `NULL` by `nullptr`, removed some 
extra whitespace within parantheses, added KF6 TODO.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18845?vs=51161=51753

REVISION DETAIL
  https://phabricator.kde.org/D18845

AFFECTED FILES
  core/polkitqt1-authority.cpp
  core/polkitqt1-authority.h

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> polkitqt1-authority.cpp:130
> + * The returned pointer needs to be freed via g_object_unref when no
> + * longer needed. Returns NULL if details is empty.
> + */

nullptr

> polkitqt1-authority.cpp:328
> +if ( details.empty() )
> +return NULL;
> +

nullptr

> polkitqt1-authority.cpp:332
> +
> +for ( const auto : details.toStdMap() ) {
> +const auto  = entry.first;

whitespace

> polkitqt1-authority.cpp:422
> +
> +if (pk_details != NULL) {
> +g_object_unref(pk_details);

nullptr, or just `if (pk_details) {`

> polkitqt1-authority.h:226
> + */
> +void checkAuthorizationWithDetails(
> +const QString , const Subject ,

please add a KF6 TODO, merge with checkAuthorization

REPOSITORY
  R563 Polkit-1 Qt Library

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Nathaniel Graham]

ngraham added reviewers: chinmoyr, fvogt, bruns.

REPOSITORY
  R563 Polkit-1 Qt Library

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks, chinmoyr, fvogt, bruns
Cc: kde-frameworks-devel

D18845: authority: add support for passing details to polkit

[Luca Beltrame]

lbeltrame added a reviewer: Frameworks.
lbeltrame added a subscriber: kde-frameworks-devel.

REPOSITORY
  R563 Polkit-1 Qt Library

REVISION DETAIL
  https://phabricator.kde.org/D18845

To: mgerstner, #frameworks
Cc: kde-frameworks-devel