Skip to content

Show offline-password caching notice after Entra login#1613

Open
Copilot wants to merge 4 commits into
002-entra-password-mfa-authfrom
copilot/wip-add-caching-indicator
Open

Show offline-password caching notice after Entra login#1613
Copilot wants to merge 4 commits into
002-entra-password-mfa-authfrom
copilot/wip-add-caching-indicator

Conversation

Copilot AI commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

UDENG-10797

After the Entra password+MFA flow caches the user's password for offline login,
users had no indication that their local password was set to their Entra password
(#1520 discussion).

Changes

  • broker: Attaches a broker-owned caching notice to the granted response so it
    surfaces through the PAM conversation. The notice lives in the broker — the component
    that knows when caching occurred.

  • broker/pam: Normalizes the granted-message envelope. Granted responses were
    emitting the notice as a bare string instead of the {"message": ...} envelope every
    consumer expects, aborting already-granted logins with invalid character 'Y'. Non-string
    message values are treated as absent rather than rejecting the login.

  • pam: Fixes a double-echo on native clients (SSH). Native clients were printing
    the notice twice — once via nativeModel.sendInfo and again via sendReturnMessageToPam.
    GDM and interactive-terminal clients do not go through the native sendInfo path and
    continue to receive the notice through the PAM conversation.

Copilot AI changed the title [WIP] [WIP] Address feedback: Add caching indicator after successful login [WIP] Address feedback: Add caching indicator after successful login across PAM flows (ssh/GDM) Jun 23, 2026
Copilot AI requested a review from nooreldeenmansour June 23, 2026 15:44
Copilot AI changed the title [WIP] Address feedback: Add caching indicator after successful login across PAM flows (ssh/GDM) Fix granted-message contract that broke GDM login after MFA; add caching indicator Jun 23, 2026
@nooreldeenmansour

Copy link
Copy Markdown
Member

GDM now shows this message before the Creating directory ...
image

@nooreldeenmansour

Copy link
Copy Markdown
Member

SSH login shows this
image

It's printed twice (which we should fix), and add a line separator or somehow emphasize it, or make it harder to miss; because it shows up before the SSH login text fills the terminal

@copilot investigate why the Your password was cached for offline login. message gets printed twice for SSH login

Copilot AI commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

SSH login shows this
image

...

Fixed in 7081af0. The notice was being shown once by the native PAM flow itself and then a second time by the module’s generic auth-return message path. The follow-up change stops re-emitting granted success notices for clients that already display or defer them, so SSH should now show it once and GDM should no longer surface it during authentication.

Copilot AI changed the title Fix granted-message contract that broke GDM login after MFA; add caching indicator Fix duplicate caching notice on SSH; restore GDM display Jun 23, 2026
@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.80488% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.86%. Comparing base (b91ee52) to head (2a1b9de).

Files with missing lines Patch % Lines
internal/services/pam/pam.go 80.00% 2 Missing ⚠️
pam/internal/adapter/nativemodel.go 33.33% 2 Missing ⚠️
internal/brokers/broker.go 94.11% 1 Missing ⚠️
Additional details and impacted files
@@                       Coverage Diff                       @@
##           002-entra-password-mfa-auth    #1613      +/-   ##
===============================================================
- Coverage                        87.03%   86.86%   -0.17%     
===============================================================
  Files                              123      123              
  Lines                             8352     8377      +25     
  Branches                           111      111              
===============================================================
+ Hits                              7269     7277       +8     
- Misses                            1027     1044      +17     
  Partials                            56       56              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nooreldeenmansour nooreldeenmansour force-pushed the copilot/wip-add-caching-indicator branch 2 times, most recently from d4a92e7 to 7a0109c Compare June 23, 2026 17:50
@nooreldeenmansour nooreldeenmansour changed the title Fix duplicate caching notice on SSH; restore GDM display Show offline-password caching notice after Entra login Jun 23, 2026
@nooreldeenmansour nooreldeenmansour force-pushed the copilot/wip-add-caching-indicator branch 2 times, most recently from deb0469 to 3aab8d1 Compare June 24, 2026 07:01
@nooreldeenmansour nooreldeenmansour marked this pull request as ready for review June 24, 2026 07:24
@nooreldeenmansour nooreldeenmansour force-pushed the 002-entra-password-mfa-auth branch 3 times, most recently from 27a6aea to e853b98 Compare June 24, 2026 09:58
@nooreldeenmansour nooreldeenmansour force-pushed the 002-entra-password-mfa-auth branch 4 times, most recently from 509900b to a1e0379 Compare June 24, 2026 13:04
@nooreldeenmansour nooreldeenmansour force-pushed the copilot/wip-add-caching-indicator branch from 3aab8d1 to 827736a Compare June 24, 2026 13:51
@nooreldeenmansour nooreldeenmansour force-pushed the 002-entra-password-mfa-auth branch 2 times, most recently from 2639346 to b91ee52 Compare June 24, 2026 14:07
@nooreldeenmansour nooreldeenmansour force-pushed the copilot/wip-add-caching-indicator branch 2 times, most recently from 58f158e to 49c71ba Compare June 24, 2026 14:12
Comment thread internal/brokers/broker.go
Comment thread internal/brokers/broker.go Outdated
// response after the user's password is cached locally (during the Entra
// password + MFA flow), letting them know they can subsequently log in
// offline. It is broker-owned so it can be localized independently of authd.
const cachedPasswordMessage = "Your password was cached for offline login."

@adombeck adombeck Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the term "cached" in that message. Claude agrees (without me biasing it, I just asked it what it thinks about the existing message):

To a non-technical user, "cached" is vague — it doesn't convey what happened (their Entra password is now their local login password) or why it matters (they can log in without a network).

However, the alternatives it provides are quite long. So before proposing something, I'd like to know for how long this message is displayed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed; happy to change the message.

For GDM, it lasts around 3 seconds, and for SSH, it lasts around a second (but the other message Creating directory .... is barley noticable as well for SSH/native logins)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For GDM, it lasts around 3 seconds

then it indeed shouldn't be too long. how about "Your Entra password now works for offline login."?

@nooreldeenmansour nooreldeenmansour Jun 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, the fact that we don't say "local password" (I think we have already dropped that term in #1520?)

We could also go with "Offline login is enabled with your Entra password."

Edit: yes, we currently use Enter your password instead of Enter your local password as suggested in #1520 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also go with "Offline login is enabled with your Entra password."

I'd be fine with that as well. Choose whichever you prefer.

Edit: yes, we currently use Enter your password instead of Enter your local password as suggested in #1520 (comment)

Yeah, that's actually the more important UX improvement. Even without this message, the user will likely just enter their Entra password without questioning it - in contrast to the "Enter your local password" prompt.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid point. Entra Multifactor Authentication is a better name for the flow; however, in the placeholder holders, we should keep using the "Entra Password"

I think you may include that as part of #1614, we can discuss it further there

@adombeck adombeck Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh and another problem. Both "Your Entra password now works for offline login." and "Offline login is enabled with your Entra password." are not accurate when force_access_check_with_provider = true, because then offline login is not possible. (Side note: it's also not possible when local_password = false, but then we shouldn't even store the hashed the password and omit the message altogether)

Given that and the fact that we still call it "Local password" in the selection prompt, maybe it is actually better to use "local password" in the message instead of "offline login". Something like "Your Entra password is now your local password too."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the placeholder holders, we should keep using the "Entra Password"

do you mean in the prompt "Enter your Entra ID password"? then I agree. or is there any other place where we use placeholders?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I meant that

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it is actually better to use "local password" in the message instead of "offline login". Something like "Your Entra password is now your local password too."

Agreed, let's not do complex logic to handle the message depending on whether the user has which config property.. or not

Comment thread authd-oidc-brokers/internal/broker/authresponses.go
@nooreldeenmansour nooreldeenmansour force-pushed the copilot/wip-add-caching-indicator branch 5 times, most recently from 034f640 to 80fad75 Compare June 24, 2026 18:18
nooreldeenmansour and others added 4 commits June 24, 2026 21:22
The Entra password+MFA login failed with "invalid character 'Y' looking
for beginning of value" after a successful grant. The broker emitted the
success message as a bare string while the consumer (dataToMsg) expects
a {"message": ...} envelope, so the PAM client's parse aborted an
already-granted login.

Forward a consistent {userinfo, message} envelope from IsAuthenticated
so consumers always parse the same shape regardless of whether the broker
attached a notice. Encode IAResponse.Msg as {"message": ...} matching
the format used for non-granted replies. Non-string values in the broker's
message field are treated as absent rather than rejected, so a malformed
cosmetic field never blocks an already-granted login at the broker boundary.

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
A cosmetic notice attached to a granted IAResponse must never abort an
already-granted login. Treat a parse error on the Msg field as a
warning and fall back to showing no notice.

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
After the Entra password+MFA flow caches the user's password for offline
login, the user had no indication that their local password was set to
their Entra password

Attach a broker-owned notice to the granted response so it surfaces
through the PAM conversation. The notice lives in the broker — the
component that knows when caching occurred — rather than being hardcoded
in authd.

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Native (SSH, non-TTY) clients receive the granted message twice: once
through nativeModel.sendInfo and again through the PAM TextInfo
conversation echo in sendReturnMessageToPam.  GDM and interactive-
terminal clients do not go through the native sendInfo path, so they
must keep receiving the echo.

Suppress the redundant PAM-conversation echo only for Native clients by
returning false from shouldSendAuthMessage when clientType == Native and
the response is a success.

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
@nooreldeenmansour nooreldeenmansour force-pushed the copilot/wip-add-caching-indicator branch from 80fad75 to 2a1b9de Compare June 24, 2026 18:23
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.

3 participants