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

feat: Expose promptForPassword function #881

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

artonge
Copy link

@artonge artonge commented Nov 5, 2024

This allows the caller to use the password for other mean of confirmation.

@artonge artonge self-assigned this Nov 5, 2024
@artonge artonge added enhancement New feature or request 3. to review Waiting for reviews javascript Pull requests that update Javascript code labels Nov 5, 2024
This allow the caller to use the password for other mean of confirmation

Signed-off-by: Louis Chemineau <[email protected]>
@artonge artonge force-pushed the artonge/feat/allow_to_get_password branch from 5db7eb6 to 5a3ca1a Compare November 5, 2024 17:00
@artonge artonge requested a review from susnux November 5, 2024 17:00
@@ -59,6 +59,13 @@ export default defineComponent({
NcPasswordField,
},

props: {
callback: {
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.

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Question would be:
Do we want to support both ways at the same time or only the new way in the future?

Because if we only use the new way in the future, then I think it would make more sense to provide a more convenient way to handle this.
Something like:

import axios from '@nextcloud/axios'
import { withPasswordConfirmation } from '@nextcloud/password-confirmation'

withPasswordConfirmation(axios).post('a/dangerous/route')

Where withPasswordConfirmation would add a once-time interceptor to the request that adds the password header.

What do you think?

@artonge
Copy link
Author

artonge commented Nov 6, 2024

What do you think?

I was looking for something like that indeed !

src/main.ts Outdated
reject: rejectPwdDialogCallback,
} = 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants