Feature/mqtt topic namespacing for emq #2887

saransh-dev · 2017-09-05T11:00:56Z

Fixed issue #2842

closes: #2842

…ub.com/apinf/platform into feature/mqtt-topic-namespacing-for-emq

…lable and make it unique

phanimahesh

I haven't tested this yet, these are comments from just looking at the diff.

phanimahesh · 2017-09-05T11:27:57Z

apinf_packages/proxy_backends/client/form/proxies/emq/emq.less

+.disabled-div{
+	pointer-events: none;
+  opacity: 0.4;
+}


Consistent indentation please. And configure your editor to end files with newlines.

phanimahesh · 2017-09-05T11:35:58Z

apinf_packages/proxy_backends/server/methods.js

@@ -125,7 +126,7 @@ Meteor.methods({
          id: rule.id,
          allow: rule.allow,
          access: rule.access,
-          topic: rule.topic,
+          topic: topicPrefix + rule.topic,


I'm not sure if this is a good idea. an enterprising user can make a request with invalid or someone else's topic prefix.

Please add a verification to ensure this is the same as saved topic prefix for this api, or even better, do not accept topic prefix as argument here. I'm asuming it would be possible to extract saved topic prefix from mongo directly on server and prepend it there.

Client side data should never be trusted blindly. Client uses topic prefix to display to users, and server should extract topic prefix from db and use it for its operations instead of trusting data that was through the client.

phanimahesh · 2017-09-05T11:36:08Z

apinf_packages/proxy_backends/server/methods.js

@@ -153,7 +154,7 @@ Meteor.methods({
          proxyId: rule.proxyId,
          allow: rule.allow,
          access: rule.access,
-          topic: rule.topic,
+          topic: topicPrefix + rule.topic,


Same as above.

phanimahesh · 2017-09-05T11:55:52Z

apinf_packages/proxy_backends/collection/regex.js

@@ -11,3 +11,5 @@ https://joinup.ec.europa.eu/community/eupl/og_page/european-union-public-licence
 export const proxyBasePathRegEx = new RegExp(/^\/[\w\-\.\?\$\*\+\'\)\(/:#@!&,;=]+\/$/);
 // eslint-disable-next-line no-useless-escape
 export const apiBasePathRegEx = new RegExp(/^\/[\w\-\?\.\$\*\+\)\'\(/:#@!&,;=]*$/);
+// allow only a-zA-Z0-9_ # +
+export const topicPrefix = new RegExp(/^\/[\w+#]+\/$/);


+ and # are special in mqtt topics. Both characters must not be allowed in topic prefix.

Topic prefix regex should be /^\/[\w-]+\/$/

Topics, on the other hand, should probably be /^\/[\w-]+\/(([\w-]+|\+)\/)*([\w-]+|\+|#)$/
To explain, topics must start with a /, have atleast one "word" (topic prefix), followed by a /, and can be followed by:

an arbitrarily long string of words or + delimited by /

optionally ending in a #

The last character of a topic can not be /. The regex is a bit unwieldy, but I can not compress it further.

BTW the topic regex is for use after prepending the prefix. Without the prefix, it will reduce to /^(([\w-]+|\+)\/)*([\w-]+|\+|#)$/

marla-singer · 2017-09-05T13:22:34Z

apinf_packages/core/lib/i18n/fi.i18n.json

@@ -28,6 +28,7 @@
    "invalidDomainMessage": "Tarvitaan validi verkkotunnus",
    "invalidEmailMessage": "Tarvitaan validi s\u00e4hk\u00f6postiosoite",
    "invalidApiBasePathMessage": "T\u00e4ytyy alkaa kauttaviivalla ja p\u00e4\u00e4tty\u00e4 kauttaviivaan. Sallittuja merkkej\u00e4 ovat kirjaimet, numerot sek\u00e4  -.?$*+'()\/:#@!&,;=",
+    "invalidTopicPrefix": "On aloitettava ja päättyttävä /. Sallitut aakkosnumeeriset merkit ja + #",


Delete Finnish text

marla-singer · 2017-09-05T13:22:40Z

apinf_packages/core/lib/i18n/fi.i18n.json

@@ -1060,6 +1061,7 @@
    "proxyBackendForm_apiPort_helpText": "Rajapintapalvelimen portin numero",
    "proxyBackendForm_disableApiKey_helpText": "Salli k\u00e4ytt\u00e4jien kutsua rajapintaa ilman rajapinta-avainta.",
    "proxyBackendForm_proxyBasePath_helpText": "Peruspolku jokaiseen v\u00e4lityspalvelinkutsuun. Esimerkiksi:",
+    "proxyBackendForm_topicPrefix_helpText": "Aiheen etuliite lisätään ennen kaikkia EMQ-aiheita.",


Delete Finnish text

marla-singer · 2017-09-05T13:24:10Z

apinf_packages/proxy_backends/client/form/proxies/emq/emq.html

  <!-- Rate limits -->
-  <div class="panel panel-default">
+  <div class="panel panel-default {{# unless proxyBackend.emq.settings.topicPrefix }} disabled-div {{/ unless}}">


Wrap part {{# unless proxyBackend.emq.settings.topicPrefix }} disabled-div {{/ unless}} in helper.
Read: http://blazejs.org/guide/spacebars.html#Attribute-Helpers

marla-singer · 2017-09-05T13:25:32Z

apinf_packages/proxy_backends/client/form/proxies/emq/emq.html

@@ -3,8 +3,35 @@
  <!-- Hidden fields -->
  {{> afQuickField name="type" type="hidden" value="emq" }}
  <!-- End hidden fields -->
+
+  <!-- Topic prefix -->
+  {{#unless proxyBackend.emq.settings.topicPrefix}}


Add spacebar between # and word
Add spacebar before }}

marla-singer · 2017-09-05T13:25:52Z

apinf_packages/proxy_backends/client/form/proxies/emq/emq.html

+      </label>
+
+      {{> afFieldInput
+        name="emq.settings.topicPrefix"


Place it in one line

marla-singer · 2017-09-05T13:26:11Z

apinf_packages/proxy_backends/client/form/proxies/emq/emq.html

+
+      <!-- validation messages -->
+      {{# if afFieldIsInvalid name='emq.settings.topicPrefix' }}
+      <p class="text-danger">


<p> block must be nested

marla-singer · 2017-09-07T05:18:41Z

@amardeep-deligence Don't generate one-type commits.

 Fixed git platform build errors 
 Fixed git platform build errors 
 Fixed git platform build errors Third Time 
 Fixed git platform build errors - Fourth attempt

These commits are unuseful, squash them
https://github.com/apinf/platform/blob/develop/CONTRIBUTING.md#git-rebase

phanimahesh · 2017-09-07T06:31:38Z

If trying to fix the last commit, amending and force pushing with lease is a good idea. or else there's a fixup flag to git commit that allows marking commits for future squashing. Even for other commits, commit messages need some work to make intent clear, but I won't block this waiting for that.

…

On Thu 7 Sep, 2017, 10:48 Daria Voytova ***@***.***> wrote: @amardeep-deligence <https://github.com/amardeep-deligence> Don't generate one-type commits. Fixed git platform build errors Fixed git platform build errors Fixed git platform build errors Third Time Fixed git platform build errors - Fourth attempt These commits are unuseful, squash them https://github.com/apinf/platform/blob/develop/CONTRIBUTING.md#git-rebase — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2887 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABnL8NrUF5MPZ_7E0euPs4Ktj0iy0hmmks5sf3yzgaJpZM4PMyAf> .

marla-singer · 2017-09-07T08:25:58Z

@phanimahesh Did you test the functional work of this code?

phanimahesh

Never merge into a feature branch. As it stands this branch can not be reviewed. Commits are present multiple times and changes in a commit don't appear in overall diff, even when I did not notice any later commit reverting them. Please do a proper rebase and push force with lease, compressing this into no more than 2-3 logical commits.

Also:

ACL rules must fit in a single line on desktop screens
Deleting all ACLs is a single accidental click away. Add a confirmation or undo option.

phanimahesh · 2017-09-07T10:35:12Z

apinf_packages/proxy_backends/server/emq.js

@@ -53,7 +53,7 @@ Meteor.methods({
        if (err) reject(err);

        // Expose only username and ID
-        const users = _.map(res.data, (user) => {
+        const users = _.map(res?res.data:[], (user) => {


Ideally this single line should be commit of its own.

phanimahesh · 2017-09-07T10:36:18Z

apinf_packages/proxy_backends/server/methods.js

@@ -130,7 +142,7 @@ Meteor.methods({
        };
        // Append ACL type & value
        data[rule.fromType] = rule.fromValue;
-
+        return


phanimahesh · 2017-09-07T10:37:03Z

apinf_packages/proxy_backends/server/methods.js

@@ -158,7 +170,7 @@ Meteor.methods({
        };
        // Append ACL type & value
        data[rule.fromType] = rule.fromValue;
-
+        return


phanimahesh · 2017-09-07T10:40:59Z

apinf_packages/proxy_backends/collection/regex.js

+// allow / then a-zA-Z0-9 then /
+export const topicPrefixRegEx = new RegExp(/^\/[\w-]+\/$/);
+// Must begin with alphanumeric. Allowed character is / and end with + or # delimited by /
+export const topicRegEx = new RegExp(/^(([\w-]+|\+)\/)*([\w-]+|\+|#)$/);


This comment is incorrect. This should make it clear:

Topic is a string of segments joined by /. A segment is either:

a string of letters, numbers or literals - or _

Or just the literal +

In addition, the last segment can be a literal #.

phanimahesh · 2017-09-07T11:21:23Z

apinf_packages/proxy_backends/server/methods.js

+      { 'emq.settings.acl.id': { $in: [rules && rules.length ? rules[0].id : ''] } },
+      { fields: { 'emq.settings.topicPrefix': 1 } });
+    // set topicPrefix if available in db
+    if (emqProxyBackend && emqProxyBackend.emq.settings.topicPrefix) {


What happens if it is not available? User gets to set rules on arbitrary topics because enforced prefix will be ''? (see line 106)

Fixed git platform build errors Fixed git platform build errors Fixed git platform build errors Third Time

bajiat · 2017-09-14T08:23:57Z

@marla-singer Can you review the code?

@phanimahesh Can you check that the PR fulfills the requirements for the enhancement issue?

…avis-ci.

amardeep-deligence · 2017-09-14T12:43:16Z

@phanimahesh "Deleting all ACLs is a single accidental click away. Add a confirmation or undo option." is not done yet. Other features are ready to test.

bajiat · 2017-09-19T08:31:08Z

@phanimahesh Can you create a separate issue for "Deleting all ACLs is a single accidental click away. Add a confirmation or undo option." ? Describe why it is needed in the issue and what needs to be done.

amardeep-deligence · 2017-09-19T09:12:32Z

@bajiat @phanimahesh , I have implemented "confirmation before deleting ACLs" in this PR already

amardeep-deligence · 2017-09-25T07:43:43Z

This PR may also close Issue- #2724

bajiat · 2017-09-28T11:27:04Z

@amardeep-deligence and @phanimahesh What is the status of the PR? Have all the requested changes been made? If not, is there a reason?

amardeep-deligence · 2017-09-28T11:39:56Z

@bajiat @phanimahesh @marla-singer @brylie this PR is ready for review.

bajiat · 2017-10-17T08:54:52Z

@phanimahesh and @marla-singer What is the status of this PR? The PR was started 1,5 months ago. Are there pending changes or is the review for changes missing? Can this still be rebased or should it be started from scratch by cherrypicking the actual changes? I would like to have a resolution (decision on what is to be done) by 16:00 EEST today.

Cancel review

marla-singer · 2018-01-09T07:04:43Z

@preriasusi What to do with this PR?

phanimahesh · 2018-01-12T14:35:57Z

@marla-singer We are effectively reimplementing this entire view in the new dashboard you are working on, after considering more feedback. This can be closed for now, or after we have absorbed the relevant parts into the new dashboard.

amardeep-deligence and others added 3 commits September 5, 2017 09:37

changes regarding mqtt-topic-namespacing-for-emq #2842 task

cafb307

Merge branch 'feature/mqtt-topic-namespacing-for-emq' of https://gith…

7936d8e

…ub.com/apinf/platform into feature/mqtt-topic-namespacing-for-emq

fix design issue and work on form disable while topic prefix not avai…

c2e19e0

…lable and make it unique

saransh-dev added the Ready for review label Sep 5, 2017

saransh-dev requested review from brylie, phanimahesh and marla-singer September 5, 2017 11:00

saransh1510 added 2 commits September 5, 2017 16:43

remove errors of travis-ci.org

62f0c02

remove error Newline required at end of file but not found eol-last

e2646a5

phanimahesh requested changes Sep 5, 2017

View reviewed changes

marla-singer previously requested changes Sep 5, 2017

View reviewed changes

marla-singer removed the Ready for review label Sep 5, 2017

phanimahesh requested changes Sep 7, 2017

View reviewed changes

phanimahesh reviewed Sep 7, 2017

View reviewed changes

amardeep-deligence added 2 commits September 8, 2017 18:56

done suggested changes

be028fd

Fixed git platform build errors Fixed git platform build errors Fixed git platform build errors Third Time

Changes regarding topicPrefix initialization and trailing space fixes

0ddd2a1

amardeep-deligence force-pushed the feature/mqtt-topic-namespacing-for-emq branch from dc7aae6 to 0ddd2a1 Compare September 8, 2017 13:30

bajiat added the in progress label Sep 12, 2017

amardeep-deligence added 2 commits September 14, 2017 17:43

Changes re. ACL rules fit in single line and other small fixes for tr…

1a9ae66

…avis-ci.

fixed trailing-spaces and indent related errors

21961ab

Changes re confirmation on ACLs deletion

bd10f98

amardeep-deligence force-pushed the feature/mqtt-topic-namespacing-for-emq branch from f47ea50 to bd10f98 Compare September 15, 2017 07:17

amardeep-deligence added the Ready for review label Sep 15, 2017

Merge branch 'develop' into feature/mqtt-topic-namespacing-for-emq

b1831ba

amardeep-deligence removed the in progress label Sep 19, 2017

travis-ci issue fix

c9ac92f

amardeep-deligence requested review from phanimahesh and marla-singer September 28, 2017 11:33

marla-singer removed request for brylie and marla-singer January 9, 2018 07:04

55 closed this Apr 23, 2018

ghost removed the Ready for review label Apr 23, 2018

preriasusi deleted the feature/mqtt-topic-namespacing-for-emq branch August 29, 2018 08:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/mqtt topic namespacing for emq #2887

Feature/mqtt topic namespacing for emq #2887

saransh-dev commented Sep 5, 2017

phanimahesh left a comment

phanimahesh Sep 5, 2017

phanimahesh Sep 5, 2017

phanimahesh Sep 5, 2017

phanimahesh Sep 5, 2017

phanimahesh Sep 5, 2017 •

edited

marla-singer Sep 5, 2017

marla-singer Sep 5, 2017

marla-singer Sep 5, 2017

marla-singer Sep 5, 2017

marla-singer Sep 5, 2017

marla-singer Sep 5, 2017

marla-singer commented Sep 7, 2017

phanimahesh commented Sep 7, 2017 via email

marla-singer commented Sep 7, 2017

phanimahesh left a comment

phanimahesh Sep 7, 2017

phanimahesh Sep 7, 2017

phanimahesh Sep 7, 2017

phanimahesh Sep 7, 2017 •

edited

phanimahesh Sep 7, 2017

bajiat commented Sep 14, 2017

amardeep-deligence commented Sep 14, 2017

bajiat commented Sep 19, 2017

amardeep-deligence commented Sep 19, 2017 •

edited

amardeep-deligence commented Sep 25, 2017

bajiat commented Sep 28, 2017

amardeep-deligence commented Sep 28, 2017

bajiat commented Oct 17, 2017 •

edited

marla-singer commented Jan 9, 2018

phanimahesh commented Jan 12, 2018

Feature/mqtt topic namespacing for emq #2887

Feature/mqtt topic namespacing for emq #2887

Conversation

saransh-dev commented Sep 5, 2017

Fixed issue #2842

phanimahesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phanimahesh Sep 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marla-singer commented Sep 7, 2017

phanimahesh commented Sep 7, 2017 via email

marla-singer commented Sep 7, 2017

phanimahesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phanimahesh Sep 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajiat commented Sep 14, 2017

amardeep-deligence commented Sep 14, 2017

bajiat commented Sep 19, 2017

amardeep-deligence commented Sep 19, 2017 • edited

amardeep-deligence commented Sep 25, 2017

bajiat commented Sep 28, 2017

amardeep-deligence commented Sep 28, 2017

bajiat commented Oct 17, 2017 • edited

marla-singer commented Jan 9, 2018

phanimahesh commented Jan 12, 2018

phanimahesh Sep 5, 2017 •

edited

phanimahesh Sep 7, 2017 •

edited

amardeep-deligence commented Sep 19, 2017 •

edited

bajiat commented Oct 17, 2017 •

edited