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.

Sign up for GitHub

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

Jump to bottom

feat: Expose promptForPassword function #881

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+90 −11
Open

feat: Expose promptForPassword function #881

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a3ca1a
feat: Expose promptForPassword function
artonge Nov 5, 2024
b9696a6
WIP: rewrite to withPasswordConfirmation
artonge Nov 6, 2024
5579613
WIP: Add reminder mode for pwd confirmation
artonge Nov 7, 2024
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
19 changes: 16 additions & 3 deletions src/components/PasswordDialog.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@
NcPasswordField,
},

props: {
callback: {

Check warning on line 63 in src/components/PasswordDialog.vue

View workflow job for this annotation

GitHub Actions / NPM lint 

Prop 'callback' requires default value to be set
Copy link
Contributor

Choose a reason for hiding this comment

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

Requires a default like

Suggested change
callback: {
callback: {
default: () => {},

Copy link
Author

Choose a reason for hiding this comment

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

If it has a default, then it won't be undefined later on and the old behavior will never be called. I think that I tried to set undefined as default, but it failed.

type: Function,
required: false

Check warning on line 65 in src/components/PasswordDialog.vue

View workflow job for this annotation

GitHub Actions / NPM lint 

Missing trailing comma
},
},

setup() {
// non reactive props
return {
Expand Down Expand Up @@ -102,10 +109,16 @@
return
}

const url = generateUrl('/login/confirm')
try {
const { data } = await axios.post(url, { password: this.password })
window.nc_lastLogin = data.lastLogin
if (this.callback === undefined) {
const url = generateUrl('/login/confirm')
const { data } = await axios.post(url, { password: this.password })
window.nc_lastLogin = data.lastLogin
} else {
await this.callback(this.password)
window.nc_lastLogin = Date.now() / 1000
}

this.$emit('confirmed')
} catch (e) {
this.showError = true
Expand Down
67 changes: 60 additions & 7 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
* SPDX-License-Identifier: MIT
*/
import Vue from 'vue'
import type { ComponentInstance } from 'vue'

import { Axios } from '@nextcloud/axios'
import { getCurrentUser } from '@nextcloud/auth'

Check failure on line 9 in src/main.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

"@nextcloud/auth" is extraneous

import PasswordDialogVue from './components/PasswordDialog.vue'
import { DIALOG_ID, MODAL_CLASS } from './globals'

import type { ComponentInstance } from 'vue'

const PAGE_LOAD_TIME = Date.now()

/**
Expand All @@ -34,15 +37,25 @@
* or confirmation is already in process.
*/
export const confirmPassword = (): Promise<void> => {
if (!isPasswordConfirmationRequired()) {
return Promise.resolve()
}

return getPasswordDialog()
}

/**
*
* @param mode

Check warning on line 49 in src/main.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

Expected @param names to be "callback". Got "mode, callback"

Check warning on line 49 in src/main.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

Missing JSDoc @param "mode" description
* @param callback

Check warning on line 50 in src/main.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

Missing JSDoc @param "callback" description
* @return

Check warning on line 51 in src/main.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

Missing JSDoc @return type
*/
function getPasswordDialog(callback?: (password: string) => Promise<any>): Promise<void> {

Check failure on line 53 in src/main.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

Unexpected any. Specify a different type
const isDialogMounted = Boolean(document.getElementById(DIALOG_ID))
if (isDialogMounted) {
return Promise.reject(new Error('Password confirmation dialog already mounted'))
}

if (!isPasswordConfirmationRequired()) {
return Promise.resolve()
}

const mountPoint = document.createElement('div')
mountPoint.setAttribute('id', DIALOG_ID)

Expand All @@ -61,7 +74,7 @@

const DialogClass = Vue.extend(PasswordDialogVue)
// Mount point element is replaced by the component
const dialog = (new DialogClass() as ComponentInstance).$mount(mountPoint)
const dialog = (new DialogClass({ propsData: { callback } }) as ComponentInstance).$mount(mountPoint)

return new Promise((resolve, reject) => {
dialog.$on('confirmed', () => {
Expand All @@ -74,3 +87,43 @@
})
})
}

/**
* Add interceptors to an axios instance that for every request
* will prompt for password confirmation and add it as Basic Auth.
* @param axios

Check warning on line 94 in src/main.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

Missing JSDoc @param "axios" description
*/
export function withPasswordConfirmation(axios: Axios): Axios {
const {
promise: passwordDialogCallbackResolution,
resolve: resolvePwdDialogCallback,
reject: rejectPwdDialogCallback,

Check failure on line 100 in src/main.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

'rejectPwdDialogCallback' is assigned a value but never used
} = Promise.withResolvers()

axios.interceptors.request.use(async (config) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should get the interceptor (const requestInterceptor = axios.interceptors.request.use(...))
And then in the response interceptor it should be removed again with axios.interceptors.request.eject(requestInterceptor) to only request a password if needed

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking of creating a new axios instance instead. So this instance would always ask for pwd confirmation

Copy link
Author

Choose a reason for hiding this comment

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

Or if we want it to be a one time thing, then something like that might make more sense:

axios.post({
  passwordConfirmation: true,
  // ...
}

And then add the interceptor in @nc/axios.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of creating a new axios instance instead. So this instance would always ask for pwd confirmation

I think this is too complex for using, if I have an API to query and only one endpoint is protected, then why do I need two axios instances? It gets even worse if I need to configure both the same way and just need the password confirmation.

Or if we want it to be a one time thing, then something like that might make more sense:
And then add the interceptor in @nc/axios.

I think one time thing makes more sense, so the two proposed ways are:

  1. Either a plugin style (the way you suggested here) like axios-retry is doing
  2. A one-time wrapper like in my example above

But either way I would not move this to @nextcloud/axios because this will be a cyclic dependency axios <> password confirmation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also like your idea with the plugin, but I am not sure how we could do this in a clean way.
Because having UI elements in purely API packages (@nextcloud/axios) is not a really good thing (IMHO) as it causes a lot of "useless" dependencies.

Also for example the password confirmation is only needed if the request is done using a user session, but it is not required if you do the requests in no-ui mode using app tokens for authentication (valid use case, but the only real using app I know for something like this currently would be the Talk desktop app).

While writing this:
I more and more like your idea! But I think we should then move it completely to @nextcloud/axios.
As this is a core functionality of our request handling, I do not think it makes sense that you need to register the password confirmation first.

So what do you think of moving this function to @nextcloud/axios? Especially if this is the new default and the old way of confirming the password will be deprecated?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, would make sense to have it all in axios then. Let me draft a PR.

return new Promise((resolve, reject) => {

Check failure on line 104 in src/main.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

'reject' is defined but never used
getPasswordDialog((password: string) => {
resolve({
...config,
auth: {
username: getCurrentUser()?.uid ?? '',
password,
},
})

// Await for request to be done
return passwordDialogCallbackResolution
})
})
})

axios.interceptors.response.use((response) => {
if (response.request.auth !== undefined) {
resolvePwdDialogCallback(undefined)
}

return response
})

return axios
}
Loading