Improve notification workflow and API key handling. #259
Conversation
notifications.go
Outdated
|
|
||
| // Extract authenticated user if provided | ||
| var requestingUser *User | ||
| if len(authenticatedUser) > 0 && authenticatedUser[0] != nil { |
There was a problem hiding this comment.
Why should passing in this EVER be necessary?
I can understand it if notifications are supposed to be personal, but otherwise, this just makes no sense, as the requesting user themselves could be missing permissions and are now bypassing algorithmic scrutiny.
notifications.go
Outdated
| if err != nil { | ||
| log.Printf("[WARNING] Error getting parent org %s in createOrgNotification: %s. This is usually a region problem, and may cause issues with notification workflows..", orgId, err) | ||
| //return err | ||
| log.Printf("[WARNING] Error getting parent org %s in createOrgNotification: %s. Keeping authOrg as current org.", org.CreatorOrg, err) |
There was a problem hiding this comment.
Why print so much? Our logs will get clogged
notifications.go
Outdated
|
|
||
| // Look for admin with API key | ||
| for _, user := range authOrg.Users { | ||
| if user.Role == "admin" && len(user.Id) > 0 && len(selectedApikey) == 0 { |
There was a problem hiding this comment.
Wasn't the entire problem here that we were only considering admins? Shouldn't it work for non-admins as well, as long as they have editor access?
notifications.go
Outdated
| if err == nil && len(foundUser.ApiKey) > 0 { | ||
| selectedApikey = foundUser.ApiKey | ||
| break | ||
| if err == nil { |
There was a problem hiding this comment.
To help with indents AKA readability, write opposite logic instead:
if err != nil {
continue
}
notifications.go
Outdated
| if err == nil { | ||
| if len(foundUser.ApiKey) > 0 { | ||
| selectedApikey = foundUser.ApiKey | ||
| log.Printf("[DEBUG] Using admin's API key for notification workflow") |
There was a problem hiding this comment.
If you want verbose things you CAN see, but don't have to see, then wrap text in
if debug {
log.Printf("[DEBUG] ....")
}then we can turn it on or off :)
notifications.go
Outdated
| // If no admin key found, use requesting user's key | ||
| if len(selectedApikey) == 0 && requestingUser != nil && len(requestingUser.ApiKey) > 0 { | ||
| selectedApikey = requestingUser.ApiKey | ||
| log.Printf("[INFO] No admin API key available. Using requesting user's (%s) API key for notification workflow", requestingUser.Username) |
There was a problem hiding this comment.
This should be an error condition when trying to run the workflow instead.
If we don't have a valid account in an org, something is VERY wrong, and we should literally get an alert called up about it. Not this flimsy fallback fallback which doesn't solve anything.
notifications.go
Outdated
|
|
||
| selectedApikey := "" | ||
| // Look for admin with API key | ||
| for _, user := range filteredUsers { |
There was a problem hiding this comment.
Waait, why do we have two for loops for this
notifications.go
Outdated
| } | ||
|
|
||
| // If no admin key found, use requesting user's key | ||
| if len(selectedApikey) == 0 && requestingUser != nil && len(requestingUser.ApiKey) > 0 { |
notifications.go
Outdated
| return | ||
| } | ||
|
|
||
| if project.Environment == "cloud" { |
There was a problem hiding this comment.
What is the point of this section?
Notifications are region specific, and since they aren't saved in the "Org" index, this will just send them to the void lol
notifications.go
Outdated
| notification.ReferenceUrl, | ||
| orgId, | ||
| false, | ||
| &user, // Pass authenticated user for API key fallback |
There was a problem hiding this comment.
No pls. It does kind of make sense in this specific scenario, but doesn't help all the other 50 places we are calling the function
…-reddy/shuffle-shared into notification-auth-fix
2 participants
This PR fixes notification failures, like when an empty API key is sent to execute a notification workflow, causing it to fail. One of the potential issues is when an admin exists in the org but has no API key; autogenerating a key helps fix this.
The second fix is for a broken access control check. It was trying to see if a user was a member of the org, but it was mistakenly comparing a user's ID to itself, which (incorrectly) always passed the check.