Skip to content
This repository has been archived by the owner on May 19, 2023. It is now read-only.

Refactor did auth #51

Merged
merged 13 commits into from
Dec 29, 2020
Merged

Refactor did auth #51

merged 13 commits into from
Dec 29, 2020

Conversation

ilanolkies
Copy link
Collaborator

This PR adds only one breaking change: rpcPersonalSign name for personalSign. It is not necessary for personal sign to be via RPC.

Other changes:

  • Encapsulates auth manager API using just getAuthToken and storedTokens messages
  • Removes duplicated tests (after changing API)

@ilanolkies ilanolkies added the refactor Refactor label Dec 27, 2020
@ilanolkies ilanolkies added this to the v0.1 milestone Dec 27, 2020
@ilanolkies ilanolkies added this to In progress in RIF Identity Q4 20' via automation Dec 27, 2020
@codecov-io
Copy link

codecov-io commented Dec 27, 2020

Codecov Report

Merging #51 (f16dc0e) into develop (1a525fd) will decrease coverage by 0.84%.
The diff coverage is 97.10%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #51      +/-   ##
===========================================
- Coverage    85.19%   84.35%   -0.85%     
===========================================
  Files           16       18       +2     
  Lines          385      377       -8     
  Branches        54       49       -5     
===========================================
- Hits           328      318      -10     
- Misses          57       59       +2     
Impacted Files Coverage Δ
modules/ipfs-cpinner-client/src/constants.ts 100.00% <ø> (ø)
...ules/ipfs-cpinner-client/src/auth-manager/store.ts 33.33% <33.33%> (ø)
.../ipfs-cpinner-client/src/auth-manager/constants.ts 100.00% <100.00%> (ø)
...ules/ipfs-cpinner-client/src/auth-manager/index.ts 100.00% <100.00%> (ø)
...pfs-cpinner-client/src/encryption-manager/index.ts 100.00% <100.00%> (ø)
modules/ipfs-cpinner-client/src/index.ts 78.43% <100.00%> (-4.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 1a525fd...f16dc0e. Read the comment docs.

Copy link
Contributor

@javiesses javiesses left a comment

Choose a reason for hiding this comment

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

Like it :)


// api
const getAccessToken = async () => {
const accessToken = await storage.get(ACCESS_TOKEN_KEY)
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
const accessToken = await storage.get(ACCESS_TOKEN_KEY)
const accessToken = await getStoredAccessToken()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in following pr

export type DIDAuthConfig = {
did?: string
serviceUrl: string
serviceDid?: string // TODO: unused, if we verify challenge request we should use it to assert signer, otherwise remove it
Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, it was used before we changed the way we sign in the login process. Anyway, I think we should verify that the signer is the one we expect to be because is supposed that the DV client must trust in the DV service.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we open an issue for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Already opened, let's remove it for now #56

RIF Identity Q4 20' automation moved this from In progress to Reviewer approved Dec 29, 2020
@javiesses javiesses merged commit ba3a693 into develop Dec 29, 2020
RIF Identity Q4 20' automation moved this from Reviewer approved to Done Dec 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the refactor-did-auth branch December 29, 2020 20:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
refactor Refactor
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants