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

Added Cognito authenticator for DTR #400

Closed
wants to merge 0 commits into from
Closed

Added Cognito authenticator for DTR #400

wants to merge 0 commits into from

Conversation

peissing
Copy link
Contributor

Feature:
DTR is at the moment only able to use Keycloak as for authentication
This feature enable DTR to use also AWS Cognito
The whole functionality and how to configure is described in the file COGNITO.md that is contained in this pull request.

The request contains several new and modified classes including unit tests.
The old class AuthorizationEvaluator is now abstract and the base class for different Evaluators. At the moment there are implementations for Keycloak and for Cognito.
I am not an expert for Helm charts, but I tried to integrate it also in those files. But this must be checked if I did it in a correct way.

@peissing
Copy link
Contributor Author

peissing commented Apr 22, 2024 •
edited
Loading

In the past I opened already a pull request for that feature, but in the meantime during review there were so many changes that I rebased and started from the beginning. So this is the improved version of the pull request #290

@peissing
Copy link
Contributor Author

peissing commented May 7, 2024

Hello @istvan-nagy-epam
did you already dind some time to analyze my new pull request?
Hope I'm not yet too much behind the official main branch.
Thanx a lot for your support and I hope I did not forget anything we discussed last time.

@gerbigf
Copy link

gerbigf commented May 16, 2024

@istvan-nagy-epam push - this feature is highly relevant for us.

@peissing
Copy link
Contributor Author

Hello together,
seems that nobody is interested in this pull request, because there is nobody
that does a review. It is existing since two months.
But a comment or hint would be nice if I should close it or not. ;-)
Thanx a lot.

@stephanbcbauer
Copy link
Member

@tunacicek can you please have a look?

@tunacicek
Copy link
Contributor

Hi, @peissing, @gerbigf ,@stephanbcbauer
sorry for the late response. I was on parental leave until mid-May and lose the focus on this topic.
I will take a look over this PR.

@tunacicek tunacicek requested a review from agg3fe June 20, 2024 10:13
@peissing
Copy link
Contributor Author

Hello @tunacicek
thank you very much for your feedback.

tunacicek
tunacicek requested changes Jun 20, 2024
View reviewed changes
COGNITO.md Outdated Show resolved Hide resolved
COGNITO.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
...d/src/main/java/org/eclipse/tractusx/semantics/registry/security/AuthorizationEvaluator.java Outdated Show resolved Hide resolved
...ain/java/org/eclipse/tractusx/semantics/registry/security/CognitoAuthorizationEvaluator.java Outdated Show resolved Hide resolved
...in/java/org/eclipse/tractusx/semantics/registry/security/KeycloakAuthorizationEvaluator.java Outdated Show resolved Hide resolved
...in/java/org/eclipse/tractusx/semantics/registry/security/KeycloakAuthorizationEvaluator.java Outdated Show resolved Hide resolved
backend/src/main/java/org/eclipse/tractusx/semantics/registry/security/OAuthSecurityConfig.java Outdated Show resolved Hide resolved
backend/src/test/java/org/eclipse/tractusx/semantics/registry/CognitoApiSecurityTest.java Outdated Show resolved Hide resolved
backend/src/test/resources/application-cognito-test.yml Outdated Show resolved Hide resolved
@peissing
Copy link
Contributor Author

Hello @tunacicek,
I made all the changes you requested. Hope it is OK now.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 21, 2024
View reviewed changes
COGNITO.md Outdated
Comment on lines 70 to 71
If `cognito` is set as an identity provider the following attributes must be set:

Check warning

Code scanning / Gitleaks

(?i)(?:key|api|token|secret|client|passwd|password|auth|access)(?:[0-9a-z\-_\t .]{0,20})(?:[\s|']|[\s|"]){0,3}(?:=|>|:{1,3}=|\|\|:|<=|=>|:|\?=)(?:'|\"|\s|=|\x60){0,5}([0-9a-z\-_.=]{10,150})(?:['|\"|\n|\r|\s|\x60|;]|$)

generic-api-key has detected secret for file COGNITO.md at commit 4da784ebb5b24ab248b1d13de99bb0137204a6cb.
COGNITO.md Outdated
Comment on lines 72 to 73
- `public-client-id`: This is the client id of the app client in Cognito that has the `view_digital_twin` scope attached
- `internal-client-id`: This is the client is of the app client in Cognito that has the edit scopes attached

Check warning

Code scanning / Gitleaks

(?i)(?:key|api|token|secret|client|passwd|password|auth|access)(?:[0-9a-z\-_\t .]{0,20})(?:[\s|']|[\s|"]){0,3}(?:=|>|:{1,3}=|\|\|:|<=|=>|:|\?=)(?:'|\"|\s|=|\x60){0,5}([0-9a-z\-_.=]{10,150})(?:['|\"|\n|\r|\s|\x60|;]|$)

generic-api-key has detected secret for file COGNITO.md at commit 4da784ebb5b24ab248b1d13de99bb0137204a6cb.
tunacicek
tunacicek reviewed Jun 21, 2024
View reviewed changes
docs/README.md Outdated Show resolved Hide resolved
COGNITO.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
tunacicek
tunacicek requested changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@tunacicek tunacicek left a comment

Choose a reason for hiding this comment

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

Thanks for your feedback. I have only small changes requested.

@tunacicek
Copy link
Contributor

tunacicek commented Jun 21, 2024 •
edited
Loading

Hi @peissing ,
thanks for your adjustments. I have small changes requested and the build is failed .You need to increase the helm chart version.
Could you also add in the changelog.md your changes as feature in the section release 0.5.0-RC1

@peissing
Copy link
Contributor Author

Hello @tunacicek
I made all the changes. I did not add the publicClientId to the INSTALL.md file, but I added the idpInternalClientId.

@peissing
Copy link
Contributor Author

Hello @tunacicek ,
sorry for that stupid 123 comment at the newest commit.
I did not check it ;-)

@tunacicek
Copy link
Contributor

Hi @peissing,
could you please resolve the conflicts and if possible you can also squash the commits :)

@peissing
Copy link
Contributor Author

Hello @tunacicek, sorry but I do not understand why there is a conflict. I just added the requested information about the new feature in the CHANGELOG.md.

@tunacicek
Copy link
Contributor

Hello @tunacicek, sorry but I do not understand why there is a conflict. I just added the requested information about the new feature in the CHANGELOG.md.

It occurs, because you have not the latest commit from the main branch. There are commits missing.

@peissing
Copy link
Contributor Author

Hello @tunacicek ,
OK I already thought about this. But then there is some effort for me, because I already saw that I'm 18 commits behind...

@peissing peissing closed this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tunacicek tunacicek tunacicek requested changes

@thomas-henn thomas-henn Awaiting requested review from thomas-henn

@agg3fe agg3fe Awaiting requested review from agg3fe

Assignees

@peissing peissing

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@peissing @gerbigf @stephanbcbauer @tunacicek