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

Basic Authentication Scheme #5

Merged
merged 1 commit into from Nov 26, 2015

Conversation

Projects
None yet
5 participants
@kylef
Copy link
Member

kylef commented Sep 29, 2015

This pull request extends on the authentication framework (proposed in #4) to provide a base authentication scheme called "Basic".

Dependencies:

  • #4: Authentication Framework

@kylef kylef added the draft label Sep 29, 2015

would be based64 encoded and placed in the authorization header as follows:

```
Authorization: Basic a3lsZTpiMjk1MmQwM2JkYTA5Y2I1ZjYzYjAxNjJmYmJlZTc3Yw==

This comment has been minimized.

@danielgtaylor

danielgtaylor Oct 2, 2015

Contributor

Seems like this is very similar to the OAuth problem mentioned in the other PR. Basically you have a field that is made up of a constant and some computed value.

@danielgtaylor

This comment has been minimized.

Copy link
Contributor

danielgtaylor commented Oct 2, 2015

👍 Looks good.

As an anonymous scheme:

```apib
+ Basic

This comment has been minimized.

@zdne

zdne Oct 21, 2015

Contributor

I believe this ought be + Authentication (Basic)

This comment has been minimized.

@pksunkara

pksunkara Nov 3, 2015

Member

+ Authenticated (Basic) to be exact.

This comment has been minimized.

@kylef

kylef Nov 4, 2015

Member

This bas been addressed.

@zdne

This comment has been minimized.

Copy link
Contributor

zdne commented Oct 21, 2015

Looks good but the #5 (comment)

@zdne

This comment has been minimized.

Copy link
Contributor

zdne commented Nov 3, 2015


As such, a basic authentication scheme may configure two properties,
`username` and `password`. These properties indicate a sample username and
password that may be used.

This comment has been minimized.

@pksunkara

pksunkara Nov 3, 2015

Member

What exactly are we configuring here? We say properties. But we are actually configuring sample username and sample password. I don't see any mention about the values being an example.

Also, I think we should have a default example username and example password for Basic.

This comment has been minimized.

@kylef

kylef Nov 4, 2015

Member

As per the MSON Specification, 2.3.1 Property Member Types, the properties within an MSON object are called "Properties". The wording here reflects that.

These are properties within the authentication scheme's object.

This comment has been minimized.

@kylef

kylef Nov 4, 2015

Member

As for the sample/example, I'm not sure I follow. What would the difference between sample and example be?

This comment has been minimized.

@pksunkara

pksunkara Nov 4, 2015

Member

Mention the default values for the properties username and password, then this looks good.

+ password: b2952d03bda09cb5f63b0162fbbee77c
```

A client when making a request that is using the Basic authentication scheme

This comment has been minimized.

@pksunkara

pksunkara Nov 3, 2015

Member

I don't understand why the following stuff is even in the PR. Isn't all this covered when we mention RFC1945 section 11.1?

This comment has been minimized.

@pksunkara

pksunkara Nov 3, 2015

Member

I might not understand correctly, so please clarify if I am wrong. What exactly are we trying to achieve here with this part of text?

This comment has been minimized.

@kylef

kylef Nov 4, 2015

Member

I was trying to make it absolutely clear that any clients or tooling around API Blueprint that understands the semantic meaning around the basic authentication scheme should implement authentication as defined.

I'm happy to remove this section of the RFC draft if we all think this is clear without this section.

This comment has been minimized.

@pksunkara

pksunkara Nov 4, 2015

Member

We do not talk about the details of GET requests and POST requests in API Blueprint. Similarly, I don't think we need to talk about the implementation details of Basic Authentication Scheme here.

I recommend removing this part entirely.

@pksunkara

This comment has been minimized.

Copy link
Member

pksunkara commented Nov 3, 2015

My review is done. I have a #5 (comment) which needs to be clarified for this to move forward.

@kylef kylef force-pushed the kylef/basic-authentication branch from 7154566 to ded953d Nov 4, 2015

@pksunkara

This comment has been minimized.

Copy link
Member

pksunkara commented Nov 4, 2015

Reviewed the latest changes.

@kylef kylef force-pushed the kylef/basic-authentication branch from ded953d to 8c05c33 Nov 4, 2015

@kylef

This comment has been minimized.

Copy link
Member

kylef commented Nov 4, 2015

@pksunkara I've updated this pull request now.

```

There are no default values for the sample `username` and `password`. When
these are both unset, there is no sample credentials.

This comment has been minimized.

@pksunkara

pksunkara Nov 4, 2015

Member

@kylef Are we sure about this? Some users might want to directly use Basic simply like this:

+ Authenticated (Basic)

This comment has been minimized.

@kylef

kylef Nov 4, 2015

Member

Yes they can, just because there are no defaults, doesn't mean you can't indicate that a resource/group/action/request example etc supports the basic scheme. It just represents there are not sample credentials available.

You don't need to define a sample username or password to use this scheme.

Does this make sense? Is there something I can add to make this a little clearer?

This comment has been minimized.

@pksunkara

pksunkara Nov 4, 2015

Member

You don't need to define a sample username or password to use this scheme.

I understand and I agree. But we also need to think about the tools which use API Blueprint. Dredd, Documentation and others like them. Let's take the example of Dredd. User uses the Basic directly for an action. But what values will dredd use now when testing the server? If we leave this upto the tools, each one of them will have their own default values and user will start getting overwhelmed. Having a default sample username and password will avoid all this.

This comment has been minimized.

@kylef

kylef Nov 4, 2015

Member

I think if there is no sample values, a user should have to supply the value to use at run-time. For example:

$ dredd --user kyle:password

I disagree that we should require services to provide a sample account which is described in the blueprint (and even dictating the default username/password).

/cc @netmilk -- would love to hear your thoughts around this matter.

This comment has been minimized.

@w-vi

w-vi Nov 10, 2015

Member

Dredd supports basic auth already by supplying the user and password from command line as described above by @kylef and I don't see a reason why we shouldn't keep it that way. I think the mandatory sample account is not necessary. Blueprint doesn't require defaults on anything so why it should all of a sudden require it it doesn't make sense. Sure I would push users to provide values as a convenient way how to do things but not force them to.

@zdne zdne referenced this pull request Nov 9, 2015

Closed

Authentication syntax proposal #201

@zdne

This comment has been minimized.

Copy link
Contributor

zdne commented Nov 26, 2015

👍

zdne added a commit that referenced this pull request Nov 26, 2015

@zdne zdne merged commit 5f725af into master Nov 26, 2015

@zdne zdne deleted the kylef/basic-authentication branch Nov 26, 2015

@imanel imanel referenced this pull request Feb 22, 2018

Open

Authentication #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment