Suggestion: refactoring Macaroon service a little bit ?

[ewjoachim]

First I'd like to say I'm extremely grateful to all the people who worked on the macaroons system, it's a really nice system and I'm a huge fan :)

This stems from the work I'm doing in #7124. It's definitely out of scope for that PR, but I'm feeling a bit of friction working with the Macaroons service, and I'd like to get some opinions before I open a PR on that new subject.

TL;DR: the methods are in some ways sightly inconsistent, and while they serve their initial objective perfectly, I'm afraid it might be complicated to work with it in the long run.

There are 5 objects representing "a macaroon":

• A serialized macaroon string (called "raw macaroon")
• A macaroon ORM object (Macaroon]
• A macaroon ORM id as string
• A PyMacaroons instance
• (A macaroon ORM id as UUID(), it's not used in the API but it's what you get if you do Macaroon().id without calling str() on the result)

What I would suggest as an API would be to have the Macaroon object as lingua franca, and then all the methods would operate on a Macaroon, or transform types from and to Macaroon.

This is the API that the service currently provides. I'll be adding some type hints:

def find_macaroon(self, macaroon_id: str) -> Optional[Macaroon]:
	# Makes 1 DB call, always joins on table users
    ...

def find_userid(self, raw_macaroon: str) -> Optional[UUID]:
	# Calls find_macaroon()
	# Is one of the only functions that can deserialize a raw_macaroon but
	# doesn't return that macaroon instance. Instead, it does something with
	# it (extract the user) and then discards the raw_macaroon.
	...

def verify(self, raw_macaroon: str, context, principals, permission) -> bool:
	# Starts with the same code as find_userid but raises
	# InvalidMacaroon instead of returning None.
	# The end of the function is different.
	# context, principals, permission are very specific to the pyramid
	# auth API, it seems
	...

def create_macaroon(self, location, user_id, description, caveats) -> Tuple[str, Macaroon]:
	# The only function that can properly serialize a macaroon
	# Returns the serialized macaroon AND the DB instance
	...

def delete_macaroon(self, macaroon_id: str) -> None:
	# Does 2 requests (find, delete)
	...

def get_macaroon_by_description(self, user_id: UUID, description: str) -> Optional[Macaroon]:
	...

My suggestion:

def find_by_id(self, macaroon_id: str) -> Optional[Macaroon]:
    ...

def find_by_description(self, user_id: str, description: str) -> Optional[Macaroon]:
	...

# Serialize and find_from_raw should share the "pypi-" constant string
def serialize(self, macaroon: Macaroon) -> str:
	...

def find_from_raw(self, raw_macaroon: str, verify: bool) -> Macaroon:
	# May raise InvalidMacaroon
	# If verify, will also check that the signature of the macaroon is correct
	...

# (There might be a _deserialize() that just deserializes and returns a PyMacaroon)

def get_associated_user(self, macaroon: Macaroon) -> User:
	...

def authorize(self, macaroon: Macaroon, context, principals, permission) -> bool:
	# If it's not possible to change this one because the API is
	# decided elsewhere, then it will call find_from_raw()
	...

def create(self, location, user_id, description, caveats) -> Macaroon:
	...

def delete(self, macaroon: Macaroon) -> None:
	...