Add Steam algorithm behind 'steam' feature

[timvisee]

This implements Algorithm::Steam behind the steam feature flag.

Fixes #45

Note that this is somewhat dirty, but keeps the API stability intact. Notably:

• Adds steam feature (disabled by default)
• Adds Algorithm::Steam variant
• Disables RFC assertions in TOTP::new() if algorithm is Steam
• TOTP::generate() uses Steam alphabet for token if Steam algorithm is used
• TOTP::from_url() parses Steam URLs if the steam path is detected.

I've tested this successfully with my personal Steam token.

I'm not sure if this is good enough to be merged, but I figured this to be a good start.

[constantoine]

One of the main reason I was thinking of using an API similar to Rfc6238 (for defaults) is that in case of steam algorithm, periods and digits do not make much sense, as they're always the same

[timvisee]

A Steam struct you mean. That can definitely be added.

This would still require a Algorithm::Steam variant (or alternatively a new TOTP property) because it uses a different token alphabet. Along with these changes in TOTP::from_url() to parse otpauth://steam URIs.

[constantoine]

The work on algorithm is fine as is, yes!

It's just that instanciating it from New would be awkward, hence a Steam struct, or more simply having a custom_providers.rs file which would contain, gated behind a feature for each provider, a impl block for TOTP with stuff like new_steam and new_battlenet, which would call the new_unchecked variant under the hood?

This would mean merging #46 and then rebasing

Edit: Because having a struct for each custom provider would actually be way too much copy/pasted code, and would probably hurt readability

[timvisee]

Alright! I'll work on these new_ functions once possible.

I agree, that adding struct for this would add a lot of boilerplate.

[constantoine]

Thanks a lot! :)

[constantoine]

Now that I think about it, the get_url function should be mindful of the algo, as in the steam case, we need to use /steam/ instead of /totp/

[timvisee]

Now that I think about it, the get_url function should be mindful of the algo, as in the steam case, we need to use /steam/ instead of /totp/

Right. This is already handled in

https://github.com/timvisee/totp-rs/blob/9d506d4ae19cb28a269ae4e56f2f23bb14684622/src/lib.rs#L453-L460

and

https://github.com/timvisee/totp-rs/blob/9d506d4ae19cb28a269ae4e56f2f23bb14684622/src/lib.rs#L480-L492

[timvisee]

This is now rebased on the latest master.

A TOTP::new_steam() method is added in custom_providers.rs and generating a Steam TOTP URL now works correctly.

Currently, it detects otpauth://steam/ as Steam TOTP. It appears that some authenticators use otpauth://totp/?issuer=Steam instead, which is likely why this didn't work.

@constantoine Are you fine with me implementing ?issuer=Steam to be parsed as Algorithm::Steam?

[constantoine]

As long as it's gated behind the Steam feature, I think it's acceptable, yes

[timvisee]

Implemented. This is ready for review.

[constantoine]

I feel like you only need to set the algo, not the digits

[timvisee]

It seems like some otpauth URLs don't set the number of digits at all for Steam. That means we must set it explicitly somewhere.

Not setting this would cause problems, such as incorrect otpauth URL generation.

Better ways to implement this would require breaking API changes, I think.

[constantoine]

I see. Should we then ignore the digit parameter, should it be present? Just like we ignore algorithm

[timvisee]

That is an option. But that will still generate invalid otpauth URLs, as it will have ?digit=6. We can implement an exception there but I don't think that is better.

[constantoine]

I'll add some tests and benchmark some stuff on my end, but this PR is ready :)

I'll ping you when the release is published on cargo

[timvisee]

Fantastic! Thanks for your help.