OCA / server-auth

https://odoo-community.org/psc-teams/tools-30
GNU Affero General Public License v3.0
150 stars 403 forks source link

[MIG][16.0] Migration of vault #611

Closed fkantelberg closed 6 months ago

fkantelberg commented 7 months ago

The migration needed quite a rework because of the JS framework changes. (I hope the next are easier) I tested every functionality during migration but plan to do another round again soon.

There are some minor fixes which were possible to do:

pedrobaeza commented 7 months ago

Thanks for the migration.

Can you please leave this one only for the module vault and o later vault_share with its story. Another thing you can do is to compact a bit the commit story, as it contains some non useful commits, like the first 11.

fkantelberg commented 7 months ago

Done and vault_share is now in #612

pedrobaeza commented 7 months ago

/ocabot migration vault

fkantelberg commented 6 months ago

I was able to reproduce the issue. Seems like a hidden bug in all versions. I reproduced it by clicking away the first password dialog when I clean all browser caches. If I try to read this.time was null which causes an early return instead of reimporting of the keys.

CarlosRoca13 commented 6 months ago

I'm giving access to demo user but he can not see any secret... See gif below

Access_error

fkantelberg commented 6 months ago

In my tests I had the issue that the key in vault.right wasn't generated correctly because of the if in the controller. I suspect that this is also the issue for your case. Not 100% sure because the right was marked red in my case but isn't in your case.

bosd commented 6 months ago

image The buttons in the topbar, don't seem to be working. Import file, Export file, Verify, Re-encrypt

Also the smart button "Entries" is no longer working.

fkantelberg commented 6 months ago

@bosd I can't reproduce it and every button is working. Maybe a module update gone wrong on your local instance? At least the smart button doesn't do anything special just uses a normal action.

CarlosRoca13 commented 6 months ago

In my tests I had the issue that the key in vault.right wasn't generated correctly because of the if in the controller. I suspect that this is also the issue for your case. Not 100% sure because the right was marked red in my case but isn't in your case.

I think the problem was that, I have deleted and re-added the user and there has been no problem.

I think it's ready :+1: :smile:

bosd commented 6 months ago

Maybe a module update gone wrong on your local instance?

I was using the runboat.. Will reset it and try again.

fkantelberg commented 6 months ago

@bosd Please check that you aren't using http. https:// is required for the module because the browser enforces it for the secure context and browser side encryption.

pedrobaeza commented 6 months ago

@fkantelberg maybe a clarification in the ROADMAP of the module mentioning explicitly runboat there?

bosd commented 6 months ago

Thanks,with https The import window is working now. I'm getting more clickable icons on the screenlike in above gif's.

Export throws an error.. image

Viryfy buttons as well: UncaughtPromiseError > OperationError Uncaught Promise OperationError

The smartbutton: image

fkantelberg commented 6 months ago

There is a line in the readme This modules requires a secure context for the browser to work properly. but I guess I'm just biased and know what it means. Roadmap sounds like that I want to work on "fixing" it but http will never be supported beside localhost.

Is there a technical reason why the runboat links to http instead of the possible https per default?

@bosd Regarding the error: I assume it just happens because it was possible to create a vault without https. The vault and user keys aren't initialized correctly and can be thrown away. I have to check if I can further prevent anything when the context isn't secured.

pedrobaeza commented 6 months ago

Roadmap sounds like that I want to work on "fixing" it but http will never be supported beside localhost.

The file is called ROADMAP.rst, but the section is "Known issues / Roadmap".

Is there a technical reason why the runboat links to http instead of the possible https per default?

@sbidoul can tell you, but if the certificate is self-signed, it's for not having the browser alert and have more questions about that.

sbidoul commented 6 months ago

Is there a technical reason why the runboat links to http instead of the possible https per default?

It's not a technical reason, but to avoid the burden of maintaining a trusted wildcard certificate.

bosd commented 6 months ago

Thanks for the clarifications. Did'nt spot that line in the readme. Even if i did I don't know what it means :zipper_mouth_face:

I'm used to test stuff on runboat. Did'nt occur to me that is not possible for this module. Guess this is the exception of the rule.

fkantelberg commented 6 months ago

You can just swap to https and confirm the self signed certificate of the runboat to test it.

I understand that a wildcard cert can be a burden. I see how I can make it a bit more error prune when not using a secure context and check if I can rephrase the hint in the readme.

fkantelberg commented 6 months ago

The latest patch does some things when the secure browser context isn't available:

Additionally I extended the readme and added it also to the roadmap. I hope this makes it visible enough :smile:

OCA-git-bot commented 6 months ago

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

pedrobaeza commented 6 months ago

/ocabot merge nobump

OCA-git-bot commented 6 months ago

Hey, thanks for contributing! Proceeding to merge this for you. Prepared branch 16.0-ocabot-merge-pr-611-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot commented 6 months ago

Congratulations, your PR was merged at 23d8df7e69306acdb299ff24c53dfbdf9a6b6724. Thanks a lot for contributing to OCA. ❤️