Allow setting custom reason prompt for Access Requests by kopiczko · Pull Request #54642 · gravitational/teleport

kopiczko · 2025-05-08T20:50:26Z

Issue: #29475

Requires #54660

E companion: https://github.com/gravitational/teleport.e/pull/6502

Changes:

if any of the user's role is configured with .spec.options.request_prompt then that request will be displayed in the reason box in the UI (screenshot below)
for multiple roles .spec.options.request_prompt prompts are sorted and deduplicated
(not a change) for auto-request only one random prompt is displayed for backward compatibility

Bonus changes:

now we can validate if reason is required in the UI before sending request to backend
as a consequence of the above, the reason is never required for the dry-run access request

After pressing "Submit Request", when 2 prompts are configured and the reason is required:

Note: Teleport Connect support is added in #55092

changelog: UI: Access Request reason prompts configured in Role.spec.options.request_prompt are now displayed in the reason text box, if such a role is assigned to the user.

gzdunek · 2025-05-15T13:32:07Z

 }) {
-  const { valid, message } = useRule(requireText(reason, requireReason));
+  const { valid, message } = useRule(
+    requireText(reason, requireReason || reasonMode === 'required')


There's already a requireReason field https://github.com/gravitational/teleport.e/blob/b24b076f260ad5900f589a04cd7debc4f3059527/web/teleport/src/Workflow/NewRequest/useRequestCheckout.tsx#L226.

How does it differ from reasonMode? Could we only have requireReason?

They come from different fields in the role.

requireReason comes from .spec.options.request_access and the reasonMode comes from .spec.allow.request.reason.mode. They have slightly different semantics:

https://goteleport.com/docs/admin-guides/access-controls/access-requests/access-request-configuration/#requiring-request-reasons

https://goteleport.com/docs/admin-guides/access-controls/access-requests/access-request-configuration/#how-clients-request-access

for RequestCheckout i don't think we need the distinction between reasonMode and requireReason (especially if my mental wiring was correct, reasonMode refers to both fields?).

all we need to know is is reason required so we can mark the textbox as required.

I removed reasonMode from there. The handling is done in https://github.com/gravitational/teleport.e/pull/6502/files#diff-b64b7aa2754e33df83b39bcee2a857c345415c9a1c8de0f5138424c48a3a7acdR228

kimlisa · 2025-05-16T17:13:41Z

+	// requireReasonForAllRoles indicates that non-empty reason is required for all access
+	// requests. This happens if any of the user roles has options.request_access "reason".


~~is the reason mode included in the flag?~~

access request isn't confusing at all 😭 (i just looked at the requiringReasonRoles)

i would mention in comment, not to be confused with spec.allow.request.reason.mode

can we also mention that this flag takes precedence over the reason mode?

There is no precedence. If any of them matches then the reason is requred.

kimlisa · 2025-05-16T17:38:52Z

 			return requestValidator{}, trace.Wrap(err)
 		}
 	}
+	slices.Sort(m.reasonPrompts)


hrm, what benefit do we get by sorting the prompts? i'd imagine a user would not expect that or what if prompts are built from previous prompts?

There is no guaranteed order they will come from the backend. I'd like to avoid situation where the order is changed on every refresh. So by extension prompts can't be build from previous prompts.

kimlisa · 2025-05-16T17:44:37Z

+			return nil, trace.Wrap(err)
+		}
+		if required {
+			enrichment.ReasonMode = types.RequestReasonModeRequired


i'm not sure enrichment.ReasonMode the ReasonMode is the right variable name to use b/c it makes it sound like that's the spec.allow.request.reason.mode value, but it's that or the options.request_access (which takes precedence).

I'd probably rename it to just enrichment.ReasonRequired or RequireReason

options.request_access doesn't take precedence. It may look like that from the implementation because it's more efficient to check options.request_access first. I'd even say checking options.request_access for regular (not autologin) access requests was a bad decision.

I prefer to stick with the enum because in unlikely case this mode is extended we won't have to change the wire format.

kimlisa · 2025-05-16T18:24:02Z

  thresholdNames: ['default'],
  resources: [],
  assumeStartTime: null,
+  reasonMode: 'optional',


i just want to make sure, is this reasonMode referring specifically to field spec.allow.request.reason.mode

It covers both spec.allow.request.reason.mode and spec.options.request_access

I think I know why you may be confused. The information take from the user state may be already outdated. This reasonMode thing will calculated from the current backend state regardless of the roles in the cert.

kimlisa · 2025-05-16T18:28:28Z

 }) {
-  const { valid, message } = useRule(requireText(reason, requireReason));
+  const { valid, message } = useRule(
+    requireText(reason, requireReason || reasonMode === 'required')


for RequestCheckout i don't think we need the distinction between reasonMode and requireReason (especially if my mental wiring was correct, reasonMode refers to both fields?).

all we need to know is is reason required so we can mark the textbox as required.

kopiczko · 2025-05-22T08:01:55Z

@gzdunek @kimlisa @smallinsky PTALA

smallinsky · 2025-05-22T11:20:23Z

+	if m.opts.dryRun {
+		return enrichment, nil
+	}
+	return nil, nil


This is quite scary interface.

I would expect that the err == nil the object is valid

Can we just req.SetDryRunEnrichment(enrichment) ?

gzdunek · 2025-05-22T14:43:39Z

            onStartTimeChange={onStartTimeChange}
            fetchKubeNamespaces={fetchKubeNamespaces}
            updateNamespacesForKubeCluster={updateNamespacesForKubeCluster}
+            reasonPrompts={reasonPrompts}


In the line 272 we should also add:

requireReason={reasonMode === 'required'}

Another thing is that these prompts work only in Web UI, not in Teleport Connect (teleterm).
To add them there too, we need to extend the below struct with these two new fields (reasonMode and reasonPrompts):

teleport/lib/teleterm/apiserver/handler/handler_access_requests.go

Lines 203 to 221 in f33bbbf

return &api.AccessRequest{

Id: req.GetName(),

State: req.GetState().String(),

ResolveReason: req.GetResolveReason(),

RequestReason: req.GetRequestReason(),

User: req.GetUser(),

Roles: req.GetRoles(),

Created: timestamppb.New(req.GetCreationTime()),

Expires: timestamppb.New(req.GetAccessExpiry()),

Reviews: reviews,

SuggestedReviewers: req.GetSuggestedReviewers(),

ThresholdNames: thresholdNames,

ResourceIds: requestedResourceIDs,

Resources: resources,

PromotedAccessListTitle: req.GetPromotedAccessListTitle(),

AssumeStartTime: getProtoTimestamp(req.GetAssumeStartTime()),

MaxDuration: timestamppb.New(req.GetMaxDuration()),

RequestTtl: timestamppb.New(req.Expiry()),

SessionTtl: timestamppb.New(req.GetSessionTLL()),

and make sure that they are passed here:

teleport/web/packages/teleterm/src/ui/DocumentAccessRequests/useAccessRequests.tsx

Lines 87 to 106 in d339c65

export function makeUiAccessRequest(request: TshdAccessRequest) {

return makeAccessRequest({

...request,

created: Timestamp.toDate(request.created),

expires: Timestamp.toDate(request.expires),

maxDuration: request.maxDuration && Timestamp.toDate(request.maxDuration),

requestTTL: request.requestTtl && Timestamp.toDate(request.requestTtl),

sessionTTL: request.sessionTtl && Timestamp.toDate(request.sessionTtl),

assumeStartTime:

request.assumeStartTime && Timestamp.toDate(request.assumeStartTime),

roles: request.roles,

reviews: request.reviews.map(review => ({

...review,

created: Timestamp.toDate(review.created),

assumeStartTime:

review.assumeStartTime && Timestamp.toDate(review.assumeStartTime),

})),

suggestedReviewers: request.suggestedReviewers,

thresholdNames: request.thresholdNames,

resources: request.resources,

Unfortunately the type for an argument in makeAccessRequest is any so TypeScript doesn't show any errors about missing fields.

In case of any problems feel free to reach out to me.

Would you be ok with adding Teleport Connect support in a subsequent PR?

Yeah, sure.

kopiczko · 2025-05-22T21:33:45Z

@gzdunek PTALA

gzdunek

The frontend side looks good to me.

gzdunek · 2025-05-23T07:02:51Z

  const labelText = hasError ? message : 'Request Reason';

+  const placeholderText =
+    reasonPrompts?.filter(s => s.length > 0).join('\n') ||


Unnecessary optional chaining (optionality can also be removed in the line 739).

gzdunek · 2025-05-23T07:04:54Z

            onStartTimeChange={onStartTimeChange}
            fetchKubeNamespaces={fetchKubeNamespaces}
            updateNamespacesForKubeCluster={updateNamespacesForKubeCluster}
+            reasonPrompts={reasonPrompts}


Yeah, sure.

backport-bot-workflows · 2025-05-23T12:46:37Z

@kopiczko See the table below for backport results.

Branch	Result
branch/v17	Failed

kopiczko force-pushed the kopiczko/request_prompt_v2 branch 2 times, most recently from a4a0be8 to 1483d13 Compare May 9, 2025 19:52

kopiczko changed the base branch from master to kopiczko/request_prompt__private_validator May 9, 2025 19:52

kopiczko force-pushed the kopiczko/request_prompt_v2 branch from 1483d13 to b49f4a0 Compare May 9, 2025 20:14

kopiczko added the backport/branch/v17 label May 9, 2025

kopiczko requested a review from smallinsky May 9, 2025 20:18

kopiczko force-pushed the kopiczko/request_prompt__private_validator branch from ef61f06 to 82e2540 Compare May 9, 2025 20:32

kopiczko mentioned this pull request May 9, 2025

Make Access Request validator private #54660

Merged

kopiczko force-pushed the kopiczko/request_prompt__private_validator branch from 82e2540 to e2de975 Compare May 9, 2025 20:34

kopiczko force-pushed the kopiczko/request_prompt_v2 branch from b49f4a0 to fff9027 Compare May 9, 2025 20:39

kopiczko force-pushed the kopiczko/request_prompt__private_validator branch from e2de975 to bd9d2a4 Compare May 13, 2025 12:59

Base automatically changed from kopiczko/request_prompt__private_validator to master May 13, 2025 20:55

kopiczko force-pushed the kopiczko/request_prompt_v2 branch 8 times, most recently from 5c0cdd8 to 10422e5 Compare May 15, 2025 10:46

kopiczko marked this pull request as ready for review May 15, 2025 11:34

github-actions Bot requested review from gzdunek and kimlisa May 15, 2025 11:35

github-actions Bot added size/md tctl tctl - Teleport admin tool ui labels May 15, 2025

gzdunek reviewed May 15, 2025

View reviewed changes

gzdunek self-requested a review May 15, 2025 13:35

kimlisa reviewed May 16, 2025

View reviewed changes

kopiczko force-pushed the kopiczko/request_prompt_v2 branch from 10422e5 to c4293fa Compare May 21, 2025 20:24

kopiczko requested a review from kimlisa May 21, 2025 20:24

kopiczko force-pushed the kopiczko/request_prompt_v2 branch 3 times, most recently from 147cf9a to d1be7c2 Compare May 22, 2025 11:05

smallinsky reviewed May 22, 2025

View reviewed changes

kopiczko force-pushed the kopiczko/request_prompt_v2 branch from d1be7c2 to f28149e Compare May 22, 2025 12:55

kopiczko requested a review from smallinsky May 22, 2025 12:56

gzdunek reviewed May 22, 2025

View reviewed changes

kopiczko force-pushed the kopiczko/request_prompt_v2 branch from f28149e to 81bd936 Compare May 22, 2025 21:27

kopiczko requested a review from gzdunek May 22, 2025 21:33

gzdunek approved these changes May 23, 2025

View reviewed changes

smallinsky approved these changes May 23, 2025

View reviewed changes

kopiczko mentioned this pull request May 23, 2025

Teleport Connect: Support requiring reason and custom reason prompts #55092

Merged

2 tasks

kopiczko added 2 commits May 23, 2025 12:58

Add proto types

29e7867

Implementation

983b77c

kopiczko force-pushed the kopiczko/request_prompt_v2 branch from 81bd936 to 983b77c Compare May 23, 2025 11:58

kopiczko enabled auto-merge May 23, 2025 12:13

kopiczko added this pull request to the merge queue May 23, 2025

Merged via the queue into master with commit 3bdb466 May 23, 2025
50 of 52 checks passed

kopiczko deleted the kopiczko/request_prompt_v2 branch May 23, 2025 12:45

This was referenced May 27, 2025

[v17] Allow setting custom reason prompt for Access Requests #55173

Merged

Allow prompting for a custom reason on regular role requests #29475

Closed

		// requireReasonForAllRoles indicates that non-empty reason is required for all access
		// requests. This happens if any of the user roles has options.request_access "reason".

	return &api.AccessRequest{
	Id: req.GetName(),
	State: req.GetState().String(),
	ResolveReason: req.GetResolveReason(),
	RequestReason: req.GetRequestReason(),
	User: req.GetUser(),
	Roles: req.GetRoles(),
	Created: timestamppb.New(req.GetCreationTime()),
	Expires: timestamppb.New(req.GetAccessExpiry()),
	Reviews: reviews,
	SuggestedReviewers: req.GetSuggestedReviewers(),
	ThresholdNames: thresholdNames,
	ResourceIds: requestedResourceIDs,
	Resources: resources,
	PromotedAccessListTitle: req.GetPromotedAccessListTitle(),
	AssumeStartTime: getProtoTimestamp(req.GetAssumeStartTime()),
	MaxDuration: timestamppb.New(req.GetMaxDuration()),
	RequestTtl: timestamppb.New(req.Expiry()),
	SessionTtl: timestamppb.New(req.GetSessionTLL()),

	export function makeUiAccessRequest(request: TshdAccessRequest) {
	return makeAccessRequest({
	...request,
	created: Timestamp.toDate(request.created),
	expires: Timestamp.toDate(request.expires),
	maxDuration: request.maxDuration && Timestamp.toDate(request.maxDuration),
	requestTTL: request.requestTtl && Timestamp.toDate(request.requestTtl),
	sessionTTL: request.sessionTtl && Timestamp.toDate(request.sessionTtl),
	assumeStartTime:
	request.assumeStartTime && Timestamp.toDate(request.assumeStartTime),
	roles: request.roles,
	reviews: request.reviews.map(review => ({
	...review,
	created: Timestamp.toDate(review.created),
	assumeStartTime:
	review.assumeStartTime && Timestamp.toDate(review.assumeStartTime),
	})),
	suggestedReviewers: request.suggestedReviewers,
	thresholdNames: request.thresholdNames,
	resources: request.resources,

Conversation

kopiczko commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Uh oh!

kopiczko commented May 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

kopiczko commented May 22, 2025

Uh oh!

gzdunek left a comment

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!

Uh oh!

backport-bot-workflows Bot commented May 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kopiczko commented May 8, 2025 •

edited

Loading