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

process: move POSIX credential accessors into node_credentials.cc #25066

Closed
wants to merge 1 commit into from

Conversation

@joyeecheung
Copy link
Member

commented Dec 15, 2018

Expose the POSIX credential accessors through
internalBinding('credentials') instead of setting them on the
process or bootstrapper object from C++ directly. Also moves
SafeGetEnv from internalBinding('util') to
internalBinding('credentials') since it's closely related to
the credentials.

In the JS land, instead of wrapping the bindings then writing
to the process object directly in main_thread_only.js, return
the wrapped functions back to bootstrap/node.js where they get
written to the process object conditionally for clarity.

Refs: #24961

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
process: move POSIX credential accessors into node_credentials.cc
Expose the POSIX credential accessors through
`internalBinding('credentials')` instead of setting them on the
process or bootstrapper object from C++ directly. Also moves
`SafeGetEnv` from `internalBinding('util')` to
`internalBinding('credentials')` since it's closely related to
the credentials.

In the JS land, instead of wrapping the bindings then writing
to the process object directly in main_thread_only.js, return
the wrapped functions back to bootstrap/node.js where they get
written to the process object conditionally for clarity.

Refs: #24961

@joyeecheung joyeecheung force-pushed the joyeecheung:mov-cred branch from 0da29ad to 39da0cc Dec 18, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2018

CI is green. Landed in 321e296, thanks!

joyeecheung added a commit that referenced this pull request Dec 18, 2018
process: move POSIX credential accessors into node_credentials.cc
Expose the POSIX credential accessors through
`internalBinding('credentials')` instead of setting them on the
process or bootstrapper object from C++ directly. Also moves
`SafeGetEnv` from `internalBinding('util')` to
`internalBinding('credentials')` since it's closely related to
the credentials.

In the JS land, instead of wrapping the bindings then writing
to the process object directly in main_thread_only.js, return
the wrapped functions back to bootstrap/node.js where they get
written to the process object conditionally for clarity.

Refs: #24961

PR-URL: #25066
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Dec 25, 2018

this does not land cleanly on v11.x, should it be backported?

@targos targos added this to Backport requested in v11.x Dec 28, 2018

@addaleax addaleax referenced this pull request Jan 5, 2019
2 of 2 tasks complete
@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

@addaleax

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

Applies cleanly now.

Edit: But once again, I had to adjust test/parallel/test-bootstrap-modules.js slightly.

addaleax added a commit that referenced this pull request Jan 14, 2019
process: move POSIX credential accessors into node_credentials.cc
Expose the POSIX credential accessors through
`internalBinding('credentials')` instead of setting them on the
process or bootstrapper object from C++ directly. Also moves
`SafeGetEnv` from `internalBinding('util')` to
`internalBinding('credentials')` since it's closely related to
the credentials.

In the JS land, instead of wrapping the bindings then writing
to the process object directly in main_thread_only.js, return
the wrapped functions back to bootstrap/node.js where they get
written to the process object conditionally for clarity.

Refs: #24961

PR-URL: #25066
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Jan 14, 2019
process: move POSIX credential accessors into node_credentials.cc
Expose the POSIX credential accessors through
`internalBinding('credentials')` instead of setting them on the
process or bootstrapper object from C++ directly. Also moves
`SafeGetEnv` from `internalBinding('util')` to
`internalBinding('credentials')` since it's closely related to
the credentials.

In the JS land, instead of wrapping the bindings then writing
to the process object directly in main_thread_only.js, return
the wrapped functions back to bootstrap/node.js where they get
written to the process object conditionally for clarity.

Refs: #24961

PR-URL: #25066
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
process: move POSIX credential accessors into node_credentials.cc
Expose the POSIX credential accessors through
`internalBinding('credentials')` instead of setting them on the
process or bootstrapper object from C++ directly. Also moves
`SafeGetEnv` from `internalBinding('util')` to
`internalBinding('credentials')` since it's closely related to
the credentials.

In the JS land, instead of wrapping the bindings then writing
to the process object directly in main_thread_only.js, return
the wrapped functions back to bootstrap/node.js where they get
written to the process object conditionally for clarity.

Refs: nodejs#24961

PR-URL: nodejs#25066
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

@BridgeAR BridgeAR moved this from Backport requested to Backported in v11.x Jan 14, 2019

@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
process: move POSIX credential accessors into node_credentials.cc
Expose the POSIX credential accessors through
`internalBinding('credentials')` instead of setting them on the
process or bootstrapper object from C++ directly. Also moves
`SafeGetEnv` from `internalBinding('util')` to
`internalBinding('credentials')` since it's closely related to
the credentials.

In the JS land, instead of wrapping the bindings then writing
to the process object directly in main_thread_only.js, return
the wrapped functions back to bootstrap/node.js where they get
written to the process object conditionally for clarity.

Refs: nodejs#24961

PR-URL: nodejs#25066
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
7 participants
You can’t perform that action at this time.