Support multiple IDs in grants #3263
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
api
api/roles
core
core/daemon
core/gen
core/perms
core/proto
labels
jefferai
force-pushed
the
jefferai-multiple-ids-grants
branch
from
e58a57e
to
686b3e8
Compare
jefferai
force-pushed
the
jefferai-multiple-ids-grants
branch
from
686b3e8
to
5af108b
Compare
irenarindos
reviewed
Jun 2, 2023
irenarindos
approved these changes
Jun 2, 2023
jimlambrt
approved these changes
Jun 2, 2023
jefferai
force-pushed
the
jefferai-multiple-ids-grants
branch
from
66922a4
to
9a9e030
Compare
johanbrandhorst
approved these changes
Jun 2, 2023
jefferai
force-pushed
the
jefferai-multiple-ids-grants
branch
from
654e9e7
to
2acb563
Compare
This allows specifying multiple IDs in grants via an `ids` field, which will eventually become the normal way to specify IDs. Internally, each grant with multiple IDs turns into multiple ACL grants each with a single ID; to make this distinction clearer and allow for better separation of concerns an AclGrant type has been introduced, so really multiple IDs in a Grant turn into multiple AclGrants. The nice thing about doing it this way is that it becomes purely a parsing concern; no actual validation logic has changed. For backwards compatibility with grants already stored in roles, we do not error out if `id` is specified. However, eventually we will simply not support grants with `id` coming via the API; they will fail API validation, such that new grants will require the use of the updated `ids` field. A follow-on PR will plumb this through the service handlers/API/CLI, but I decided to do it in separate chunks to make reviewing easier, given that they are easily separable into the internal and external aspects of the change.
jefferai
force-pushed
the
jefferai-multiple-ids-grants
branch
from
2acb563
to
7624183
Compare
jefferai
changed the title
jefferai
added a commit
that referenced
this pull request
Jun 12, 2023
* Support multiple IDs in grants (internal) This allows specifying multiple IDs in grants via an `ids` field, which will eventually become the normal way to specify IDs. Internally, each grant with multiple IDs turns into multiple ACL grants each with a single ID; to make this distinction clearer and allow for better separation of concerns an AclGrant type has been introduced, so really multiple IDs in a Grant turn into multiple AclGrants. The nice thing about doing it this way is that it becomes purely a parsing concern; no actual validation logic has changed. For backwards compatibility with grants already stored in roles, we do not error out if `id` is specified. However, eventually we will simply not support grants with `id` coming via the API; they will fail API validation, such that new grants will require the use of the updated `ids` field. A follow-on PR will plumb this through the service handlers/API/CLI, but I decided to do it in separate chunks to make reviewing easier, given that they are easily separable into the internal and external aspects of the change. * Add CLI/API checks around using ID in grants (#3264) * Plumb through some contexts to remove a lot of deprecated errors (#3268)
jefferai
added a commit
that referenced
this pull request
Jun 12, 2023
This allows specifying multiple IDs in grants via an `ids` field, which will eventually become the normal way to specify IDs. Internally, each grant with multiple IDs turns into multiple ACL grants each with a single ID; to make this distinction clearer and allow for better separation of concerns an AclGrant type has been introduced, so really multiple IDs in a Grant turn into multiple AclGrants. The nice thing about doing it this way is that it becomes purely a parsing concern; no actual validation logic has changed. For backwards compatibility with grants already stored in roles, we do not error out if `id` is specified. However, eventually we will simply not support grants with `id` coming via the API; they will fail API validation, such that new grants will require the use of the updated `ids` field. A follow-on PR will plumb this through the service handlers/API/CLI, but I decided to do it in separate chunks to make reviewing easier, given that they are easily separable into the internal and external aspects of the change. * Add CLI/API checks around using ID in grants (#3264) * Plumb through some contexts to remove a lot of deprecated errors (#3268)
jefferai
added a commit
that referenced
this pull request
Jun 12, 2023
This allows specifying multiple IDs in grants via an `ids` field, which will eventually become the normal way to specify IDs. Internally, each grant with multiple IDs turns into multiple ACL grants each with a single ID; to make this distinction clearer and allow for better separation of concerns an AclGrant type has been introduced, so really multiple IDs in a Grant turn into multiple AclGrants. The nice thing about doing it this way is that it becomes purely a parsing concern; no actual validation logic has changed. For backwards compatibility with grants already stored in roles, we do not error out if `id` is specified. However, eventually we will simply not support grants with `id` coming via the API; they will fail API validation, such that new grants will require the use of the updated `ids` field. A follow-on PR will plumb this through the service handlers/API/CLI, but I decided to do it in separate chunks to make reviewing easier, given that they are easily separable into the internal and external aspects of the change. * Add CLI/API checks around using ID in grants (#3264) * Plumb through some contexts to remove a lot of deprecated errors (#3268)
jefferai
added a commit
that referenced
this pull request
Jun 14, 2023
This allows specifying multiple IDs in grants via an `ids` field, which will eventually become the normal way to specify IDs. Internally, each grant with multiple IDs turns into multiple ACL grants each with a single ID; to make this distinction clearer and allow for better separation of concerns an AclGrant type has been introduced, so really multiple IDs in a Grant turn into multiple AclGrants. The nice thing about doing it this way is that it becomes purely a parsing concern; no actual validation logic has changed. For backwards compatibility with grants already stored in roles, we do not error out if `id` is specified. However, eventually we will simply not support grants with `id` coming via the API; they will fail API validation, such that new grants will require the use of the updated `ids` field. A follow-on PR will plumb this through the service handlers/API/CLI, but I decided to do it in separate chunks to make reviewing easier, given that they are easily separable into the internal and external aspects of the change. * Add CLI/API checks around using ID in grants (#3264) * Plumb through some contexts to remove a lot of deprecated errors (#3268)
jefferai
added a commit
that referenced
this pull request
Jun 15, 2023
This allows specifying multiple IDs in grants via an `ids` field, which will eventually become the normal way to specify IDs. Internally, each grant with multiple IDs turns into multiple ACL grants each with a single ID; to make this distinction clearer and allow for better separation of concerns an AclGrant type has been introduced, so really multiple IDs in a Grant turn into multiple AclGrants. The nice thing about doing it this way is that it becomes purely a parsing concern; no actual validation logic has changed. For backwards compatibility with grants already stored in roles, we do not error out if `id` is specified. However, eventually we will simply not support grants with `id` coming via the API; they will fail API validation, such that new grants will require the use of the updated `ids` field. A follow-on PR will plumb this through the service handlers/API/CLI, but I decided to do it in separate chunks to make reviewing easier, given that they are easily separable into the internal and external aspects of the change. * Add CLI/API checks around using ID in grants (#3264) * Plumb through some contexts to remove a lot of deprecated errors (#3268)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows specifying multiple IDs in grants via an
idsfield, which will eventually become the normal way to specify IDs. Internally, each grant with multiple IDs turns into multiple ACL grants each with a single ID; to make this distinction clearer and allow for better separation of concerns an AclGrant type has been introduced, so really multiple IDs in a Grant turn into multiple AclGrants.The nice thing about doing it this way is that it becomes purely a parsing and validation concern; no actual authorization checking logic has changed.
For backwards compatibility with grants already stored in roles, we do not error out if
idis specified. However, eventually we will simply not support grants withidcoming via the API; they will fail API validation, such that new grants will require the use of the updatedidsfield.A follow-on PR will plumb this through the service handlers/API/CLI, but I decided to do it in separate chunks to make reviewing easier, given that they are easily separable into the internal and external aspects of the change.