add initial message endpoint #31

brngylni · 2024-07-09T23:34:25Z

This PR adds the initial message endpoint. #5

gridhead

@brngylni Please squash the commits and every other changes required on the codebase can be done after the merge.

gridhead · 2024-07-10T05:14:05Z

webhook_to_fedora_messaging/endpoints/parser/parser.py

@@ -0,0 +1,63 @@
+


Please remove the empty line from the top of the file.

gridhead · 2024-07-10T05:14:25Z

webhook_to_fedora_messaging/endpoints/parser/parser.py

+
+
+


Please leave two lines of difference between each segregated block definition.

gridhead · 2024-07-10T05:15:08Z

webhook_to_fedora_messaging/endpoints/parser/parser.py

+
+    headers = get_github_headers(request.headers)
+
+    secret = "00b863c62c96aa67aeea966104362b1136ce883d" # will come from a data source


Please read this from a configuration file rather than hardcoding it into the code itself.

Will change here after the database models are changed

gridhead · 2024-07-10T05:17:04Z

webhook_to_fedora_messaging/endpoints/util.py

+def verify_signature(payload_body: str, secret_token: str, signature_header: str) -> bool:
+    """Verify that the payload was sent from GitHub by validating SHA256.
+
+    Raise and return 403 if not authorized.
+
+    Args:
+        payload_body: original request body to verify (request.body())
+        secret_token: GitHub app webhook token (WEBHOOK_SECRET)
+        signature_header: header received from GitHub (x-hub-signature-256)
+    """
+    if not signature_header:
+        return False
+    hash_object = hmac.new(secret_token.encode('utf-8'), msg=payload_body, digestmod=hashlib.sha256)
+    expected_signature = "sha256=" + hash_object.hexdigest()
+
+    if not hmac.compare_digest(expected_signature, signature_header):
+        return False
+    return True


Is it intentional to have the identical code block for the function definition of verify_signature in the parser module? If not, please remove it from either of the locations.

I'll remove it from util file

gridhead · 2024-07-10T05:17:44Z

webhook_to_fedora_messaging/endpoints/util.py

+
+
+def fas_by_github(username: str) -> str:
+    """Get the Fedora Account System Username of the given github username


GitHub is a proper noun.

Suggested change

"""Get the Fedora Account System Username of the given github username

"""Get the Fedora Account System Username of the given GitHub username

gridhead · 2024-07-10T05:18:35Z

webhook_to_fedora_messaging/endpoints/util.py

+    """Get the Fedora Account System Username of the given github username
+
+        Args:
+            username: Github Username"""


GitHub is a proper noun.

Suggested change

username: Github Username"""

username: GitHub Username"""

abompard · 2024-07-12T06:03:14Z

webhook_to_fedora_messaging/config/defaults.py

@@ -9,7 +9,7 @@ class Defaults:
    PERMANENT_SESSION_LIFETIME = 604800
    SESSION_COOKIE_NAME = "user"

-
+FASJSON_URL = "https://fasjson.tinystage.test/fasjson"


The default should be something reasonable for production use, such as https://fasjson.fedoraproject.org.

The development value should be put in the development configuration file: devel/ansible/roles/dev/files/config.toml

You still need to set the development value in devel/ansible/roles/dev/files/config.toml

webhook_to_fedora_messaging/endpoints/message.py

abompard · 2024-07-12T06:07:00Z

webhook_to_fedora_messaging/endpoints/message.py

+
+
+app = Flask(__name__)
+message_endpoint = Blueprint("message_endpoint", __name__)


Blueprints are supposed to contain more than one view. You can define a generic blueprint for the app in endpoints.__init__.py and register the views with it.
Examples: https://github.com/fedora-infra/noggin/blob/dev/noggin/controller/__init__.py and https://github.com/fedora-infra/noggin/blob/dev/noggin/controller/root.py

This may conflict with other branches I guess. We might need another pr to switch this to the one you supposed. Since the other branches are merged.

I'm not sure I understand. Which branches will it conflict with?

I have the code you suggested on local but after a meeting with @gridhead we thought the current one might be better. So whichever you guys decide I can push.

abompard · 2024-07-12T06:10:57Z

webhook_to_fedora_messaging/endpoints/message.py

+    Used for creating a new message by sending a post request to /message path
+
+        Request Body:
+        service_uuid: Service related to message.    


We can't expect the message body to always contain a key named service_uuid, because we don't control the content of the webhook. Instead, we can put the UUID in the URL route. You can change the route pattern in the decorator to: /message/<service_uuid>, and add service_uuid to the function's arguments.

abompard · 2024-07-12T06:11:11Z

webhook_to_fedora_messaging/endpoints/message.py

+        service_uuid: Service related to message.    
+
+    """
+    if not validate_request(request, fields=['service_uuid']):


You don't need to pass request, it's a global object.

abompard · 2024-07-12T06:53:41Z

webhook_to_fedora_messaging/endpoints/util.py

+
+
+def not_found() -> Response:
+    return Response({'message': 'Not Found'}, status=404, mimetype='application/json')


This is equivalent to doing return {'message', 'Not Found'}, 404:

if you return a dict from a view, Flask will automatically convert it to JSON and set the Content-Type header

if you return a tuple from a view with a number as the second element, Flask will use it as the return code.

Please look at this Flask doc about error handling in API servers

I'm going to fix it here https://github.com/fedora-infra/webhook-to-fedora-messaging/pull/32/files

abompard · 2024-07-12T06:54:00Z

webhook_to_fedora_messaging/endpoints/util.py

+
+
+def success(data: dict) ->Response:
+    return Response(data, status=200, mimetype='application/json')


This is equivalent to just returning data, no need for this function.

abompard · 2024-07-12T06:56:52Z

webhook_to_fedora_messaging/endpoints/util.py

+
+
+def bad_request() -> Response:
+    return Response("{'message': 'Bad Request'}", status=400, mimetype='application/json')


This is equivalent to returning {'message': 'Bad Request'}, 400. Please see the doc above to setup a JSON-aware error handler that will allow us to just raise exceptions or use flask.abort().

abompard · 2024-07-12T06:57:23Z

webhook_to_fedora_messaging/endpoints/util.py

+
+
+def created(data: dict) -> Response:
+    return Response(data, status=201, mimetype='application/json')


This is equivalent to returning data, 201, no need for this function

abompard · 2024-07-12T06:57:58Z

webhook_to_fedora_messaging/endpoints/util.py

+
+
+def conflict(data: dict) -> Response:
+    return Response(data, status=409, mimetype='application/json')


Same thing here.

gridhead

I am not dragging this any longer than it has to be. Other required changes can be made to the codebase after it is merged.

Merging now.

abompard · 2024-07-23T10:53:21Z

Please rebase this branch, I'll review it afterwards.

brngylni · 2024-07-23T11:25:13Z

Please rebase this branch, I'll review it afterwards.

Rebased it

abompard · 2024-07-23T12:10:55Z

webhook_to_fedora_messaging/endpoints/message.py

+    try:
+        service = db.session.scalar(select(Service).where(Service.uuid == service_uuid)).one()
+    except NoResultFound:
+        return {'message': 'Not Found'}, 404


You can provide a better message, such as Service UUID not found.

abompard · 2024-07-23T12:14:11Z

webhook_to_fedora_messaging/endpoints/message.py

+
+    if msg is not None:
+        api.publish(msg)
+        return {'message': 'Success'}, 200


You don't need to add , 200, it's the default.
Also, you can maybe provide a better response, such as:

Suggested change

return {'message': 'Success'}, 200

return {'status': 'OK', 'message_id': msg.id}

This will help debugging in case something goes wrong: we'll be able to search for the message by ID in datanommer.

I'm not sure if messages have an id attribute in our case. Since we don't log the messages, I thought we don't have that

Yeah this message object is a Fedora Messaging Message instance, so it does have an ID. It's not a database object.

abompard · 2024-07-23T12:14:57Z

webhook_to_fedora_messaging/endpoints/message.py

+        api.publish(msg)
+        return {'message': 'Success'}, 200
+    else:
+        return abort(400, {'message': "Bad Request"})


You can provide a better message here, for example The webhook could not be converted to a message (if that's what's going on).

I think we can remove the else block since we already use a try-except block above

abompard · 2024-07-23T12:16:20Z

webhook_to_fedora_messaging/endpoints/parser/parser.py

+    if service_type.lower() == "github":
+        return _github_parser(secret)
+    else:
+        raise ValueError


Please use an instance with a message:

Suggested change

raise ValueError

raise ValueError(f"Unsupported service: {service_type}")

abompard · 2024-07-23T12:16:54Z

webhook_to_fedora_messaging/endpoints/parser/parser.py

