Skip to content

Allows users to select browser for authentication#2910

Merged
gingi merged 7 commits intomainfrom
dpwatrous/entra-issues
Aug 30, 2024
Merged

Allows users to select browser for authentication#2910
gingi merged 7 commits intomainfrom
dpwatrous/entra-issues

Conversation

@gingi
Copy link
Member

@gingi gingi commented Jun 3, 2024

  • Main app window now opens without prior authentication
  • New welcome screen prompts user to sign in, while hiding side menu items and allowing the user to access settings
  • An overlay prompts the user to authenticate against each tenant that requires interactive authentication, and includes a toggle for using the external (system) browser
  • App settings now have ability to toggle external browser for auth
  • Upgrades Angular to 11.2.12

Fixes #2446

image image

@codecov
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 74.35065% with 158 lines in your changes missing coverage. Please review.

Project coverage is 67.90%. Comparing base (8e438ff) to head (13cddff).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
.../core/aad/authentication/authentication.service.ts 53.60% 42 Missing and 3 partials ⚠️
desktop/src/client/core/aad/auth/aad.service.ts 25.00% 30 Missing ⚠️
...ktop/src/client/core/batch-explorer-application.ts 12.12% 29 Missing ⚠️
...esktop/src/client/core/aad/auth-loopback-client.ts 76.54% 9 Missing and 10 partials ⚠️
desktop/src/app/services/aad/auth.service.ts 78.04% 9 Missing ⚠️
.../components/tenant-picker/tenant-card.component.ts 0.00% 6 Missing ⚠️
desktop/src/client/core/aad/auth-provider.ts 87.80% 5 Missing ⚠️
...pp/services/batch-account/batch-account.service.ts 0.00% 3 Missing ⚠️
desktop/src/client/main-window/main-window.ts 25.00% 3 Missing ⚠️
.../aad/authentication/authentication.service.spec.ts 96.61% 1 Missing and 1 partial ⚠️
... and 5 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2910      +/-   ##
==========================================
+ Coverage   67.62%   67.90%   +0.28%     
==========================================
  Files        1269     1271       +2     
  Lines       34418    34750     +332     
  Branches     6276     6319      +43     
==========================================
+ Hits        23276    23598     +322     
+ Misses      10970    10968       -2     
- Partials      172      184      +12     
Files with missing lines Coverage Δ
...-flask/core/aad/access-token/access-token.model.ts 90.47% <ø> (ø)
desktop/src/@batch-flask/core/material.module.ts 100.00% <100.00%> (ø)
desktop/src/@batch-flask/ui/form/form.module.ts 100.00% <ø> (ø)
...ask/ui/form/slide-toggle/slide-toggle.component.ts 100.00% <100.00%> (ø)
.../src/app/components/settings/settings.component.ts 85.36% <100.00%> (+1.15%) ⬆️
...omponents/tenant-picker/tenant-picker.component.ts 86.66% <100.00%> (+76.66%) ⬆️
desktop/src/app/services/batch-explorer.service.ts 8.00% <ø> (+0.59%) ⬆️
desktop/src/app/services/navigator.service.ts 94.28% <100.00%> (+81.38%) ⬆️
...p/src/client/core/aad/auth-loopback-client.spec.ts 100.00% <100.00%> (ø)
desktop/src/client/core/aad/auth-provider.spec.ts 100.00% <100.00%> (+6.75%) ⬆️
... and 17 more

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e438ff...13cddff. Read the comment docs.

@gingi gingi force-pushed the dpwatrous/entra-issues branch from 3e1ede7 to ea9149d Compare June 21, 2024 19:30
@BMurri
Copy link

BMurri commented Jul 5, 2024

@gingi Is there anything any of us outside of this team could do to help get this over the finish line?

@gingi
Copy link
Member Author

gingi commented Jul 8, 2024

@gingi Is there anything any of us outside of this team could do to help get this over the finish line?

We are almost done, mostly focusing on some end-to-end testing. In the meantime, you can check out the branch and test it locally. We are trying to cover all scenarios.

@gingi gingi force-pushed the dpwatrous/entra-issues branch from b2115ae to d45e4f5 Compare July 19, 2024 16:08
@GBianca
Copy link

GBianca commented Aug 1, 2024

@gingi, this is a great update! Are you also planning to take a look at the Windows build? I'm looking forward to having the CA issues fixed on Windows too, but the app is unfortunately not starting

@MichaConrad
Copy link

MichaConrad commented Aug 7, 2024

@gingi I briefly tested this branch and I can actually log in now again.
There was one issue though, I never use Edge and my default browser is Chrome. Chrome seems to deny access to the listener on port 3874, I guess because the SSL cert isn't valid?

It works when I change my default browser to Edge. I would never do that voluntarily though, is there some way to make this work with other browsers?
If that is intentional, because... Microsoft, then you got me good and I finally have to use Edge 😀
^ kidding

Great work though, waiting for this to get released hopefully soon...?

@gingi gingi force-pushed the dpwatrous/entra-issues branch 5 times, most recently from d176ba5 to 4a3b987 Compare August 20, 2024 00:40
@gingi gingi force-pushed the dpwatrous/entra-issues branch from f2ec811 to 09a744f Compare August 27, 2024 15:57
@eusebiu
Copy link

eusebiu commented Aug 28, 2024

