Giter Site home page Giter Site logo

client's People

Contributors

dependabot[bot] avatar inztinkt avatar mrcasual avatar shawnhooper avatar shelob9 avatar zackkatz avatar

Stargazers

avatar

Watchers

avatar avatar avatar

Forkers

shawnhooper shelob9 danieliser

client's Issues

Allow user to modify the decay time for access

The idea is to allow the customer to change how long the developer has access to a site. This will resolve the friction that people may feel when clicking the button "why do they need a week!?". Even having the option to change the value will solve the problem.

This is for after launch, but shown in the auth screen mockup:

Add timeline in the Auth screen upon success

  • Add a configuration flag for whether to show a timeline of activity on Client site

If enabled, show a vertical "milestone"-like timeline of access to site, when it was granted, by whom, etc.

• Juaquin Rogers granted access to GravityView support at 1:30 AM on August 7, 2020
• GravityView Support logged-in at 2:30 PM on August 7, 2020
• GravityView Support revoked access on 3:32 PM on August 7, 2020

Just like this!

Consistently apply $title vs $display_name

  • Deprecate using first_name and last_name; support teams don't have names
  • Use display_name in place of where we assume people want to have a "support" suffix, like {{title}} Support

Convert md5 to sha256

Let's just do this.

hash( 'sha256' , $data );

Edit: It may not be available in PHP 5.2…Can we use sha256 on 5.3+ and md5 on 5.2?

Cron class: Undefined Property $support_user

Found this on the trustedlogin.support (client plugin test site) server debug log.

[30-Apr-2021 18:54:02 UTC] PHP Fatal error:  Uncaught Error: Call to a member function delete() on null in /sites/trustedlogin.support/files/wp-content/plugins/GravityView/vendor/GravityView/TrustedLogin/Cron.php:109
Stack trace:
#0 /sites/trustedlogin.support/files/wp-includes/class-wp-hook.php(292): GravityView\TrustedLogin\Cron->revoke()
#1 /sites/trustedlogin.support/files/wp-includes/class-wp-hook.php(316): WP_Hook->apply_filters()
#2 /sites/trustedlogin.support/files/wp-includes/plugin.php(551): WP_Hook->do_action()
#3 phar:///usr/local/bin/wp/vendor/wp-cli/cron-command/src/Cron_Event_Command.php(293): do_action_ref_array()
#4 phar:///usr/local/bin/wp/vendor/wp-cli/cron-command/src/Cron_Event_Command.php(262): Cron_Event_Command::run_event()
#5 [internal function]: Cron_Event_Command->run()
#6 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/CommandFactory.php(98): call_user_func()

Add “click to copy” for sharable access key

This used to require Flash, but now it’s a browser API.

It should give visual feedback to show it’s been copied (a little transparent “copied” floating up from the click target or something).

We shouldn’t need this library: https://clipboardjs.com but something like that.

Won’t be supported in IE apparently.

https://developer.mozilla.org/en-US/docs/Web/API/Element/copy_event

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Interact_with_the_clipboard

https://hackernoon.com/copying-text-to-clipboard-with-javascript-df4d4988697f

Encryption: If vendor site returns 404, error message is wrong.

Encryption::get_remote_encryption_key() pings the vendor's site for the public key.
If a 404 is returned, the error message returned is 'The TrustedLogin vendor was not found.'

Which is incorrect.

Caused by this being handled by Remote::handle_response(...) which assumes it's pinging the SaaS.

switch from modals to authpage

Dropping jquery.confirm from the plugin.

  • Connect grant button to inline JS -> AJAX -> inline Status
  • Connect extend button to inline JS -> AJAX -> inline Status
  • Show the AccessKey on the auth page.
  • Link buttons to auth page and remove any hooked JS events.

When SaaS triggers 'Reset', handling cached public_key

Background:

In TrustedLogin\Encryption() we add a 'cache' of a Vendor's encryption public_key (fetched from their WP API endpoint) into a namespaced site option (tl_<ns>_public_key).

Problem:

If the keys are reset on Vendor after an initial TL support agent is created, any future TL envelopes will be encrypted with the wrong public key, and thus won't be possible to decrypt once received.

Proposed Solution:

We switch from site_option to transient with a 1 hour expiry.
We could also add a url param to the authpage to force a fetch of a new public_key from vendor.

Alternative Solution:

Don't cache the public key, and always fetch it live from Vendor's site.

Thoughts @zackkatz ?

Show when vendor will have access to eCommerce information

One of the biggest security problems that TL solves, especially for GDPR, is support people being able to see customer data, even when they don't need to.

We should have a way to check roles/caps for the user being granted and show the user whether the support has access to eCommerce data.

Here's a mock-up:

I was thinking along these lines:

Add a boolean allow ecommerce access setting in the configuration array. If true, add additional caps for roles added by Woo, EDD, other shops. If false, remove access using exclude caps trustedlogin/trustedlogin-example#14 list.

  • Add a check for WooCommerce, EDD, and and provide hooks for other stores, so that if a site has eCommerce, when granting an user role
  • Get an array of caps for each plugin from the classes…here's the code in EDD, and here's some of the code in WooCommerce Note: Both EDD and Woo have shop_manager roles that map to administrators.
  • Add a filter to allow for other shops to add support to this

klogger not found

debug.log contains the following:

ReplaceMe\TrustedLogin\Logging::setup_klogger (error): KReplaceMe_ReplaceMe_Logger not found.

Could this be due to composer/mozart @zackkatz ?

Decouple admin-ajax callbacks from business logic.

A WordPress plugin using core's admin-ajax API is a common practice, but it's a really bad smell for a product that is security focused to repropose it for plugin settings. It's also not performant. It's also the endpoint that gets attacked the most.

If you want to have this as the insecure default fine, I get it WordPress. But please refactor so that the callbacks for admin-ajax are only responsible for validating/ authorizing the request and then dispatching to a class that was responsible only for handling the actual business logic of the request.

For example in the god class there is "ajax_generate_support" method, from line 489 on is the business logic

https://github.com/trustedlogin/trustedlogin-client/blob/develop/src/Client.php#L489-L526

If that was in its own class, with the single responsibility of generating a support user, then I -- as a plugin developer -- could hook that up to the REST API with my own standard abstraction, endpoint naming, and avoid the performance and security concerns of admin-ajax.

Let SaaS know that session was extended

From TrustedLogin\Client->extend_access()

Data we have:

$return_data = array(
			'site_url'    => get_site_url(),
			'identifier' => $identifier_hash,
			'user_id'    => $support_user_id,
			'expiry'     => $expiration_timestamp,
			'is_ssl'     => is_ssl(),
			'timing'     => array(
				'local' => $timing_local,
				'remote' => null,
			),
		);

As well as which WP User clicked 'extend'.

Useful for logging on SaaS-side.

Asset registration

Things I see as issues:

  • No way to remove this hook add_action( 'admin_enqueue_scripts', array( $this, 'register_assets' ) );
  • Assets handles are not vendor prefixed -- for the client plugin. How can I unregistered jquery-confirm or trusted-login registered by plugin A, but not the copy registered by plugin B?
  • What if I want to use my own CSS that matches my plugin's branding?
  • What if jQuery confirm has a conflict in my admin.

So I think at a minimum namespacing these handles is necessary and adding filters or something to prevent any of the wp_register_script or wp_enqueue_script calls from running. Ideally, this responsibility would be removed from the Client class and I as a plugin developer could easily remove the default hooks that the client adds and use my own system.

The encryption process is slow…

Only if you are 100% certain that your processor is safe, you can set ParagonIE_Sodium_Compat::$fastMult = true; without harming the security of your cryptography keys. If your processor isn't safe, then decide whether you want speed or security because you can't have both.
"Help, Sodium_Compat is Slow! How can I make it fast?"

The difference when enabling \ParagonIE_Sodium_Compat::$fastMult = true; is huge:

  • Disabled: 4.56 seconds
  • Enabled: 1.43 seconds

The user experience difference is huge: 1.4 seconds is fine. 4.5 seconds feels like something is going wrong.

If this is just "how things are" we need to create an animation or something showing us generating secure cryptographic strings or something. Can't just have a spinner. It feels wrong.

[UPDATE]

We could have a progressive loading page, where it shows progress as it happens ("reticulating splines"):

  • Generating secure login keys…
  • Encrypting user access token…
  • Encrypting website URL…

And then that hides, and it says "DONE!"

