Use webfinger for OIDC parameter discovery

[kaivol]

Closes #811.

Client side implementation of the changes described in https://github.com/opencloud-eu/opencloud/blob/main/docs/adr/0003-oidc-client-config-discovery.md.

Based on #776.

[RichardFevrier]

Did this PR worked for you @kulmann ?

Just tried it without success on my side, when both web + iOS are working perfectly.

I have tested the webfinger with:

curl -L cloud.mydomain.com/.well-known/webfinger?resource=https://cloud.opencloud.test&rel=http://openid.net/specs/connect/1.0/issuer&platform=desktop | jq
  % Total    % Received % Xferd  Average Speed  Time    Time    Time   Current
                                 Dload  Upload  Total   Spent   Left   Speed
  0      0   0      0   0      0      0      0                              0
100    305 100    305   0      0   2324      0                              0
{
  "subject": "https://cloud.opencloud.test",
  "properties": {
    "http://opencloud.eu/ns/oidc/client_id": "OpenCloudDesktop",
    "http://opencloud.eu/ns/oidc/scopes": [
      "openid",
      "profile",
      "email",
      "groups",
      "offline_access"
    ]
  },
  "links": [
    {
      "rel": "http://openid.net/specs/connect/1.0/issuer",
      "href": "https://auth.mydomain.com"
    }
  ]
}

Which returns groups that is mandatory on my setup since Authelia groups binds to OpenCloud roles.

But when the Authelia page appears I can see that groups are not part of the request.

Edit:
debugged it further:

26-04-28 13:38:21:512 [ debug sync.credentials.oauth ]  [ OCC::OAuth::openBrowser ]:    opening browser
26-04-28 13:38:21:512 [ debug sync.credentials.oauth ]  [ isUrlValid ]: Checking URL for validity: QUrl("https://auth.mydomain.com/api/oidc/authorization?response_type=code&client_id=OpenCloudDesktop&redirect_uri=http://127.0.0.1:32979&code_challenge=Kgkj2cHF9MwXPDyTkZETDhc4Pv07hAXBxgSV8DfBhCI&code_challenge_method=S256&scope=openid offline_access email profile&prompt=consent select_account&state=c5bNXSEMgYIazaZ3aMqJjz3mI7SfpbP00YlzmVl-Q2w%3D")
[2] Sandbox: CanCreateUserNamespace() clone() failure: EPERM

You can see that scopes (scope=openid offline_access email profile) doesn't contain groups.

[kaivol]

@RichardFevrier thanks for testing the PR!
I made some changes, so it should work now.

I also bumped the C++ standard version to 23, i hope this is fine.

[RichardFevrier]

Thanks for your work @kaivol I'll test that tomorrow! 🤩

[guruz]

@RichardFevrier Have you tested this PR? :)

[guruz]

@kaivol FYI CI ^ says this:

/Users/runner/work/desktop/desktop/src/libsync/creds/oauth.cpp:622:35: error: no member named 'join_with' in namespace 'std::__1::ranges::views'
  622 |                     | std::views::join_with(QStringLiteral(" "))
      |                       ~~~~~~~~~~~~^

Might be related to

I also bumped the C++ standard version to 23, i hope this is fine.

I can't say anything to this topic specifically, if it's easier to keep a lower C++ standard or check if all our platforms would support 23.
CC @TheOneRing

[kaivol]

@guruz Thanks for bringing that up.
I got rid of the join_with call and implemented the logic in a good ol' loop: diff

[guruz]

@kaivol one more failure :) ^

@TheOneRing FYI, i can't seem to be able to ask macos and windows to re-run if linux already failed ^

[kaivol]

I'm a little confused, the CI error seems to be in the OpenVFS plugin, not at all related to my changes 😕
A quick look at the CI overview shows that builds are failing for some time already.

[guruz]

@kaivol You're right, sorry!
Could you re-base on current main branch, it should be green now for Linux.
https://github.com/opencloud-eu/desktop/commits/main/

[guruz]

^ OAuth test fails

[kaivol]

Okay, I adjusted the OAuth test and added a response for the .well-known/webfinger request.

Let me know if you agree with the changes.

[guruz]

I can confirm this PR works, I'm logged in a test server with a different client_id and additional scopes.

I'll post my review comments later, it's only small stuff I think.

Thank you for the PR @kaivol

By the way I see this in log:

26-06-06 11:18:37:232 [ warning sync.credentials.manager ]:	set "OpenCloud_credentials:(REMOVED).opencloud.rocks:f6fc19c6-aad5-43ba-a244-191f75967795:http/oauthtoken" has not yet finished. duration(0h, 3min, 45s, 491ms)

[guruz]

Here in code before, it differs in error signal emission if it's about refresh token or not:
https://github.com/kaivol/opencloud-desktop/blob/b7e471209a0ede21fb12b59bd22aba26bfbad19e/src/libsync/creds/oauth.cpp#L646

While your new code does not:
https://github.com/kaivol/opencloud-desktop/blob/b7e471209a0ede21fb12b59bd22aba26bfbad19e/src/libsync/creds/oauth.cpp#L556
(and following emissions)

Maybe you could just always emit both in your added code (would need to be checked and tested if really only one is connected)?
Q_EMIT result(Error);
Q_EMIT refreshError(Error);
Or of course check if (_isRefreshingToken) and only emit one of them.

[kaivol]

I took a look at the current OAuth::fetchWellKnown function and now I'm a little confused about the emitted signals:

• On network error, we emit result(Error) (or refreshError(...) if _isRefreshingToken)
• If the JSON value is illegal we emit fetchWellKnownFinished()
• On any other JSON related issue we emit both result(Error) and fetchWellKnownFinished()

Also, the _wellKnownFinished variable is set to true before a signal is emitted.

Could you please clarify the meaning of _wellKnownFinished and the different signals, in particular which signals should be emitted under which conditions?

[guruz]

I was more talking about the lack of refreshError being emitted.

This is about the refresh token:
https://github.com/kaivol/opencloud-desktop/blob/b7e471209a0ede21fb12b59bd22aba26bfbad19e/src/libsync/creds/httpcredentials.cpp#L264

Signal refreshError (but not error) is connected a few lines above:
https://github.com/kaivol/opencloud-desktop/blob/b7e471209a0ede21fb12b59bd22aba26bfbad19e/src/libsync/creds/httpcredentials.cpp#L246

So _oAuthJob->refreshAuthentication(_refreshToken) is called.
Which then here https://github.com/kaivol/opencloud-desktop/blob/b7e471209a0ede21fb12b59bd22aba26bfbad19e/src/libsync/creds/oauth.cpp calls fetchWellKnown() (from restored()) ..
Which (...) then sends a webfinger request https://github.com/kaivol/opencloud-desktop/blob/b7e471209a0ede21fb12b59bd22aba26bfbad19e/src/libsync/creds/oauth.cpp#L552
When then here
https://github.com/kaivol/opencloud-desktop/blob/b7e471209a0ede21fb12b59bd22aba26bfbad19e/src/libsync/creds/oauth.cpp#L556 and https://github.com/kaivol/opencloud-desktop/blob/b7e471209a0ede21fb12b59bd22aba26bfbad19e/src/libsync/creds/oauth.cpp#L563 and https://github.com/kaivol/opencloud-desktop/blob/b7e471209a0ede21fb12b59bd22aba26bfbad19e/src/libsync/creds/oauth.cpp#L573 (... etc following)
doesn't emit refreshError but just error.
So it gets stuck here (in error case) while refreshing token.

On network error, we emit result(Error) (or refreshError(...) if _isRefreshingToken)

No you're not for the webfinger stuff, see my links ^
Only the code for the later req.setUrl(oidcWellKnownUrl); does (partly, hmmm)

Could you please clarify the meaning of _wellKnownFinished and the different signals, in particular which signals should be emitted under which conditions?

I cannot, I'm not versed with this code :) I was only wondering about the error vs refreshError.

[guruz]

@kaivol Do you need any help? Do you want me to do the commit on top of your changes?

Other than that, I think your PR could be merged...

(if @TheOneRing @dragotin don't disagree)

[guruz]

@kaivol I did it here: c4c9e91

If you agree with the code, do you want to cherry-pick it into your PR here?

Otherwise I can also add it as new PR if necessary.

[kaivol]

Should there be a return after emitting the error signal?

[guruz]

Probably yes, although there was not in original code!? https://github.com/opencloud-eu/desktop/pull/847/changes/BASE..52d360a904a9ee83fd8daec331a7b0e9ece1330a#diff-030afc7423b8921f965c6ffa0af5f7312a2e7aa06e1046628ddbde8e6394ac50L599

Maybe check what happens in fetchWellKnownFinished slot what would happen in this path if it is called in that case?

[kaivol]

While we're at it: Is it correct, that this particular JSON error is handled differently than all other JSON errors?

[guruz]

Legacy code, might be from long time ago from servers before OIDC? Would need to git blame and git deep to find out this one...
Could still happen with a mis configured server or proxy?

[guruz]

But now I see what you mean, there is no error emission there either... weird. Should probably also emit error adn return?!

[kaivol]

@guruz Thanks for the detailed explanation!
I’ve been pretty busy the last few days, so it took me a while to fully understand everything.

I cherry-picked your commit, and also added two comments you might want to take a look at.

[kaivol]

I did some more changes to the error handling, trying to keep your comments in mind.
Let me know if you agree.