-
Notifications
You must be signed in to change notification settings - Fork 215
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
Improve repo documentation #312
Conversation
@@ -104,16 +104,11 @@ | |||
## [5.1.0](https://github.com/auth0/auth0-PHP/tree/5.1.0) (2018-03-02) | |||
[Full Changelog](https://github.com/auth0/auth0-PHP/compare/5.0.6...5.1.0) | |||
|
|||
[State validation](https://auth0.com/docs/protocols/oauth2/oauth-state) was added in 5.1.0 for improved security. By default, this uses session storage and will happen automatically if you are using a combination of `Auth0::login()` and any method which calls `Auth0::exchange()` in your callback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to the Troubleshooting section of the SDK docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep a small mention of it. Normally when people find an error the first thing is go check what changed (on the changelog). You could link the troubleshooting section in here as well.
**Closed issues** | ||
- Support for php-jwt 5 [\#210](https://github.com/auth0/auth0-PHP/issues/210) | ||
|
||
**Added** | ||
- Added XSRF State Storage / Validation [\#214](https://github.com/auth0/auth0-PHP/pull/214) ([cocojoe](https://github.com/cocojoe)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the main PR here ... no label previously
CONTRIBUTING.md
Outdated
To run tests on the SDK, you'll need to create a `.env` file in the root of this package with the following entries: | ||
|
||
- `DOMAIN` - Auth0 domain for your test tenant | ||
- `APP_CLIENT_ID` - Client ID for a test Regular Web Application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
X for a test Y is a bit confusing phrase. Maybe "A Client ID for a Regular Web Application within your test tenant" ?
## Author | ||
- implement authentication with multiple identity providers, including social (e.g., Google, Facebook, Microsoft, LinkedIn, GitHub, Twitter, etc), or enterprise (e.g., Windows Azure AD, Google Apps, Active Directory, ADFS, SAML, etc.) | ||
- log in users with username/password databases, passwordless, or multi-factor authentication | ||
- link multiple user accounts together |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a marketing approved block of text from somewhere? Linking multiple user accounts together could be misconstrued I suspect without clarifying it's consolidation from multiple sign on providers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a similar footer on every repo. Mostly copy/paste whenever a new repo is created. I don't know if this is also copy-paste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blurb comes from our template:
https://github.com/auth0/open-source-template/blob/master/README-sample.md#what-is-auth0
... which came from our content team.
- `DOMAIN` - Auth0 domain for your test tenant | ||
- `APP_CLIENT_ID` - Client ID for a test Regular Web Application | ||
- `APP_CLIENT_SECRET` - Client Secret for a test Regular Web Application | ||
- `NIC_ID` - Client ID for a test Non-Interactive Client Application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note in case of a future refactor. Since you're using the "CLIENT" word for other constants, might as well use it here and be more descriptive. In the end these are just names one should easily see and remember. e.g. NON_INTERACTIVE_CLIENT_ID
|
||
``` | ||
```bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
README.md
Outdated
} | ||
} | ||
``` | ||
The values above should not be hard-coded in a production application but will suffice for testing or local development. Please see our complete guide on the [main documentation page](https://auth0.com/docs/libraries/auth0-php#getting-started) for more information on how to store and use these values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we give the first sentence more relevance? e.g. bold or >
README.md
Outdated
To run tests on the SDK, you'll need to create a `.env` file in the root of this package with the following entries: | ||
- Use [Community](https://community.auth0.com/) for usage, questions, specific cases | ||
- Use [Issues](https://github.com/auth0/auth0-PHP/issues) here for code-level support | ||
- Paid customers can use [Support](https://support.auth0.com/) to submit a trouble ticket for production-affecting issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd normally merge both "community" and "support" under https://support.auth0.com/
which will route them accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good to call them both out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Paid customers" sounds off. How about "Customers with a paid Auth0 subscription" ? Keeps it more in-line with the phrasing at https://auth0.com/docs/support
@@ -104,16 +104,11 @@ | |||
## [5.1.0](https://github.com/auth0/auth0-PHP/tree/5.1.0) (2018-03-02) | |||
[Full Changelog](https://github.com/auth0/auth0-PHP/compare/5.0.6...5.1.0) | |||
|
|||
[State validation](https://auth0.com/docs/protocols/oauth2/oauth-state) was added in 5.1.0 for improved security. By default, this uses session storage and will happen automatically if you are using a combination of `Auth0::login()` and any method which calls `Auth0::exchange()` in your callback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep a small mention of it. Normally when people find an error the first thing is go check what changed (on the changelog). You could link the troubleshooting section in here as well.
README.md
Outdated
|
||
## License | ||
|
||
This project is licensed under the MIT license. See the [LICENSE](LICENSE.txt) file for more info. | ||
The Auth0-PHP SDK is licensed under MIT - [LICENSE](LICENSE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this link won't work. Needs to be LICENSE.txt
CONTRIBUTING.md
Outdated
|
||
- `DOMAIN` - Auth0 domain for your test tenant | ||
- `APP_CLIENT_ID` - Client ID for a Regular Web Application within your test tenant | ||
- `APP_CLIENT_SECRET` - Client Secret for a Regular Web Application within your test tenan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `APP_CLIENT_SECRET` - Client Secret for a Regular Web Application within your test tenan | |
- `APP_CLIENT_SECRET` - Client Secret for a Regular Web Application within your test tenant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR removes all extra documentation from the README in favor of the a new set of pages on the docs site and a CONTRIBUTING.md.