I downloaded the pipeline artifacts for windows (https://azurebatch.visualstudio.com/BatchExplorer/_build/results?buildId=8480&view=artifacts&pathAsName=false&type=publishedArtifacts) and if the "Use system browser for authentication" is unchecked, I get an empty popup when pressing Sign in to Azure.
image

@gingi gingi force-pushed the dpwatrous/entra-issues branch from 09a744f to 42c74a1 Compare August 28, 2024 21:40
@gingi
Copy link
Member Author

gingi commented Aug 28, 2024

I downloaded the pipeline artifacts for windows (https://azurebatch.visualstudio.com/BatchExplorer/_build/results?buildId=8480&view=artifacts&pathAsName=false&type=publishedArtifacts) and if the "Use system browser for authentication" is unchecked, I get an empty popup when pressing Sign in to Azure. image

Hi @eusebiu. That has just been addressed. Authentication should now be supported both through the built-in window and the system browser (currently Edge).

@gingi
Copy link
Member Author

gingi commented Aug 28, 2024

@gingi I briefly tested this branch and I can actually log in now again. There was one issue though, I never use Edge and my default browser is Chrome. Chrome seems to deny access to the listener on port 3874, I guess because the SSL cert isn't valid?

It works when I change my default browser to Edge. I would never do that voluntarily though, is there some way to make this work with other browsers? If that is intentional, because... Microsoft, then you got me good and I finally have to use Edge 😀 ^ kidding

Great work though, waiting for this to get released hopefully soon...?

The PR is pretty much ready and should be merged soon to appear in an upcoming insider build. The issue with Chrome will unfortunately need to be addressed later on, as it requires a service change with Entra, which will allow us to use a custom protocol for the auth redirect URI. Presently we're using localhost loopback, which browsers aren't too happy about.

gingi and others added 5 commits August 28, 2024 19:14
Needed for slide toggles.
* Main app window now opens without prior authentication
* New welcome screen prompts user to sign in, while hiding side menu items and allowing the user to access settings
* An overlay prompts the user to authenticate against each tenant that requires interactive authentication, and includes a toggle for using the external (system) browser
* App settings now have ability to toggle external browser for auth
gingi added 2 commits August 28, 2024 19:14
Moved out of the constructor in the case when the windows are reloaded. Fixes an issue where a system error causes IPC messages to be ignored because they are not recreated.
@gingi gingi force-pushed the dpwatrous/entra-issues branch from 42c74a1 to 13cddff Compare August 28, 2024 23:14
@BMurri
Copy link

BMurri commented Aug 29, 2024

I just tested this, and I'm able to authenticate again It's wonderful, but I cannot use it as-is. My one big remaining problem? I need to be able to select the account I authenticate with: It's NOT the account I'm signed into Windows with, it's a separate account used to isolate access to production systems from people who might steal my credentials without my participation.

When using this, I just get a "... you can close this window" without any opportunity to select which account to use.

@gingi
Copy link
Member Author

gingi commented Aug 30, 2024

I just tested this, and I'm able to authenticate again It's wonderful, but I cannot use it as-is. My one big remaining problem? I need to be able to select the account I authenticate with: It's NOT the account I'm signed into Windows with, it's a separate account used to isolate access to production systems from people who might steal my credentials without my participation.

When using this, I just get a "... you can close this window" without any opportunity to select which account to use.

I understand. Are you able to visit the Authentication Settings view to select which Entra tenants (or "directories") you want to retrieve Batch accounts from?
Batch_Explorer

@gingi gingi merged commit 917df4f into main Aug 30, 2024
@gingi gingi deleted the dpwatrous/entra-issues branch August 30, 2024 18:55
@gingi
Copy link
Member Author

gingi commented Aug 30, 2024

@BMurri Here's a PR that prompts you to select an account. Let me know if this addresses your issue.

@gingi
Copy link
Member Author

gingi commented Aug 30, 2024

@MichaCo Here's some documentation on how to make Entra authentication work with non-Edge browsers. Hopefully it's a good stop-gap until our back-end configuration is sorted out.

@illgitthat
Copy link

@BMurri Here's a PR that prompts you to select an account. Let me know if this addresses your issue.

Wow, what timing when I experience an issue after downloading batch explorer for the first time and see it was fixed a mere 45 mins ago. Thanks @gingi !

Downloading from https://azurebatch.visualstudio.com/3426cbfe-4c9a-4da4-88df-70f025a77017/_apis/build/builds/8495/artifacts?artifactName=windows&api-version=7.1&%24format=zip got it working for me.

@BMurri
Copy link

BMurri commented Aug 30, 2024

@BMurri Here's a PR that prompts you to select an account. Let me know if this addresses your issue.

@gingi It works perfectly! Thank you so much!

@MichaConrad
Copy link

MichaConrad commented Sep 1, 2024

merged soon to appear in an upcoming insider build. The issue with Chrome will unfortunately need to be addressed later on, as it requires a service change with Entra, which will allow us to use a custom protocol for the auth redirect URI. Presently we're using localhost loopback, which

Thanks for into and the docs link @gingi !
Turns out the plugin was already installed on my work laptop, the login flow still did not work though.
Reason was that Chrome auto redirects all http to https calls.

I must have set some hsts policy for localhost, probably by one of our services because I was able to fix it by removing the hsts policy for localhost.

To do so:

  • go to chrome://net-internals/#hsts
  • type "localhost" in the last box and click delete
    image

Now I can login with Chrome, too 🎉

@illgitthat
Copy link

@BMurri Here's a PR that prompts you to select an account. Let me know if this addresses your issue.

Wow, what timing when I experience an issue after downloading batch explorer for the first time and see it was fixed a mere 45 mins ago. Thanks @gingi !

Downloading from https://azurebatch.visualstudio.com/3426cbfe-4c9a-4da4-88df-70f025a77017/_apis/build/builds/8495/artifacts?artifactName=windows&api-version=7.1&%24format=zip got it working for me.

Following up here @gingi - this worked for me on initial download and startup but then when I was timed out / tried to reauthenticate I ran into the same error.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to login with an AAD account that have Conditional Access enabled

8 participants