D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Dan Leinir Turthra Jensen]

leinir abandoned this revision.
leinir added a comment.


  In D25961#676900 , @nicolasfella 
wrote:
  
  > @leinir it seems like this was merged so I guess this should be closed?
  
  
  Probably make sense, yeah - it basically got moved to invent and worked on 
there, so it would really want closing anyway :)

REPOSITORY
  R119 Plasma Desktop

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

To: leinir
Cc: nicolasfella, apol, zachus, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, 
mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Nicolas Fella]

nicolasfella added a comment.


  @leinir it seems like this was merged so I guess this should be closed?

REPOSITORY
  R119 Plasma Desktop

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

To: leinir
Cc: nicolasfella, apol, zachus, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, 
mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Dan Leinir Turthra Jensen]

leinir added inline comments.

INLINE COMMENTS

> nicolasfella wrote in kdeplatformdependent.cpp:165
> This sounds like a use case for KCMultiDialog, like e.g. in 
> https://invent.kde.org/kde/kdeconnect-kde/-/blob/master/settings/main.cpp

That certainly could be! i'll take a look, thanks for the hint :)

REPOSITORY
  R119 Plasma Desktop

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

To: leinir
Cc: nicolasfella, apol, zachus, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> kdeplatformdependent.cpp:165
> +// directly requests the accounts kcm to start adding a new account if 
> it's not there, maybe even pre-fills the fields...)
> +KService::List services = 
> KServiceTypeTrader::self()->query(QStringLiteral("KCModule"), 
> QStringLiteral("Library == 'kcm_kaccounts'"));
> +// If we failed to get the kcm, tell the caller we failed

This sounds like a use case for KCMultiDialog, like e.g. in 
https://invent.kde.org/kde/kdeconnect-kde/-/blob/master/settings/main.cpp

REPOSITORY
  R119 Plasma Desktop

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

To: leinir
Cc: nicolasfella, apol, zachus, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Dan Leinir Turthra Jensen]

leinir added a comment.


  In D25961#617057 , @apol wrote:
  
  > +1 cool stuff, much better than using kwallet for it.
  
  
  Thanks! :)
  
  > Have you checked how well it works regarding startup? In Discover we do a 
bunch of queries at start for every KNSEngine and we need to make sure we don't 
block the whole thing (more than QNAM does, at least)
  
  Not yet, no; the intention is to postpone the qnam creation in there (which 
is currently a touch on the ugly side, and also not acceptable given the whole 
redirection situation), and then also a mix of caching and delayed creation for 
the accounts stuff, so... yeah, bit of work to be done in that bit, for thought 
i'd delay that until it actually works :)

REPOSITORY
  R119 Plasma Desktop

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

To: leinir
Cc: apol, zachus, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Aleix Pol Gonzalez]

apol added a comment.


  +1 cool stuff, much better than using kwallet for it.
  
  Have you checked how well it works regarding startup? In Discover we do a 
bunch of queries at start for every KNSEngine and we need to make sure we don't 
block the whole thing (more than QNAM does, at least)

REPOSITORY
  R119 Plasma Desktop

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

To: leinir
Cc: apol, zachus, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 76309.
leinir added a comment.


  - Switch to using the kaccounts cmake commands

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25961?vs=76049=76309

