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

Issue #5451 - Private Work Dir #5457

Closed
wants to merge 4 commits into from

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Oct 16, 2020

No description provided.

@joakime joakime requested review from gregw and sbordet October 16, 2020 15:18
@joakime joakime self-assigned this Oct 16, 2020
@joakime joakime requested a review from janbartel October 16, 2020 16:48
@joakime
Copy link
Contributor Author

joakime commented Oct 16, 2020

Do we need the start module for the configuration of the posix perms?

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

@janbartel and I have reviewed. We are OK with the general behaviour, but we don't like all the extra getters and setters and would prefer to use attributes.

API is forever!

Comment on lines +519 to +524
if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class))
{
String workDirPerms = context.getTempDirectoryPosixPermissions();
Set<PosixFilePermission> permSet = PosixFilePermissions.fromString(workDirPerms);
tmpDir = Files.createTempDirectory(parentPath, temp, PosixFilePermissions.asFileAttribute(permSet)).toFile();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need to create a lot of new API for this. Can we just do it with attributes: context, server then system

Suggested change
if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class))
{
String workDirPerms = context.getTempDirectoryPosixPermissions();
Set<PosixFilePermission> permSet = PosixFilePermissions.fromString(workDirPerms);
tmpDir = Files.createTempDirectory(parentPath, temp, PosixFilePermissions.asFileAttribute(permSet)).toFile();
}
if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class))
{
String workDirPerms = (String) context.getAttribute(TEMP_DIRECTORY_POSIX_PERMISSIONS);
if (workDirPerms == null)
workDirPerms = (String) context.getServer().getAttribute(TEMP_DIRECTORY_POSIX_PERMISSIONS);
if (workDirPerms == null)
workDirPerms = (String) System.getProperty(TEMP_DIRECTORY_POSIX_PERMISSIONS, "rwx------");
Set<PosixFilePermission> permSet = PosixFilePermissions.fromString(workDirPerms);
tmpDir = Files.createTempDirectory(parentPath, temp, PosixFilePermissions.asFileAttribute(permSet)).toFile();
}

We've changed tmp dir stuff a number of times... once a thing is said in API it is forever! So I think we just lean towards to loose attributes rather than API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the String you see for TEMP_DIRECTORY_POSIX_PERMISSIONS ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The biggest issue with attributes is making them friendly for start modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, where does TEMP_DIRECTORY_POSIX_PERMISSIONS exist?
Not the WebAppContext, how about IO ? or the Server?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is closed, but will answer so record my thoughts.

I see these attributes as hints to the implementation, so the string should be "org.eclipse.jetty.webapp.WebInfConfiguration.TEMP_DIRECTORY_POSIX_PERMISSIONS" and defined as a constant in WebInfConfiguration.

It is easy to make friendly for start modules, because we write the XML for those modules, thus we write the XML like:

  <Call name="setAttribute">
    <Arg>org.eclipse.jetty.webapp.WebInfConfiguration.TEMP_DIRECTORY_POSIX_PERMISSIONS</arg>
    <Arg><Property name="jetty.webapp.tmpDirPosixPermissions"/></Arg>
  </Call>

and then we just add to the ini-template of webapp.mod:

## Set the permissions of temporary directory on POSIX file systems
# jetty.webapp.tmpDirPosixPermissions=rwx------

Ultimately, this is no different at the start module level than if we had dedicated APIs

Comment on lines +463 to +466
if (context.getServer().isWorkDirectoryPersistent())
{
context.setPersistTempDirectory(true);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should also do this with an attribute so we don't commit to a forever API:

Suggested change
if (context.getServer().isWorkDirectoryPersistent())
{
context.setPersistTempDirectory(true);
}
Object workNotPersistent = context.getServer().getAttribute(WORK_DIRECTORY_NOT_PERSISTENT);
if (workNotPersistent != null && Boolean.valueOf(workNotPersistent))
context.setPersistTempDirectory(true);

@joakime
Copy link
Contributor Author

joakime commented Oct 16, 2020

Closing in favor of replacement PR #5458

@joakime joakime closed this Oct 16, 2020
@joakime joakime deleted the jetty-9.4.x-5451-user-private-work-dir branch October 16, 2020 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants