Update Azure AD/OIDC sample #29

guardrex · 2017-10-03T09:28:34Z

Addresses dotnet/AspNetCore.Docs#4449

@jmprieur Here it is. First draft, of course. 😄

Notes:

xplat and x-tooling:tm: I call out command-line setup vs. VS setup and config where needed.
Derived from the dotnet new mvc template with --auth SingleOrg. They can get the sample via the command line or by cloning/downloading the repo.
README is updated.
- It might need a touch here and there on review, but this is a good initial draft. I'm not accustomed to working with README for built topics, so I won't be surprised if some changes are required to conform to your build system/rendered topic.
- Guidelines adherence and style/grammar updates ... the usual stuff.
On the mapping of Azure portal values to app config settings, I attempt to bridge the gap in technologies:
- Client ID and Application ID
- Tenant ID appearing in endpoint URLs in the portal
I didn't look 👀 to see if this sample is linked into Azure topics. If that's a potential problem (i.e., topics say things about the sample that require updates given these changes), let me know ... I'll search (tomorrow ... time for some 💤 😴 here lol).

cc/ @Rick-Anderson

acomsmpbot · 2017-10-03T09:28:40Z

No issues were found in this pull request.

msftclas · 2017-10-03T09:28:45Z

All CLA requirements met.

acomsmpbot · 2017-10-03T21:04:36Z

No issues were found in this pull request.

jmprieur

Very nice, thank you.
I've proposed a few addition and have asked a question, otherwise this looks very good to me!

jmprieur · 2017-10-07T02:32:36Z

README.md


-### Step 1:  Clone or download this repository
+1. Sign in to the [Azure portal](https://portal.azure.com).
+2. Navigate to the Azure Active Directory blade.


We need to explain a bit more here. people might have several tenant:
2. On the top bar, click on your account and under the Directory list, choose the Active Directory tenant where you wish to register your application
3. Click on More Services in the left hand nav, and choose Azure Active Directory.

jmprieur · 2017-10-07T02:36:42Z

README.md


-### Step 4:  Run the sample
+- Add Bower to the project file:


Why do we need to do that? I did not (I did Dotnet run immediately) and that worked?

jmprieur · 2017-10-07T02:37:44Z

README.md


-## About The Code
+Make a request to the app. The app immediately attempts to authenticate you via Azure AD. Sign in with your Global Administrator account.


Why do we need to be a global admin account? that should work with any account of the tenant?

jmprieur · 2017-10-07T02:39:19Z

Startup.cs

+            app.UseAuthentication();
+
+            app.UseMvcWithDefaultRoute();
+        }


when doing Dotnet new, also got the following code:
app.UseMvc(routes =>
{
routes.MapRoute(
name: "default",
template: "{controller=Home}/{action=Index}/{id?}");
});

acomsmpbot · 2017-10-08T17:47:47Z

No issues were found in this pull request.

guardrex · 2017-10-08T17:48:06Z

We need to explain a bit more here.

Yes, I was a bit overzealous in attempting to simplify the steps. I put that back. I add a link over to the tenant topic in case the reader would like more info on tenants. Also, the list there should auto-number for us with a "1." on each list item. Makes it a little easier in the future to insert and delete items.

Why do we need to be a global admin account? that should work with any account of the tenant?

I was just trying to keep it simple. I added more information that includes using a user account.

when doing Dotnet new, also got the following code:

Yes, they're probably going to be switching over to the UseMvcWithDefaultRoute convenience method shortly (including for the 2.0 templates) (aspnet/templating 31). I ask them there if and how fast they plan to get a PR in, which will signal that they approve of the change. Even if they only make the change for 2.1, going ahead here with the change will harden the instructions, and a new issue won't be required to update at 2.1 (that's just a suggestion, of course). It's just one less thing to have to deal with later. 😄

Why do we need to do that? I did not (I did Dotnet run immediately) and that worked?

Ah ... yes. TL;DR here ... but there's a bit to explain on this one, so bear with me.

This is most interesting (and slightly disturbing) part! lol It turns out that this sample (and many others that were produced in VS for topics) are broken when running outside of VS for Bower and BundlerMinfier.Core.

The sample itself (the app) runs OOB (e.g., dotnet run) because the static assets are present in the sample ... the wwwroot/lib assets and minified+bundled CSS and JS are there. Bower and BundlerMinifier.Core features work in VS because VS has light-up features when the configuration files for these technologies are present. However, the sample is broken outside of VS for Bower and BundlerMinfier.Core. They simply won't run outside of VS as there's nothing to trigger them to run.

As u know, there's a huge ASP.NET Core xplat push (and x-tooling for that matter, e.g., VS Code and command line). I take MS at its word on this: Samples for topics that describe and use features that work xplat and x-tooling shouldn't rely upon baked-in features of VS in order to function (logically I mean 😄).

To make this sample run Bower and BundlerMinifier.Core outside of VS, the project file needs a little help. [btw- I made a small revision there on the last commit to use the BuildBunderMinifier package to avoid the need for the bower install target, which composes the project file a little better.]

Now having said all of that, they're removing Bower and BundlerMinifier.Core as we speak. However, they won't say what's replacing it exactly. I mean they do say that they're going to use LibraryInstaller (or something like it). I've asked for more info on xplat/x-tooling scenarios (aspnet/LibraryInstaller 3), but they haven't revealed yet how they plan to make it work in non-VS scenarios.

What we could do here for now is follow their lead and strip out all of the Bower and BundlerMinifier.Core bits and leave the wwwroot/lib assets and minfied+bundled styles and scripts in place. The sample will run fine, as you say, but it will simply use static assets without any machinery to produce those assets. Let me know if you want to do that, and I'll make another pass to drop the tooling bits. Then when they announce what they would like to do with xplat/x-tooling samples, samples can have a pass to make them bundle, minify, and obtain static assets using LibraryInstaller.

For reference on all of that, see:

acomsmpbot · 2017-10-12T03:44:02Z

No issues were found in this pull request.

guardrex · 2017-10-12T03:50:19Z

Now that we have their answer on the Bower/BundlerMinifier.Core, I removed those from the sample and from the README. We're good there. Later, they'll add Library Manager (PackMan), and the sample (and others) should get the new system for managing, minifying, and bundling static assets.

On the UseMvcWithDefaultPath, they still haven't decided what to do on that one yet ... there's discussion on which way they want to go. aspnet/Templating#31

I recommend leaving UseMvcWithDefaultPath in the sample for now. I agree with [@]Rick-Anderson on this one that using the routes.MapRoute is overkill and a source of confusion. If they decide to stick with the nasty one 😄, then it's an easy/quick patch later.

acomsmpbot · 2017-11-21T07:22:58Z

No issues were found in this pull request.

acomsmpbot · 2017-11-21T07:23:51Z

No issues were found in this pull request.

acomsmpbot · 2017-11-21T07:24:30Z

No issues were found in this pull request.

acomsmpbot · 2017-11-21T07:39:40Z

No issues were found in this pull request.

Rick-Anderson · 2017-11-21T21:24:08Z

@jmprieur What can I do to help get this merged? @guardrex is under strict orders to say on vacation and not work for free. I'd like to make a few minor edits (for example, per ms style guide, avoid please), but that could be done after it's merged.

Update Azure AD/OIDC sample

1df77e6

danieldobalian requested review from danieldobalian, jmprieur and andretms October 3, 2017 17:49

Add a note on the sign-on and logout URLs

01fbad1

jmprieur suggested changes Oct 7, 2017

View reviewed changes

React to feedback

ac658f3

Drop Bower and BundlerMinifier.Core

d7ba884

Updates

229a9d2

Merge branch 'master' into guardrex/sample-update

4e435a8

Merge branch 'master' into guardrex/sample-update

657af64

Add back Bower + BundlerMinifier files

c80a0dd

guardrex mentioned this pull request Nov 21, 2017

Update "Integrating Azure AD into an ASP.NET Core web app" to ASP.NET 2.0 dotnet/AspNetCore.Docs#4449

Closed

jmprieur approved these changes Nov 22, 2017

View reviewed changes

jmprieur merged commit 57f7794 into Azure-Samples:master Nov 22, 2017

guardrex deleted the guardrex/sample-update branch November 22, 2017 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Azure AD/OIDC sample #29

Update Azure AD/OIDC sample #29

guardrex commented Oct 3, 2017 •

edited

Loading

acomsmpbot commented Oct 3, 2017

msftclas commented Oct 3, 2017 •

edited

Loading

acomsmpbot commented Oct 3, 2017

jmprieur left a comment

jmprieur Oct 7, 2017

jmprieur Oct 7, 2017

jmprieur Oct 7, 2017

jmprieur Oct 7, 2017

acomsmpbot commented Oct 8, 2017

guardrex commented Oct 8, 2017 •

edited

Loading

acomsmpbot commented Oct 12, 2017

guardrex commented Oct 12, 2017

acomsmpbot commented Nov 21, 2017

acomsmpbot commented Nov 21, 2017

acomsmpbot commented Nov 21, 2017

acomsmpbot commented Nov 21, 2017

Rick-Anderson commented Nov 21, 2017


		## About The Code
		Make a request to the app. The app immediately attempts to authenticate you via Azure AD. Sign in with your Global Administrator account.

Update Azure AD/OIDC sample #29

Update Azure AD/OIDC sample #29

Conversation

guardrex commented Oct 3, 2017 • edited Loading

acomsmpbot commented Oct 3, 2017

msftclas commented Oct 3, 2017 • edited Loading

acomsmpbot commented Oct 3, 2017

jmprieur left a comment

Choose a reason for hiding this comment

jmprieur Oct 7, 2017

Choose a reason for hiding this comment

jmprieur Oct 7, 2017

Choose a reason for hiding this comment

jmprieur Oct 7, 2017

Choose a reason for hiding this comment

jmprieur Oct 7, 2017

Choose a reason for hiding this comment

acomsmpbot commented Oct 8, 2017

guardrex commented Oct 8, 2017 • edited Loading

acomsmpbot commented Oct 12, 2017

guardrex commented Oct 12, 2017

acomsmpbot commented Nov 21, 2017

acomsmpbot commented Nov 21, 2017

acomsmpbot commented Nov 21, 2017

acomsmpbot commented Nov 21, 2017

Rick-Anderson commented Nov 21, 2017

guardrex commented Oct 3, 2017 •

edited

Loading

msftclas commented Oct 3, 2017 •

edited

Loading

guardrex commented Oct 8, 2017 •

edited

Loading