Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stronger typing using the NewType pattern #38

Closed
wants to merge 11 commits into from
Closed

Add stronger typing using the NewType pattern #38

wants to merge 11 commits into from

Conversation

ramosbugs
Copy link
Owner

NOTE: This is another breaking change that will be part of the 2.0.0 release.

This diff replaces weakly typed Strings, Urls, and other types with new
types generated using the NewType pattern. Using stronger types here
should avoid common mistakes (e.g., switching the order of the authorization
and endpoint URLs when instantiating a new Client).

In addition to adding a NewType trait, this diff adds a NewSecretType
trait, which implements Debug in a way that redacts the secret. This
behavior avoids a common source of security bugs: logging secrets,
especially when errors occur. Unlike the NewType trait, the
NewSecretType does not implement Deref. Instead, the secret must
be explicitly extracted by calling the secret method.

Finally, this PR resolves #28 by having the authorize_url method accept a
closure for generating a fresh CSRF token on each invocation. The token is
returned by the method as #[must_use], which the caller should compare
against the response sent by the authorization server to the redirect URI.
Note that #[must_use] currently has no effect in this context, but it should
once rust-lang/rust#39524 is resolved.

@ramosbugs
Copy link
Owner Author

Closing this PR since it points to an orphaned branch after the repo was transferred to me. Reopening separately.

@ramosbugs ramosbugs closed this Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant