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

Add API tokens #838

Merged
merged 5 commits into from Mar 12, 2021
Merged

Add API tokens #838

merged 5 commits into from Mar 12, 2021

Conversation

@rashley-iqt
Copy link
Contributor

@rashley-iqt rashley-iqt commented Mar 10, 2021

Configures the authentication route to provide users with a JWT token upon login. Additionally, configures express.js to authenticate the JWT token when accessing any routes of the form http://<deployment ip>:3000/api/db/* to ensure that only authenticated users can interact with the subscriber database.

Please note that when deploying with this in place users should create the environment variable JWT_SECRET_KEY to override the default value or the JWT tokens can be spoofed.

I believe that this PR addresses #837

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Mar 10, 2021

CLA assistant check
All committers have signed the CLA.

@acetcom
Copy link
Member

@acetcom acetcom commented Mar 10, 2021

Hi @rashley-iqt

Your contribution does not work like below in my environment.

  • First page
    Screenshot from 2021-03-10 23-09-06

  • After pressing SAVE button
    Screenshot from 2021-03-10 23-11-20

Here is my execution.

$ node -v
v14.16.0
$ npm -v
6.14.11
$ git clone https://github.com/rashley-iqt/open5gs.git
$ cd open5gs
$ git checkout api-tokens 
$ cd webui
$ npm install
$ npm run dev

Then use Chrome Browser to connect Open5GS WebUI.

Please let me know if my test is wrong.

Thank you so much for your contribution.
Sukchan

@acetcom
Copy link
Member

@acetcom acetcom commented Mar 10, 2021

One more notes! Logging out of the WebUI works fine. Is it your intention?

@rashley-iqt
Copy link
Contributor Author

@rashley-iqt rashley-iqt commented Mar 10, 2021

that looks like you had an existing session without the token hanging around. does it behave properly if you log out and log back in?

@acetcom
Copy link
Member

@acetcom acetcom commented Mar 10, 2021

Yes! When I logout and log back in, everything works fine

However, it seems to have found another problem. If I install npm module using NPM 6 version instead of 7 version, then package-lock.json is changed as below.

diff --git a/webui/package-lock.json b/webui/package-lock.json
index 545e0571..8362e1b4 100644
--- a/webui/package-lock.json
+++ b/webui/package-lock.json
@@ -1,7937 +1,8 @@
 {
   "name": "open5gs",
   "version": "2.2.0",
-  "lockfileVersion": 2,
+  "lockfileVersion": 1,
   "requires": true,
-  "packages": {
-    "": {
-      "name": "open5gs",
-      "version": "2.2.0",
-      "license": "GPL-3.0",
-      "dependencies": {
-        "axios": "^0.16.2",
-        "babel-plugin-polished": "^1.1.0",
-        "babel-plugin-styled-components": "^1.5.1",
-        "body-parser": "^1.18.3",
-        "connect-mongo": "^1.3.2",
-        "express": "^4.16.3",
-        "express-restify-mongoose": "^4.3.0",
-        "express-session": "^1.15.2",
-        "immutable": "^3.8.1",
-        "jsonwebtoken": "^8.5.1",
-        "lodash": "^4.17.10",
-        "lusca": "^1.6.0",
-        "method-override": "^2.3.9",
-        "mongoose": "^4.13.14",
-        "mongoose-long": "^0.1.1",
-        "morgan": "^1.8.2",
-        "next": "^3.2.3",
-        "next-redux-wrapper": "^1.3.5",
-        "nprogress": "^0.2.0",

I really want to merge your contribution to the main branch. However, I'm worried about these parts as someone can get confused at first. Let me think about those part a little more.

If you have a good idea, please feel free to give me any suggestions.

Thank you so much for your contribution.
Sukchan

@rashley-iqt
Copy link
Contributor Author

@rashley-iqt rashley-iqt commented Mar 10, 2021

what command did you use to install under npm 6?

@acetcom
Copy link
Member

@acetcom acetcom commented Mar 10, 2021

npm install

As mentioned earlier, I did it as below.

$ node -v
v14.16.0
$ npm -v
6.14.11
$ git clone https://github.com/rashley-iqt/open5gs.git
$ cd open5gs
$ git checkout api-tokens 
$ cd webui
$ npm install
$ npm run dev

@rashley-iqt
Copy link
Contributor Author

@rashley-iqt rashley-iqt commented Mar 10, 2021

ok, having tried this myself and checking out the docs I think this is expected behavior. If you compare
v6 to v7 you'll see that v6 versions of the package-lock.json schema don't have the packages attribute. Since npm install rebuilds the entire node_modules tree it has to also rewrite the lock file. The intention of the lock file is to allow reconstruction of the node_modules tree as it was built so it makes sense that building under different versions would change the schema to match. Given that the application appears to work in both versions and that it is the lock file that is changing and not package.json itself I don't think this should be a cause for concern.

@matt9j
Copy link
Contributor

@matt9j matt9j commented Mar 10, 2021

As someone coming from a cpp/rust background and doing a bit of nodejs, prorgramming, I've been very confused by npm install vs npm update. In the philosophy of npm, npm install is not intended to do a clean install, and will do the updates to inplace lock files you've noticed. It will also actually update the versions of dependencies according to the package file constraints ignoring the versions pinned in the lock file if they are not already installed! It was apparently intended as an interactive command to be mostly used by human developers. The command npm ci was later added to do a true "clean install," and is really what you want to be using in cases like this. Here are the docs for npm ci: https://docs.npmjs.com/cli/v6/commands/npm-ci

@rashley-iqt
Copy link
Contributor Author

@rashley-iqt rashley-iqt commented Mar 10, 2021

I would say that I largely agree with that. In docker files for my other projects I typically prefer npm ci --no-optional. The ci installation path is intended for CI/CD pipelines so I think it makes more sense for containerized applications. I'm old enough with npm that i reflexively rm -rf node_modules before any kind of install. I think for the purposes of the kind of testing we have been doing for this PR both options are equivalent, but it may be worth revisiting this in a different context. If nothing else, I find it usually speeds up the build process.

@matt9j
Copy link
Contributor

@matt9j matt9j commented Mar 10, 2021

I think for the purposes of the kind of testing we have been doing for this PR both options are equivalent

That's what I am trying to clarify-- they are not, although you might reasonably expect them to be coming from other build tools! npm install, unlike other build tools, does not treat the package.lock file as sacred, and will update it as needed to reflect the current state of node_modules. You see the inverse change to the version controlled package.lock file on the main branch if you do a clean clone and npm install the webui with npm 7.

@acetcom
Copy link
Member

@acetcom acetcom commented Mar 10, 2021

Hi @rashley-iqt and @matt9j

I first knew npm ci --no-optional today. It seems that all of the open5gs documentation should be replaced with this command when installing npm modules. Also, we need to update the install script.

I don't think it's a good idea to merge right now since all users need logout and log back in.

I plan to release v2.2.1 within 2-3 weeks. At that time, I will apply this pull request and notify users that a new login is required in WebUI.

You can change the necessary work in your api-tokens branch if you want to. I'll do the rest in package release v2.2.1.

Thank you so much for your effort.
Sukchan

@rashley-iqt
Copy link
Contributor Author

@rashley-iqt rashley-iqt commented Mar 10, 2021

I could try to catch the lack of a token and redirect to login if that would help.

@acetcom
Copy link
Member

@acetcom acetcom commented Mar 10, 2021

@rashley-iqt Wow! That sounds great!

@rashley-iqt
Copy link
Contributor Author

@rashley-iqt rashley-iqt commented Mar 11, 2021

i think that should address the issue of logging out and back in

@acetcom acetcom merged commit 7848b6c into open5gs:main Mar 12, 2021
1 check passed
acetcom added a commit that referenced this issue Mar 12, 2021
- Update document
- Fix the install script
- Remove last commit to maintain login session
@acetcom
Copy link
Member

@acetcom acetcom commented Mar 12, 2021

@rashley-iqt

I've merged your pull request today.

Your last commit is not working in my environment. Since login session is not maintained, the user has to log in every time. So, I removed it.

You can see my follow-up on 569f98f.

Please feel free to let me know if I have done anything wrong.

I'd really appreciate you for this work.
Sukchan

acetcom added a commit that referenced this issue Mar 12, 2021
Security patch is applied. If you want to use this version, you need to
log out and log back in.

See Pull Request #838 for more detailed information.
@rashley-iqt
Copy link
Contributor Author

@rashley-iqt rashley-iqt commented Mar 12, 2021

That all looks great. Thank you for all of your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants