redirect uri from route

jimmypuckett commented 3 years ago

This will Fix #37

I am using a named route azure.callback when defining the route, so there is really nothing extra to configure.

jimmypuckett commented 3 years ago

@danieltjewett I got to thinking about this change, & it would be breaking for someone that already had the package installed without the named route, so I am not sure if you would want to put logic testing for the named route or tag a minor version change?

jimmypuckett commented 3 years ago

One note about this...

Yes, I would agree. And, for those like us who are using the callback from the portal as oppose this convention would also have potential issues. I would assume the callback param passed would take precedence over what is in the portal.

You still have to set up the callback in the portal, and the callback is passed in will not work unless it is in the portal first. This just allows you to have a single app configured with multiple callbacks for different environments (local, staging, production, etc).

jimmypuckett commented 3 years ago

@danieltjewett I will try to look over these suggestions/changes later tonight. It is going to be a very busy week, so I will see what I can do. Thanks for the feedback.

danieltjewett commented 3 years ago

@jimmypuckett Not a problem! I understand the busyness (which is why I took a couple days for me to review).

danieltjewett commented 3 years ago

@jimmypuckett Missed the first comment. Love the idea of having multiple environments. We've been creating separate apps for different environments. This is way better :D

jimmypuckett commented 3 years ago

@danieltjewett I finally got a break to make the tweaks that you suggested. Please let me know if there is anything else.

Thanks.

jimmypuckett commented 3 years ago

@danieltjewett I know that I got tied up & took a bit to implement your suggestions, but do you think that you could review/merge this. We have been working on a project that is a few weeks from being live, so this would be super nice to have for the UAT & prod environments without having to set up additional applications in the portal.

Thanks!

danieltjewett commented 3 years ago

@jimmypuckett sorry about that. I am actually no longer a part of the rootinc org, and didn't get this notification via normal email. I just happened to see this in my github notifications. @jaredrhodes could you review this?

jimmypuckett commented 3 years ago

@danieltjewett thanks for the update.

@jaredrhodes if this is not something that you guys have the time to keep alive, I would be happy to take over the support & maintenance of it along with all of our other Laravel packages. Just let me know. Thanks.

jaredrhodes commented 3 years ago

Hi @jimmypuckett, #thanksdan @danieltjewett. Since I'm fresh on this, correct me wrong...

Summary: Handle multiple environments within a single App Registration.

In a single Azure App Registration, one can register multiple "Redirect URIs" like "https://myapp.test/auth", and "https://staging.myapp.com/auth" to handle multiple environments.
If no 'azure.callback' named route exists carry on as previous using the only or last "Redirect URI" registered in Azure App Registration.
If a route is named 'azure.callback' will be turned into a a URL and sent off in the auth request. If a match is found in Azure App Registration's "Redirect URIs", a valid login will redirect to that URL.

This looks to be backward compatible where if someone does not register a route with ->name('azure.callback'); we carry on as usual and if they do register a route with ->name('azure.callback'); we use that value?

jimmypuckett commented 3 years ago

Yes, that is the key concept of the MR, and yes, it should be backward compatible.

danieltjewett commented 3 years ago

@jaredrhodes That is correct. :shipit:

rootinc / laravel-azure-middleware

redirect uri from route #53