FEATURE: Add probe-only functionality to SSO Provider protocol

[mdoggydog]

This commit adds support for an optional "probe" parameter in the payload of the /session/sso_provider endpoint. If an SSO Consumer adds a "probe=true" parameter to the encoded/signed "sso" payload, then Discourse will avoid trying to login a not-logged-in user:

• If the user is already logged in, Discourse will immediately redirect back to the Consumer with the user's credentials in a signed payload, as usual.

• If the user is not logged in, Discourse will immediately redirect back to the Consumer with a signed payload bearing the parameter "failed=true".

This allows the SSO Consumer to simply test whether or not a user is logged in, without forcing the user to try to log in. This is useful when the SSO Consumer allows both anonymous and authenticated access. (E.g., users that are already logged-in to Discourse can be seamlessly logged-in to the Consumer site, and anonymous users can remain anonymous until they explicitly ask to log in.)

[discoursebot]

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/discourse-development-contributor-guidelines/3823/109

[gschlager]

I'm not sure if we want to have this feature in core, but the code seems to be okay. And the feature seems simple enough. But since this concerns auth, lets get more eyes on it. @OsamaSayegh Can you have a look as well?

If we merge this, @mdoggydog would you be so kind and update the documentation at https://meta.discourse.org/t/use-discourse-as-an-identity-provider-sso-discourseconnect/32974?

[mdoggydog]

If we merge this, @mdoggydog would you be so kind and update the documentation at https://meta.discourse.org/t/use-discourse-as-an-identity-provider-sso-discourseconnect/32974?

Sure, I would be happy to do that. Thanks for getting this PR moving.

(I am still curious about whether or not failed checks interfere with the review process, and if there is a way to trigger a recheck. 🤔 )

[gschlager]

I am still curious about whether or not failed checks interfere with the review process, and if there is a way to trigger a recheck.

No, it doesn't affect the review process. Sometimes reviews take time, especially when many team members are on vacation.

[mdoggydog]

Hi, @OsamaSayegh, any feedback on this PR?

[davidtaylorhq]

I think the functionality here is reasonable, but I'm not sure about the the naming of 'probe'. It doesn't really explain what's happening at first glance.

The OpenIDConnect specification uses ?prompt=none for this kind of functionality.

none: The Authorization Server MUST NOT display any authentication or consent user interface pages. An error is returned if an End-User is not already authenticated or the Client does not have pre-configured consent for the requested Claims or does not fulfill other conditions for processing the request. The error code will typically be login_required, interaction_required, or another code defined in Section 3.1.2.6. This can be used as a method to check for existing authentication and/or consent.

How about we do the same? So we replace probe=true with prompt=none (and reject any other values for prompt)

[mdoggydog]

How about we do the same? So we replace probe=true with prompt=none (and reject any other values for prompt)

I like it; and, it makes it clear how to extend the API to handle the "require user to reauth even if they are already logged in" case if/when somebody needs it.

Where should the reject-other-values occur? I can see three places that seem reasonable:

1. DiscourseConnectProvider::parse
2. SecondFactor::Actions::DiscourseConnectProvider::get_sso
3. DiscourseConnectBase::parse

Each of these already raises one or more errors in response to the contents of the SSO request, so it is not clear to me which is the "right" place for this check. (Probably not (3), but that still leaves (1) or (2).)

Also, what kind of error should be raised?

• DiscourseConnectProvider::ParseError seems to be intended to apply to problems with checking the signature and unwrapping the sso payload in the request, yet... it is also used for a missing return_sso_url in the payload.
• ...::BlankSecret and ...::BlankReturnUrl are obviously for their very specific conditions.
• Why does a missing return_sso_url yield ParseError, but a blank return_sso_url yields BlankReturnUrl??
• Should I create a new error class? Or, just use ParseError?
• If I create a new error class, does this need an internationalized response string? (ParseError and BlankSecret get I18n.t() messages, but BlankReturnUrl gets a vanilla string --- see SessionController::sso_provider.)

Funny... the "reject-other-values" is the most difficult part of this exercise...

[mdoggydog]

Ok, I have pushed a revised PR that features the prompt=none parameter, but this does not yet have any rejection of unsupported prompt= values.

I await some guidance/opinions on that.

[davidtaylorhq]

Thanks @mdoggydog. I think let's put the rejection as early as possible, which looks like DiscourseConnectProvider::parse

And then I think let's match how BlankSecret is handled. So create another error class under DiscourseConnectProvider, and rescue it in SessionController#sso_provider to return a human-readable error.

[mdoggydog]

This should take care of it.

The rejection/validation is performed in DiscourseConnectProvider::parse, as suggested (and where I agree that it belongs, since it is a *Provider parameter). I put it after super() is called, because (1) until the request signature is verified, the request is untrusted; and (2) super() has all the proper machinery to unpack parameters. (The goofy stuff going on before super() is due to the needs of figuring out the correct sso-secret to hand to super().)

[mdoggydog]

Argh... I somehow missed some linting nits before the last push. Pushed again.

(I am not a fan of postfix bar if foo conditionals --- sure, it fits on one line, but putting the effect before the cause does not improve the clarity of code. If it were up to me... alas, it's not.)

[mdoggydog]

Pinging @OsamaSayegh @davidtaylorhq @gschlager ... Anyone around to review this?

[discoursebot]

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/use-discourse-as-an-identity-provider-sso-discourseconnect/32974/141

[davidtaylorhq]

Sorry for the very long delay! This looks great - thanks for the contribution @mdoggydog 🙏