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

feat(intel-security): Add com-intel-security-cordova-plugin Support #1256

Merged
merged 10 commits into from
Mar 27, 2017

Conversation

ehorodyski
Copy link
Contributor

This PR adds support for the com-intel-security-cordova-plugin.

Eric J Horodyski and others added 5 commits March 24, 2017 08:34
/**
* @hidden
*/
export class IntelSecurityData {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can add another @Plugin() tag here with a different pluginRef and no repo property. You can also make it injectable if you think that's a better way to do it.

If you make it Injectable. Then the class above should be empty and only used for documentation purposes.


Apply this fix to the class below as well.

* @param id {string} Storage resource identifier.
* @returns {Promise<any>} Returns a Promise that resolves with the instanceID of the created data instance, or rejects with an error.
*/
createFromData(data: string): Promise<any> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

After making the changes I mentioned above, you can remove the if (!window.intel) ... check and add the regular @Cordova decorator.

Apply this fix to all methods below.

@ehorodyski
Copy link
Contributor Author

Changes made, thank you!

The only thing I can't figure out is now the console is logging cordova_not_available from polyfills.js.

Copy link
Collaborator

@ihadeed ihadeed left a comment

Choose a reason for hiding this comment

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

Looks good now. Just one last change mentioned above and then this will be good to go.

export class IntelSecurityData {

/**
* This creates a new instance of secure data using plain-text data.
* @param id {string} Storage resource identifier.
* @returns {Promise<any>} Returns a Promise that resolves with the instanceID of the created data instance, or rejects with an error.
*/
@Cordova()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since now we have the Cordova decorator, there is no need to write any functionally. We can just have the keyword return in there.

If the real method takes an object as a parameter instead of a string, then we need to change the parameter type in the method definition/docs.

@@ -4,32 +4,6 @@ import { Injectable } from '@angular/core';
declare var window: any;
declare var intel: any;

export interface IntelSecurityDataOptions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I liked the interface better than having the params listed below.

this.storage = new IntelSecurityStorage();
this.data = new IntelSecurityData();

try {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove these 5 lines as they are not needed.

pluginName: 'IntelSecurity',
plugin: 'com-intel-security-cordova-plugin',
pluginRef: 'intel.security.secureData',
repo: ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove the repo property completely.

* .catch((error: any) => console.log(error));
*
* ```
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add the following

* @classes
* IntelSecurityData 
* IntelSecurityStorage

* @param [options.webOwners] {String[]} List of trusted web domains.
* @returns {Promise<any>} Returns a Promise that resolves with the instanceID of the created data instance, or rejects with an error.
*/
@Cordova()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace all @Cordova() decorators with @Cordova({ otherPromise: true }).

I just realized their plugin returns a promise instead of using callbacks. This decorator config will convert their Q promise to a regular ES6 Promise.

@ehorodyski
Copy link
Contributor Author

Thanks @ihadeed -- I'm working on the changes now.

Some "author notes" from me: :-P

  • I didn't set IntelSecurityData or IntelSecurityStorage as Injectable classes because they really work in-tandem. It's my attempt at trying to keep the original intel.security.storage and intel.security.data namespacing in-tact for consumers.
  • The hesitation on adding Interfaces is based on the lack of examples and documentation from Intel on the additional (optional) properties. Some of the method documentations state that there are parameters that exist that aren't currently being used...also, some methods use object containers as a parameter, but there are a total of like 2 properties for it. Actually, I think if any method in this plugin has more than one parameter it's expected to be passed in as an object container.

Anyway, changes coming soon once I can verify they work in a sample Ionic project. Thank you for the help and guidance through this!

@ehorodyski
Copy link
Contributor Author

@ihadeed -- I'm running into this issue now after changing the @Cordova() decorator:

TypeError: Cannot read property 'constructor' of null
    at checkAvailability (main.js:81923)
    at callCordovaPlugin (main.js:82018)
    at main.js:82046
    at main.js:55921
    at new t (polyfills.js:3)
    at tryNativePromise (main.js:55920)
    at getPromise (main.js:55928)
    at wrapOtherPromise (main.js:82045)
    at main.js:82139
    at value (main.js:115414)
    at t.invoke (polyfills.js:3)
    at Object.onInvoke (main.js:35928)
    at t.invoke (polyfills.js:3)
    at e.run (polyfills.js:3)
    at polyfills.js:3

@ihadeed
Copy link
Collaborator

ihadeed commented Mar 27, 2017

@ehorodyski just pushed a commit to the master branch to fix this issue.

@ihadeed ihadeed merged commit aedc9d6 into danielsogl:master Mar 27, 2017
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