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

CBG-2894: Reject user auth when channel threshold is over 500 #6214

Merged
merged 14 commits into from
May 9, 2023
Merged

Conversation

gregns1
Copy link
Contributor

@gregns1 gregns1 commented Apr 26, 2023

CBG-2894

When channels set on a user have changed we need to be able to see how many channels this user has to warn if over warning threshold and to reject any future authorization requests for the user if they have exceeded the channel limit on the user. This code will only be called if a change in channel set has been detected removing the need for this check to be performed each time a user authenticates.

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

bbrks
bbrks previously requested changes Apr 27, 2023
Copy link
Member

@bbrks bbrks left a comment

Choose a reason for hiding this comment

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

some initial comments

auth/auth.go Outdated Show resolved Hide resolved
auth/auth.go Outdated Show resolved Hide resolved
auth/auth.go Outdated Show resolved Hide resolved
@gregns1 gregns1 marked this pull request as ready for review April 28, 2023 14:08
@gregns1 gregns1 assigned bbrks and torcolvin and unassigned bbrks Apr 28, 2023
auth/auth.go Outdated Show resolved Hide resolved
auth/auth.go Outdated
// only warn/limit if the threshold is set and if we are in this function as a "user" not a role
if auth.ServerlessChannelThreshold != 0 && princUser != nil {
// Warning at 50 channels
princUser.GetWarnChanSync().Do(func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to not work as intended, since this only works the version first time this code is run, I'd write a test where it is over threshold the first time and then under threshold the second time, and it should only warn once.

What happens if it high (warning), then low, then high again (error)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this apporach as elsewhere we do this warning using the sync once. I don't think you can reset the sync.Once so I either keep as is to follow what is done elsewhere in the code or I remove it (but could end up with a lot of warnings in logs as result)

auth/auth.go Outdated Show resolved Hide resolved
base/error.go Outdated Show resolved Hide resolved
auth/auth_test.go Outdated Show resolved Hide resolved
auth/auth_test.go Outdated Show resolved Hide resolved
auth/auth_test.go Outdated Show resolved Hide resolved
auth/auth_test.go Outdated Show resolved Hide resolved
auth/auth.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@torcolvin torcolvin left a comment

Choose a reason for hiding this comment

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

Just some nitpicking comments.

auth/auth.go Show resolved Hide resolved
auth/auth.go Show resolved Hide resolved
auth/auth.go Outdated Show resolved Hide resolved
auth/auth.go Show resolved Hide resolved
auth/auth.go Outdated Show resolved Hide resolved
@torcolvin torcolvin dismissed bbrks’s stale review May 5, 2023 14:23

addressed separately

@gregns1 gregns1 merged commit f9dd5e9 into master May 9, 2023
@gregns1 gregns1 deleted the CBG-2894 branch May 9, 2023 14:03
bbrks pushed a commit that referenced this pull request Mar 28, 2024
* CBG-2894: Reject user auth when channel threshold is over 500 in serverless mode

* fix panic where authetciator was needed and it wasn't availible

* linter issue

* linter issue again

* remove extra methods off interface

* pass user into function

* rebase

* ensure 500 code is retruned for http error added

* updates based off comments

* fix panic

* updates based off comments

* updates based off dicussion yesterday

* lint error

* updates based of comments
bbrks pushed a commit that referenced this pull request Apr 3, 2024
* CBG-2894: Reject user auth when channel threshold is over 500 in serverless mode

* fix panic where authetciator was needed and it wasn't availible

* linter issue

* linter issue again

* remove extra methods off interface

* pass user into function

* rebase

* ensure 500 code is retruned for http error added

* updates based off comments

* fix panic

* updates based off comments

* updates based off dicussion yesterday

* lint error

* updates based of comments
mohammed-madi pushed a commit that referenced this pull request Apr 8, 2024
* CBG-2894: Reject user auth when channel threshold is over 500 in serverless mode

* fix panic where authetciator was needed and it wasn't availible

* linter issue

* linter issue again

* remove extra methods off interface

* pass user into function

* rebase

* ensure 500 code is retruned for http error added

* updates based off comments

* fix panic

* updates based off comments

* updates based off dicussion yesterday

* lint error

* updates based of comments
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.

3 participants