Feature: Airtable Analyzer for Personal Access Tokens #3941
Conversation
nabeelalam
changed the title
nabeelalam
changed the title
There was a problem hiding this comment.
Great job @nabeelalam! Left a few small suggestion and questions, but otherwise looks great.
| return nil, err | ||
| } | ||
|
|
||
| scopeStatusMap[common.PermissionStrings[common.UserEmailRead]] = userInfo.Email != nil |
There was a problem hiding this comment.
question: Is the idea here that if the response from the FetchAirtableUserInfo API contains an email, they have the user.email:read scope?
There was a problem hiding this comment.
Yes. Here's the API reference for that: https://airtable.com/developers/web/api/get-user-id-scopes
| } | ||
| } | ||
|
|
||
| func determineScope(token string, scope common.Permission, ids map[string]string) (bool, error) { |
There was a problem hiding this comment.
nit: For consistency and better readability, can we rename scope to permission or perm? Since it's later used as an argument for getEndpointByPermission, this would make the code clearer.
There was a problem hiding this comment.
You're right. The terms got a bit shuffled since each AirTable scope maps to a Permission enum in the auto generated permissions.go file, but each AirTable scope also has its own granular "permissions".
I agree that the usage of the terms should be more consistent. I'll update this and any other instances I see with inconsistent use of the terms scope/permission.
| } | ||
|
|
||
| func determineScopes(token string, basesInfo *common.AirtableBases) error { | ||
| if basesInfo != nil && len(basesInfo.Bases) > 0 { |
There was a problem hiding this comment.
suggestion: Similar to my earlier "line of sight" comment, we can invert the logic of this if statement to return early, allowing us to de-indent the following for loop for better readability.
Ex:
if basesInfo == nil || len(basesInfo.Bases) == 0 {
return nil
}
for _ base := range baseInfo.Bases {
...
}
| return nil | ||
| } | ||
|
|
||
| var Endpoints = map[EndpointName]Endpoint{ |
There was a problem hiding this comment.
This is a nice pattern! One small, optional suggestion: consider constructing this in an init() function and making it private with a public getter.
It’s not a big deal, but this approach prevents unintended modifications by external clients since it wouldn’t be directly exported. I don’t think it's a major concern here, but just something to consider! Totally optional. 😊
Ex:
var endpoints map[EndpointName]Endpoint
func init() {
endpoints = map[EndpointName]Endpoint{
GetUserInfoEndpoint: {
URL: "https://api.airtable.com/v0/meta/whoami",
Method: "GET",
},
// ... existing code ...
}
}
// GetEndpoint returns the endpoint for the given name and whether it exists
func GetEndpoint(name EndpointName) (Endpoint, bool) {
endpoint, exists := endpoints[name]
return endpoint, exists
}
Thanks for the detailed review, @ahrav. Much appreciated. I'm going through your comments and suggestions. |
Feel free to merge whenever you are ready. None of my comments should be blocking. |
… issues; improved code readability
Description:
Airtable PAT Analyzer
Checklist:
make test-community)?make lintthis requires golangci-lint)?