Bitwarden access manager #10

yannickhilber · 2023-02-23T15:27:15Z

Description

Add script to retrieve Bitwarden configuration using API and to compare with a configuration file.

The script reuse the logic of github_access_manager script and some parts can be share between the two script. This will be perform in another PR.

Note for the reviewer

member attribute access_all is set to true only when a member has the role type owner
member attribute type refers to the member role: 0 = user, 1 = owner, 2 = admin, 3 = manager, 4 = custom (when writing this, I realize that I forgot to add a logic to make it readable in the toml config. I will add it later)
group attribute access_all is set to true only when Grant access to all current and future collections box is tick.
GroupCollectionAccess read_only attribute is set to true when the permission Can view or Can view, except password are set. This is set to false when the permission Can edit or Can edit, except password are set. The API doesn't do the distinction Can ... and Can ..., except password permissions.
The API doesn't allow to retrieve the collection name, we need to set external ID in the UI to retrieve his name

Example output

The following collections on Bitwarden need to be changed to match organization.toml:

  [[collection]]
  collection_id = 8e69ce49-85ae-4e09-a52c-afaf00a90a3f
  external_id = ""
  member_access = [
    { member_id = 2564c11f-fc1b-4ec7-aa0b-afaf00a9e4a4, member_name = "yan", role = "c6a13b93-edc1-4c3b-9fc5-afaf00a8d33f" },
  ]
  group_access = [
-   { group_id = c6a13b93-edc1-4c3b-9fc5-afaf00a8d33f, group_name = "group1", readOnly = "True" },
+   { group_id = c6a13b93-edc1-4c3b-9fc5-afaf00a8d33f, group_name = "group2", readOnly = "True" },
  ]

The following members on Bitwarden need to be changed to match organization.toml:

  [[member]]
  member_id = 2564c11f-fc1b-4ec7-aa0b-afaf00a9e4a4
  member_name = yan
- email = [email protected]
- type = 3
+ email = [email protected]
+ type = 2
  access_all = False

The following groups on Bitwarden need to be changed to match organization.toml:

  [[group]]
  group_id = c6a13b93-edc1-4c3b-9fc5-afaf00a8d33f
  group_name = group1
- access_all = False
+ access_all = True

The following members of group 'group1' are specified in organization.toml, but are not present on Bitwarden:

  yunkel

Issue chorus-infrastructure #3941

* Member * Group * Collection * GroupMember * MemberCollectionAccess * GroupCollectionAccess

ruuda

I reviewed the docstring 😅 Will continue the review next week!

bitwarden_access_manager.py

ruuda

I finally reviewed the full thing now. Overall it looks really nice! I left many comments, but most of them are fairly superficial Python things.

I mostly looked superficially at the implementation now that I did not try to make sense of the diffing logic on a high level, but that is very similar to what we already have for GitHub, so if the data structures are good for Bitwarden, it should work. I’ll do a more high-level review later then, we can focus on the Python things first.

Unrelated to the content of the PR, but you ran into both mypy and black and I realize I didn’t document this at all, it would be nice to mention these in the readme or in a CONTRIBUTING.md.

bitwarden_access_manager.py

ruuda · 2023-02-28T17:59:59Z

bitwarden_access_manager.py

+    email: str
+    type: str
+    access_all: Optional[bool]
+    # groups: Tuple[str, ...]


(Note to self, we need to make a decision on this before merging.)

Following the docstring, the group should go in the member, right? Then this should be uncommented, no?

Yes the group should go there in order to keep track of member assigned group in the toml file.

As we go with this approach a member not part of a group will be reported in the user diff and we don't need a specific group_diff to check if a member is whether part of a group of not.

I implemented the change in b0e8a2f

bitwarden_access_manager.py

ruuda · 2023-03-07T16:31:01Z

bitwarden_access_manager.py

+            f"member_id = {self.id}",
+            f"member_name = {self.name}",
+            f"email = {self.email}",
+            f"type = {str(self.type)}",


This is still unresolved.

bitwarden_access_manager.py

yannickhilber · 2023-03-16T13:33:11Z

This is still unresolved.

What is unresolved here ?

ruuda · 2023-03-17T09:01:24Z

What is unresolved here ?

This: https://github.com/ChorusOne/github-access-manager/pull/10/files#r1128160013. If you go to the “files changed” tab you can see the full context, from the “conversation” tab sometimes things are missing.

yannickhilber · 2023-03-17T11:20:17Z

This: https://github.com/ChorusOne/github-access-manager/pull/10/files#r1128160013. If you go to the “files changed” tab you can see the full context, from the “conversation” tab sometimes things are missing.

Thanks, I will check with the full context next time !

ruuda

I think it’s almost good! I have a few more superficial comments about the Python, but the structure of the program looks good.

bitwarden_access_manager.py

ruuda · 2023-03-22T09:53:59Z

bitwarden_access_manager.py

+    email: str
+    type: str
+    access_all: Optional[bool]
+    # groups: Tuple[str, ...]


Following the docstring, the group should go in the member, right? Then this should be uncommented, no?

ruuda · 2023-03-22T09:54:15Z

bitwarden_access_manager.py

+    email: str
+    type: str
+    access_all: Optional[bool]
+    # groups: Tuple[str, ...]


Following the docstring, the group should go in the member, right? Then this should be uncommented, no?

bitwarden_access_manager.py

ruuda · 2023-03-22T10:17:18Z

bitwarden_access_manager.py

+    def get_group_members(self, id: str, name: str) -> Iterable[GroupMember]:
+        members = json.load(self._http_get(f"/public/groups/{id}/member-ids"))
+
+        for member in members:


Now we will request every member once for every group that they are part of. It’s not a huge problem, but we could reduce the number of API calls by keeping a dict of member id to member, and only requesting the member if we haven’t requested it before.

But this PR is already open for a long time, let’s not do that right now, we can do it as a follow-up.

bitwarden_access_manager.py

When a member is not part of a group, it will be reflected in Member diff. Therefore group diff is not necessary and will result in duplicated information. group_diff is removed.

ruuda

This looks good to me! I have a few more small Python comments, but at this point it’s only nitpicks, I think the structure of the program looks good, and the access checks all look correct too!

I’ll try and run this on our organization and report back if I run into any issues.

bitwarden_access_manager.py

ruuda · 2023-03-29T09:27:59Z

bitwarden_access_manager.py

+        groups = self.groups or ()
+        if len(groups) > 0:
+            groups_str = ", ".join(f'"{g}"' for g in sorted(groups))
+            result = result + "groups = [ " + groups_str + " ]"
+        else:
+            result = result + "groups = []"


Suggested change

groups = self.groups or ()

if len(groups) > 0:

groups_str = ", ".join(f'"{g}"' for g in sorted(groups))

result = result + "groups = [ " + groups_str + " ]"

else:

result = result + "groups = []"

result = result + "groups = [" + ", ".join(json.dumps(g) for g in sorted(groups)) + "]"

groups is no longer Optional, so we don’t need the or (). Also, I don’t see a reason to have the spaces after [ and before ] if the list is non-empty, if we omit those, then the case distinction goes away.

Why do we need to convert g into a json string with json.dumps ?

If we just surround g by quotes, then if the string g contains a " or a \, it will produce invalid toml. If we use json.dumps to format a json string literal, then it will escape " and \ to \" and \\. This works because toml has the same escaping rules as json.

bitwarden_access_manager.py

ruuda · 2023-03-30T12:42:21Z

Ah small thing, can you chmod +x bitwarden_access_manager.py?

ruuda · 2023-03-30T12:45:38Z

bitwarden_access_manager.py

+
+    @staticmethod
+    def new(client_id: str, client_secret: str) -> BitwardenClient:
+        response = requests.post(


I noticed now that I try to run this ... this is using the third-party library requests. We don’t need it to make requests, we already import http.client from the standard library, and we even construct the client already. You can see here how we do this in the GitHub version:

github-access-manager/main.py

Lines 609 to 624 in 6fc4782

self.connection.request(

method="GET",

url=url,

headers={

"Authorization": f"token {self.github_token}",

"Accept": "application/vnd.github.v3+json",

"User-Agent": "Github Access Manager",

},

)

# TODO: Respect these response headers

# X-RateLimit-Limit: 5000

# X-RateLimit-Remaining: 4994

# X-RateLimit-Reset: 1653495296

# X-RateLimit-Used: 6

# X-RateLimit-Resource: core

return self.connection.getresponse()

I can push a commit to fix this, I’m looking into it anyway.

ruuda · 2023-03-30T12:47:46Z

bitwarden_access_manager.py

+            data={
+                "grant_type": "client_credentials",
+                "scope": "api.organization",
+                "Accept": "application/json",


Does the Accept really go here? This is an http header, shouldn’t it go in headers?

ruuda · 2023-03-30T17:53:48Z

bitwarden_access_manager.py

+* The access_all key for members and groups is optional, default is false.
+* The member_access key for a collection only list members with direct access
+to collection. It omits direct access for members with the role
+owners or admins because they have implicit access to all collections.


It turns out, this is not true, I thought it was because I could access everything in our organization, but it was because access_all was true for me (which I discovered using this tool :D). So could we remove this special case?

yannickhilber added 12 commits February 23, 2023 15:30

Add BitwardenClient class

3ef32c2

Add structural classes

c749769

* Member * Group * Collection * GroupMember * MemberCollectionAccess * GroupCollectionAccess

Add BitwardenClient methods to populate objects from API

09d62a1

Add Configuration class

0f54dec

Copy print diff functions from github-access-manager script

41c3359

Add main

33f3c80

Add diff to main

da3cdd6

Rename bitwarden script

24d96db

Add example configuration in script documentation

3ae6439

Fix type error

a8de610

Add format_toml missing method to Collection

d9dce1a

Rename variable to follow python convention

f33121c

yannickhilber requested a review from ruuda February 23, 2023 15:27

ruuda reviewed Feb 24, 2023

View reviewed changes

yannickhilber added 5 commits February 28, 2023 08:54

User friendly member type and optional access_all member key

7913d5a

Optional group_access and member_access keys for collection

e8827ec

Change read_only parameter for readable access parameter

511286a

Optional access_all group key

9316ca8

Remove redundant ids in collection group_access and member_access

166f7aa

ruuda reviewed Feb 28, 2023

View reviewed changes

yannickhilber added 7 commits March 2, 2023 08:50

Remove optional for access_all

63296ca

Apply black formatting

1d26b3a

Remove comment about collection external_id splicing

7250ebf

Remove redundant and not used BitwardenClient methods

c23a41c

Change implicit to explicit boolean conversion

ebf5372

Fix mypy errors

6fd0242

Update docstring

7c6e97f

yannickhilber force-pushed the bitwarden_access_manager branch from 0bb41b4 to e3a0b84 Compare March 3, 2023 08:52

ruuda self-requested a review March 6, 2023 12:23

ruuda reviewed Mar 7, 2023

View reviewed changes

yannickhilber force-pushed the bitwarden_access_manager branch from 0a98f16 to 1168dc0 Compare March 16, 2023 13:28

List direct member_access in collections

afdb1ca

yannickhilber added 3 commits March 17, 2023 10:58

Update docstring and add copyright

bed3c8d

Remove not useful string conversion for access format_toml

f7c0ffb

Add quotes to format_toml outputs

96bf04a

ruuda reviewed Mar 22, 2023

View reviewed changes

yannickhilber added 2 commits March 28, 2023 10:54

Add groups to Member

b0e8a2f

When a member is not part of a group, it will be reflected in Member diff. Therefore group diff is not necessary and will result in duplicated information. group_diff is removed.

Change group/member_access type for Collection

cda04f0

yannickhilber force-pushed the bitwarden_access_manager branch from 6fddc72 to cda04f0 Compare March 28, 2023 08:54

yannickhilber added 4 commits March 28, 2023 11:16

Rename get_access to map_access

22d7d44

Rename MemberCollectionAccess and GroupCollectionAccess attributes

00bd820

Automaticaly build int_to_member_type dict

5b326d5

Use default_dict

cc2c081

yannickhilber force-pushed the bitwarden_access_manager branch from df16e5a to cc2c081 Compare March 28, 2023 09:48

inline get_collection_groups in get_collections

d097be8

yannickhilber force-pushed the bitwarden_access_manager branch from 66ed801 to d097be8 Compare March 28, 2023 10:05

Automaticaly build member_groups dict

1c9f2c7

ruuda approved these changes Mar 29, 2023

View reviewed changes

Script cleanup

f858a73

ruuda reviewed Mar 30, 2023

View reviewed changes

ruuda mentioned this pull request Apr 4, 2023

Add a few improvements for Bitwarden Access Manager #13

Merged

Add execute flag

150d179

yannickhilber merged commit fbda58b into master Apr 4, 2023

yannickhilber deleted the bitwarden_access_manager branch April 4, 2023 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bitwarden access manager #10

Bitwarden access manager #10

yannickhilber commented Feb 23, 2023

ruuda left a comment

ruuda left a comment

ruuda Feb 28, 2023

ruuda Mar 22, 2023

ruuda Mar 22, 2023

yannickhilber Mar 28, 2023 •

edited

Loading

ruuda Mar 7, 2023

yannickhilber commented Mar 16, 2023

ruuda commented Mar 17, 2023

yannickhilber commented Mar 17, 2023

ruuda left a comment

ruuda Mar 22, 2023

ruuda Mar 22, 2023

ruuda Mar 22, 2023

ruuda left a comment

ruuda Mar 29, 2023

yannickhilber Mar 30, 2023 •

edited

Loading

ruuda Mar 31, 2023

ruuda commented Mar 30, 2023

ruuda Mar 30, 2023

ruuda Mar 30, 2023

ruuda Mar 30, 2023

ruuda Mar 30, 2023

	self.connection.request(
	method="GET",
	url=url,
	headers={
	"Authorization": f"token {self.github_token}",
	"Accept": "application/vnd.github.v3+json",
	"User-Agent": "Github Access Manager",
	},
	)
	# TODO: Respect these response headers
	# X-RateLimit-Limit: 5000
	# X-RateLimit-Remaining: 4994
	# X-RateLimit-Reset: 1653495296
	# X-RateLimit-Used: 6
	# X-RateLimit-Resource: core
	return self.connection.getresponse()

Bitwarden access manager #10

Bitwarden access manager #10

Conversation

yannickhilber commented Feb 23, 2023

Description

Note for the reviewer

ruuda left a comment

Choose a reason for hiding this comment

ruuda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannickhilber Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannickhilber commented Mar 16, 2023

ruuda commented Mar 17, 2023

yannickhilber commented Mar 17, 2023

ruuda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruuda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannickhilber Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruuda commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannickhilber Mar 28, 2023 •

edited

Loading

yannickhilber Mar 30, 2023 •

edited

Loading