Skip to content

SDK refactor: Auth controller #288

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 25 commits into from
Mar 16, 2018
Merged

SDK refactor: Auth controller #288

merged 25 commits into from
Mar 16, 2018

Conversation

jenow
Copy link
Contributor

@jenow jenow commented Mar 13, 2018 •
edited
Loading

Added the auth controller with the following method:

  • checkToken
  • createMyCredentials
  • credentialsExist
  • deleteMyCredentials
  • getCurrentUser
  • getMyCredentials
  • getMyRights
  • getStrategies
  • login
  • logout
  • updateMyCredentials
  • User updateSelf
  • validateMyCredentials

Merge the branch feature/query-pormise so query returns promise

benoitvidis and others added 20 commits March 6, 2018 12:41
@jenow jenow self-assigned this Mar 13, 2018
@codecov-io
Copy link

codecov-io commented Mar 13, 2018 •
edited
Loading

Codecov Report

Merging #288 into 6.x will increase coverage by 3.85%.
The diff coverage is 94.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##              6.x     #288      +/-   ##
==========================================
+ Coverage   80.27%   84.13%   +3.85%     
==========================================
  Files          19       20       +1     
  Lines        2023     1973      -50     
==========================================
+ Hits         1624     1660      +36     
+ Misses        399      313      -86
Impacted Files Coverage Δ
src/networkWrapper/protocols/socketio.js 96% <ø> (ø) ⬆️
src/security/SecurityDocument.js 100% <ø> (ø) ⬆️
src/Auth.js 100% <100%> (ø)
src/Kuzzle.js 45.85% <72.72%> (+8.91%) ⬆️

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 95e692b...2eae7d7. Read the comment docs.

@ballinette
Copy link

Note: to help the code review and concentrate only on Auth controller, the feature/query-pormise branch should have its own PR.
Or, this should be delayed at the last step of the refactoring process (see my comment here: #285 (comment)

ballinette
ballinette suggested changes Mar 13, 2018
View reviewed changes
Copy link

@ballinette ballinette left a comment •
edited
Loading

Choose a reason for hiding this comment

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

except of my comments above about query promise, and a small nitpicking about signature of login method, it's OK for me.

src/Auth.js Outdated
* @param expiresIn
* @returns {Promise|*|PromiseLike<T>|Promise<T>}
*/
login(strategy, ...args) {
Copy link

Choose a reason for hiding this comment

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

We should avoid implicit arguments, and use better an explicit signature:
login(strategy, credentials, expiresIn)
If we need to call login without credentials, just call login(strategy, null, expiresIn)

@jenow jenow merged commit 4f24bd9 into 6.x Mar 16, 2018
@jenow jenow deleted the #102-auth-ctrl branch March 16, 2018 15:15
@ballinette ballinette mentioned this pull request Mar 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ballinette ballinette ballinette approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jenow jenow

Labels

changelog:breaking-changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jenow @codecov-io @ballinette @benoitvidis