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

Drop 'origin' from public interfaces after #100. #131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

mikewest
Copy link
Member

@mikewest mikewest commented Dec 3, 2018

@mikewest mikewest requested a review from equalsJeffH December 5, 2018 11:26
@equalsJeffH equalsJeffH self-assigned this May 7, 2019
@equalsJeffH
Copy link
Collaborator

equalsJeffH commented May 7, 2019

HI @mikewest -- thanks for this! Apologies for taking so long to pivot back to this...

IIUC, this fixes some things I got wrong or forgot about in PR #100 wherein we're passing the origin down through the algorithms and so didn't need to add it to PasswordCredentialData nor FederatedCredentialInit -- rather we just need to add an origin param to some relevant algs, as well as calling said algs with an origin param, yes?

this PR in and of itself does not address the as-yet unresolved aspects of finishing-up PR #100 that are noted in the updated original post in PR #100, correct?

Copy link
Collaborator

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

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

Overall, this seems correct, thanks for fixing my sloppiness o_0

Tho, looks like there's a couple missing things:

(a) at new lines 1193-1197, perhaps ought to read (adding "origin" in there):

:   <dfn constructor>PasswordCredential(data)</dfn>
::  This constructor accepts a {{PasswordCredentialData}} (|data|), 
    and runs the following steps:
    1.  Let |origin| be the [=current settings object=]'s 
        [=environment settings object/origin=].
    2.  Let |r| be the result of executing 
        <a abstract-op>Create a `PasswordCredential` 
        from `PasswordCredentialData`</a> on |data| and |origin|.

...?

(b) at new lines 1523-1527, perhaps ought to read (adding "origin" in there):

:   <dfn constructor>FederatedCredential(data)</dfn>
::  This constructor accepts a {{FederatedCredentialInit}} (|data|), 
    and runs the following steps:
    1.  Let |origin| be the [=current settings object=]'s 
        [=environment settings object/origin=].
    2.  Let |r| be the result of executing 
        <a abstract-op>Create a `FederatedCredential` from
        `FederatedCredentialInit`</a> on |data| and |origin|.

..?

thanks, HTH,

=JeffH

Base automatically changed from master to main February 16, 2021 23:21
@marcoscaceres
Copy link
Member

@mikewest, @equalsJeffH, seems this got forgotten about... can you let us know what you'd like to do with it?

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