-
Notifications
You must be signed in to change notification settings - Fork 6
[Issue #70] Allow setting the list of allowed permissions for a client #71
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,10 +4,11 @@ | |
|
|
||
| 'use strict'; | ||
|
|
||
| import config from '../config'; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import config from '../config'; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import Sequelize from 'sequelize'; | ||
| import sequelizeFixtures from 'sequelize-fixtures'; | ||
|
|
||
| const IDLE = 0 | ||
| const INITIALIZING = 1; | ||
|
|
@@ -77,6 +78,13 @@ export default function() { | |
| deferreds.pop().resolve(db); | ||
| } | ||
| state = READY; | ||
|
|
||
| // Load default permissions. | ||
| const permissions = config.get('permissions').map(permission => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this really belong in config ? I'm not sure what you have in mind for the future. How will the permissions be required ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just though the config would be a good place cause it allows overriding the list of default permissions (for future testing purposes for example) and it provides an easy way to access the list ( I don't have a strong about this, so we could use an independent json file if you think is more appropriate.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think it's better; so that we can also ship it in git. Alternatively you can provide the default list in the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, I missed it. I still find it clumsy but up to you :) maybe just file an issue if you prefer to move faster.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll file a follow-up.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| return { model: 'Permissions', data: { name: permission }}; | ||
| }); | ||
| return sequelizeFixtures.loadFixtures(permissions, db); | ||
| }).then(() => { | ||
| return db; | ||
| }).catch(e => { | ||
| console.error(e); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| /* This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| 'use strict'; | ||
|
|
||
| module.exports = (sequelize, DataTypes) => { | ||
| const Permission = sequelize.define('Permissions', { | ||
| name: { type: DataTypes.STRING, primaryKey: true } | ||
| }, { timestamps: false }); | ||
|
|
||
| return Permission; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
|
|
||
| import express from 'express'; | ||
|
|
||
| import db from '../models/db'; | ||
| import db from '../models/db'; | ||
| import { | ||
| ApiError, | ||
| BAD_REQUEST, | ||
|
|
@@ -17,6 +17,7 @@ import { | |
| ERRNO_FORBIDDEN, | ||
| ERRNO_INTERNAL_ERROR, | ||
| ERRNO_INVALID_API_CLIENT_NAME, | ||
| ERRNO_INVALID_API_CLIENT_PERMISSION, | ||
| ERRNO_INVALID_API_CLIENT_REDIRECT_URL, | ||
| INTERNAL_ERROR, | ||
| modelErrors, | ||
|
|
@@ -50,6 +51,15 @@ router.post('/', (req, res) => { | |
| .isArrayOfUrls({ require_valid_protocol: true }); | ||
| } | ||
|
|
||
| const permissions = req.body.permissions; | ||
| if (permissions) { | ||
| if (!Array.isArray(permissions)) { | ||
| req.body.permissions = [permissions]; | ||
| } | ||
| req.checkBody('permissions', 'invalid "permissions"') | ||
| .isArrayOfPermissions(); | ||
| } | ||
|
|
||
| const error = req.validationErrors()[0]; | ||
| if (error) { | ||
| let errno; | ||
|
|
@@ -61,32 +71,58 @@ router.post('/', (req, res) => { | |
| case 'name': | ||
| errno = ERRNO_INVALID_API_CLIENT_NAME; | ||
| break; | ||
| case 'permissions': | ||
| errno = ERRNO_INVALID_API_CLIENT_PERMISSION; | ||
| break; | ||
| default: | ||
| errno = ERRNO_BAD_REQUEST; | ||
| } | ||
| return ApiError(res, 400, errno, BAD_REQUEST); | ||
| } | ||
|
|
||
| db().then(models => { | ||
| models.Clients.create(req.body).then(client => { | ||
| res.status(201).send(client); | ||
| }).catch(error => { | ||
| if (error.name && error.name === modelErrors[RECORD_ALREADY_EXISTS]) { | ||
| return ApiError(res, 403, ERRNO_FORBIDDEN, FORBIDDEN); | ||
| } | ||
| ApiError(res, 500, ERRNO_INTERNAL_ERROR, INTERNAL_ERROR); | ||
| return models.sequelize.transaction(transaction => { | ||
| return models.Clients.create(req.body, { transaction }).then(client => { | ||
| if (!req.body.permissions) { | ||
| return client; | ||
| } | ||
| return client.addPermissions(req.body.permissions, { | ||
| transaction | ||
| }).then(() => client); | ||
| }); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we can simplify with a create with
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was my first approach, but somehow I failed to apply it... so if you don't mind, I'll stick with the current approach.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oki |
||
| }); | ||
| }).then(client => { | ||
| res.status(201).send(client); | ||
| }).catch(error => { | ||
| if (error.name && error.name === modelErrors[RECORD_ALREADY_EXISTS]) { | ||
| return ApiError(res, 403, ERRNO_FORBIDDEN, FORBIDDEN); | ||
| } | ||
| ApiError(res, 500, ERRNO_INTERNAL_ERROR, INTERNAL_ERROR); | ||
| }); | ||
| }); | ||
|
|
||
| const normalizeClient = client => { | ||
| if (client.Permissions) { | ||
| client.dataValues.permissions = client.Permissions.map( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it really needed to use
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, unfortunately we need to modify |
||
| permission => permission.name | ||
| ); | ||
| delete client.dataValues.Permissions; | ||
| } | ||
| return client; | ||
| }; | ||
|
|
||
| // Get the list of registered API clients. | ||
| router.get('/', (req, res) => { | ||
| db().then(models => { | ||
| models.Clients.findAll({ | ||
| attributes: ['key', 'name', 'authRedirectUrls', | ||
| 'authFailureRedirectUrls'] | ||
| 'authFailureRedirectUrls'], | ||
| include: [{ | ||
| model: models.Permissions, | ||
| attributes: ['name'], | ||
| }], | ||
| }).then(clients => { | ||
| res.status(200).send(clients); | ||
| res.status(200).send(clients.map(normalizeClient)); | ||
| }).catch(error => { | ||
| ApiError(res, 500, ERRNO_INTERNAL_ERROR, INTERNAL_ERROR); | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quite a good idea to have it here instead of defining it below, so that it's closer to the definition.