Dependency Update for Ch1 Vanilla JS by BenBagBag · Pull Request #85 · Azure-Samples/ms-identity-ciam-javascript-tutorial

BenBagBag · 2024-06-06T18:11:19Z

Purpose

Updates NPM packages to most recent versions
Fixes some bugs in the sample
Fixes tests

Does this introduce a breaking change?

[ ] Yes
[ x ] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ x ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ x ] Other... Please describe: Dependency update

How to Test

Get the code

git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install

Test the code

Add your app creds to authConfig.js

cd 1-Authentication\0-sign-in-vanillajs\App
npm start

Go to localhost:3000
Sign in, with an external ID
Test popup option by commenting out lines indicated in index.html and redirect.html
Run tests by deleting your creds in authConfig.js and then running jest test

What to Check

Verify that the following are valid

You can log in
You see a page verifying that you're logged in
You can log out
You can click a link in the logout page to get back to the signin/welcome page

Other Information

iambmelt · 2024-06-06T21:05:03Z

@BenBagBag - The diff for this PR is pretty noisy so I'm having trouble seeing where the differences are from main branch, are there specific sets of this change you want to call attention to for review?

BenBagBag · 2024-06-06T21:19:53Z

@@ -10,13 +10,13 @@
    "author": "",
    "license": "ISC",


Updated dependencies.

BenBagBag · 2024-06-06T21:20:40Z

-        authority: 'https://Enter_the_Tenant_Subdomain_Here.ciamlogin.com/', // Replace the placeholder with your tenant subdomain
-        redirectUri: '/', // You must register this URI on Azure Portal/App Registration. Defaults to window.location.href e.g. http://localhost:3000/
+        authority: 'https://Enter_the_Tenant_Subdomain_Here.ciamlogin.com/', // Replace the placeholder with your tenant subdomain        
+        redirectUri: 'http://localhost:3000/redirect', // You must register this URI on Azure Portal/App Registration. Defaults to window.location.href e.g. http://localhost:3000/


Fixed the redirect URI so that the popup option works properly.

BenBagBag · 2024-06-06T21:21:26Z

+     * https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-browser/docs/request-response-object.md#request
+     */
+    loginRequest.redirectUri = "/redirect";
+    myMSALObj.loginPopup(loginRequest)


Added a value to loginRequest or else this option gives a blank page after authing.

BenBagBag · 2024-06-06T21:22:37Z

+ * response returned from redirect flow. For more information, visit:
+ * https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-browser/docs/initialization.md#redirect-apis
+ */
+myMSALObj.handleRedirectPromise()


Wrapped this in a promise to make sure it doesn't run before the myMSALObj is initialized.

BenBagBag · 2024-06-06T21:23:50Z

-    <!-- uncomment the above line and comment the line below if you would like to use the redirect flow -->
    <script type="text/javascript" src="./authRedirect.js"></script>
+    <!-- comment the above line and uncomment the line below if you would like to use the popup flow -->
+    <!-- <script type="text/javascript" src="./authPopup.js"></script> -->


Made this a little clearer, as the tutorial assumes that redirect is your default, not popup.

BenBagBag · 2024-06-06T21:24:23Z

    https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-browser/docs/login-user.md#redirecturi-considerations 
-->
+-->
+<script src="/msal-browser.min.js"></script>


Without the script tags, the redirect page won't actually redirect.

The original intention for this page was for it to be used in the popup flow, where MSAL polls the URL from the main window. I think it'd be worth trying to just use /redirect as the redirectUri in the popup request and leave the redirect as localhost:3000 in the redirect request. This way you can remove these scripts from this page and leave it actually blank.

BenBagBag · 2024-06-06T21:25:42Z

-        global.document.documentElement.innerHTML = html.toString();
-    });
-
-    it('should have valid cdn link', () => {


Tutorial doesn't actually use the CDN link option, so removed this test.

Also updated some file paths so that the tests pass.

BenBagBag · 2024-06-06T21:27:15Z

@iambmelt Sorry, that's due to the Git issue I mentioned in Teams. I put in comments to show where the changes happened. Shouldn't happen in future commits.

hectormmg · 2024-06-06T22:20:48Z

+
+    // Choose which account to logout from by passing a username.
+    const logoutRequest = {
+        account: myMSALObj.getAccountByUsername(username),


Can you update this to use the new getAccount API, this is deprecated...

Suggested change

account: myMSALObj.getAccountByUsername(username),

account: myMSALObj.getAccount({ username: username }),

Please make sure the change works ofc

Updated, please take another look.

hectormmg · 2024-06-06T22:23:43Z

    https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-browser/docs/login-user.md#redirecturi-considerations 
-->
+-->
+<script src="/msal-browser.min.js"></script>


The original intention for this page was for it to be used in the popup flow, where MSAL polls the URL from the main window. I think it'd be worth trying to just use /redirect as the redirectUri in the popup request and leave the redirect as localhost:3000 in the redirect request. This way you can remove these scripts from this page and leave it actually blank.

iambmelt

signing off as well, thanks @BenBagBag

BenBagBag · 2024-06-07T16:25:01Z

@iambmelt and @hectormmg I do not have merge permissions. :)

BenBagBag added 2 commits June 6, 2024 11:06

history fix

3a47f4c

remove creds

b04d5ef

BenBagBag changed the title ~~history fix~~ Dependency Update for Ch1 Vanilla JS Jun 6, 2024

iambmelt self-requested a review June 6, 2024 20:59

BenBagBag commented Jun 6, 2024

View reviewed changes

hectormmg suggested changes Jun 6, 2024

View reviewed changes

update api call

c6b56cb

hectormmg approved these changes Jun 7, 2024

View reviewed changes

iambmelt approved these changes Jun 7, 2024

View reviewed changes

iambmelt merged commit 08a3ba5 into Azure-Samples:main Jun 7, 2024

	account: myMSALObj.getAccountByUsername(username),
	account: myMSALObj.getAccount({ username: username }),

Conversation

BenBagBag commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Does this introduce a breaking change?

Pull Request Type

How to Test

What to Check

Other Information

Uh oh!

iambmelt commented Jun 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenBagBag commented Jun 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iambmelt left a comment

Choose a reason for hiding this comment

Uh oh!

BenBagBag commented Jun 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BenBagBag commented Jun 6, 2024 •

edited

Loading