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.

Sign up for GitHub

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

Jump to bottom

Add auth data to RecordMetadata #13993

Merged
merged 13 commits into from
Sep 7, 2023
Merged

Add auth data to RecordMetadata #13993

merged 13 commits into from
Sep 7, 2023

Conversation

koevskinikola
Copy link
Member

Description

Includes (encoded) auth data in the RecordMetadata and ExecuteCommandRequest classes.

ℹ️

  • This PR is based on Add auth data interchange API #13992. Any changes made there should be reflected here before this PR is merged with the main branch.
  • Commit 9da69d2 is not fully related to this PR. It adds a static field for the default tenant identifier that will be useful in further multi-tenancy development.

Related issues

closes #13989

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Other teams:
If the change impacts another team an issue has been created for this team, explaining what they need to do to support this change.

Please refer to our review guidelines.

@koevskinikola koevskinikola self-assigned this Aug 23, 2023
@koevskinikola koevskinikola force-pushed the 13988-auth-data-interchange branch 2 times, most recently from 531d13f to 58a8977 Compare August 24, 2023 06:52
@koevskinikola koevskinikola force-pushed the 13989-auth-data-in-metadata branch 2 times, most recently from fcab2eb to 418e72d Compare August 24, 2023 11:29
@koevskinikola koevskinikola force-pushed the 13989-auth-data-in-metadata branch 2 times, most recently from 51e5367 to 6ee79ba Compare August 28, 2023 11:04
@koevskinikola koevskinikola marked this pull request as ready for review August 29, 2023 09:27
@koevskinikola koevskinikola mentioned this pull request Aug 29, 2023
Merged
14 tasks
Base automatically changed from 13988-auth-data-interchange to main August 29, 2023 09:42
remcowesterhoud
remcowesterhoud approved these changes Aug 29, 2023
View reviewed changes
Copy link
Contributor

@remcowesterhoud remcowesterhoud left a comment

Choose a reason for hiding this comment

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

Thanks @koevskinikola 🏆

I think there is 1 comment which isn't true. Please change this, I'll still approve the PR as it's only a comment 😄

protocol/src/main/resources/protocol.xml Show resolved Hide resolved
korthout
korthout requested changes Aug 31, 2023
View reviewed changes
Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

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

Awesome stuff @koevskinikola 👏

👍 I really like how you've set up the authorization metadata to be swappable using a format property

❌ Two things require attention:

  • I think we cannot bump the protocol version safely. This leads to rolling update problems, e.g. on followers replaying newer events. Do you see any other solutions for this? If not, I propose we discuss this with ZDP.
  • Instead of depending on auth0's jwt library, the procotol-impl should depend on our zeebe-auth module.

🔧 The rest of my comments are suggestions

protocol/src/main/java/io/camunda/zeebe/protocol/record/value/TenantOwned.java Show resolved Hide resolved
protocol/src/main/java/io/camunda/zeebe/protocol/record/Record.java Outdated Show resolved Hide resolved
protocol-impl/pom.xml Outdated Show resolved Hide resolved
stream-platform/src/test/java/io/camunda/zeebe/stream/impl/RecordBatchTest.java Outdated Show resolved Hide resolved
protocol/pom.xml Show resolved Hide resolved
@koevskinikola
feat(protocol): add default tenant id
516192d 
Include the default tenant id in the TenantOwned interface.
@koevskinikola
feat(protocol): add an authorization field to record metadata
d08d581 
Add an authorization field to the RecordMetadata SBE message of the Zeebe protocol.
Initially,the authorization field will be populated only for Command records, and
this data won't be exported. The field will contain information on the authorizations
of the user that made the client request that created the Command record. The field type
is of variable length, as authorizations may vary per user.
@koevskinikola
feat(protocol-impl): add metadata authorization implementation
ed7f755 
Add the AuthInfo class, which is an implementation for the authorization field of
Record Metadata. The class contains format and authData properties. The authData
property is encoded into a String value, and the format property provides information
on what mechanism encoded/decodes the authData value. Initially, only the JWT format
is available, but new ones can easily be added (ex. msg-pack).
@koevskinikola
test(protocol-impl): add test coverage for authorization
88227d0 
Test the new authorization field of the RecordMetadata class
@koevskinikola
feat(protocol): add auth data to execute command request
f358651 
Extends the ExecuteCommandRequest to contain authorization data.
The ExecuteCommandRequest class is used to forward requests from
the Zeebe Gateway to the Broker. Since auth data originates in
the Gateway, it needs to be provided to the Broker request there.
@koevskinikola
feat(exporters): add authorizations property
171fbd1 
Add the new authorizations property to the Elasticsearch and
OpenSearch record index templates.
@koevskinikola
refactor(stream-platform): adjust test to account for new record size
147d3ba 
The RecordMetadata now contains a new authorizations property that increases the
general size of a record. This causes certain test cases that depend on the
record size to fail. This commit adjusts the expected record sizes to account for
the increase of the metadata.
@koevskinikola
refactor(auth): adjust helper method for easier use
9ea6c17 
Adjust a helper method for the TenantAuthorizationChecker to make it
easier for use with the AuthInfo and Record classes by accepting
a Map instead of a JWT decoder instance.
@koevskinikola
feat(protocol): bump protocol version
eee0d0d 
Bump protocol version since the addition of the variable length
authorization fields is incompatible with records from older
versions, i.e. they can't be SBE decoded correctly.
@koevskinikola
refactor(auth): simplify jwt builder implementation
6a730a7 
* Move the default values for the issuer, audience, and subject claims
  of the encoder and decoder to the builder for easier maintenance.
* Set the value of the subject claim to zeebe-client instead to
  make it easiert to understand from where the auth data comes from.
* Remove a comment about keeping the java-jwt library version aligned
  with the identity-sdk. The Zeebe auth module was designed to be
  flexible on what encoding/decoding mechanism is used and it shouldn't
  be tied to the libraries that Identity uses.
* Adjust JSON serialization test to account for the changed JWT subject.
@koevskinikola
refactor(auth): move authorized tenants claim key
354aeb5 
Move the AUTHORIZED_TENANTS_CLAIM constant to the Authorization
class to make it less coupled with the JWT implementation.
@koevskinikola
refactor: hide jwt implementation of auth api
d3598ff 
Avoid using java-jwt APIs outside of the Zeebe auth module
to avoid tightly coupling the JWT library with the rest
of Zeebe. Instead, return a Map<String, Object> instance
to provide the various authorizations.
@koevskinikola
refactor(protocol): expand javadoc of new record method
13ff4a7 
Expand the Record#getAuthorizations() javadoc to explain what
entries the authorizations Map may provide.
@koevskinikola
Copy link
Member Author

@korthout all the review hints have been implemented. The protocol version bug will be handled in #14204. Have a look when you have the time.

korthout
korthout approved these changes Sep 7, 2023
View reviewed changes
Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

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

Thanks @koevskinikola 🚀

LGTM 👍

@koevskinikola
Copy link
Member Author

bors merge

@zeebe-bors-camunda
Copy link
Contributor

Build succeeded:

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit f572b6a into main Sep 7, 2023
31 of 32 checks passed
@zeebe-bors-camunda zeebe-bors-camunda bot deleted the 13989-auth-data-in-metadata branch September 7, 2023 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remcowesterhoud remcowesterhoud remcowesterhoud approved these changes

@korthout korthout korthout approved these changes

Assignees

@koevskinikola koevskinikola

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add auth data to Zeebe records
3 participants
@koevskinikola @korthout @remcowesterhoud