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

Add partial role support for manager only using web-vault v2024.12.0 #5219

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

Add partial role support for manager only using web-vault v2024.12.0 #5219

wants to merge 5 commits into from

Conversation

BlackDex
Copy link
Collaborator

@BlackDex BlackDex commented Nov 21, 2024 •
edited
Loading

  • Add the custom role which replaces the manager role
  • Added mini-details endpoint used by v2024.12.0

These changes try to add the custom role in such a way that it stays compatible with the older manager role. It will convert a manager role into a custom role, and if a manager has access-all rights, it will enable the correct custom roles. Upon saving it will convert these back to the old format.

What this does is making sure you are able to revert back to an older version of Vaultwarden without issues. This way we can support newer web-vault's and still be compatible with a previous Vaultwarden version if needed.

In the future this needs to be changed to full role support though.

@BlackDex BlackDex mentioned this pull request Nov 21, 2024
Open
@BlackDex
Copy link
Collaborator Author

BlackDex commented Nov 21, 2024 •
edited
Loading

This PR needs a web-vault built from this PR dani-garcia/bw_web_builds#186
I'm have yet to fix the CSS, since it now still hides some elements which need to be visible.

But it would be nice if some people are able to test this PR using a web-vault built from the PR linked above.

@tessus
Copy link
Contributor

tessus commented Nov 21, 2024

In the future this needs to be changed to full role support though.

May I suggest to add more comments to the code, where your changes from this PR have to be reverted when full role support is implemented. Unless you you'll keep the compat layer even then.

It is quite possible to forget to remove/change current code/behavior when the "future" changes are not close to this PR. Please ignore, if I am way off.

@BlackDex BlackDex force-pushed the mngr-custom-role branch 2 times, most recently from ae62f5b to 6e337c6 Compare November 22, 2024 19:40
@BlackDex
Copy link
Collaborator Author

@tessus done :)

@stefan0xC I have fixed the icon in the collapsed menu.

@BlackDex BlackDex force-pushed the mngr-custom-role branch 2 times, most recently from de0b54c to 0a4ffc7 Compare November 25, 2024 15:58
@BlackDex BlackDex marked this pull request as ready for review November 25, 2024 16:23
@BlackDex
Copy link
Collaborator Author

BlackDex commented Nov 25, 2024 •
edited
Loading

As mentioned before, this PR needs a new web-vault version for which a PR can be found here:

CI builds of this web-vault can be downloaded here:

@tessus
Copy link
Contributor

tessus commented Dec 6, 2024 •
edited
Loading

If this is the only PR required for the new web vault, is it possible to merge this and then release the new web vault - maybe this weekend?
I've been running it (and the web vault from the artifact link) for a few days now and didn't run into any issues. Although I am sure I haven't tested all edge cases and every single functionality.

@BlackDex
Copy link
Collaborator Author

BlackDex commented Dec 7, 2024

I'd rather first fix a native client issue and release a version and after that merge this.

I also think it is better to first release the web-vault and then add that release to this PR.

@dfunkt
Copy link
Contributor

dfunkt commented Dec 7, 2024

Not sure if this belongs here or in the web vault PR, but I seem to be offered the option to log in with a passkey (which obviously doesn't work):
image

@BlackDex
Copy link
Collaborator Author

BlackDex commented Dec 7, 2024

Not sure if this belongs here or in the web vault PR, but I seem to be offered the option to log in with a passkey (which obviously doesn't work):

Thanks! I fixed it just now.

@dfunkt
Copy link
Contributor

dfunkt commented Dec 7, 2024

Now that I think about it, shouldn't the create account line also be hidden? I have signups disabled.

@BlackDex
Copy link
Collaborator Author

BlackDex commented Dec 7, 2024

Now that I think about it, shouldn't the create account line also be hidden? I have signups disabled.

Also fixed now :)

@BlackDex
Copy link
Collaborator Author

I'm checking to see if we can update to v2024.12.0. There seem to be some minor changes and some endpoints added.

@BlackDex BlackDex changed the title Add partial role support for manager only Add partial role support for manager only using web-vault v2024.12.0 Dec 11, 2024
@BlackDex
Copy link
Collaborator Author

Code is adjusted to support v2024.12.0 now. Not too much changed.

@BlackDex BlackDex force-pushed the mngr-custom-role branch 4 times, most recently from a16668a to 748a242 Compare December 15, 2024 11:25
@tessus
Copy link
Contributor

tessus commented Dec 16, 2024

I'd rather first fix a native client issue and release a version and after that merge this.

The native client issue has been fixed and a new version has been released.

Is it possible to merge this now and release the new web-vault 2024.12.0 (or the other way around as you suggested)?

@larena1
Copy link

larena1 commented Dec 17, 2024

I'd rather first fix a native client issue and release a version and after that merge this.

The native client issue has been fixed and a new version has been released.

Is it possible to merge this now and release the new web-vault 2024.12.0 (or the other way around as you suggested)?

What's the matter with you and the new web vault version? Isn't the current one working for you?

If not then just try to build and run it yourself

@tessus
Copy link
Contributor

tessus commented Dec 17, 2024

If not then just try to build and run it yourself

I already do. If you had read my earlier comments, you would have known.

@larena1
Copy link

larena1 commented Dec 17, 2024

If not then just try to build and run it yourself

I already do. If you had read my earlier comments, you would have known.

I'm just trying to understand why you're so keen on this being merged for the second time already.

@stefan0xC

This comment was marked as resolved.

Sign in to view
@BlackDex
Copy link
Collaborator Author

No it's not. It is there.
Let me check.

@BlackDex
Copy link
Collaborator Author

BlackDex commented Dec 17, 2024 •
edited
Loading

@stefan0xC seems to work for me:

[response][INFO] (get_groups_details) GET /api/organizations/<org_id>/groups/details => 200 OK

Be sure to pull in the latest commit and are on:

git rev-parse HEAD
b0920a621c01f255753f3a878c0851d2addc861e

@BlackDex
Add partial role support for manager only
adacdda 
- Add the custom role which replaces the manager role
- Added mini-details endpoint used by v2024.11.1

These changes try to add the custom role in such a way that it stays compatible with the older manager role.
It will convert a manager role into a custom role, and if a manager has `access-all` rights, it will enable the correct custom roles.
Upon saving it will convert these back to the old format.

What this does is making sure you are able to revert back to an older version of Vaultwarden without issues.
This way we can support newer web-vault's and still be compatible with a previous Vaultwarden version if needed.

In the future this needs to be changed to full role support though.

Fixed the 2FA hide CSS since the order of options has changed

Signed-off-by: BlackDex <[email protected]>
@BlackDex
Fix hide passkey login
b60a8ec 
Signed-off-by: BlackDex <[email protected]>
@BlackDex
Fix hide create account
f958731 
Signed-off-by: BlackDex <[email protected]>
@BlackDex
Small changes for v2024.12.0
b0920a6 
Signed-off-by: BlackDex <[email protected]>
@stefan0xC
Copy link
Contributor

Ah, you're right. It's there. I just confused the error message:
image

@stefan0xC

This comment was marked as resolved.

Sign in to view
@BlackDex
Copy link
Collaborator Author

@stefan0xC What happens then exactly?
It doesn't work? And is it mixed with with an accessAll?

@stefan0xC
Copy link
Contributor

stefan0xC commented Dec 17, 2024 •
edited
Loading

@stefan0xC What happens then exactly? It doesn't work? And is it mixed with with an accessAll?

The exception happened when I tried to open the Custom user member (which used to be a Manager role without accessAll) or the group. But now after changing the group I can't seem to be able to replicate the issue anymore, even if I change the permissions back.

edit: I'm unable to reproduce the issue. It might have been caused by using an in-between version of the web-vault...

stefan0xC

This comment was marked as resolved.

Sign in to view

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfunkt dfunkt dfunkt left review comments

@stefan0xC stefan0xC stefan0xC requested changes

@tessus tessus tessus left review comments

@dani-garcia dani-garcia Awaiting requested review from dani-garcia

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BlackDex @tessus @dfunkt @larena1 @stefan0xC