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

Apply the changed requirement for the Admin endpoints #3294

Merged
merged 2 commits into from
Apr 21, 2022

Conversation

linked0
Copy link
Contributor

@linked0 linked0 commented Apr 14, 2022

Added the new login endpoint and changed the name and return value for the existing validator and encriptionkey endpoints.

Relates #3280

@codecov
Copy link

codecov bot commented Apr 14, 2022

Codecov Report

Merging #3294 (514c280) into v0.x.x (be9b8d1) will increase coverage by 1.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           v0.x.x    #3294      +/-   ##
==========================================
+ Coverage   87.83%   88.84%   +1.00%     
==========================================
  Files         164      165       +1     
  Lines       16964    17081     +117     
==========================================
+ Hits        14901    15176     +275     
+ Misses       2063     1905     -158     
Flag Coverage Δ
integration 40.31% <0.00%> (?)
unittests 87.59% <0.00%> (-0.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/agora/node/admin/AdminInterface.d 52.54% <0.00%> (ø)
source/agora/common/Task.d 88.88% <0.00%> (-11.12%) ⬇️
source/agora/consensus/BlockStorage.d 68.90% <0.00%> (-8.49%) ⬇️
source/agora/test/NetworkDiscovery.d 84.09% <0.00%> (-4.55%) ⬇️
source/agora/consensus/protocol/Nominator.d 90.94% <0.00%> (-0.72%) ⬇️
source/agora/consensus/state/Ledger.d 88.85% <0.00%> (-0.66%) ⬇️
source/agora/node/Validator.d 91.45% <0.00%> (-0.47%) ⬇️
source/agora/flash/Node.d 80.67% <0.00%> (-0.43%) ⬇️
source/agora/node/TransactionRelayer.d 89.09% <0.00%> (-0.20%) ⬇️
source/agora/network/Client.d 88.13% <0.00%> (-0.10%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be9b8d1...514c280. Read the comment docs.

Copy link
Contributor

@hewison-chris hewison-chris left a comment

Choose a reason for hiding this comment

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

If I understand correctly, the login page is only used to give the user feedback if the credentials are incorrect, otherwise it will store those credentials in the session to use in the AUTH header to pass for other endpoints.


Log in to the admin server

This is dummy endpoint for checking the username and password in
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This is dummy endpoint for checking the username and password in
This is endpoint used only for checking the username and password in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated in next commit btw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I will update, Thanks.

// GET: /login
public override void login ()
{
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we should use the AUTH header to find out if user id and password are correct.

Copy link
Contributor Author

@linked0 linked0 Apr 18, 2022

Choose a reason for hiding this comment

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

We use AUTH header for all the requests. So I introduced the dummy endpoint. It will return an HTTP Status Code(200 or 401). But, I will find a more reasonable way to handle as you mean and the requirement is described.

bool checkPassword (string user, string password) @safe
{
    return user == config.admin.username && password == config.admin.pwd;
}
auto adminrouter = new URLRouter();
adminrouter.any("*", performBasicAuth("Agora Admin", &checkPassword));

Copy link
Contributor

Choose a reason for hiding this comment

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

After thinking more about this we should probably return a JWT which can be used on the subsequent requests to the other endpoints. But for now I think what you have is ok to proceed.

Copy link
Contributor

@hewison-chris hewison-chris left a comment

Choose a reason for hiding this comment

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

LGTM

@mkykadir mkykadir enabled auto-merge (rebase) April 21, 2022 06:14
@mkykadir mkykadir merged commit 26b7df8 into bosagora:v0.x.x Apr 21, 2022
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