Faulty logic in disconnect external account check

[slint]

Package version (if known): v1.5.3

Describe the bug

Even though someone might have more than one external accounts connected, the check performed in require_more_than_one_external_account, is faulty in terms of how client configuration is usually advertised in the documentation.

Usually, configurations are like:

OAUTHCLIENT_REMOTE_APPS = dict(
    github=github.REMOTE_APP,
    orcid=orcid.REMOTE_APP,
)
GITHUB_APP_CREDENTIALS = dict(
    consumer_key="abc123",
    consumer_secret="...",
)
ORCID_APP_CREDENTIALS = dict(
    consumer_key="def456",
    consumer_secret="...",
)

In the code, the acc.client_id is actually the application's Client ID/Key, e.g. in this example abc123 (for GitHub) or def456 (for ORCiD), and the keys in remote_app (which is OAUTHCLIENT_REMOTE_APPS config) are github and orcid.

This means that disconnecting a linked account will always fail since len(linked_accounts) will always be 0, even if the user has actually connected more than one external account.

Steps to Reproduce

1. Setup two OAuth authentication apps (using the usually proposed config described above).
2. Sign-up with one of the above external methods and connect other
3. (Don't set a password)
4. Go to the linked accounts settings page (e.g. https://localhost:5000/account/settings/linkedaccounts/)
5. Click the "Disconnect" button for any of the two linked accounts
6. Get a "400 Bad Request" response page

Additional context

For backward compatibility (because for users that want this to work with multiple external accounts, they might have used the actual client IDs as keys in the OAUTHCLIENT_REMOTE_APPS config), we might have to check both client ID and remote app config key?

Note: Another issue I see in general throughout the module is that the external_method and the remote app config keys are meant/supposed (?) to match, but there's nothing enforcing this.

cc @max-moser

[max-moser]

oh yes, good catch! :eyes: maybe something like the following would be more appropriate (untested pseudocode)?

# the remote_account.client_id actually references consumer keys rather than
# the key under which remote apps are registered in the config
consumer_keys = {
    app.config[remote_app["params"]["app_key"]]["consumer_key"]
    for remote_app in app.config["OAUTHCLIENT_REMOTE_APPS"].values()
    if not remote_app.get("hide", False)
}

linked_accounts = [acc for acc in accounts if acc.client_id in consumer_keys]

now i'm not sure if the acc.client_id could even reference a key in the OAUTHCLIENT_REMOTE_APPS dictionary at all, but if so, the consumer_keys set could easily be extended.

regarding the second issue, i'm unfortunately not deep enough in the design and precise workings of the module to give a qualified answer... however, since it's been made possible to register several different Keycloak instances relatively flexibly, several of those could probably be used to check out if there's a problem. ping @ntarocco