Move configuration validation to its own class.

The Client class has a lot of responsibilities. The __constructor method alone is responsible for:

  • validating the configuration.
  • generating a variety of exceptions
  • Generating some WP_Error objects that get passed to a different exception.

I have a few major issues with this:

  • It's really hard to reason with this code/ debug it beacuse the validation logic and the other logic like logging, WordPress plugins API, callbacks for admin-ajax, settings API, etc.
  • I'm unclear how to catch these exceptions.

Also line 229 if ( ! isset( $passed_config['auth']['public_key'] ) ) { to 264 does not validate a lot of potential errors. That line will generate a warning with the default argument for $config. It does not check if the variable is an array -- the __construct method signature has no type hint. The unit tests not testing what happens when a stClass object or a JSON string are passed to __construct is bad smell.

Imagine a Config class like this:

namespace TrustedLogin;

class Config
{
	public function __construct(array $config )
	{
		
	}

	public function validate(): Config
	{
		if( '1' == '2'){
			throw new \Exception();
		}
		return $this;
	}
//Getter and setter methods?
}

and if the __construct method of Client looked like this:

public function __construct( Config $config ) {

Then I could instantiate the client like this:

$config = new Config(['namespace' => 'shawn' ]);
try{
	$config = $config->validate();
	$client = new Client($config);
}catch (\Exception $e ){
	//Don't tell me, a plugin developer
	// how to log errors or show an admin notice using my own special abstraction
	// I will handle this myself.
}

Add brute force attack protection

Allow same key many times, but not brute-forcing the accesskey.

Assume user has the endpoint hash and are trying wrong keys.

Allow X bad attempts in X minutes (both should be constants set in the code).

Bug with Cron callback

Getting the following in debug.log:

PHP Warning:  call_user_func_array() expects parameter 1 to be a valid callback, class 'ReplaceMe\TrustedLogin\Cron' does not have a method 'cron_revoke_access' in .../hosts/trustedlogin-dev/wp-includes/class-wp-hook.php on line 288

Use lib_sodium for encryption

It is better to rely on PHP's encryption system than to create something custom with openssl_* functions.

https://github.com/trustedlogin/trustedlogin-client/blob/987efd9a362a3294e921e9f64ae0f796cbc84158/src/Client.php#L2498

This library is included in PHP so it runs faster and there is a polyfill for out of date versions of PHP.

I would STRONGLY recommend using a secure encryption system like this one: https://gist.github.com/ericmann/1e54625d81a35b243eec0d67ae7b868f

wp_generate_password() is misused as a secure hash generator

Here, the core function wp_generate_password is used to generate some sort of hash value:

https://github.com/trustedlogin/trustedlogin-client/blob/987efd9a362a3294e921e9f64ae0f796cbc84158/src/Client.php#L501-L503

This has two key weakness:

Related #3

Receiving 'reset' request from SaaS.

hey @zackkatz I was looking at building this out a REST API endpoint.

ie <siteurl>/<api-slug>/tl-<ns>/<version>/revoke

however, now i'm just working through how:

  • the SaaS would know the URL (as it's encrypted and secret to the SaaS).
  • if we did have the URL, how would we'd verify that this is coming from SaaS.
  • does this open a new potential attack surface?

Might it be best to let the site access timeout given that the encrypted envelope will be useless once the encryption keys are reset on vendor side?

Also means we can stop caching the public key and also drop issue #39.

When Vendor changes their website, public key lookup paths break

Currently the public key lookup is fetched in the client code by reading the vendor/website URL. This assumes that the same URL will be used for hosting the Vendor plugin.

Furthermore, this would prevent vendor from changing their websites without providing redirects for the public key endpoint (they probably would do a site-wide 301, but it's still a problem).

I propose either:

  1. Fetching this information from the SaaS instead. TrustedLogin\Encryption::get_remote_encryption_key() could ping the SaaS instead of the Vendor. The SaaS would need to round-trip to the Vendor site to fetch the key, but it would allow modification and remove a setting that could be problematic in the future.
  2. Adding/modifying the vendor configuration file to provide a path to the /wp-json/ endpoint. It's not clear that the vendor/website configuration needs to be the same as the public key endpint.

trustedlogin/trustedlogin-ecommerce#405

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.