Specify rate_limit.blocked event

[tung2744]

ref DEV-3253

@fungc-io @carmenlau Please see the design of the new rate limit blocked event, and let me know if you think any other additional information is needed in the payload.

[tung2744]

Moved url into audit_context in 885e7b3

For other fields of audit_context, and the feature of adding additional fields to it in admin api, maybe I will keep them as unspecified at the moment because maybe we need to design it carefully before exposing as a public api. The implementation will alway guarantee audit_context is an object with a field http_url.

[carmenlau]

• Update event payload to remove bucket
• Check where have we used rate limit names
• [In authgear core] Add comments to the existing API to clarify that we are using the bucket field and that is for internal use only

[tung2744]

Updated:

1. Rewritten the rate limit spec, now rate limit names include per_ip or per_user suffix.
2. Updated the rate_limit.blocked event payload to use the new rate limit names, and do not expose the bucket name.

[tung2744]

Added one more commit to update the hook spec, which reference the rate limit names.

[tung2744]

A second thought:

Maybe we can change this to an object:

{
  "rate_limit": {
    "name": "authentication.password.per_ip",
    "group": "authentication.password"
  }
}

[carmenlau]

Sorry for late reply. 🙇🏻‍♀️ This payload looks good to me.

At the same time, I think we should also find out where we expose rate_limit in the public API. I believe it should be here: error.go. We currently expose bucket there, and we should consider proposing some adjustments. One idea is to add rate_limit.name and rate_limit.group to the payload, and mark rate_limit_name and bucket_name as legacy while keeping them to avoid a breaking change. However, this would make the payload look a bit confusing.🫤

{
  "name": "TooManyRequest",
  "reason": "RateLimited",
  "message": "request rate limited",
  "code": 429,
  "info": {
    "FlowType": "login",
    "bucket_name": "AccountEnumerationPerIP",
    "rate_limit_name": "authentication.account_enumeration",
    "rate_limit": {
      "name": "authentication.account_enumeration",
      "group": "authentication.account_enumeration.per_ip"
    }
  }
}

Also check if anywhere else have similar problems.

[tung2744]

I have checked we only use bucket_name at the moment. rate_limit_name should be safe to remove.

The place that actually uses the rate limit group name is hook. You can see this commit: c1eaa57

[carmenlau]

I have checked we only use bucket_name at the moment. rate_limit_name should be safe to remove.

But is there any chance we return this error in our APIs that users might reference? Is it the Custom UI API?

The place that actually uses the rate limit group name is hook.

We’re using the group name as the key there, not something like rate_limit_name, so it looks fine, right?

[tung2744]

But is there any chance we return this error in our APIs that users might reference? Is it the Custom UI API?

I would not say there is no chance, but this thing is introduced recently, and I've also checked custom UI codes which I have access to and do not see any usage of the key. I think it should be safe to remove. And actually the original key is rate_limit_name but it is a group, which is more confusing if we do not remove it.

We’re using the group name as the key there, not something like rate_limit_name, so it looks fine, right?

It is named rate_limits. An example:

{
  "is_allowed": true,
  "rate_limits": {
    "authentication.account_enumeration": {
      "weight": 2
    }
  }
}

It is designed in this way because we do not want the user to list all related rate limits when he need to modify the weight.

[carmenlau]

The rest looks fine

[carmenlau]

's/rate limit group names/rate limit groups/g'

[tung2744]

Updated, thanks!