+def _github_parser(secret: str) -> GithubMessageV1:
+    """Convert Flask request objects into desired FedMsg format.
+
+    Args:


This args list does not match the actual arguments.

abompard · 2024-07-23T12:19:12Z

webhook_to_fedora_messaging/endpoints/message.py

+    except SignatureMatchError:
+        return abort(400, {'message': "Bad Request"})


You can also provide a better message here, just reuse the message associated with the exception:

Suggested change

except SignatureMatchError:

return abort(400, {'message': "Bad Request"})

except SignatureMatchError as e:

return abort(400, {'message': str(e)})

abompard · 2024-07-23T12:20:06Z

webhook_to_fedora_messaging/endpoints/parser/parser.py

+    Return false if not authorized.
+
+    Args:
+        payload_body: original request body to verify (request.body())


This arg is not there anymore.

abompard · 2024-07-23T12:21:02Z

webhook_to_fedora_messaging/endpoints/parser/parser.py

+    if not hmac.compare_digest(expected_signature, signature_header):
+        return False
+    return True


You can just do:

Suggested change

if not hmac.compare_digest(expected_signature, signature_header):

return False

return True

return hmac.compare_digest(expected_signature, signature_header)

abompard · 2024-07-23T12:21:57Z

webhook_to_fedora_messaging/exceptions/exceptions.py

@@ -0,0 +1,2 @@
+class SignatureMatchError(Exception):


There is already an exceptions.py file in the main directory, please use it.

abompard · 2024-07-23T12:24:20Z

webhook_to_fedora_messaging/endpoints/parser/parser.py

+        raise ValueError
+
+
+def _github_parser(secret: str) -> GithubMessageV1:


Please put this Github-specific code into a github.py module (also put the verify_signature() function there as it is Github-specific as well).

Signed-off-by: Mehmet Baran Geylani <[email protected]>

abompard

Looks good, thanks!

gridhead requested changes Jul 10, 2024

View reviewed changes

abompard requested changes Jul 12, 2024

View reviewed changes

brngylni force-pushed the dev_message_endpoint branch 2 times, most recently from 858fe10 to 259b420 Compare July 16, 2024 06:35

gridhead approved these changes Jul 16, 2024

View reviewed changes

brngylni force-pushed the dev_message_endpoint branch from 259b420 to 18b8888 Compare July 23, 2024 11:23

brngylni requested a review from abompard July 23, 2024 11:47

abompard requested changes Jul 23, 2024

View reviewed changes

brngylni force-pushed the dev_message_endpoint branch 3 times, most recently from 568fc73 to 3cda15b Compare July 24, 2024 07:38

Add message endpoint

742e8d8

Signed-off-by: Mehmet Baran Geylani <[email protected]>

brngylni force-pushed the dev_message_endpoint branch from 5ccdf8f to 742e8d8 Compare July 24, 2024 09:06

abompard approved these changes Jul 24, 2024

View reviewed changes

abompard merged commit c5d101f into fedora-infra:main Jul 24, 2024
5 of 10 checks passed


		headers = get_github_headers(request.headers)

		secret = "00b863c62c96aa67aeea966104362b1136ce883d" # will come from a data source



		def fas_by_github(username: str) -> str:
		"""Get the Fedora Account System Username of the given github username



		app = Flask(__name__)
		message_endpoint = Blueprint("message_endpoint", __name__)



		def not_found() -> Response:
		return Response({'message': 'Not Found'}, status=404, mimetype='application/json')



		def success(data: dict) ->Response:
		return Response(data, status=200, mimetype='application/json')



		def bad_request() -> Response:
		return Response("{'message': 'Bad Request'}", status=400, mimetype='application/json')



		def created(data: dict) -> Response:
		return Response(data, status=201, mimetype='application/json')



		def conflict(data: dict) -> Response:
		return Response(data, status=409, mimetype='application/json')

	return {'message': 'Success'}, 200
	return {'status': 'OK', 'message_id': msg.id}

	raise ValueError
	raise ValueError(f"Unsupported service: {service_type}")

		except SignatureMatchError:
		return abort(400, {'message': "Bad Request"})

		raise ValueError


		def _github_parser(secret: str) -> GithubMessageV1:

add initial message endpoint #31

add initial message endpoint #31

Conversation

brngylni commented Jul 9, 2024

gridhead 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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gridhead left a comment

Choose a reason for hiding this comment

abompard commented Jul 23, 2024

brngylni commented Jul 23, 2024

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

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

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

Choose a reason for hiding this comment

abompard left a comment

Choose a reason for hiding this comment