BRANCH
  switch-attica-plugin-to-kaccounts (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  attica-kde/kdeplugin/CMakeLists.txt
  attica-kde/kdeplugin/kdeplatformdependent.cpp
  attica-kde/kdeplugin/kdeplatformdependent.h
  attica-kde/kdeplugin/opendesktop-rating.service.in
  attica-kde/kdeplugin/opendesktop.provider.in

To: leinir
Cc: zachus, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 76049.
leinir added a comment.


  Fair bit of work gone on here, but in short, given a patch which handles the
  OpenID ID token in the OAuth2 requests in signon-plugin-oauth2, we are now
  able to construct a bearer-authenticated request and whatnot. There is more
  work to be done, but it seems like things are happening now! Progress.
  
  The merge request for the signon-plugin-oauth2 patch can be found at:
  https://gitlab.com/accounts-sso/signon-plugin-oauth2/-/merge_requests/25
  
  - Add the provider and service files (these use temp client data for now)
  - Actually install the service and provider files
  - Add functionality to open the accounts kcm on saveCredentials calls
  - Actually make the service locator work (need to query for the right thing)
  - Add the SignOn OAuth plugin as a runtime dependency
  - Minor cleanup for attica_kde cmakelists
  - The opendesktop provider file is... not google
  - Fetch the access token (actually ID token) from AccountsManager via 
KAccounts

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25961?vs=74208=76049

BRANCH
  switch-attica-plugin-to-kaccounts (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  attica-kde/kdeplugin/CMakeLists.txt
  attica-kde/kdeplugin/kdeplatformdependent.cpp
  attica-kde/kdeplugin/kdeplatformdependent.h
  attica-kde/kdeplugin/opendesktop-rating.service
  attica-kde/kdeplugin/opendesktop.provider

To: leinir
Cc: zachus, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 74208.
leinir added a comment.


  - Actually make the service locator work (need to query for the right thing)

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25961?vs=73756=74208

BRANCH
  switch-attica-plugin-to-kaccounts (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  attica-kde/kdeplugin/CMakeLists.txt
  attica-kde/kdeplugin/kdeplatformdependent.cpp
  attica-kde/kdeplugin/kdeplatformdependent.h
  attica-kde/kdeplugin/opendesktop-rating.service
  attica-kde/kdeplugin/opendesktop.provider

To: leinir
Cc: zachus, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 73756.
leinir added a comment.


  Rebase on master
  
  - Add the provider and service files (these use temp client data for now)
  - Actually install the service and provider files
  - Add functionality to open the accounts kcm on saveCredentials calls

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25961?vs=71607=73756

BRANCH
  switch-attica-plugin-to-kaccounts (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  attica-kde/kdeplugin/CMakeLists.txt
  attica-kde/kdeplugin/kdeplatformdependent.cpp
  attica-kde/kdeplugin/kdeplatformdependent.h
  attica-kde/kdeplugin/opendesktop-rating.service
  attica-kde/kdeplugin/opendesktop.provider

To: leinir
Cc: zachus, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Dan Leinir Turthra Jensen]

leinir added a comment.


  In D25961#578727 , @zachus wrote:
  
  > is there a server out there to run it against?
  
  
  The provider file points it at the opendesktop.org server (that is to say, 
the KDE Store), so testing can be done against the live servers that already 
provide all our content :)
  
  Note also that the current client id and secret means you will be told that 
Bhushan Shah is asking for access to your things during sign-in - this is 
because that's who asked for test access in order to build this, so don't 
worry, that's currently expected. Before merging, it'll want to be swapped out 
for a more proper id, though we'll probably want to give this some thought - do 
we want one default for all of KDE's software, do we want one that can be 
replaced by distributions, or do we want distributions to have to do this? i'm 
currently unsure, but leaning towards option two (meaning, ship with a default 
id and secret set, but offer the ability for distributions and app developers 
to swap this out with one of their own).

REPOSITORY
  R119 Plasma Desktop

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

To: leinir
Cc: zachus, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Zacharias Zachus]

zachus added a comment.


  is there a server out there to run it against?

REPOSITORY
  R119 Plasma Desktop

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

To: leinir
Cc: zachus, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 71607.
leinir added a comment.


  - Add functionality to open the accounts kcm on saveCredentials calls

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25961?vs=71495=71607

BRANCH
  switch-attica-plugin-to-kaccounts (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  attica-kde/kdeplugin/CMakeLists.txt
  attica-kde/kdeplugin/kdeplatformdependent.cpp
  attica-kde/kdeplugin/kdeplatformdependent.h
  attica-kde/kdeplugin/opendesktop-rating.service
  attica-kde/kdeplugin/opendesktop.provider

To: leinir
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 71495.
leinir added a comment.


  - Add the provider and service files (these use temp client data for now)
  - Actually install the service and provider files

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25961?vs=71463=71495

BRANCH
  switch-attica-plugin-to-kaccounts (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  attica-kde/kdeplugin/CMakeLists.txt
  attica-kde/kdeplugin/kdeplatformdependent.cpp
  attica-kde/kdeplugin/kdeplatformdependent.h
  attica-kde/kdeplugin/opendesktop-rating.service
  attica-kde/kdeplugin/opendesktop.provider

To: leinir
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25961: [WIP] Switch the Attica KDE plugin to use KAccounts

[Dan Leinir Turthra Jensen]

leinir created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
leinir requested review of this revision.

REVISION SUMMARY
  Don't require KWallet, and require KAccounts and libaccounts-Qt5
  
  Link and use includes from new requirements
  
  Remove KWallet bits, and use KAccounts/libaccounts-qt instead
  
  This removes the custom KWallet integration in favour of an approach
  based on the more generic KAccounts framework. It is a work in progress
  which currently does not allow for creating new accounts (which would
  be done through saveCredentials), but which does load and use the data
  from any accounts which already exist.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  switch-attica-plugin-to-kaccounts (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  attica-kde/kdeplugin/CMakeLists.txt
  attica-kde/kdeplugin/kdeplatformdependent.cpp
  attica-kde/kdeplugin/kdeplatformdependent.h

To: leinir
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart