The core mistake was that the webconn doesn't really go out of scope
once the connection disconnects. It is kept in the webhub connIndex
to be reconnected if the user connects again. This was the new
behavior as part of reliable websockets.
Therefore, it was a mistake to return the session to the pool
once the connection drops. Because the connection would still
recieve events from the web_hub.
And once you release the session, another login might acquire the
session and set some props, while the web_hub might still try
to send events to it, which will cause a read of the map prop.
The following test case illustrates such a race. It is very
hard to trigger it organically, hence I artificially wrote
the code.
The right fix is to release the session only when the connection
is stale and gets deleted from the conn index. The PR has been
load tested in `-race` mode just for extra sanity check.
```go
func TestHubSessionRace(t *testing.T) {
th := Setup(t).InitBasic()
defer th.TearDown()
s := httptest.NewServer(dummyWebsocketHandler(t))
defer s.Close()
th.Server.HubStart()
wc1 := registerDummyWebConn(t, th.App, s.Listener.Addr(), th.BasicUser.Id)
defer wc1.Close()
var wg sync.WaitGroup
wg.Add(2)
go func() {
defer wg.Done()
token := wc1.GetSessionToken()
// Return to pool after *WebConn.Pump finishes
wc1.App.Srv().userService.ReturnSessionToPool(wc1.GetSession())
// A new HTTP requests acquires a session which gets it from the pool
sess, _ := wc1.App.GetSession(token)
// Login happens which sets some session properties
sess.AddProp(model.SessionPropPlatform, "chrome")
}()
go func() {
defer wg.Done()
// Called from *WebConn.shouldSendEvent
t.Log("session: ", wc1.GetSession().Props[model.SessionPropIsGuest] == "true")
}()
wg.Wait()
}
```
https://mattermost.atlassian.net/browse/MM-46604
```release-note
NONE
```
* Allow embedded JSON in config.json for AdvancedLoggingConfig
* fix escaped JSON case
* Add unit test cases for escaped JSON
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
* Adds OmitConnection parameter to broadcast
Currently we have no means to omit sending a websocket event to a
specific connection id.
This is needed mainly so that the initiator won't receive an event for
the action it just initiated.
Will be used for the global drafts feature, so that we won't update
drafts through ws when a user is typing.
This commit adds OmitConnection to the Broadcast struct and to the
NewWebSocketEvent function signature.
shouldSendEvent should return false for that specific connection.
* Return early only if connection id matches the omitted
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
Apparently, this can happen in normal situations as well
and is causing confusion amongst customers. Reverting.
This reverts commit 9534efe534.
```release-note
NONE
```
* Delete profile image and invalidate cache on permanent user deletion
* Modify request to send 202 with error information on failing to delete profile image
* Add api endpoints, app layers for top inactive channels with dummy store calls
* Add store functions for top inactive channels
* Add model, store, app tests.
* Add client function and api tests
* Add participants information to TopInactiveChannel
* Translation fix
* Style fix while writing response
* Return channelmember IDs instead of profiles, query in batch avoiding inside the loop
* Make the following changes
- move DeleteAt to subqueries, to avoid select, group by
- Remove TeamId from response
- Count bots and webhook posts
* SQL query lint fix, store test fix to include bot messages
* make app-layers
* Fix empty participant lists being sent as [""]
* Track channel joins, to distinguish 0 activity channels vs new channels
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
Summary
We need a pointer to a pointer to set the original pointer to nil
The original task was not being set to nil (the local variable containing the pointer was being set to nil). The cancel function was being called even though the task had already been cancelled. This repeated cancellation was causing a panic because we were trying to close a channel that had already been closed the first time the task was cancelled.
Ticket Link
https://mattermost.atlassian.net/browse/MM-46402
* re-assign cluster interface after initializing enterprise interfaces
* add a unit test to check if cluster is triggered
* remove env overrides
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
* MM-45120: Starts tracking team join time. Adds API to retrieve team members who joined after a given time.
* MM-45120: Updates json casing to match model.User.
There were lots of cases where we were using the code 501
in an incorrect manner, and since 5xx codes are considered
as SLO violations, these were flagged by our monitoring incorrectly.
These were the guidelines to change the codes:
1. Any case of missing license or config not correctly enabled
is set as Forbidden. The logic is that the user is correctly
authenticated with a session, but does not have enough privileges
to have the higher license, or hasn't changed the config correctly.
2. Any case of incorrectly formed request is obviously a 400.
https://mattermost.atlassian.net/browse/MM-43202
```release-note
NONE
```
During attaching an object to a websocket message, we would
marshal it to json and attach the string output. But if the
marshalling failed, we would just log a warning and move on.
This would add an empty string to the message. But the client
assumes that the object is correctly attached and would
fail silently if it cannot find it.
So we become more strict and return the error so that
it reaches the caller.
https://mattermost.atlassian.net/browse/MM-45993
```release-note
NONE
```
The method a.postRemoveFromChannelMessage was being called
from a goroutine. Therefore, when SystemAdminClient.GetFlaggedPostsForUser
was being called later in the test with a mock post store,
it would naturally fail because the store would now be a
different store but the goroutine was supposed to be finished.
A hacky solution would be to add a sleep before
starting the mocked API call. But a deeper question
is why was the method run in a goroutine in the first place.
Removing a user from a channel is not a very common operation
and even if we look at the method, if the user is trying to
remove themselves, that message happens synchronously, but if
they are removing another user, that runs in a goroutine.
This seems like a very weird behavior. Therefore, to be consistent
I have just removed the goroutine and made everything synchronous.
The next step would be to stop logging an error and just return
the error upwards instead. Because that's what happens in the other
condition. But that would be exceeding the scope too much. Maybe
in a separate PR.
https://mattermost.atlassian.net/browse/MM-45021
```release-note
NONE
```
* Revert "Revert "MM-43828: Pass object length for some image operations (#20711)" (#20740)"
This reverts commit c7ae090dad.
* Fix to work in bifrost mode
```release-note
NONE
```
