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

Publisher Class #4727

Merged
merged 12 commits into from
Jun 13, 2021
Merged

Publisher Class #4727

merged 12 commits into from
Jun 13, 2021

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented May 24, 2021

Description
This PR introduces a new component: the Publisher Class. This combines a handful of different solutions I've been using in my modules to solve a variety of problems:

  1. How do I maintain project assets with version dependencies?
  2. How do I manage uploads and other "dynamic" files that need to be web accessible?
  3. How can I update my project when the framework or modules change?
  4. How can the framework/modules publish their modified content to a project?

Publisher has two functional modes: it can be instantiated and used dynamically with runtime input, or can use predefined workflows staged for automatic discovery. With either approach developers use fluent-style methods to build a payload manifest and then one of the "write" commands to deliver the files. For example, to inject the latest Bootstrap minimized JS & CSS files:

$publisher = new Publisher();
$publisher
	->addPath('vendor/twbs/bootstrap/dist')
	->retainPattern('*.min.*)
	->merge();

By default Publisher assumes the source is ROOTPATH and the destination is FCPATH but this can be changed with the constructor:

$publisher = new Publisher('vendor', FCPATH . 'assets/vendor/');

... or set directly on extensions:

class BootstrapPublisher extends Publisher
{
	protected $source = ROOTPATH . 'vendor/twbs/bootstrap';
	protected $destination = FCPATH . 'bootstrap/';
}

I will have lots more explanations and examples in the User Guide but I wanted to get this PR up for reviews since it has been a while since we added an entirely new component.

Note: This will need to be re-styled after the coding standards release.

A few motivations for this library:

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

system/Language/en/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Exceptions/PublisherException.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
@MGatner
Copy link
Member Author

MGatner commented May 24, 2021

Thanks @mostafakhudair, great clean up!

system/Publisher/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Outdated Show resolved Hide resolved
tests/_support/Publishers/TestPublisher.php Outdated Show resolved Hide resolved
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

Can we get some initial docs for this? I can't visualize this in action. 😅

@MGatner
Copy link
Member Author

MGatner commented May 25, 2021

Oh for sure! Lots more coming, just other priorities at the moment. I'll mark this Draft so it isn't confusing.

@MGatner MGatner marked this pull request as draft May 25, 2021 16:47
@MGatner MGatner marked this pull request as ready for review May 29, 2021 20:14
@MGatner MGatner requested a review from paulbalandan May 29, 2021 20:14
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

I think I grasp the concept and process flow. Just some comments.

user_guide_src/source/libraries/publisher.rst Show resolved Hide resolved
user_guide_src/source/libraries/publisher.rst Show resolved Hide resolved
user_guide_src/source/libraries/publisher.rst Outdated Show resolved Hide resolved
user_guide_src/source/libraries/publisher.rst Outdated Show resolved Hide resolved
user_guide_src/source/libraries/publisher.rst Outdated Show resolved Hide resolved
system/Publisher/Publisher.php Show resolved Hide resolved
system/Publisher/Publisher.php Show resolved Hide resolved
tests/_support/Publishers/TestPublisher.php Outdated Show resolved Hide resolved
user_guide_src/source/libraries/publisher.rst Show resolved Hide resolved
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

In general I like this a lot. My biggest concern is from a security standpoint. Maybe I missed it but is there anything preventing a bad package from copying files from elsewhere in the system into the public folder? Or similar dastardly deeds?

I think there has to be some way to limit the available scope of where they can work. In old versions of Bonfire I limited it to anything within the working directory for both reading and writing.

I have seen instances in other languages where a trusted package that has been around for years is taken over by another maintainer and malevolent code gets inserted which is then potentially added to many projects and developers were completely unaware. Even if they had done their homework and inspected the package nicely to begin with, after trusting it for years, you would have no reason to expect.

That's why I think we need to minimize the possibility of this at the framework level here.

user_guide_src/source/libraries/publisher.rst Outdated Show resolved Hide resolved
@MGatner
Copy link
Member Author

MGatner commented Jun 2, 2021

I had some security initially, especially around what files you could acquire from URIs, but I scrapped it after working on the discovery. We may want to think about some framework-level security or something, because the truth is at this point with our autodetection and execution this would be really easy in dozens of places (like sticking arbitrary code in a module's Config/Routes.php).

@lonnieezell like the extra attention to public/ though since anything there is web-accessible. What about denying PHP files sourced from vendor/ heading to public/? Or some similar extension-level filtering?

@lonnieezell
Copy link
Member

Hm. This one's tricky. I can imagine typical scenarios where multiple sites are built on on instance of CI and they may have shared resources, meaning restricting this to just the web root would be problematic for those cases.

Maybe we make a white list of directories it can operate within? By default it's just the project root and any directories within that folder? Granted that makes public available but it needs to be for your example of copying bootstrap files to public. Would it be overkill to have a separate whitelist for readable and writable paths?

@MGatner
Copy link
Member Author

MGatner commented Jun 4, 2021

your example of copying bootstrap files

I think this is actually the number one target use of Publisher. That's my guess at least. A lot of this was built on my Assets module whose purpose is to publish JS/CSS to web root and assist loading them. I wouldn't want to hinder that, but putting in restrictions (like filetypes) seems wise.

Would it be overkill to have a separate whitelist for readable and writable paths?

Actually I think this is a pretty good idea. Since we already have retainPattern() and removePattern() this could be very easy to implement. I'll take a shot and ask for another review.

@MGatner
Copy link
Member Author

MGatner commented Jun 8, 2021

@lonnieezell I went through a few iterations of this. It's a bit of a tricky business because code executed in an auto-discovered class already technically has access to do anything it wants. I decided to focus on "what ends up where": f33d880

This introduces restrictions, which is a way of controlling the allowed destinations and types of files that can go to each destination. I've added some additional measures to make sure child classes of Publisher cannot tamper with their own restrictions to make it even more unlikely that someone's var/mail folder ends up copied into their web-accessible root.

Copy link
Contributor

@mostafakhudair mostafakhudair left a comment

Choose a reason for hiding this comment

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

With PSR-12, I think this should be reverted to bool.

@MGatner
Copy link
Member Author

MGatner commented Jun 9, 2021

@mostafakhudair This whole PR will be run through CS Fixer once Paul is done with it. There are a lot of changes it will need, including the bool/boolean you noted.

@mostafakhudair
Copy link
Contributor

Yeah also brackets, my fault, I didn't know that it will be auto fix, review deleted, thanks for let me know <3

@MGatner
Copy link
Member Author

MGatner commented Jun 9, 2021

No worries! I appreciate your attention to detail. I noted it in the description, but it was a later edit so you probably never saw it:

Note: This will need to be re-styled after the coding standards release.

@MGatner MGatner changed the base branch from develop to 4.2 June 11, 2021 00:17
@MGatner
Copy link
Member Author

MGatner commented Jun 13, 2021

I have a plan to refactor this a bit but the current PR has gotten rather cumbersome so I am going to merge it and then open a new one.

@MGatner MGatner merged commit 3b54e81 into codeigniter4:4.2 Jun 13, 2021
@MGatner MGatner deleted the publisher branch June 13, 2021 20:09
sclubricants added a commit to sclubricants/CodeIgniter4 that referenced this pull request Jun 29, 2022
commit fd19629
Merge: d454c87 4e2ecf3
Author: MGatner <[email protected]>
Date:   Tue Jun 15 09:41:16 2021 -0400

    Merge pull request codeigniter4#4834 from totoprayogo1916/fix-folder-doc

    rename `application` to `app`

commit 4e2ecf3
Author: Toto Prayogo <[email protected]>
Date:   Tue Jun 15 11:30:43 2021 +0700

    rename `application` to `app`

commit d454c87
Merge: 3b54e81 9c0446d
Author: MGatner <[email protected]>
Date:   Tue Jun 15 07:41:21 2021 -0400

    Merge pull request codeigniter4#4828 from Vizzielli/patch-3

commit 3b54e81
Merge: 3d0a4a7 f33d880
Author: MGatner <[email protected]>
Date:   Sun Jun 13 16:09:04 2021 -0400

    Merge pull request codeigniter4#4727 from MGatner/publisher

    Publisher Class

commit 9c0446d
Author: Gianluigi Vizzielli <[email protected]>
Date:   Sat Jun 12 13:25:40 2021 +0200

    Graphic fix on some screen res

    Before: https://prnt.sc/1559re2
    After: https://prnt.sc/1559uou

commit f33d880
Author: MGatner <[email protected]>
Date:   Tue Jun 8 16:22:12 2021 +0000

    Add Publisher restrictions

commit fb70807
Author: MGatner <[email protected]>
Date:   Tue Jun 1 17:23:22 2021 +0000

    Tweak formatting

commit 45d776e
Author: MGatner <[email protected]>
Date:   Tue Jun 1 14:44:25 2021 +0000

    Implement review changes

commit 5906310
Author: MGatner <[email protected]>
Date:   Sat May 29 20:17:28 2021 +0000

    Fix UG format

commit c974add
Author: MGatner <[email protected]>
Date:   Sat May 29 15:43:14 2021 +0000

    Fix test bleeding

commit ad41218
Author: MGatner <[email protected]>
Date:   Sat May 29 15:18:50 2021 +0000

    Add Publish command

commit 3987bff
Author: MGatner <[email protected]>
Date:   Thu May 27 20:15:37 2021 +0000

    Add Guide, tweak class

commit 865c73a
Author: MGatner <[email protected]>
Date:   Tue May 25 16:13:09 2021 +0000

    Apply additional suggestions

commit 116fc82
Author: MGatner <[email protected]>
Date:   Mon May 24 15:51:16 2021 -0400

    Apply suggestions from code review

    Co-authored-by: Mostafa Khudair <[email protected]>

commit 18ae7ea
Author: MGatner <[email protected]>
Date:   Mon May 24 01:10:02 2021 +0000

    Add tests

commit cd66294
Author: MGatner <[email protected]>
Date:   Sun May 23 19:09:49 2021 +0000

    Add Publisher

commit f13dc33
Author: MGatner <[email protected]>
Date:   Sun May 23 19:00:08 2021 +0000

    Remove pasted imports
Merge branch 'master' of https://github.com/codeigniter4/CodeIgniter4 into develop
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.

4 participants