Support OpenID

[max-baz]

OpenID is part of login scenario, therefore we need to support it.

I'm not decided on one particular detail: if a password entry contains "openid" field, and a website has two forms, one asking for username and password, and another one asking for OpenID url, should we fill both forms or only the latter one?

Currently I implemented the former, Browserpass fills both forms (and focuses login button in the OpenID form), because if the OpenID field cannot be detected, we anyway proceed with trying to find and fill a regular form with username and password... but I'm not 100% sure this is the right approach.

@mcnesium, @erayd, thoughts on this?

The example page with two forms: https://www.openstreetmap.org/login (click on OpenID button)

Fixes browserpass/browserpass-legacy#208

[erayd]

@maximbaz I think we should be filling one or the other, not both. If someone is using OpenID, they are often doing so because they don't trust (or don't want to need to trust) the target website with authentication credentials. If we are filling user / password into that site regardless, we are leaking the user's credentials where they may not want them leaked, even though they have requested a login on the target page.

Also, we should not fill the OpenID field if we are already entering a user / pass. Some sites assume that if the OpenID field has something in it, then they should use OpenID (and not the user / pass), even if other credentials have been entered.

I personally feel that the most appropriate approach is to never fill user / pass if an openid: field is present in the password entry. If we can find an OpenID field and fill that in, perfect. If we can't, then we should fail safe and not fill anything.

[erayd]

The null check was explicit in order to detect the difference between a configured (but empty) field, and a nonexistent field. Is there a reason you've changed it to a loosely-typed test?

[max-baz]

Could you please clarify? Based on how our password entry parsing looks now, I don't think the value can ever be null. Also, given that this function is only called with either ["login", "secret"] (which are guaranteed to always be present) or ["openid"] (which is only the case when "openid" was parsed, so again it's guaranteed to always be present), I don't really understand what this entire loop is trying to catch...

Also see the new line 685, where I added another loosely-typed test, is that an issue?

let fields = message.login.fields.openid ? ["openid"] : ["login", "secret"];

[erayd]

Could you please clarify? Based on how our password entry parsing looks now, I don't think the value can ever be null.

You're right; our key / value parsing disallows an empty value. I forgot that this was the case - and therefore, we don't need to detect an empty value here.

Also, given that this function is only called with either ["login", "secret"] (which are guaranteed to always be present) or ["openid"] (which is only the case when "openid" was parsed, so again it's guaranteed to always be present), I don't really understand what this entire loop is trying to catch...

I have that PR for multi-stage login pending; once that is eventually merged then we'll be calling this with other combinations of options. So tests need to be "does this set include these fields" rather than "is this set exactly these fields".

Given that there isn't an empty-string problem, loose-type comparisons on the new L685 aren't a problem, but the explicit sets are.

[max-baz]

OK makes sense, I'll remove the check for now, add it back in your PR so that it is highlighted as "new code" and we remember to talk about it again in the context of multi-stage login 👍

[max-baz]

Interesting idea about filling only OpenID if it is present in a password entry, it seems to make sense on all example websites that I collected for testing.

Here's a particularly good example, this website has both password and OpenID in the same form, but then if you only fill OpenID (try value https://id.mcnesium.com), and submit the form, you are redirected elsewhere to submit ID and password.

@mcnesium would like your opinion as well, in particular because in an example you posted earlier your password entry contains password, username AND openid: was it just an example, and in real life you only have openid field password entries for those websites where you want to login with OpenID? Do you agree that if an openid is present in the password file, Browserpass should never even attempt to fill username and password fields, or can you think of a counter-example?

[mcnesium]

Thanks a lot for bringing this forward, folks! 👍

I totally agree with @erayd in terms of only filling openid, if present in both the login form and the password file.

For now, I never fill OpenID fields using browserpass. If OpenID is supported, I focus the form field and hope my browser has cached an earlier entry, otherwise I just type it.

You are right, my earlier post has just been an example, regarding the structure of the password file. Haven't had the chance to analyse the browserpass code, but I assume my suggestion requires too complicated logic.

Some services still require users to register an account and eventually provide a password, even when using OpenID (e.g. Gitea). So even if not using password login, I would still like to store the password. So the first line of the file must still be reserved for it.

In compliance with the zx2c4 suggestion, a dedicated openid: … field in the file, that if present is being preferred over user/pass, would be cool for me.

[max-baz]

Some services still require users to register an account and eventually provide a password, even when using OpenID (e.g. Gitea). So even if not using password login, I would still like to store the password. So the first line of the file must still be reserved for it.

But as per @erayd's suggestion, regardless if a form contains OpenID field, because an openid: field is present in your gitea.io.gpg file, Browserpass will never attempt to auto-fill password for you, is that correct behavior from your point of view?

[mcnesium]

regardless if a form contains OpenID field, because an openid: field is present in your gitea.io.gpg file, Browserpass will never attempt to auto-fill password for you, is that correct behavior from your point of view?

I guess so, yes. It seems to make sense that if I want to use OpenID to login on a site, I add the openid: field to the particular file. If I don't want to use OpenID, then I should not add such a field.

[max-baz]

Some services still require users to register an account and eventually provide a password

Just to double check that I understand everything correctly (because I'm not using OpenID), so such a password you will store in this password entry, but you will never use it to login, because you will always be loggin in with OpenID, correct?

[mcnesium]

That's right.

[max-baz]

Thanks! I'll ping you for testing once I adapt the implementation 😉

[max-baz]

Ready for your testing and a second round of reviews @erayd, @mcnesium 😉

[mcnesium]

This works perfectly now, thank you! 🥇