Add LICENSE file and update README.md with improved language/guides #36
Conversation
Replaced 'we' with you and that. Made instruction a bit more user friendly. Changed routes to be more intuitive. Added names to routes. Changed name of AzureApp middleware class to AzureAuthentication.
There was a problem hiding this comment.
@Kvaksrud Thanks for contributing! We're glad you find this package awesome and useful.
I like most of the updates provided. If you could make the following updates, we can merge this in.
README.md
Outdated
| { | ||
| protected function handlecallback($request, Closure $next, $access_token, $refresh_token) | ||
| { | ||
| $user = Auth::user(); | ||
| if($user === null) // If the application looses user data while user is authenticated with Laravel "catch" (i.e: User is deleted) | ||
| return response()->redirectToRoute('azure.login'); // Make sure the route name is correct if you changed it |
There was a problem hiding this comment.
A couple of things here;
- I'd suggest redirecting to the built in redirect method https://github.com/rootinc/laravel-azure-middleware/blob/master/src/Azure.php#L122
- We may also want to flush
session()->get('_rootinc_azure_access_token');andsession()->get('_rootinc_azure_refresh_token'); - Though this is an example and might be getting to verbose?
There was a problem hiding this comment.
This idea is also echoed in the Custom Redirect section as well.
There was a problem hiding this comment.
I'm not sure how to do nr. 2. Could you help @danieltjewett?
3. Personally I like examples with a lot of content to help me build from. It helps me avoid mistakes other might have done before me, that I eventually will run into.
There was a problem hiding this comment.
2 and 3 kind of go together. If we add the proposed in the example, and somehow we get to a spot where user is null (because they were deleted from the DB), we should also add
session()-> forget(['_rootinc_azure_access_token', '_rootinc_azure_refresh_token']);
in the condition right before we redirect the user back to the login page. Otherwise, https://github.com/rootinc/laravel-azure-middleware/blob/master/src/Azure.php#L41 would be ignored, potentially reaching an infinite redirect situation. Adding all of this could be verbose though as (at least originally) the documentation was showing a simple example of how to update the user field. I do understand your point though. I'll let you decide 👍 If you think it is for the best to include it all for the documentation, go ahead and make the updates with forgetting the session keys and we can push it through.
I would like to suggest the added changes to README.md as well as inserting the LICENSE file for MIT to the project.
In short, this is what I have done:
I felt like this is some boring documenting tasks that nobody wants to do, but having implemented this awesome package in a lot of projects I keep wanting better documentation, so hopefully it falls within good taste :)