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.

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

Adds authorization support for preconfigured AAS Environment #325

Merged
merged 16 commits into from
Jul 23, 2024

Conversation

FriedJannik
Copy link
Contributor

No description provided.

FriedJannik and others added 7 commits June 20, 2024 09:02
Signed-off-by: FriedJannik <Jannik.Fried@iese.fraunhofer.de>
Co-authored-by: Aaron Zielstorff <aaron.zielstorff@iese.fraunhofer.de>
Co-authored-by: Mohammad Ghazanfar Ali Danish <ghazanfar.danish@iese.fraunhofer.de>
- Embeds Preconfiguration to current Test Classes

Signed-off-by: FriedJannik <Jannik.Fried@iese.fraunhofer.de>
Co-authored-by: Aaron Zielstorff <aaron.zielstorff@iese.fraunhofer.de>
private Logger logger = LoggerFactory.getLogger(AasEnvironmentPreconfigurationLoader.class);

@Value("${basyx.environment:#{null}}")
private List<String> pathsToLoad;

private ResourceLoader resourceLoader;


@Value("${basyx.aasenvironment.authorization.preconfiguration.token-endpoint:#{null}}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest documenting this new pars in the README

@@ -80,10 +125,15 @@ public void loadPreconfiguredEnvironments(AasEnvironment aasEnvironment)
int filesCount = files.size();
int currenFileIndex = 0;

if(authorizationEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest moving these changes to a new class: AuthorizedAasEnvironmentPreconfig... in aasenvironment-feature-authorization

Choose a reason for hiding this comment

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

We need this part here, because we need to authorize the Action performed by the PreConfiguration Loader, so imo it is not possible to extract this

@@ -106,7 +106,6 @@
<dependency>
<groupId>org.apache.httpcomponents.client5</groupId>
<artifactId>httpclient5</artifactId>
Copy link
Contributor

@mateusmolina-iese mateusmolina-iese Jul 4, 2024

Choose a reason for hiding this comment

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

Is this change necessary? (line 109)

this.password = password;
}

public AccessTokenProvider create(){
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding unit tests to this factory

@@ -31,9 +31,11 @@
import org.eclipse.digitaltwin.basyx.aasenvironment.AasEnvironmentFactory;
import org.eclipse.digitaltwin.basyx.aasenvironment.feature.AasEnvironmentFeature;
import org.eclipse.digitaltwin.basyx.aasenvironment.feature.DecoratedAasEnvironmentFactory;
import org.eclipse.digitaltwin.basyx.aasenvironment.preconfiguration.AasEnvironmentPreconfigurationLoader;
Copy link
Contributor

Choose a reason for hiding this comment

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

Imports not needed

@@ -71,6 +71,10 @@
<groupId>org.eclipse.digitaltwin.basyx</groupId>
<artifactId>basyx.http</artifactId>
</dependency>
<dependency>
<groupId>org.eclipse.digitaltwin.basyx</groupId>
<artifactId>basyx.client</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this dep. required?

@aaronzi aaronzi merged commit d9de413 into eclipse-basyx:main Jul 23, 2024
2 checks passed
@FriedJannik FriedJannik deleted the detached branch July 24, 2024 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants