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

Adicionando api JWT no Mapos #2366

Closed

Conversation

juliolobo
Copy link
Contributor

Adicionando API no mapos, utilizando JWT para autenticação, utilizando as models do mapos

@Fesantt
Copy link
Contributor

Fesantt commented Mar 30, 2024

😱😱😱😱😱😱😱😱

| JWT Secure Key
|--------------------------------------------------------------------------
*/
$config['jwt_key'] = 'cf51557196367f204f2a824c3db7c3cabdc5611a766feadfeeb23f208de65746';
Copy link
Collaborator

@Pr3d4dor Pr3d4dor Mar 30, 2024

Choose a reason for hiding this comment

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

Deixar essa chave fixa é uma grande vulnerabilidade, vai ser necessário gerar ela dinamicamente.

Sugiro gerar-la durante o processo de instalação.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pensei nisso também, vou dar uma "estudada" em como colocar pra gerar na instalação do MapOS

| ( 1 Hour ) : 60 * 60 = 3600
| ( 1 Minute ) : 60 = 60
*/
$config['token_expire_time'] = 86400;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sugiro deixar esse tempo de expiração configurável via processo de instalação.

$route['api/os/(:num)/anotacoes/(:num)'] = 'api/OsController/anotacoes/$1/$2';
$route['api/os/(:num)/anexos'] = 'api/OsController/anexos/$1';
$route['api/os/(:num)/anexos/(:num)'] = 'api/OsController/anexos/$1/$2';
$route['api/os/(:num)/desconto'] = 'api/OsController/desconto/$1';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dúvida: Não daria pra criar outro arquivo api.php e fazer o include?

Acho que ficaria mais organizado.

*
* @extends REST_Controller
*/
require(APPPATH.'/libraries/REST_Controller.php');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sugestão: Acho melhor tentar colocar o REST_Controller no autoload do composer, pra não ter que colocar o require em todos os arquivos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Não pensei em colocar pq só será usado na API, e quem não for usar a api não precisa carregar o RET_Controller.
Realmente recomenda colocar no autoload?

Copy link
Collaborator

Choose a reason for hiding this comment

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

O custo de carregar no autoload é minimo e o benefício compensa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

'result' => $logs
], REST_Controller::HTTP_OK);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sugestão: Ajustar a indentação.


$result = new stdClass;
$result->appName = $this->getConfig('app_name');
$result->emitente = $this->mapos_model->getEmitente() ?: false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sugestão: Alterar todos os result que usam stdClass pra usar array

@Pr3d4dor
Copy link
Collaborator

Sugestão Geral: Rodar o formatador/linter para formatar todos os arquivos.

@juliolobo
Copy link
Contributor Author

Fechando devido às atualizações

@juliolobo juliolobo closed this Apr 6, 2024
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.

3 participants