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

Add the feature to check the authorization header #170

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

kyeongsoosoo
Copy link
Contributor

Summary

I added the feature that checks the Authorization header.
I implemented this feature to test this PR

@mattermost-build
Copy link
Contributor

Hello @kyeongsoosoo,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2023

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (f006ddc) 2.90% compared to head (88d5846) 2.93%.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #170      +/-   ##
=========================================
+ Coverage    2.90%   2.93%   +0.03%     
=========================================
  Files          14      14              
  Lines        1823    1838      +15     
=========================================
+ Hits           53      54       +1     
- Misses       1768    1782      +14     
  Partials        2       2              
Files Coverage Δ
server/http_hooks.go 11.60% <6.66%> (-0.24%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hanzei hanzei added the 3: QA Review Requires review by a QA tester label Nov 6, 2023
@hanzei
Copy link
Contributor

hanzei commented Nov 6, 2023

Thanks @kyeongsoosoo 👍

@mickmister
Copy link
Contributor

I've tested this with a few different permutations, and it looks good other than one thing that seems odd to me:

$ ./authenticated-with-cookie
You are an authenticated user. The Authorization header should be an empty string.
Authorization header: 

$ ./authenticated-with-cookie-and-custom-header 
Authorization header: Bearer CustomToken

$ ./authenticated-with-cookie-and-mm-header 
You are an authenticated user. The Authorization header should be an empty string.
Authorization header: 

$ ./authenticated-with-mm-header-and-without-cookie 
You are an authenticated user. The Authorization header should be an empty string.
Authorization header: 

$ ./unauthenticated-with-custom-header 
Authorization header: Bearer MyToken

$ ./unauthenticated-without-header 
Authorization header:

This is more of an observation than anything - For the authenticated-with-cookie-and-custom-header run, it seems the server does not check the cookie in the case of a Authorization header value being present. This seems harmless, though I originally expected the request to be authenticated since the cookie provided is valid.

@hanzei What do you think? I'm thinking this and mattermost/mattermost#24391 are good to merge as is.

@hanzei
Copy link
Contributor

hanzei commented Nov 7, 2023

@kyeongsoosoo Heads up that there is conflict to resolve

@hanzei hanzei added the Awaiting Submitter Action Blocked on the author label Nov 7, 2023
@kyeongsoosoo
Copy link
Contributor Author

@kyeongsoosoo Heads up that there is conflict to resolve

@hanzei Conflicts resolved :)

Comment on lines +89 to +90
func (p *Plugin) handleCheckAuthHeader(w http.ResponseWriter, r *http.Request) {
isAuthenticatedUser := r.Header.Get("Mattermost-User-ID") != ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing the plugin.Context to the handler became difficult, so I switched to using the Mattermost-User-Id in the request header instead of the context's SessionId.

@mickmister mickmister removed the Awaiting Submitter Action Blocked on the author label Nov 9, 2023
@mickmister
Copy link
Contributor

@hanzei Do you think this is good to merge based on my comment above?

@hanzei
Copy link
Contributor

hanzei commented Nov 9, 2023

Yep, I think we are good the merge both PRs.

cc @DHaussermann

@mickmister mickmister merged commit 439112e into mattermost:master Nov 9, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: QA Review Requires review by a QA tester Contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants