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 a few improvements for Bitwarden Access Manager #13

Merged
merged 9 commits into from
May 1, 2023
Merged

Add a few improvements for Bitwarden Access Manager #13

merged 9 commits into from
May 1, 2023

Conversation

ruuda
Copy link
Contributor

@ruuda ruuda commented Mar 30, 2023

I ran it against our actual Bitwarden organization, and ran into a few issues. I fixed them all and now it runs smoothly.

  • Drop the dependency on requests for initial authentication, we already use http.client.
  • Fix sorting of member access, because of the enum we cannot sort on the class itself, but we can sort on a property.
  • Fix printing of Group.access_all, it should be a boolean, not a string.
  • Add a progress indicator on stderr, it takes a ~minute to run the script against our organization, so this shows it is still making progress.
  • Make Member.name optional, it is not always present.
  • Because of this, we can’t identify collection members by name, identify them by email instead.
  • Make Collection.external_id optional, it can be null in Bitwarden (which is different from empty string!).

Open questions

Should we drop Member.name entirely? It is not always there, and people can put nonsense names in there. We can already identify members by email address, so we could just omit the name.

@yannickhilber
Copy link
Contributor

Closes #11

yannickhilber
yannickhilber approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@yannickhilber yannickhilber left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements.

@yannickhilber
Copy link
Contributor

Open questions

Should we drop Member.name entirely? It is not always there, and people can put nonsense names in there. We can already identify members by email address, so we could just omit the name.

I agree with removing Member.name, we can't use it to identify properly a member and I don't see how I don't see another usage for this information.

You can merge this branch and I will drop Member.name in bitwarden__access_manager branch before merging it into master.

@ruuda
Copy link
Contributor Author

ruuda commented Apr 4, 2023

You can merge this branch and I will drop Member.name in bitwarden__access_manager branch before merging it into master.

Let’s make a separate pull request, this one is getting big enough already 😅 Actually, can you merge #10? I will rebase this on top of the merge commit and then merge it.

Base automatically changed from bitwarden_access_manager to master April 4, 2023 15:27
@yannickhilber yannickhilber mentioned this pull request Apr 4, 2023
Open
ruuda added 9 commits May 1, 2023 17:06
@ruuda
Remove dependency on "requests"
0ea4a42 
We can do the initial authentication request just fine using the
standard library.
@ruuda
Ensure we can load member group access
5acc67b 
Sorting does not work because GroupAccess does not implement __lt__, we
can sidestep the issue by providing a comparison key and sorting only by
name.
@ruuda
Make group access_all a bool in toml, not str
2acbe67
@ruuda
Print status to stderr while fetching
09a82c2
@ruuda
Make member name optional, fix formatting of member
4caaa89 
Members on Bitwarden don't necessarily have a name. Maybe we should omit
it entirely from the config, because it's not how we identify users
anyway.
@ruuda
Identify collection members by email
b87000f 
As it turns out, members do not always have a name, sometimes it's null.
We can use the email instead.
@ruuda
Delete member_name from GroupMember
ec0cd1a 
It is not needed, and sometimes not present.
@ruuda
Make collection external_id optional
ecc427b 
It is optional in Bitwarden, so it needs to be optional here. Also print
progress when fetching collections.
@ruuda
Run autoformatter
bfab93e
@ruuda ruuda merged commit bf6ece6 into master May 1, 2023
@ruuda ruuda deleted the bw-improvements branch May 1, 2023 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yannickhilber yannickhilber yannickhilber approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@ruuda @yannickhilber