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.

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

Introduce InitiatorIDs #4587

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Introduce InitiatorIDs #4587

merged 1 commit into from
Mar 21, 2024

Conversation

kobergj
Copy link
Contributor

@kobergj kobergj commented Mar 20, 2024

Allows sending a header Initiator-ID on http requests. This will be passed down ocis and returned on sse events. Clients can use this to avoid unnecessary requests.

See ticket for more details: owncloud/ocis#8677

@kobergj
Copy link
Contributor Author

kobergj commented Mar 20, 2024

Note: This hacks into the auth middleware because it is only adding 2 lines of code there. The alternative would be to add a lot more code for a dedicated middleware. However I am a bit reluctant to do that as it will bring more complexity to a already complex part of the code

pkg/ctx/initiatorctx.go Outdated Show resolved Hide resolved
@butonic
Copy link
Contributor

butonic commented Mar 20, 2024

Yeah ... not pretty ... the correct way would be a dedicated middleware. The current implementation only passes the initiator-id when the user is authenticated, but the initiator-id should not depend on that.

@butonic
Copy link
Contributor

butonic commented Mar 20, 2024

I'm ok with this to get a PoC, but we should take the time and implement this properly.

@kobergj
Copy link
Contributor Author

kobergj commented Mar 20, 2024

I'm ok with this to get a PoC, but we should take the time and implement this properly.

It is not really about time. I'm concerned additional middlewares make the logic even more complex. Also I am not really sure having a middleware that just extracts one parameter is worth it. Maybe if we had more ids or other information we want to pass down to ocis then this would make more sense

The current implementation only passes the initiator-id when the user is authenticated, but the initiator-id should not depend on that.

If you are not authenticated, you do not have an sse connection, hence you do not receive any clientlog events anyways. So there is no need for passing the initiatorid in that case

@kobergj kobergj requested a review from butonic March 20, 2024 13:45
@kobergj kobergj marked this pull request as ready for review March 20, 2024 15:38
@kobergj kobergj requested review from labkode, glpatcern and a team as code owners March 20, 2024 15:38
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

YOLO

@kobergj kobergj merged commit ef4dac6 into cs3org:edge Mar 21, 2024
9 checks passed
@kobergj kobergj deleted the InitiatorID branch March 21, 2024 08:20
@micbar micbar mentioned this pull request Jun 19, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants