Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ fun AddEditSendContent(
state: AddEditSendState.ViewState.Content,
policyDisablesSend: Boolean,
policySendOptionsInEffect: Boolean,
shouldHideEmailAddressToggle: Boolean,
isAddMode: Boolean,
isShared: Boolean,
addSendHandlers: AddEditSendHandlers,
Expand Down Expand Up @@ -181,6 +182,7 @@ fun AddEditSendContent(
AddEditSendOptions(
state = state,
isSendsRestrictedByPolicy = policyDisablesSend,
shouldHideEmailAddressToggle = shouldHideEmailAddressToggle,
isAddMode = isAddMode,
addSendHandlers = addSendHandlers,
)
Expand Down Expand Up @@ -383,6 +385,7 @@ private fun ColumnScope.FileTypeContent(
private fun AddEditSendOptions(
state: AddEditSendState.ViewState.Content,
isSendsRestrictedByPolicy: Boolean,
shouldHideEmailAddressToggle: Boolean,
isAddMode: Boolean,
addSendHandlers: AddEditSendHandlers,
) {
Expand Down Expand Up @@ -466,19 +469,22 @@ private fun AddEditSendOptions(
},
)
}
Spacer(modifier = Modifier.height(height = 8.dp))
BitwardenSwitch(
modifier = Modifier
.testTag("SendHideEmailSwitch")
.fillMaxWidth()
.standardHorizontalMargin(),
label = stringResource(id = BitwardenString.hide_email),
isChecked = state.common.isHideEmailChecked,
onCheckedChange = addSendHandlers.onHideEmailToggle,
readOnly = isSendsRestrictedByPolicy,
enabled = state.common.isHideEmailChecked || state.common.isHideEmailAddressEnabled,
cardStyle = CardStyle.Full,
)
if (!shouldHideEmailAddressToggle) {
Spacer(modifier = Modifier.height(height = 8.dp))
BitwardenSwitch(
modifier = Modifier
.testTag("SendHideEmailSwitch")
.fillMaxWidth()
.standardHorizontalMargin(),
label = stringResource(id = BitwardenString.hide_email),
isChecked = state.common.isHideEmailChecked,
onCheckedChange = addSendHandlers.onHideEmailToggle,
readOnly = isSendsRestrictedByPolicy,
enabled = state.common.isHideEmailChecked ||
state.common.isHideEmailAddressEnabled,
cardStyle = CardStyle.Full,
)
}
Comment on lines +472 to +487

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.

⚠️ IMPORTANT: Hiding the switch removes the only way to turn off hideEmail on an existing send.

Details and fix

The retained enabled = isHideEmailChecked || isHideEmailAddressEnabled exists so that a send created before the policy took effect (with hideEmail = true) can still be turned off by the user, even while the policy forbids turning it on. With the flag on and disableHideEmail = true, that switch is now gone, but AddEditSendState.toSendView() still writes hideEmail = common.isHideEmailChecked (util/AddEditSendStateExtensions.kt:36), so editing that send silently re-saves the policy-violating value with no way for the user to comply.

Two possible fixes:

  • Keep the switch visible when isHideEmailChecked is true (shouldHideEmailAddressToggle gains && !isHideEmailChecked), preserving the legacy escape hatch, or
  • Coerce isHideEmailChecked = false in handleEffectiveSendPolicyReceive when effectiveSendPolicy.disableHideEmail is true, so the hidden control cannot carry a restricted value into the save.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is the intended behaviour.

Spacer(modifier = Modifier.height(8.dp))
BitwardenTextField(
label = stringResource(id = BitwardenString.private_notes),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ fun AddEditSendScreen(
state = viewState,
policyDisablesSend = state.policyDisablesSend,
policySendOptionsInEffect = state.shouldDisplayPolicyWarning,
shouldHideEmailAddressToggle = state.shouldHideEmailAddressToggle,
isAddMode = state.isAddMode,
isShared = state.isShared,
addSendHandlers = addSendHandlers,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import android.net.Uri
import android.os.Parcelable
import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.viewModelScope
import com.bitwarden.core.data.manager.model.FlagKey
import com.bitwarden.core.data.repository.model.DataState
import com.bitwarden.core.data.repository.util.takeUntilLoaded
import com.bitwarden.data.repository.util.baseWebSendUrl
Expand All @@ -23,6 +24,7 @@ import com.bitwarden.ui.util.asText
import com.bitwarden.ui.util.concat
import com.x8bit.bitwarden.data.auth.repository.AuthRepository
import com.x8bit.bitwarden.data.billing.manager.PremiumStateManager
import com.x8bit.bitwarden.data.platform.manager.FeatureFlagManager
import com.x8bit.bitwarden.data.platform.manager.PolicyManager
import com.x8bit.bitwarden.data.platform.manager.SpecialCircumstanceManager
import com.x8bit.bitwarden.data.platform.manager.clipboard.BitwardenClipboardManager
Expand Down Expand Up @@ -51,6 +53,7 @@ import com.x8bit.bitwarden.ui.tools.feature.send.util.toSendUrl
import dagger.hilt.android.lifecycle.HiltViewModel
import kotlinx.collections.immutable.persistentListOf
import kotlinx.collections.immutable.toImmutableList
import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.map
Expand Down Expand Up @@ -83,6 +86,7 @@ class AddEditSendViewModel @Inject constructor(
private val clock: Clock,
private val clipboardManager: BitwardenClipboardManager,
private val environmentRepo: EnvironmentRepository,
private val featureFlagManager: FeatureFlagManager,
private val specialCircumstanceManager: SpecialCircumstanceManager,
private val vaultRepo: VaultRepository,
private val policyManager: PolicyManager,
Expand Down Expand Up @@ -148,6 +152,7 @@ class AddEditSendViewModel @Inject constructor(
dialogState = null,
baseWebSendUrl = environmentRepo.environment.baseWebSendUrl,
policyDisablesSend = effectiveSendPolicy.disableSend,
isSendControlsEnabled = featureFlagManager.getFeatureFlag(key = FlagKey.SendControls),
allowedDomains = effectiveSendPolicy.allowedDomains,
allowedSendTypes = effectiveSendPolicy.allowedSendTypes,
deletionHours = effectiveSendPolicy.deletionHours,
Expand Down Expand Up @@ -182,6 +187,20 @@ class AddEditSendViewModel @Inject constructor(
}
.onEach(::sendAction)
.launchIn(viewModelScope)

// The effective policy itself depends on the feature flag, so both are observed together
// to keep the derived state consistent whenever either one changes.
combine(
policyManager.getEffectiveSendPolicyFlow(),
featureFlagManager.getFeatureFlagFlow(key = FlagKey.SendControls),
) { effectiveSendPolicy, isSendControlsEnabled ->
AddEditSendAction.Internal.EffectiveSendPolicyReceive(
effectiveSendPolicy = effectiveSendPolicy,
isSendControlsEnabled = isSendControlsEnabled,
)
}
.onEach(::sendAction)
.launchIn(viewModelScope)
}

override fun handleAction(action: AddEditSendAction): Unit = when (action) {
Expand Down Expand Up @@ -233,6 +252,10 @@ class AddEditSendViewModel @Inject constructor(
handleRemovePasswordResultReceive(action)
}

is AddEditSendAction.Internal.EffectiveSendPolicyReceive -> {
handleEffectiveSendPolicyReceive(action)
}

is AddEditSendAction.Internal.SendDataReceive -> handleSendDataReceive(action)

is AddEditSendAction.Internal.GeneratorResultReceive -> {
Expand Down Expand Up @@ -384,6 +407,31 @@ class AddEditSendViewModel @Inject constructor(
}
}

private fun handleEffectiveSendPolicyReceive(
action: AddEditSendAction.Internal.EffectiveSendPolicyReceive,
) {
val effectiveSendPolicy = action.effectiveSendPolicy
mutableStateFlow.update { currentState ->
currentState.copy(
policyDisablesSend = effectiveSendPolicy.disableSend,
isSendControlsEnabled = action.isSendControlsEnabled,
allowedDomains = effectiveSendPolicy.allowedDomains,
allowedSendTypes = effectiveSendPolicy.allowedSendTypes,
deletionHours = effectiveSendPolicy.deletionHours,
whoCanAccess = effectiveSendPolicy.whoCanAccess,
viewState = (currentState.viewState as? AddEditSendState.ViewState.Content)
?.let { content ->
content.copy(
common = content.common.copy(
isHideEmailAddressEnabled = !effectiveSendPolicy.disableHideEmail,
),
)
}
?: currentState.viewState,
)
}
}

@Suppress("LongMethod")
private fun handleSendDataReceive(action: AddEditSendAction.Internal.SendDataReceive) {
when (val sendDataState = action.sendDataState) {
Expand Down Expand Up @@ -905,6 +953,7 @@ data class AddEditSendState(
val isShared: Boolean,
val baseWebSendUrl: String,
val policyDisablesSend: Boolean,
val isSendControlsEnabled: Boolean,
val allowedDomains: String?,
val allowedSendTypes: List<SendTypeJson>?,
val deletionHours: Int?,
Expand All @@ -929,12 +978,24 @@ data class AddEditSendState(
}

/**
* Helper to determine if the policy notice should be displayed.
* Helper to determine if the policy notice should be displayed. The notice is only relevant to
* the legacy send options policy, which disables the affected controls rather than hiding them.
* The SendControls policy removes those controls entirely, so there is nothing to explain.
*/
val shouldDisplayPolicyWarning: Boolean
get() = !policyDisablesSend &&
!isSendControlsEnabled &&
(viewState as? ViewState.Content)?.common?.isHideEmailAddressEnabled != true

/**
* Helper to determine if the "hide my email" toggle should be hidden entirely rather than
* simply disabled. The SendControls policy hides the toggle, while the legacy send options
* policy continues to only disable it.
*/
val shouldHideEmailAddressToggle: Boolean
get() = isSendControlsEnabled &&
(viewState as? ViewState.Content)?.common?.isHideEmailAddressEnabled == false
Comment on lines +990 to +997

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.

❓ QUESTION: The gate is the feature flag, not the policy source, so legacy SendOptions orgs also lose the toggle and the notice.

Details

resolveEffectiveSendPolicy folds legacy SendOptions into the effective policy when the flag is on for orgs with no SendControls policy (PolicyManagerImpl.kt:239-250: remainingSendOptions.any { it.shouldDisableHideEmail ?: false }). Once the flag rolls out, those users get the hidden toggle and, via shouldDisplayPolicyWarning, no explanatory notice either β€” which is the opposite of what this KDoc describes ("the legacy send options policy continues to only disable it").

Is flag-based gating the intent for rollout? If so, the KDoc on both helpers reads as policy-source-based and could be reworded; if not, both helpers would need to key off whether a SendControls policy is actually in effect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is working as intended because once the Flag is enabled it will also use the new Send Control policy.


/**
* Helper to determine if the UI should display the content in add send mode.
*/
Expand Down Expand Up @@ -1252,6 +1313,14 @@ sealed class AddEditSendAction {
*/
data class CreateSendResultReceive(val result: CreateSendResult) : Internal()

/**
* Indicates an updated effective send policy has been received.
*/
data class EffectiveSendPolicyReceive(
val effectiveSendPolicy: EffectiveSendPolicy,
val isSendControlsEnabled: Boolean,
) : Internal()

/**
* Indicates that the vault totp code result has been received.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -789,48 +789,93 @@ class AddEditSendScreenTest : BitwardenComposeTest() {
.assertIsOn()
}

@Suppress("MaxLineLength")
@Test
fun `hide email toggle should be disabled according to state`() = runTest {
// Expand options section:
composeTestRule
.onNodeWithText("Additional options")
.performScrollTo()
.performClick()

mutableStateFlow.update {
it.copy(
viewState = DEFAULT_VIEW_STATE.copy(
common = DEFAULT_COMMON_STATE.copy(
isHideEmailAddressEnabled = false,
fun `hide email toggle should be disabled when restricted and send controls is disabled`() =
runTest {
// Expand options section:
composeTestRule
.onNodeWithText("Additional options")
.performScrollTo()
.performClick()

mutableStateFlow.update {
it.copy(
viewState = DEFAULT_VIEW_STATE.copy(
common = DEFAULT_COMMON_STATE.copy(
isHideEmailAddressEnabled = false,
),
),
),
)
}
isSendControlsEnabled = false,
)
}

// Legacy behavior: the toggle remains visible but is not interactive.
composeTestRule
.onNodeWithText("Hide my email address", substring = true)
.performScrollTo()
.assertIsDisplayed()
.assertIsNotEnabled()

mutableStateFlow.update {
it.copy(
viewState = DEFAULT_VIEW_STATE.copy(
common = DEFAULT_COMMON_STATE.copy(
isHideEmailAddressEnabled = true,
),
),
)
}

// Toggle should be disabled
composeTestRule
.onNodeWithText("Hide my email address", substring = true)
.performScrollTo()
.assertIsDisplayed()
.assertIsNotEnabled()
composeTestRule
.onNodeWithText("Hide my email address", substring = true)
.performScrollTo()
.assertIsDisplayed()
.assertIsEnabled()
}

mutableStateFlow.update {
it.copy(
viewState = DEFAULT_VIEW_STATE.copy(
common = DEFAULT_COMMON_STATE.copy(
isHideEmailChecked = true,
@Suppress("MaxLineLength")
@Test
fun `hide email toggle should be hidden when restricted and send controls is enabled`() =
runTest {
// Expand options section:
composeTestRule
.onNodeWithText("Additional options")
.performScrollTo()
.performClick()

mutableStateFlow.update {
it.copy(
viewState = DEFAULT_VIEW_STATE.copy(
common = DEFAULT_COMMON_STATE.copy(
isHideEmailAddressEnabled = false,
),
),
),
)
}
isSendControlsEnabled = true,
)
}

// The toggle is hidden entirely rather than simply disabled.
composeTestRule
.onNodeWithText("Hide my email address", substring = true)
.assertDoesNotExist()

mutableStateFlow.update {
it.copy(
viewState = DEFAULT_VIEW_STATE.copy(
common = DEFAULT_COMMON_STATE.copy(
isHideEmailAddressEnabled = true,
),
),
)
}

// Toggle should be enabled
composeTestRule
.onNodeWithText("Hide my email address", substring = true)
.performScrollTo()
.assertIsDisplayed()
.assertIsEnabled()
}
composeTestRule
.onNodeWithText("Hide my email address", substring = true)
.performScrollTo()
.assertIsDisplayed()
.assertIsEnabled()
}

@Test
fun `progressbar should be displayed according to state`() {
Expand Down Expand Up @@ -1020,6 +1065,13 @@ class AddEditSendScreenTest : BitwardenComposeTest() {
composeTestRule
.onNodeWithText(text)
.assertIsDisplayed()

// The notice is not relevant once send controls removes the affected options entirely.
mutableStateFlow.update { it.copy(isSendControlsEnabled = true) }

composeTestRule
.onNodeWithText(text)
.assertIsNotDisplayed()
Comment thread
Copilot marked this conversation as resolved.
Outdated
}

//region Authentication UI Tests
Expand Down Expand Up @@ -1578,6 +1630,7 @@ private val DEFAULT_STATE = AddEditSendState(
isShared = false,
baseWebSendUrl = "https://vault.bitwarden.com/#/send/",
policyDisablesSend = false,
isSendControlsEnabled = false,
allowedDomains = null,
allowedSendTypes = null,
deletionHours = null,
Expand Down
Loading
Loading