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.

Sign up for GitHub

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

Jump to bottom

Websocket CORS Support #5667

Merged
merged 5 commits into from
Mar 23, 2017
Merged

Websocket CORS Support #5667

merged 5 commits into from
Mar 23, 2017

Conversation

bradhowes
Copy link
Contributor

Summary

Honor 'AllowCorsFrom' config setting when upgrading HTTP sockets to web sockets.

This is a second attempt after the first had build errors (but none due to api/websocket.go) Merged with latest Mattermost master just to be safe. Sorry for the noise.

Ticket Link

None

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Added or updated unit tests (required for all new features)
  • Added API documentation (required for all new APIs)
  • All new/modified APIs include changes to the drivers
  • Has enterprise changes (please link)
  • Has UI changes
  • Includes text changes and localization file (.../i18n/en.json and .../webapp/i18n/en.json) updates
  • Touches critical sections of the codebase (auth, upgrade, etc.)

@jasonblais jasonblais added this to the v3.8.0 milestone Mar 6, 2017
@jasonblais jasonblais added the 2: Dev Review Requires review by a developer label Mar 6, 2017
@bradhowes
Copy link
Contributor Author

What's with gofmt failure in build? Is there something I can/should do to fix that?

@jasonblais
Copy link
Contributor

@grundleborg any thoughts?

@grundleborg
Copy link
Contributor

@bradhowes looks like you've got a gofmt failure in your changes to api/websocket.go. You can run gofmt over that file locally to fix this, and then update the PR. You can also run make check-style locally to see if there are any similar errors before making a commit, so you don't have to wait for the build server to get to it.

@jasonblais jasonblais mentioned this pull request Mar 13, 2017
Closed
hmhealey
hmhealey approved these changes Mar 15, 2017
View reviewed changes
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Looks good to me. @crspeller, you might want to take a look at this

@hmhealey hmhealey requested a review from crspeller March 15, 2017 13:43
@slothbag
Copy link

Just updated to v3.7.0 and websockets are broken. Looking forward to getting this merged :)

crspeller
crspeller requested changes Mar 20, 2017
View reviewed changes
Copy link
Member

@crspeller crspeller left a comment

Choose a reason for hiding this comment

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

Looks good! I think it will work, but can you prove it by updating the unit tests please? This is not something we want breaking in the future!

@bradhowes
Copy link
Contributor Author

I've added tests that I believe cover the CORS settings. The tests revealed that the order of the arguments to the strings.contains call should be swapped, so I did. But that code I originally copied from the CORS support for regular HTTP connections. Note that I'm talking substring matching, not exact match. If exact match is all that is desired then the order of arguments would be immaterial.

@crspeller crspeller merged commit 120f5a6 into mattermost:master Mar 23, 2017
@crspeller
Copy link
Member

Looks good! Thanks!

@esethna esethna added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Not Needed New release tests not required labels Mar 23, 2017
jasonblais added a commit to mattermost/docs that referenced this pull request Mar 27, 2017
@jasonblais
Update troubleshooting.rst
cd79cc7 
Follow-up from
 - https://mattermost.atlassian.net/browse/PLT-5635
 - mattermost/mattermost#5667
 - mattermost/mattermost#5386

Also fix two broken links
This was referenced Mar 27, 2017
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crspeller crspeller crspeller approved these changes

@hmhealey hmhealey hmhealey approved these changes

Assignees
No one assigned
Labels
2: Dev Review Requires review by a developer Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Not Needed New release tests not required
Projects
None yet
Milestone
v3.8.0
Development

Successfully merging this pull request may close these issues.
8 participants
@bradhowes @jasonblais @grundleborg @slothbag @crspeller @hmhealey @esethna @BRH-Enrian