Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate against Banned-Passwords List #9727

Merged
merged 15 commits into from
Sep 26, 2023
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Enhancement: Show error if password is on a banned password list

We now show a meaningful error if the user tries to set a public link password,
that's on a server side banned password list.

https://github.com/owncloud/web/pull/9727
https://github.com/owncloud/web/issues/9726
2 changes: 2 additions & 0 deletions dev/docker/ocis/password-policy-banned-passwords.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
password
12345678
2 changes: 2 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ services:
FRONTEND_SEARCH_MIN_LENGTH: "2"
FRONTEND_OCS_ENABLE_DENIALS: "true"
FRONTEND_FULL_TEXT_SEARCH_ENABLED: "true"
FRONTEND_PASSWORD_POLICY_BANNED_PASSWORDS_LIST: '/etc/ocis/password-policy-banned-passwords.txt'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added some demo data, can't hurt


# IDM
IDM_CREATE_DEMO_USERS: "${DEMO_USERS:-true}"
Expand Down Expand Up @@ -61,6 +62,7 @@ services:
# make the registry available to the app provider containers
MICRO_REGISTRY: "mdns"
volumes:
- ./dev/docker/ocis/password-policy-banned-passwords.txt:/etc/ocis/password-policy-banned-passwords.txt
- ./dev/docker/ocis.idp.config.yaml:/etc/ocis/idp.yaml
- ./dev/docker/ocis-ca:/var/lib/ocis/proxy
- ./dist:/web/dist
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
<template>
<div class="oc-text-input-password-wrapper">
<input
v-bind="$attrs"
v-model="password"
:type="showPassword ? 'text' : 'password'"
@input="onPasswordEntered"
/>
<div
class="oc-text-input-password-wrapper"
:class="{
'oc-text-input-password-wrapper-warning': hasWarning,
'oc-text-input-password-wrapper-danger': hasError
}"
>
<input v-bind="$attrs" v-model="password" :type="showPassword ? 'text' : 'password'" />
<oc-button
v-if="password"
v-oc-tooltip="$gettext('Show password')"
Expand All @@ -32,7 +33,7 @@
class="oc-text-input-generate-password-button oc-px-s oc-background-default"
appearance="raw"
size="small"
@click="showGeneratedPassword"
@click="generatePassword"
>
<oc-icon size="small" name="refresh" fill-type="line" />
</oc-button>
Expand Down Expand Up @@ -91,14 +92,22 @@ export default defineComponent({
}
},
emits: ['passwordChallengeCompleted', 'passwordChallengeFailed'],
setup(props, { emit }) {
setup(props, { emit, attrs }) {
const { $gettext } = useGettext()
const password = ref(props.value)
const showPassword = ref(false)
const passwordEntered = ref(false)
const copyPasswordIconInitial = 'file-copy'
const copyPasswordIcon = ref(copyPasswordIconInitial)

const hasError = computed(() => {
return (attrs?.class as string)?.includes('oc-text-input-danger')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not ideal, but I don't want to add more component props

})

const hasWarning = computed(() => {
return (attrs?.class as string)?.includes('oc-text-input-warning')
})

const showPasswordPolicyInformation = computed(() => {
return !!(Object.keys(props.passwordPolicy?.rules || {}).length && unref(passwordEntered))
})
Expand All @@ -123,21 +132,19 @@ export default defineComponent({
setTimeout(() => (copyPasswordIcon.value = copyPasswordIconInitial), 500)
}

const showGeneratedPassword = () => {
const generatePassword = () => {
const generatedPassword = props.generatePasswordMethod()
password.value = generatedPassword
showPassword.value = true
}

const onPasswordEntered = () => {
passwordEntered.value = true
}

watch(password, (value) => {
if (!Object.keys(props.passwordPolicy).length) {
return
}

passwordEntered.value = true

if (!props.passwordPolicy.check(value)) {
return emit('passwordChallengeFailed')
}
Expand All @@ -148,14 +155,15 @@ export default defineComponent({
return {
$gettext,
password,
hasError,
hasWarning,
showPassword,
copyPasswordIcon,
showPasswordPolicyInformation,
testedPasswordPolicy,
generatePassword,
getPasswordPolicyRuleMessage,
copyPasswordToClipboard,
showGeneratedPassword,
copyPasswordIcon,
onPasswordEntered
copyPasswordToClipboard
}
}
})
Expand All @@ -177,6 +185,18 @@ export default defineComponent({
input:focus {
outline: none;
}

&-warning,
&-warning:focus {
border-color: var(--oc-color-swatch-warning-default) !important;
color: var(--oc-color-swatch-warning-default) !important;
}

&-danger,
&-danger:focus {
border-color: var(--oc-color-swatch-danger-default) !important;
color: var(--oc-color-swatch-danger-default) !important;
}
}
.oc-text-input-password-wrapper:focus-within {
border-color: var(--oc-color-swatch-passive-default);
Expand Down
62 changes: 40 additions & 22 deletions packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,13 @@ export default defineComponent({
},
methods: {
...mapActions('Files', ['addLink', 'updateLink', 'removeLink']),
...mapActions(['showMessage', 'showErrorMessage', 'createModal', 'hideModal']),
...mapActions([
'showMessage',
'showErrorMessage',
'createModal',
'hideModal',
'setModalInputErrorMessage'
]),
...mapMutations('Files', ['REMOVE_FILES']),

toggleLinkListCollapsed() {
Expand Down Expand Up @@ -394,7 +400,7 @@ export default defineComponent({
})
},

checkLinkToCreate({ link, onError = () => {} }) {
checkLinkToCreate({ link }) {
const paramsToCreate = this.getParamsForLink(link)

if (this.isPasswordEnforcedFor(link)) {
Expand All @@ -405,15 +411,15 @@ export default defineComponent({
passwordPolicyService: this.passwordPolicyService
},
(newPassword) => {
this.createLink({ params: { ...paramsToCreate, password: newPassword }, onError })
this.createLink({ params: { ...paramsToCreate, password: newPassword } })
}
)
} else {
this.createLink({ params: paramsToCreate, onError })
this.createLink({ params: paramsToCreate })
}
},

checkLinkToUpdate({ link, onSuccess = () => {} }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed all those onSuccess and onError callbacks because they actually weren't used and added to confusion.

checkLinkToUpdate({ link }) {
const params = this.getParamsForLink(link)

if (!link.password && this.isPasswordEnforcedFor(link)) {
Expand All @@ -424,11 +430,11 @@ export default defineComponent({
passwordPolicyService: this.passwordPolicyService
},
(newPassword) => {
this.updatePublicLink({ params: { ...params, password: newPassword }, onSuccess })
this.updatePublicLink({ params: { ...params, password: newPassword } })
}
)
} else {
this.updatePublicLink({ params, onSuccess })
this.updatePublicLink({ params })
}
},

Expand Down Expand Up @@ -481,7 +487,7 @@ export default defineComponent({
}
},

async createLink({ params, onError = (e) => {} }) {
async createLink({ params }) {
let path = this.resource.path
// sharing a share root from the share jail -> use resource name as path
if (this.hasShareJail && path === '/') {
Expand All @@ -494,41 +500,53 @@ export default defineComponent({
storageId: this.resource.fileId || this.resource.id,
params
})
this.hideModal()
this.showMessage({
title: this.$gettext('Link was created successfully')
})
} catch (e) {
onError(e)
if (true) {
return this.setModalInputErrorMessage(
this.$gettext(
'Unfortunately, your password is commonly used. Please pick a harder-to-guess password for your safety.'
)
)
}

console.error(e)
this.showErrorMessage({
title: this.$gettext('Failed to create link'),
error: e
})
return
}

this.showMessage({
title: this.$gettext('Link was created successfully')
})
},

async updatePublicLink({ params, onSuccess = () => {}, onError = (e) => {} }) {
async updatePublicLink({ params }) {
try {
await this.updateLink({
id: params.id,
client: this.$client,
params
}).then(onSuccess)
})
this.hideModal()
this.showMessage({
title: this.$gettext('Link was updated successfully')
})
} catch (e) {
onError(e)
if (true) {
return this.setModalInputErrorMessage(
this.$gettext(
'Unfortunately, your password is commonly used. Please pick a harder-to-guess password for your safety.'
)
)
}

console.error(e)
this.showErrorMessage({
title: this.$gettext('Failed to update link'),
error: e
})
return
}

this.showMessage({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't be here.

title: this.$gettext('Link was updated successfully')
})
},

deleteLinkConfirmation({ link }) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -472,9 +472,6 @@ export default defineComponent({
link: {
...this.link,
name
},
onSuccess: () => {
this.hideModal()
}
})
}
Expand All @@ -496,6 +493,7 @@ export default defineComponent({
inputPlaceholder: this.link.password ? '●●●●●●●●' : null,
inputType: 'password',
onCancel: this.hideModal,
onInput: () => this.setModalInputErrorMessage(''),
onPasswordChallengeCompleted: () => this.setModalConfirmButtonDisabled(false),
onPasswordChallengeFailed: () => this.setModalConfirmButtonDisabled(true),
onConfirm: (password) => {
Expand Down
18 changes: 4 additions & 14 deletions packages/web-app-files/src/quickActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,12 @@ export function showQuickLinkPasswordModal({ $gettext, store, passwordPolicyServ
inputGeneratePasswordMethod: () => passwordPolicyService.generatePassword(),
inputLabel: $gettext('Password'),
inputType: 'password',
onInput: () => store.dispatch('setModalInputErrorMessage', ''),
onPasswordChallengeCompleted: () => store.dispatch('setModalConfirmButtonDisabled', false),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed those in the last pr.

onPasswordChallengeFailed: () => store.dispatch('setModalConfirmButtonDisabled', true),
onCancel: () => store.dispatch('hideModal'),
onConfirm: async (password) => {
if (!password || password.trim() === '') {
store.dispatch('showErrorMessage', {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed anymore as built in default password policy. Also closing the modal instantly was not correct, because we might want to show an error in the modal.

title: $gettext('Password cannot be empty')
})
} else {
await store.dispatch('hideModal')
onConfirm(password)
}
},
onInput: (password) => {
if (password.trim() === '') {
return store.dispatch('setModalInputErrorMessage', $gettext('Password cannot be empty'))
}
return store.dispatch('setModalInputErrorMessage', null)
onConfirm(password)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,11 @@ export class PasswordPolicyService {

private useDefaultRules(): boolean {
return (
Object.keys(this.capability).length === 0 ||
(Object.keys(this.capability).length === 1 &&
Object.keys(this.capability)[0] === 'max_characters')
!this.capability.min_characters &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are always set, defaults to 0, so had to change the logic

!this.capability.min_lowercase_characters &&
!this.capability.min_uppercase_characters &&
!this.capability.min_digits &&
!this.capability.min_special_characters
)
}

Expand Down