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

UI: settings show/hide update display #1072

Merged
merged 11 commits into from Apr 21, 2024
Merged

Conversation

Ludy87
Copy link
Contributor

@Ludy87 Ludy87 commented Apr 15, 2024

Description

This PR replaces the PR #1003

In this PR, the visual for available update is added to the foreground.

There are new settings to generally show/hide the update display, and only administrators receive the update display.

Checklist:

  • I have read the Contribution Guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

Contributor License Agreement

By submitting this pull request, I acknowledge and agree that my contributions will be included in Stirling-PDF and that they can be relicensed in the future under the MPL 2.0 (Mozilla Public License Version 2.0) license.

(This does not change the general open-source nature of Stirling-PDF, simply moving from one license to another license)

This PR replaces the PR Stirling-Tools#1003

In this PR, the visual for available update is added to the foreground.

There are new settings to generally show/hide the update display, and only administrators receive the update display.
@Ludy87 Ludy87 requested a review from Frooodle as a code owner April 15, 2024 18:58
import stirling.software.SPDF.repository.UserRepository;

@Service
public class AppUpdateShowService {
Copy link
Member

Choose a reason for hiding this comment

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

Having this in the security folder means non security enabled jars will have build issues

Copy link
Member

Choose a reason for hiding this comment

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

Also adding it as parameter to all view files isnt a good solution personally i prefer doing it via bean such as the global properties in
https://github.com/Stirling-Tools/Stirling-PDF/blob/main/src/main/java/stirling/software/SPDF/config/AppConfig.java

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in AppConfig.java I cannot query the user's status, as in the case of whether he is an admin or not

Copy link
Member

Choose a reason for hiding this comment

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

I dont mean placing in AppConfig but that @bean(name = "appName") is a nicer way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will try to use it 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

 ./gradlew  compileJava

> Configure project :
0.22.1

BUILD SUCCESSFUL in 6s
4 actionable tasks: 2 executed, 2 up-to-date

Copy link
Member

@sbplat sbplat left a comment

Choose a reason for hiding this comment

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

Everything else looks good!

this.applicationProperties = applicationProperties;
}

public boolean isShow() {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should call this shouldShow.

}

public boolean isShow() {
boolean showUpdate = applicationProperties.getSystem().getShowUpdate();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should have a check for this and return it immediately since it's used in every branch?

@Ludy87
Copy link
Contributor Author

Ludy87 commented Apr 19, 2024

Everything else looks good!

There will be problems, and unfortunately I can't find a solution if the condition is correct

if (System.getenv('DOCKER_ENABLE_SECURITY') == 'false') {

@Frooodle
Copy link
Member

I can help with that, I'll take a look tonight

@Frooodle
Copy link
Member

I cant commit to this PR but basically something like

@Service
class AppUpdateService {

    @Autowired private ApplicationProperties applicationProperties;

    @Autowired(required = false)
    ShowAdminInterface showAdmin;

    @Bean
    public boolean isShow() {
        boolean showUpdate = applicationProperties.getSystem().getShowUpdate();
        boolean showAdminResult = (showAdmin != null) ? showAdmin.getShowUpdateOnlyAdmin() : true;
        return showUpdate && showAdminResult;
    }
}

public interface ShowAdminInterface {
    default boolean getShowUpdateOnlyAdmin() {
        return true;
    }
}

This class is in the security package, and is the only thing containing security stuff

@Service
class AppUpdateAuthService implements ShowAdminInterface {

    @Autowired private UserRepository userRepository;
    @Autowired private ApplicationProperties applicationProperties;

    public boolean getShowUpdateOnlyAdmin() {
        boolean showUpdateOnlyAdmin = applicationProperties.getSystem().getShowUpdateOnlyAdmin();
        Authentication authentication = SecurityContextHolder.getContext().getAuthentication();

        if (authentication == null || !authentication.isAuthenticated()) {
            return !showUpdateOnlyAdmin;
        }

        if (authentication.getName().equalsIgnoreCase("anonymousUser")) {
            return !showUpdateOnlyAdmin;
        }

        Optional<User> user = userRepository.findByUsername(authentication.getName());
        if (user.isPresent() && showUpdateOnlyAdmin) {
            return "ROLE_ADMIN".equals(user.get().getRolesAsString());
        }

        return false;
    }
}

@Frooodle
Copy link
Member

Naming and things might be garbage, change whatever, but thats the basic idea

@Ludy87
Copy link
Contributor Author

Ludy87 commented Apr 20, 2024

Thanks for the code, there is the problem that AppUpdateAuthService is already called when the user is not logged in yet or it is not called again.

@Frooodle
Copy link
Member

Frooodle commented Apr 20, 2024

Could do something like

@Bean
    @Scope("request")
    public boolean isShow() {
        boolean showUpdate = applicationProperties.getSystem().getShowUpdate();
        boolean showAdminResult = (showAdmin != null) ? showAdmin.getShowUpdateOnlyAdmin() : true;
        return showUpdate && showAdminResult;
    }

with
org.springframework.context.annotation.Scope

@Frooodle
Copy link
Member

AppUpdateAuthService.java needs moving to security folder

@Ludy87
Copy link
Contributor Author

Ludy87 commented Apr 20, 2024

Then the ShowAdminInterface.java shouldn't be in the repository folder, right?

Copy link
Member

@Frooodle Frooodle left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

sonarcloud bot commented Apr 21, 2024

Quality Gate Passed Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@Frooodle
Copy link
Member

Good to merge?

@Ludy87
Copy link
Contributor Author

Ludy87 commented Apr 21, 2024

The birth was difficult. You can merge.

@Frooodle Frooodle merged commit a5000fb into Stirling-Tools:main Apr 21, 2024
4 checks passed
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

3 participants